Wrapper API for static bus_dma allocations

[Marius Strobl]

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