From: Mark Kettenis Subject: Re: amd64 bus dma useless lastaddr variable To: Alexander Bluhm Cc: tech@openbsd.org Date: Tue, 27 Aug 2024 22:28:41 +0200 > Date: Mon, 26 Aug 2024 14:23:16 +0200 > From: Alexander Bluhm > > Hi, > > I think the lastaddr variable in amd64 bus dma is redundant. Instead > of storing the final address of a segment in the caller, > _bus_dmamap_load_buffer() can inspect the previous segment. Also > in _bus_dmamap_load_raw() and _bus_dmamem_alloc_range() the previous > segement is available and can be used directly. > > ok? Hmm, thois makes it a bit harder to understand the code. Also, other architectures use a _dmamap_load_buffer() with a lastaddr pointer argument. By only changing amd64 you make them needlessly diverge. > Index: arch/amd64/amd64/bus_dma.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/bus_dma.c,v > diff -u -p -r1.57 bus_dma.c > --- arch/amd64/amd64/bus_dma.c 22 Aug 2024 11:36:24 -0000 1.57 > +++ arch/amd64/amd64/bus_dma.c 23 Aug 2024 17:48:50 -0000 > @@ -102,7 +102,7 @@ > #endif > > int _bus_dmamap_load_buffer(bus_dma_tag_t, bus_dmamap_t, void *, bus_size_t, > - struct proc *, int, paddr_t *, int *, int *, int); > + struct proc *, int, int *, int *, int); > > /* > * Common function for DMA map creation. May be called by bus-specific > @@ -254,7 +254,6 @@ int > _bus_dmamap_load(bus_dma_tag_t t, bus_dmamap_t map, void *buf, > bus_size_t buflen, struct proc *p, int flags) > { > - bus_addr_t lastaddr = 0; > int seg, used, error; > > /* > @@ -269,7 +268,7 @@ _bus_dmamap_load(bus_dma_tag_t t, bus_dm > seg = 0; > used = 0; > error = _bus_dmamap_load_buffer(t, map, buf, buflen, p, flags, > - &lastaddr, &seg, &used, 1); > + &seg, &used, 1); > if (error == 0) { > map->dm_mapsize = buflen; > map->dm_nsegs = seg + 1; > @@ -285,7 +284,6 @@ int > _bus_dmamap_load_mbuf(bus_dma_tag_t t, bus_dmamap_t map, struct mbuf *m0, > int flags) > { > - paddr_t lastaddr = 0; > int seg, used, error, first; > struct mbuf *m; > > @@ -311,7 +309,7 @@ _bus_dmamap_load_mbuf(bus_dma_tag_t t, b > if (m->m_len == 0) > continue; > error = _bus_dmamap_load_buffer(t, map, m->m_data, m->m_len, > - NULL, flags, &lastaddr, &seg, &used, first); > + NULL, flags, &seg, &used, first); > first = 0; > } > if (error == 0) { > @@ -329,7 +327,6 @@ int > _bus_dmamap_load_uio(bus_dma_tag_t t, bus_dmamap_t map, struct uio *uio, > int flags) > { > - paddr_t lastaddr = 0; > int seg, used, i, error, first; > bus_size_t minlen, resid; > struct proc *p = NULL; > @@ -366,7 +363,7 @@ _bus_dmamap_load_uio(bus_dma_tag_t t, bu > addr = (caddr_t)iov[i].iov_base; > > error = _bus_dmamap_load_buffer(t, map, addr, minlen, > - p, flags, &lastaddr, &seg, &used, first); > + p, flags, &seg, &used, first); > first = 0; > > resid -= minlen; > @@ -387,7 +384,7 @@ int > _bus_dmamap_load_raw(bus_dma_tag_t t, bus_dmamap_t map, bus_dma_segment_t *segs, > int nsegs, bus_size_t size, int flags) > { > - bus_addr_t paddr, baddr, bmask, lastaddr = 0; > + bus_addr_t paddr, baddr, bmask; > bus_size_t plen, sgsize, mapsize; > int first = 1; > int i, seg = 0; > @@ -439,14 +436,15 @@ _bus_dmamap_load_raw(bus_dma_tag_t t, bu > map->dm_segs[seg].ds_len = sgsize; > first = 0; > } else { > - if (paddr == lastaddr && > + if (paddr == (map->dm_segs[seg].ds_addr + > + map->dm_segs[seg].ds_len) && > (map->dm_segs[seg].ds_len + sgsize) <= > map->_dm_maxsegsz && > (map->_dm_boundary == 0 || > (map->dm_segs[seg].ds_addr & bmask) == > - (paddr & bmask))) > + (paddr & bmask))) { > map->dm_segs[seg].ds_len += sgsize; > - else { > + } else { > if (++seg >= map->_dm_segcnt) > return (EINVAL); > map->dm_segs[seg].ds_addr = paddr; > @@ -457,8 +455,6 @@ _bus_dmamap_load_raw(bus_dma_tag_t t, bu > paddr += sgsize; > plen -= sgsize; > size -= sgsize; > - > - lastaddr = paddr; > } > } > > @@ -686,18 +682,17 @@ _bus_dmamem_mmap(bus_dma_tag_t t, bus_dm > * DMA utility functions > **********************************************************************/ > /* > - * Utility function to load a linear buffer. lastaddrp holds state > - * between invocations (for multiple-buffer loads). segp contains > + * Utility function to load a linear buffer. segp contains > * the starting segment on entrance, and the ending segment on exit. > * first indicates if this is the first invocation of this function. > */ > int > _bus_dmamap_load_buffer(bus_dma_tag_t t, bus_dmamap_t map, void *buf, > - bus_size_t buflen, struct proc *p, int flags, paddr_t *lastaddrp, > + bus_size_t buflen, struct proc *p, int flags, > int *segp, int *usedp, int first) > { > bus_size_t sgsize; > - bus_addr_t curaddr, lastaddr, baddr, bmask; > + bus_addr_t curaddr, baddr, bmask; > vaddr_t pgva = -1, vaddr = (vaddr_t)buf; > int seg, page, off; > pmap_t pmap; > @@ -710,7 +705,6 @@ _bus_dmamap_load_buffer(bus_dma_tag_t t, > pmap = pmap_kernel(); > > page = *usedp; > - lastaddr = *lastaddrp; > bmask = ~(map->_dm_boundary - 1); > > for (seg = *segp; buflen > 0 ; ) { > @@ -762,7 +756,8 @@ _bus_dmamap_load_buffer(bus_dma_tag_t t, > map->dm_segs[seg]._ds_bounce_va = pgva; > first = 0; > } else { > - if (curaddr == lastaddr && > + if (curaddr == (map->dm_segs[seg].ds_addr + > + map->dm_segs[seg].ds_len) && > (map->dm_segs[seg].ds_len + sgsize) <= > map->_dm_maxsegsz && > (map->_dm_boundary == 0 || > @@ -781,14 +776,12 @@ _bus_dmamap_load_buffer(bus_dma_tag_t t, > } > } > > - lastaddr = curaddr + sgsize; > vaddr += sgsize; > buflen -= sgsize; > } > > *segp = seg; > *usedp = page; > - *lastaddrp = lastaddr; > > /* > * Did we fit? > @@ -807,7 +800,7 @@ _bus_dmamem_alloc_range(bus_dma_tag_t t, > bus_size_t boundary, bus_dma_segment_t *segs, int nsegs, int *rsegs, > int flags, bus_addr_t low, bus_addr_t high) > { > - paddr_t curaddr, lastaddr; > + paddr_t curaddr; > struct vm_page *m; > struct pglist mlist; > int curseg, error, plaflag; > @@ -837,7 +830,7 @@ _bus_dmamem_alloc_range(bus_dma_tag_t t, > */ > m = TAILQ_FIRST(&mlist); > curseg = 0; > - lastaddr = segs[curseg].ds_addr = VM_PAGE_TO_PHYS(m); > + segs[curseg].ds_addr = VM_PAGE_TO_PHYS(m); > segs[curseg].ds_len = PAGE_SIZE; > > for (m = TAILQ_NEXT(m, pageq); m != NULL; m = TAILQ_NEXT(m, pageq)) { > @@ -853,14 +846,13 @@ _bus_dmamem_alloc_range(bus_dma_tag_t t, > panic("_bus_dmamem_alloc_range"); > } > #endif > - if (curaddr == (lastaddr + PAGE_SIZE)) > + if (curaddr == (segs[curseg].ds_addr + segs[curseg].ds_len)) { > segs[curseg].ds_len += PAGE_SIZE; > - else { > + } else { > curseg++; > segs[curseg].ds_addr = curaddr; > segs[curseg].ds_len = PAGE_SIZE; > } > - lastaddr = curaddr; > } > > *rsegs = curseg + 1; > >