Re: [PATCH][1/2] usb: dma bounce buffer support V3

[David Brownell <david-b@xxxxxxxxxxx>]

On Monday 21 January 2008, Magnus Damm wrote:
> usb: dma bounce buffer support V3
> 
> This patch adds dma bounce buffer support to the usb core. These buffers
> can be enabled with the HCD_LOCAL_MEM flag, and they make sure that all data
> passed to the host controller is allocated using dma_alloc_coherent().
> 
> Signed-off-by: Magnus Damm <damm@xxxxxxxxxx>
> Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>

And FWIW, after the updates:

Acked-by: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>

You already got most of my first-round comments addressed (in V2)
but one more is the "needs comments" issue.   See below.


> --- 0001/drivers/usb/core/hcd.c
> +++ work/drivers/usb/core/hcd.c	2008-01-21 17:55:15.000000000 +0900
> >	...
> 
> +static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> +			   gfp_t mem_flags)
> +{
> +	enum dma_data_direction dir;
> +	int ret = 0;
> +
>  	/* Map the URB's buffers for DMA access.
>  	 * Lower level HCD code should use *_dma exclusively,
>  	 * unless it uses pio or talks to another transport.
>  	 */
> -	if (hcd->self.uses_dma && !is_root_hub(urb->dev)) {
> -		if (usb_endpoint_xfer_control(&urb->ep->desc)
> -			&& !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
> -			urb->setup_dma = dma_map_single (
> +	if (is_root_hub(urb->dev))
> +		return 0;
> +
> +	if (usb_endpoint_xfer_control(&urb->ep->desc)
> +	    && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP)) {
> +		if (hcd->self.uses_dma)
> +			urb->setup_dma = dma_map_single(
>  					hcd->self.controller,
>  					urb->setup_packet,
>  					sizeof (struct usb_ctrlrequest),
>  					DMA_TO_DEVICE);
> -		if (urb->transfer_buffer_length != 0
> -			&& !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))

Right about here, I think, is where a comment is needed to explain
what's really going on here.  Key points:

	- We need "local" memory, canonical example being
	  a small SRAM on a discrete controller being the
	  only memory that the controller can read ...
	  (a) "normal" kernel memory is no good, and
	  (b) there's not enough to share

	- The only *portable* hook for such stuff in the
	  DMA framework is dma_declare_coherent_memory()

	- So we use that, even though the primary requirement
	  is that the memory be "local" (hence addressible
	  by that device), not "coherent".

Someday DMA memory management may be smart enough to handle this
stuff without such workarounds.  Having a clear description of
this particular sub-problem should help.  Having that knowledge
living outside the source tree ... won't.


> +		else if (hcd->driver->flags & HCD_LOCAL_MEM)
> +			ret = hcd_alloc_coherent(
> +					urb->dev->bus, mem_flags,
> +					&urb->setup_dma,
> +					(void **)&urb->setup_packet,
> +					sizeof (struct usb_ctrlrequest),
> +					DMA_TO_DEVICE);
> +	}
> +
> +	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +	if (ret == 0 && urb->transfer_buffer_length != 0
> +	    && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
> +		if (hcd->self.uses_dma)
>  			urb->transfer_dma = dma_map_single (
>  					hcd->self.controller,	
>	...
>	
> --- 0001/drivers/usb/core/hcd.h
> +++ work/drivers/usb/core/hcd.h	2008-01-21 17:48:52.000000000 +0900
> @@ -165,6 +165,7 @@ struct hc_driver {
>  
>  	int	flags;
>  #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
> +#define	HCD_LOCAL_MEM	0x0002		/* HC needs dma_alloc_coherent() */

And again here.  "HC needs local memory" is the issue.
The fact that we can only achieve it with dma_alloc_coherent()
for now is both an implementation hack, and too long to fit
onto the tail end of a #define line.  ;)

>  #define	HCD_USB11	0x0010		/* USB 1.1 */
>  #define	HCD_USB2	0x0020		/* USB 2.0 */
>  
> 



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@xxxxxxxxxxxxxxxxxxxxx
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel