[RFC PATCH v2 8/9] USB: add HCD_NO_COHERENT_MEM host controller driver flag

[Alan Stern]

On Sun, 28 Feb 2010, Albert Herranz wrote:

> The HCD_NO_COHERENT_MEM USB host controller driver flag can be enabled
> to instruct the USB stack to avoid allocating coherent memory for USB
> buffers.
> 
> This flag is useful to overcome some esoteric memory access restrictions
> found in some platforms.
> For example, the Nintendo Wii video game console is a NOT_COHERENT_CACHE
> platform that is unable to safely perform non-32 bit uncached writes
> to RAM because the byte enables are not connected to the bus.
> Thus, in that platform, "coherent" DMA buffers cannot be directly used
> by the kernel code unless it guarantees that all write accesses
> to said buffers are done in 32 bit chunks (which is not the case in the
> USB subsystem).
> 
> To avoid this unwanted behaviour HCD_NO_COHERENT_MEM can be enabled at
> the HCD controller, causing USB buffer allocations to be satisfied from
> normal kernel memory. In this case, the USB stack will make sure that
> the buffers get properly mapped/unmapped for DMA transfers using the DMA
> streaming mapping API.
> 
> Note that other parts of the USB stack may also use coherent memory,
> like for example the hardware descriptors used in the EHCI controllers.
> This needs to be checked and addressed separately. In the EHCI example,
> hardware descriptors are accessed in 32-bit (or 64-bit) chunks, causing
> no problems in the described scenario.

> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1260,6 +1260,34 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
>  	*dma_handle = 0;
>  }
>  
> +static int urb_needs_setup_dma_map(struct usb_hcd *hcd, struct urb *urb)
> +{
> +	return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) ||
> +	       ((hcd->driver->flags & HCD_NO_COHERENT_MEM) &&
> +		urb->setup_dma == ~(dma_addr_t)0);
> +}
> +
> +static int urb_needs_setup_dma_unmap(struct usb_hcd *hcd, struct urb *urb)
> +{
> +	return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) ||
> +	       ((hcd->driver->flags & HCD_NO_COHERENT_MEM) &&
> +		urb->setup_dma && urb->setup_dma != ~(dma_addr_t)0);
> +}
> +
> +static int urb_needs_transfer_dma_map(struct usb_hcd *hcd, struct urb *urb)
> +{
> +	return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) ||
> +	       ((hcd->driver->flags & HCD_NO_COHERENT_MEM) &&
> +		urb->transfer_dma == ~(dma_addr_t)0);
> +}
> +
> +static int urb_needs_transfer_dma_unmap(struct usb_hcd *hcd, struct urb *urb)
> +{
> +	return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) ||
> +	       ((hcd->driver->flags & HCD_NO_COHERENT_MEM) &&
> +		urb->transfer_dma && urb->transfer_dma != ~(dma_addr_t)0);
> +}
> +

These functions would be a lot easier to understand if they were 
expanded into multiple test and return statements, rather than 
squeezing all the Boolean manipulations into single expressions.  (Not 
to mention the fact that other developement is going to make them even 
more complicated than they are now...)

Also, I can't help thinking that the corresponding *_map() and 
*_unmap() routines are so similar, it ought to be possible to combine 
them.  The only difference is a check for a NULL DMA address, and it's 
not clear to me why it is present.  It's also not clear why the test 
for a DMA address of all ones is present.  Maybe they both can be 
removed.

Alan Stern