Re: [RFC PATCH v2 8/9] USB: add HCD_NO_COHERENT_MEM host controller driver flag
From: Alan Stern <stern@rowland.harvard.edu>
Date: 2010-03-01 14:49:48
Also in:
linux-arm-kernel
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.
quoted hunk ↗ jump to hunk
--- 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