From owner-freebsd-arch@freebsd.org Sat Feb 27 02:13:43 2016 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7364EAB5BA1 for ; Sat, 27 Feb 2016 02:13:43 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4C80114B4 for ; Sat, 27 Feb 2016 02:13:43 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id F2555B97D for ; Fri, 26 Feb 2016 21:13:41 -0500 (EST) From: John Baldwin To: freebsd-arch@freebsd.org Subject: Re: Wrapper API for static bus_dma allocations Date: Fri, 26 Feb 2016 17:10:53 -0800 Message-ID: <2856669.mkVhDvxH7k@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.2-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: <2800970.jY4xzTy9Hz@ralph.baldwin.cx> References: <2800970.jY4xzTy9Hz@ralph.baldwin.cx> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 26 Feb 2016 21:13:42 -0500 (EST) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Feb 2016 02:13:43 -0000 On Thursday, January 29, 2015 03:37:19 PM John Baldwin wrote: > The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for > descriptor rings) can be a bit obtuse to use in that it require a bit of > boilerplate to create a tag, allocate memory for the tag, then load it to get > the bus address. Similarly, freeing it also requires several steps. In > addition, some of the steps are a bit subtle (e.g. the need to check for an > error in the bus_dma callback instead of from bus_dmamap_load()) and not all > drivers get those correct. > > To try to make this simpler I've written a little wrapper API that tries to > provide a single call to allocate a buffer and a single call to free a buffer. > Each buffer is described by a structure defined by the API, and if the call to > allocate a buffer succeeds, the structure contains both a pointer to the > buffer in the kernel's address space as well as a bus address of the buffer. > > In the interests of simplicity, this API does not allow the buffer to be quite > as fully configured as the underlying bus_dma API, instead it aims to handle > the most common cases that are used in most drivers. As such, it assumes that > the buffer must be one contiguous range of DMA address space, and the only > parameters that can be specified are the parent tag, the alignment of the > buffer, the lowaddr parameter, the size of the buffer to allocate, and the > flags parameter that is normally passed to bus_dmamem_alloc(). I believe that > this should be sufficient to cover the vast majority of the drivers in our > tree. > > I've included below a patch that contains the wrapper API along with a sample > conversion of the ndis driver (chosen at random). If folks like this idea I > will update the patch to include manpage changes as well. > > --- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c > +++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c > @@ -186,7 +186,6 @@ > static ndis_status NdisMAllocateMapRegisters(ndis_handle, > uint32_t, uint8_t, uint32_t, uint32_t); > static void NdisMFreeMapRegisters(ndis_handle); > -static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int); > static void NdisMAllocateSharedMemory(ndis_handle, uint32_t, > uint8_t, void **, ndis_physaddr *); > static void ndis_asyncmem_complete(device_object *, void *); > @@ -1387,23 +1386,6 @@ > bus_dma_tag_destroy(sc->ndis_mtag); > } > > -static void > -ndis_mapshared_cb(arg, segs, nseg, error) > - void *arg; > - bus_dma_segment_t *segs; > - int nseg; > - int error; > -{ > - ndis_physaddr *p; > - > - if (error || nseg > 1) > - return; > - > - p = arg; > - > - p->np_quad = segs[0].ds_addr; > -} > - > /* > * This maps to bus_dmamem_alloc(). > */ > @@ -1443,35 +1425,17 @@ > * than 1GB of physical memory. > */ > > - error = bus_dma_tag_create(sc->ndis_parent_tag, 64, > - 0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL, > - NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL, > - &sh->ndis_stag); > + error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64, > + NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT | BUS_DMA_ZERO); > > if (error) { > free(sh, M_DEVBUF); > return; > } > > - error = bus_dmamem_alloc(sh->ndis_stag, vaddr, > - BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap); > - > - if (error) { > - bus_dma_tag_destroy(sh->ndis_stag); > - free(sh, M_DEVBUF); > - return; > - } > + *vaddr = sh->ndis_mem.dma_vaddr; > + paddr->np_quad = sh->ndis_mem.dma_baddr; > > - error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr, > - len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT); > - > - if (error) { > - bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap); > - bus_dma_tag_destroy(sh->ndis_stag); > - free(sh, M_DEVBUF); > - return; > - } > - > /* > * Save the physical address along with the source address. > * The AirGo MIMO driver will call NdisMFreeSharedMemory() > @@ -1482,8 +1446,6 @@ > */ > > NDIS_LOCK(sc); > - sh->ndis_paddr.np_quad = paddr->np_quad; > - sh->ndis_saddr = *vaddr; > InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list)); > NDIS_UNLOCK(sc); > } > @@ -1581,13 +1543,13 @@ > l = sc->ndis_shlist.nle_flink; > while (l != &sc->ndis_shlist) { > sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list); > - if (sh->ndis_saddr == vaddr) > + if (sh->ndis_mem.dma_vaddr == vaddr) > break; > /* > * Check the physaddr too, just in case the driver lied > * about the virtual address. > */ > - if (sh->ndis_paddr.np_quad == paddr.np_quad) > + if (sh->ndis_mem.dma_baddr == paddr.np_quad) > break; > l = l->nle_flink; > } > @@ -1604,9 +1566,7 @@ > > NDIS_UNLOCK(sc); > > - bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap); > - bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap); > - bus_dma_tag_destroy(sh->ndis_stag); > + bus_dma_mem_free(&sh->ndis_mem); > > free(sh, M_DEVBUF); > } > --- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h > +++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h > @@ -66,10 +66,7 @@ > > struct ndis_shmem { > list_entry ndis_list; > - bus_dma_tag_t ndis_stag; > - bus_dmamap_t ndis_smap; > - void *ndis_saddr; > - ndis_physaddr ndis_paddr; > + struct bus_dmamem ndis_mem; > }; > > struct ndis_cfglist { > --- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c > +++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c > @@ -540,3 +540,66 @@ > > return (0); > } > + > +struct bus_dma_mem_cb_data { > + struct bus_dmamem *mem; > + int error; > +}; > + > +static void > +bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error) > +{ > + struct bus_dma_mem_cb_data *d; > + > + d = arg; > + d->error = error; > + if (error) > + return; > + d->mem->dma_baddr = segs[0].ds_addr; > +} > + > +int > +bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent, > + bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags) > +{ > + struct bus_dma_mem_cb_data d; > + int error; > + > + bzero(mem, sizeof(*mem)); > + error = bus_dma_tag_create(parent, alignment, 0, lowaddr, > + BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL, > + &mem->dma_tag); > + if (error) { > + bus_dma_mem_free(mem); > + return (error); > + } > + error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags, > + &mem->dma_map); > + if (error) { > + bus_dma_mem_free(mem); > + return (error); > + } > + d.mem = mem; > + error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len, > + bus_dma_mem_cb, &d, BUS_DMA_NOWAIT); > + if (error == 0) > + error = d.error; > + if (error) { > + bus_dma_mem_free(mem); > + return (error); > + } > + return (0); > +} > + > +void > +bus_dma_mem_free(struct bus_dmamem *mem) > +{ > + > + if (mem->dma_baddr != 0) > + bus_dmamap_unload(mem->dma_tag, mem->dma_map); > + if (mem->dma_vaddr != NULL) > + bus_dmamem_free(mem->dma_tag, mem->dma_vaddr, mem->dma_map); > + if (mem->dma_tag != NULL) > + bus_dma_tag_destroy(mem->dma_tag); > + bzero(mem, sizeof(*mem)); > +} > --- //depot/vendor/freebsd/src/sys/sys/bus_dma.h > +++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h > @@ -337,4 +337,29 @@ > > #endif /* __sparc64__ */ > > +/* > + * A wrapper API to simplify management of static mappings. > + */ > + > +struct bus_dmamem { > + bus_dma_tag_t dma_tag; > + bus_dmamap_t dma_map; > + void *dma_vaddr; > + bus_addr_t dma_baddr; > +}; > + > +/* > + * Allocate a mapping. On success, zero is returned and the 'dma_vaddr' > + * and 'dma_baddr' fields are populated with the virtual and bus addresses, > + * respectively, of the mapping. > + */ > +int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent, > + bus_size_t alignment, bus_addr_t lowaddr, > + bus_size_t len, int flags); > + > +/* > + * Release a mapping created by bus_dma_mem_create(). > + */ > +void bus_dma_mem_free(struct bus_dmamem *mem); > + > #endif /* _BUS_DMA_H_ */ Ping. The last time I brought this up we ended up off in the weeds a bit about changes to the bus dma API in general. However, I think that even if we reduce the number of args to bus_dma_create_tag you are still left with managing a tag per static allocation etc. I'm working on porting a driver today where I basically just copied this into the driver directly rather than having to implement it all by hand for the umpteenth time. Are folks seriously opposed to having a single function you can call to allocate a static DMA mapping? -- John Baldwin