Wrapper API for static bus_dma allocations

[John Baldwin]

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