Re: [PATCH 2/2] USB: ehci: tegra: Align DMA transfers to 32 bytes

[Greg KH <gregkh@suse.de>]

On Fri, Dec 17, 2010 at 01:58:49PM -0800, Robert Morell wrote:
> The Tegra2 USB controller doesn't properly deal with misaligned DMA
> buffers, causing corruption.  This is especially prevalent with USB
> network adapters, where skbuff alignment is often in the middle of a
> 4-byte dword.
> 
> To avoid this, allocate a temporary buffer for the DMA if the provided
> buffer isn't sufficiently aligned.
> 
> Signed-off-by: Robert Morell <rmorell@nvidia.com>
> Reviewed-by: Scott Williams <scwilliams@nvidia.com>
> Reviewed-by: Janne Hellsten <jhellsten@nvidia.com>
> ---
>  drivers/usb/host/ehci-tegra.c |   94 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h           |    1 +
>  2 files changed, 95 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index d990c1c..a75e4db 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -32,6 +32,10 @@
>  #define TEGRA_USB_USBMODE_HOST			(3 << 0)
>  #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
>  
> +#define TEGRA_USB_DMA_ALIGN 32
> +
> +#define URB_ALIGNED_TEMP_BUFFER 0x80000000
> +
>  struct tegra_ehci_context {
>  	bool valid;
>  	u32 command;
> @@ -457,6 +461,94 @@ static int tegra_ehci_bus_resume(struct usb_hcd *hcd)
>  }
>  #endif
>  
> +struct temp_buffer {
> +	void *kmalloc_ptr;
> +	void *old_xfer_buffer;
> +	u8 data[0];
> +};
> +
> +static void free_temp_buffer(struct urb *urb)
> +{
> +	enum dma_data_direction dir;
> +	struct temp_buffer *temp;
> +
> +	if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER))
> +		return;
> +
> +	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +
> +	temp = container_of(urb->transfer_buffer, struct temp_buffer,
> +			    data);
> +
> +	if (dir == DMA_FROM_DEVICE)
> +		memcpy(temp->old_xfer_buffer, temp->data,
> +		       urb->transfer_buffer_length);
> +	urb->transfer_buffer = temp->old_xfer_buffer;
> +	kfree(temp->kmalloc_ptr);
> +
> +	urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER;
> +}
> +
> +static int alloc_temp_buffer(struct urb *urb, gfp_t mem_flags)
> +{
> +	enum dma_data_direction dir;
> +	struct temp_buffer *temp, *kmalloc_ptr;
> +	size_t kmalloc_size;
> +	void *data;
> +
> +	if (urb->num_sgs || urb->sg ||
> +	    urb->transfer_buffer_length == 0 ||
> +	    !((uintptr_t)urb->transfer_buffer & (TEGRA_USB_DMA_ALIGN - 1)))
> +		return 0;
> +
> +	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +
> +	/* Allocate a buffer with enough padding for alignment */
> +	kmalloc_size = urb->transfer_buffer_length +
> +		sizeof(struct temp_buffer) + TEGRA_USB_DMA_ALIGN - 1;
> +
> +	kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
> +	if (!kmalloc_ptr)
> +		return -ENOMEM;
> +
> +	/* Position our struct temp_buffer such that data is aligned */
> +	temp = PTR_ALIGN(kmalloc_ptr + 1, TEGRA_USB_DMA_ALIGN) - 1;
> +
> +	temp->kmalloc_ptr = kmalloc_ptr;
> +	temp->old_xfer_buffer = urb->transfer_buffer;
> +	if (dir == DMA_TO_DEVICE)
> +		memcpy(temp->data, urb->transfer_buffer,
> +		       urb->transfer_buffer_length);
> +	urb->transfer_buffer = temp->data;
> +
> +	BUILD_BUG_ON(!(URB_ALIGNED_TEMP_BUFFER & URB_DRIVER_PRIVATE));

See below why this should be removed

> +	urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER;
> +
> +	return 0;
> +}
> +
> +static int tegra_ehci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> +				      gfp_t mem_flags)
> +{
> +	int ret;
> +
> +	ret = alloc_temp_buffer(urb, mem_flags);
> +	if (ret)
> +		return ret;
> +
> +	ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
> +	if (ret)
> +		free_temp_buffer(urb);
> +
> +	return ret;
> +}
> +
> +static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> +{
> +	usb_hcd_unmap_urb_for_dma(hcd, urb);
> +	free_temp_buffer(urb);
> +}
> +
>  static const struct hc_driver tegra_ehci_hc_driver = {
>  	.description		= hcd_name,
>  	.product_desc		= "Tegra EHCI Host Controller",
> @@ -472,6 +564,8 @@ static const struct hc_driver tegra_ehci_hc_driver = {
>  	.shutdown		= tegra_ehci_shutdown,
>  	.urb_enqueue		= ehci_urb_enqueue,
>  	.urb_dequeue		= ehci_urb_dequeue,
> +	.map_urb_for_dma	= tegra_ehci_map_urb_for_dma,
> +	.unmap_urb_for_dma	= tegra_ehci_unmap_urb_for_dma,
>  	.endpoint_disable	= ehci_endpoint_disable,
>  	.endpoint_reset		= ehci_endpoint_reset,
>  	.get_frame_number	= ehci_get_frame,
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 35fe6ab..cd07173 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -975,6 +975,7 @@ extern int usb_disabled(void);
>  #define URB_SETUP_MAP_SINGLE	0x00100000	/* Setup packet DMA mapped */
>  #define URB_SETUP_MAP_LOCAL	0x00200000	/* HCD-local setup packet */
>  #define URB_DMA_SG_COMBINED	0x00400000	/* S-G entries were combined */
> +#define URB_DRIVER_PRIVATE	0x80000000	/* For driver-private use */

Um, what?  You are using this for a build check, which seems pointless
as you already modified the code to not need this.

So it doesn't look like this is needed, right?

thanks,

greg k-h