Date: Sun, 28 Feb 2016 17:30:51 +0100 From: Marius Strobl <marius@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-arch@freebsd.org Subject: Re: Wrapper API for static bus_dma allocations Message-ID: <20160228163051.GA64049@alchemy.franken.de> In-Reply-To: <2856669.mkVhDvxH7k@ralph.baldwin.cx> References: <2800970.jY4xzTy9Hz@ralph.baldwin.cx> <2856669.mkVhDvxH7k@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Feb 26, 2016 at 05:10:53PM -0800, John Baldwin wrote:
> 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?
No, I'm fine with your proposal.
Marius
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160228163051.GA64049>