Date: Fri, 26 Feb 2016 17:10:53 -0800 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Subject: Re: Wrapper API for static bus_dma allocations Message-ID: <2856669.mkVhDvxH7k@ralph.baldwin.cx> In-Reply-To: <2800970.jY4xzTy9Hz@ralph.baldwin.cx> References: <2800970.jY4xzTy9Hz@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2856669.mkVhDvxH7k>