Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 01 Feb 2015 07:50:14 -0700
From:      Ian Lepore <ian@freebsd.org>
To:        John Baldwin <jhb@freebsd.org>
Cc:        'freebsd-arch' <freebsd-arch@freebsd.org>
Subject:   Re: Wrapper API for static bus_dma allocations
Message-ID:  <1422802214.15718.253.camel@freebsd.org>
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 Thu, 2015-01-29 at 15:37 -0500, 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
[...]
> +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_ */
> 
> 

You introduce new functions named bus_dma_mem_xxxx(), and a new
structure bus_dmamem.  It's already hard enough to remember the oddball
mix of underbars in busdma naming, can the new struct be bus_dma_mem?

Also, the combo of create/free is most unsatisfying.  Typically
alloc/free come in pairs, as do create/destroy.  These pairings are used
pretty consistantly in busdma now, it'd be nice to not have to remember
this pair is an exception.  (I'm agnostic about whether the new
functionality should be alloc/delete or create/destroy.)

-- Ian





Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1422802214.15718.253.camel>