[RFC] Align tegra-ehci DMA transfers to 32B

[RFC] Align tegra-ehci DMA transfers to 32B

[Robert Morell]

This small set of patches fixes an issue where DMA from the tegra EHCI
controller could be corrupted.  It was most commonly seen with USB network
adapters, though in theory it could happen with any USB traffic.

(Note: An attempt was made to fix this with commit 367c3aab, which set
NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.  Unfortunately, not all network
drivers honor them (presumably since these are intended as optimizations rather
than hard rules).  This does mean that properly-written network drivers should
fall through this code with very little overhead, however.)

[PATCH 1/2] USB: Add driver hooks for (un)?map_urb_for_dma

[Robert Morell]

These are useful in case your device has quirks involving what memory
is and is not DMAable.

Signed-off-by: Robert Morell <rmorell@nvidia.com>
Reviewed-by: Scott Williams <scwilliams@nvidia.com>
Reviewed-by: Janne Hellsten <jhellsten@nvidia.com>
---
 drivers/usb/core/hcd.c  |   19 ++++++++++++++++++-
 include/linux/usb/hcd.h |    8 ++++++++
 2 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5cca00a..84fee0f 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1265,6 +1265,14 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
 
 static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
+	if (hcd->driver->unmap_urb_for_dma)
+		hcd->driver->unmap_urb_for_dma(hcd, urb);
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
+void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
 	enum dma_data_direction dir;
 
 	if (urb->transfer_flags & URB_SETUP_MAP_SINGLE)
@@ -1311,6 +1319,15 @@ static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			   gfp_t mem_flags)
 {
+	if (hcd->driver->map_urb_for_dma)
+		return hcd->driver->map_urb_for_dma(hcd, urb, mem_flags);
+	else
+		return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
+int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+			    gfp_t mem_flags)
+{
 	enum dma_data_direction dir;
 	int ret = 0;
 
@@ -1400,7 +1417,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 		}
 		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
 				URB_SETUP_MAP_LOCAL)))
-			unmap_urb_for_dma(hcd, urb);
+			usb_hcd_unmap_urb_for_dma(hcd, urb);
 	}
 	return ret;
 }
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 3b571f1..8aaf687 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -233,6 +233,11 @@ struct hc_driver {
 	int	(*urb_dequeue)(struct usb_hcd *hcd,
 				struct urb *urb, int status);
 
+	/* dma support */
+	int	(*map_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				   gfp_t mem_flags);
+	void    (*unmap_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb);
+
 	/* hw synch, freeing endpoint resources that urb_dequeue can't */
 	void	(*endpoint_disable)(struct usb_hcd *hcd,
 			struct usb_host_endpoint *ep);
@@ -327,6 +332,9 @@ extern void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb);
 
 extern int usb_hcd_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb(struct urb *urb, int status);
+extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+		gfp_t mem_flags);
+extern void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb);
 extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
 extern void usb_hcd_flush_endpoint(struct usb_device *udev,
-- 
1.7.2.2

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

[Robert Morell]

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));
+	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 */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
-- 
1.7.2.2

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

[Greg KH]

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

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

[rmorell]

On Fri, Dec 17, 2010 at 02:35:02PM -0800, Greg KH wrote:
> 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?

The intention was to reserve space in the URB flags for
implementation-specific use.  This placeholder should keep anybody from
adding driver-agnostic flags to that mask.  The BUILD_BUG_ON is intended
to make sure that the driver private space doesn't move out from under
the Tegra-specific flag.

Thanks,
Robert

> thanks,
> 
> greg k-h
> 

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

[Greg KH]

On Fri, Dec 17, 2010 at 02:42:21PM -0800, rmorell@nvidia.com wrote:
> On Fri, Dec 17, 2010 at 02:35:02PM -0800, Greg KH wrote:
> > 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?
> 
> The intention was to reserve space in the URB flags for
> implementation-specific use.

Yes, but _which_ driver are you talking about here?  You didn't say that
this was a HCD-only flag, right?

> This placeholder should keep anybody from adding driver-agnostic flags
> to that mask.  The BUILD_BUG_ON is intended to make sure that the
> driver private space doesn't move out from under the Tegra-specific
> flag.

I still don't like it, it feels like a hack that is not going to be able
to be maintained very well.

And I still think the individual drivers should be fixed...

thanks,

greg k-h

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

[Oliver Neukum]

Am Samstag, 18. Dezember 2010, 00:09:40 schrieb Greg KH:
> I still don't like it, it feels like a hack that is not going to be able
> to be maintained very well.
> 
> And I still think the individual drivers should be fixed...
> 

But they are not buggy. The USB API was written under the
assumption that HCDs can deal with byte level granularity and alignment.
Hence the network driver pass DMA-able memory with an alignment
that suits them.

	Regards
		Oliver

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

[Greg KH]

On Sat, Dec 18, 2010 at 12:17:40AM +0100, Oliver Neukum wrote:
> Am Samstag, 18. Dezember 2010, 00:09:40 schrieb Greg KH:
> > I still don't like it, it feels like a hack that is not going to be able
> > to be maintained very well.
> > 
> > And I still think the individual drivers should be fixed...
> > 
> 
> But they are not buggy. The USB API was written under the
> assumption that HCDs can deal with byte level granularity and alignment.
> Hence the network driver pass DMA-able memory with an alignment
> that suits them.

Ok, fair enough.

But I still don't like that urb flag stuff as a solution to this...

thanks,

greg k-h

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

[rmorell]

On Fri, Dec 17, 2010 at 03:35:27PM -0800, Greg KH wrote:
> On Sat, Dec 18, 2010 at 12:17:40AM +0100, Oliver Neukum wrote:
> > Am Samstag, 18. Dezember 2010, 00:09:40 schrieb Greg KH:
> > > I still don't like it, it feels like a hack that is not going to be able
> > > to be maintained very well.
> > > 
> > > And I still think the individual drivers should be fixed...
> > > 
> > 
> > But they are not buggy. The USB API was written under the
> > assumption that HCDs can deal with byte level granularity and alignment.
> > Hence the network driver pass DMA-able memory with an alignment
> > that suits them.
> 
> Ok, fair enough.
> 
> But I still don't like that urb flag stuff as a solution to this...

Yeah, I wish I knew of a better way to store this information.
urb->hcpriv seems like a nice place for host controller-private data,
but it's used by the general ehci framework throughout the lifetime of
an URB so isn't suitable for this case.

Thanks,
Robert

> thanks,
> 
> greg k-h
> 

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

[rmorell]

On Fri, Dec 17, 2010 at 03:09:40PM -0800, Greg KH wrote:
> On Fri, Dec 17, 2010 at 02:42:21PM -0800, rmorell@nvidia.com wrote:
> > On Fri, Dec 17, 2010 at 02:35:02PM -0800, Greg KH wrote:
> > > On Fri, Dec 17, 2010 at 01:58:49PM -0800, Robert Morell wrote:
> > > > --- 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?
> > 
> > The intention was to reserve space in the URB flags for
> > implementation-specific use.
> 
> Yes, but _which_ driver are you talking about here?  You didn't say that
> this was a HCD-only flag, right?

Sorry, I may not have been clear.  I did mean HCD-only flag; its exact
meaning depends on the HCD in question.  In the case of tegra-ehci, it
means that a temporary buffer has been allocated that needs freeing
later.

Would it make more sense if I called it URB_HCD_PRIVATE_MASK?

Thanks,
Robert

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

[Greg KH]

On Fri, Dec 17, 2010 at 03:40:08PM -0800, rmorell@nvidia.com wrote:
> On Fri, Dec 17, 2010 at 03:09:40PM -0800, Greg KH wrote:
> > On Fri, Dec 17, 2010 at 02:42:21PM -0800, rmorell@nvidia.com wrote:
> > > On Fri, Dec 17, 2010 at 02:35:02PM -0800, Greg KH wrote:
> > > > On Fri, Dec 17, 2010 at 01:58:49PM -0800, Robert Morell wrote:
> > > > > --- 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?
> > > 
> > > The intention was to reserve space in the URB flags for
> > > implementation-specific use.
> > 
> > Yes, but _which_ driver are you talking about here?  You didn't say that
> > this was a HCD-only flag, right?
> 
> Sorry, I may not have been clear.  I did mean HCD-only flag; its exact
> meaning depends on the HCD in question.  In the case of tegra-ehci, it
> means that a temporary buffer has been allocated that needs freeing
> later.
> 
> Would it make more sense if I called it URB_HCD_PRIVATE_MASK?

How about naming it what it is being used for?

thanks,

greg k-h

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

[rmorell]

On Fri, Dec 17, 2010 at 04:37:47PM -0800, Greg KH wrote:
> On Fri, Dec 17, 2010 at 03:40:08PM -0800, rmorell@nvidia.com wrote:
> > On Fri, Dec 17, 2010 at 03:09:40PM -0800, Greg KH wrote:
> > > On Fri, Dec 17, 2010 at 02:42:21PM -0800, rmorell@nvidia.com wrote:
> > > > On Fri, Dec 17, 2010 at 02:35:02PM -0800, Greg KH wrote:
> > > > > On Fri, Dec 17, 2010 at 01:58:49PM -0800, Robert Morell wrote:
> > > > > > --- 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?
> > > > 
> > > > The intention was to reserve space in the URB flags for
> > > > implementation-specific use.
> > > 
> > > Yes, but _which_ driver are you talking about here?  You didn't say that
> > > this was a HCD-only flag, right?
> > 
> > Sorry, I may not have been clear.  I did mean HCD-only flag; its exact
> > meaning depends on the HCD in question.  In the case of tegra-ehci, it
> > means that a temporary buffer has been allocated that needs freeing
> > later.
> > 
> > Would it make more sense if I called it URB_HCD_PRIVATE_MASK?
> 
> How about naming it what it is being used for?

Sure, I can do that.  I was just trying to avoid polluting the general
USB namespace with HCD-specific constants, but I'll send out a revised
patchset with this change.

Thanks,
Robert

Re: [RFC] Align tegra-ehci DMA transfers to 32B

[Greg KH]

On Fri, Dec 17, 2010 at 01:58:47PM -0800, Robert Morell wrote:
> This small set of patches fixes an issue where DMA from the tegra EHCI
> controller could be corrupted.  It was most commonly seen with USB network
> adapters, though in theory it could happen with any USB traffic.
> 
> (Note: An attempt was made to fix this with commit 367c3aab, which set
> NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.  Unfortunately, not all network
> drivers honor them (presumably since these are intended as optimizations rather
> than hard rules).  This does mean that properly-written network drivers should
> fall through this code with very little overhead, however.)

We don't have many USB network drivers, why not just fix them up to
handle this properly, then you will not need to change any core USB
code, right?

thanks,

greg k-h

Re: [RFC] Align tegra-ehci DMA transfers to 32B

[rmorell]

On Fri, Dec 17, 2010 at 02:32:27PM -0800, Greg KH wrote:
> On Fri, Dec 17, 2010 at 01:58:47PM -0800, Robert Morell wrote:
> > This small set of patches fixes an issue where DMA from the tegra EHCI
> > controller could be corrupted.  It was most commonly seen with USB network
> > adapters, though in theory it could happen with any USB traffic.
> > 
> > (Note: An attempt was made to fix this with commit 367c3aab, which set
> > NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.  Unfortunately, not all network
> > drivers honor them (presumably since these are intended as optimizations rather
> > than hard rules).  This does mean that properly-written network drivers should
> > fall through this code with very little overhead, however.)
> 
> We don't have many USB network drivers, why not just fix them up to
> handle this properly, then you will not need to change any core USB
> code, right?

The USB core code is used by devices other than USB adapters.  We've
only seen this problem so far with usbnet devices, but I can't test
every USB device ever to make sure that they always align their DMA to
32 bytes.

Thanks,
Robert

> thanks,
> 
> greg k-h
> 

Re: [RFC] Align tegra-ehci DMA transfers to 32B

[Greg KH]

On Fri, Dec 17, 2010 at 02:44:29PM -0800, rmorell@nvidia.com wrote:
> On Fri, Dec 17, 2010 at 02:32:27PM -0800, Greg KH wrote:
> > On Fri, Dec 17, 2010 at 01:58:47PM -0800, Robert Morell wrote:
> > > This small set of patches fixes an issue where DMA from the tegra EHCI
> > > controller could be corrupted.  It was most commonly seen with USB network
> > > adapters, though in theory it could happen with any USB traffic.
> > > 
> > > (Note: An attempt was made to fix this with commit 367c3aab, which set
> > > NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.  Unfortunately, not all network
> > > drivers honor them (presumably since these are intended as optimizations rather
> > > than hard rules).  This does mean that properly-written network drivers should
> > > fall through this code with very little overhead, however.)
> > 
> > We don't have many USB network drivers, why not just fix them up to
> > handle this properly, then you will not need to change any core USB
> > code, right?
> 
> The USB core code is used by devices other than USB adapters.  We've
> only seen this problem so far with usbnet devices, but I can't test
> every USB device ever to make sure that they always align their DMA to
> 32 bytes.

Then it might just be easier for your driver to throw up a huge
WARN_ON() if it detects such memory so that the device driver could be
fixed, right?

thanks,

greg k-h

Re: [RFC] Align tegra-ehci DMA transfers to 32B

[David Brownell]

Odd.  This is a functional regression with respect
to previous NVidia EHCI IP; the stuff in your PCI
south bridges behaves normally, as I recall; no
such  alignment restrictions.

[PATCH v2] Align tegra-ehci DMA transfers to 32B

[Robert Morell]

This small set of patches fixes an issue where DMA from the tegra EHCI
controller could be corrupted.  It was most commonly seen with USB network
adapters, though in theory it could happen with any USB traffic.

Note: An attempt was made to fix this specifically for network devices with
commit 367c3aab, which set NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.
Unfortunately, not all network drivers honor them (presumably since these are
intended as optimizations rather than hard rules).  This does mean that
properly behaved network drivers should fall through this code with very little
overhead, however.

Version 2 of this patchset gets rid of the HCD placeholder in the flags and
instead simply reserves the flag globally for this use.

[PATCH] Align tegra-ehci DMA transfers to 32B

[Robert Morell]

This small set of patches fixes an issue where DMA from the tegra EHCI
controller could be corrupted.  It was most commonly seen with USB network
adapters, though in theory it could happen with any USB traffic.

Note: An attempt was made to fix this specifically for network devices with
commit 367c3aab, which set NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.
Unfortunately, not all network drivers honor them (presumably since these are
intended as optimizations rather than hard rules).  This does mean that
properly behaved network drivers should fall through this code with very little
overhead, however.

Version 3 of this patchset makes the hook definition and commit message much
more verbose, and moves the new flag from the second patch to the first.

[PATCH v4] Align tegra-ehci DMA transfers to 32B

[Robert Morell]

This small set of patches fixes an issue where DMA from the tegra EHCI
controller could be corrupted.  It was most commonly seen with USB network
adapters, though in theory it could happen with any USB traffic.

Note: An attempt was made to fix this specifically for network devices with
commit 367c3aab, which set NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.
Unfortunately, not all network drivers honor them (presumably since these are
intended as optimizations rather than hard rules).  This does mean that
properly behaved network drivers should fall through this code with very little
overhead, however.

Version 4 of this patchset adds logic to prevent data copy-out on failure.

[PATCH v5] Align tegra-ehci DMA transfers to 32B

[Robert Morell]

This small set of patches fixes an issue where DMA from the tegra EHCI
controller could be corrupted.  It was most commonly seen with USB network
adapters, though in theory it could happen with any USB traffic.

Note: An attempt was made to fix this specifically for network devices with
commit 367c3aab, which set NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.
Unfortunately, not all network drivers honor them (presumably since these are
intended as optimizations rather than hard rules).  This does mean that
properly behaved network drivers should fall through this code with very little
overhead, however.

Version 2 of this patchset gets rid of the HCD placeholder in the flags and
instead simply reserves the flag globally for this use.

Version 3 makes the hook definition and commit message much more verbose, and
moves the new flag from the second patch to the first.

Version 4 adds logic to prevent data copy-out on failure.

Version 5 rebases against linux-tegra-2.6.37.

[PATCH v6] Align tegra-ehci DMA transfers to 32B

[Robert Morell]

This small set of patches fixes an issue where DMA from the tegra EHCI
controller could be corrupted.  It was most commonly seen with USB network
adapters, though in theory it could happen with any USB traffic.

Note: An attempt was made to fix this specifically for network devices with
commit 367c3aab, which set NET_IP_ALIGN to 0 and NET_SKB_PAD to 32.
Unfortunately, not all network drivers honor them (presumably since these are
intended as optimizations rather than hard rules).  This does mean that
properly behaved network drivers should fall through this code with very little
overhead, however.

Version 2 of this patchset gets rid of the HCD placeholder in the flags and
instead simply reserves the flag globally for this use.

Version 3 makes the hook definition and commit message much more verbose, and
moves the new flag from the second patch to the first.

Version 4 adds logic to prevent data copy-out on failure.

Version 5 rebases against linux-tegra-2.6.37.

Version 6 renames the map/unmap exported functions from the hcd core to have
usb_hcd prefixes as an additional patch.  It also fixes some comment
formatting, and removes the logic added in version 4 to skip the memcpy from
the temp buffer on failure (some data may still be good).

[PATCH 1/3] USB: HCD: Add usb_hcd prefix to exported functions

[Robert Morell]

The convention is to prefix symbols exported from the USB HCD core with
"usb_hcd".  This change makes unmap_urb_setup_for_dma() and
unmap_urb_for_dma() consistent with that.

Signed-off-by: Robert Morell <rmorell@nvidia.com>
---
 drivers/usb/core/hcd.c       |   16 ++++++++--------
 drivers/usb/host/imx21-hcd.c |    5 +++--
 drivers/usb/musb/musb_host.c |    4 ++--
 include/linux/usb/hcd.h      |    4 ++--
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index ced846a..cd7150a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1263,7 +1263,7 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
 	*dma_handle = 0;
 }
 
-void unmap_urb_setup_for_dma(struct usb_hcd *hcd, struct urb *urb)
+void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
 	if (urb->transfer_flags & URB_SETUP_MAP_SINGLE)
 		dma_unmap_single(hcd->self.controller,
@@ -1280,13 +1280,13 @@ void unmap_urb_setup_for_dma(struct usb_hcd *hcd, struct urb *urb)
 	/* Make it safe to call this routine more than once */
 	urb->transfer_flags &= ~(URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL);
 }
-EXPORT_SYMBOL_GPL(unmap_urb_setup_for_dma);
+EXPORT_SYMBOL_GPL(usb_hcd_unmap_urb_setup_for_dma);
 
-void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
 	enum dma_data_direction dir;
 
-	unmap_urb_setup_for_dma(hcd, urb);
+	usb_hcd_unmap_urb_setup_for_dma(hcd, urb);
 
 	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 	if (urb->transfer_flags & URB_DMA_MAP_SG)
@@ -1315,7 +1315,7 @@ void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 	urb->transfer_flags &= ~(URB_DMA_MAP_SG | URB_DMA_MAP_PAGE |
 			URB_DMA_MAP_SINGLE | URB_MAP_LOCAL);
 }
-EXPORT_SYMBOL_GPL(unmap_urb_for_dma);
+EXPORT_SYMBOL_GPL(usb_hcd_unmap_urb_for_dma);
 
 static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			   gfp_t mem_flags)
@@ -1411,7 +1411,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 		}
 		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
 				URB_SETUP_MAP_LOCAL)))
-			unmap_urb_for_dma(hcd, urb);
+			usb_hcd_unmap_urb_for_dma(hcd, urb);
 	}
 	return ret;
 }
@@ -1452,7 +1452,7 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
 		if (likely(status == 0)) {
 			status = hcd->driver->urb_enqueue(hcd, urb, mem_flags);
 			if (unlikely(status))
-				unmap_urb_for_dma(hcd, urb);
+				usb_hcd_unmap_urb_for_dma(hcd, urb);
 		}
 	}
 
@@ -1558,7 +1558,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 			!status))
 		status = -EREMOTEIO;
 
-	unmap_urb_for_dma(hcd, urb);
+	usb_hcd_unmap_urb_for_dma(hcd, urb);
 	usbmon_urb_complete(&hcd->self, urb, status);
 	usb_unanchor_urb(urb);
 
diff --git a/drivers/usb/host/imx21-hcd.c b/drivers/usb/host/imx21-hcd.c
index e49b75a..f8ba23c 100644
--- a/drivers/usb/host/imx21-hcd.c
+++ b/drivers/usb/host/imx21-hcd.c
@@ -927,7 +927,8 @@ static void schedule_nonisoc_etd(struct imx21 *imx21, struct urb *urb)
 		if (state == US_CTRL_SETUP) {
 			dir = TD_DIR_SETUP;
 			if (unsuitable_for_dma(urb->setup_dma))
-				unmap_urb_setup_for_dma(imx21->hcd, urb);
+				usb_hcd_unmap_urb_setup_for_dma(imx21->hcd,
+					urb);
 			etd->dma_handle = urb->setup_dma;
 			etd->cpu_buffer = urb->setup_packet;
 			bufround = 0;
@@ -943,7 +944,7 @@ static void schedule_nonisoc_etd(struct imx21 *imx21, struct urb *urb)
 		dir = usb_pipeout(pipe) ? TD_DIR_OUT : TD_DIR_IN;
 		bufround = (dir == TD_DIR_IN) ? 1 : 0;
 		if (unsuitable_for_dma(urb->transfer_dma))
-			unmap_urb_for_dma(imx21->hcd, urb);
+			usb_hcd_unmap_urb_for_dma(imx21->hcd, urb);
 
 		etd->dma_handle = urb->transfer_dma;
 		etd->cpu_buffer = urb->transfer_buffer;
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 4d5bcb4..4d2f3f7 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1336,7 +1336,7 @@ void musb_host_tx(struct musb *musb, u8 epnum)
 	if (length > qh->maxpacket)
 		length = qh->maxpacket;
 	/* Unmap the buffer so that CPU can use it */
-	unmap_urb_for_dma(musb_to_hcd(musb), urb);
+	usb_hcd_unmap_urb_for_dma(musb_to_hcd(musb), urb);
 	musb_write_fifo(hw_ep, length, urb->transfer_buffer + offset);
 	qh->segsize = length;
 
@@ -1758,7 +1758,7 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 
 		if (!dma) {
 			/* Unmap the buffer so that CPU can use it */
-			unmap_urb_for_dma(musb_to_hcd(musb), urb);
+			usb_hcd_unmap_urb_for_dma(musb_to_hcd(musb), urb);
 			done = musb_host_packet_rx(musb, urb,
 					epnum, iso_err);
 			DBG(6, "read %spacket\n", done ? "last " : "");
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 0b6e751..5cee57f 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -329,8 +329,8 @@ extern int usb_hcd_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb(struct urb *urb, int status);
 extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
-extern void unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *);
-extern void unmap_urb_for_dma(struct usb_hcd *, struct urb *);
+extern void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *);
+extern void usb_hcd_unmap_urb_for_dma(struct usb_hcd *, struct urb *);
 extern void usb_hcd_flush_endpoint(struct usb_device *udev,
 		struct usb_host_endpoint *ep);
 extern void usb_hcd_disable_endpoint(struct usb_device *udev,
-- 
1.7.3.4

Re: [PATCH 1/3] USB: HCD: Add usb_hcd prefix to exported functions

[Alan Stern]

On Wed, 26 Jan 2011, Robert Morell wrote:

> The convention is to prefix symbols exported from the USB HCD core with
> "usb_hcd".  This change makes unmap_urb_setup_for_dma() and
> unmap_urb_for_dma() consistent with that.
> 
> Signed-off-by: Robert Morell <rmorell@nvidia.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>

[PATCH 2/3] USB: HCD: Add driver hooks for (un)?map_urb_for_dma

[Robert Morell]

Provide optional hooks for the host controller driver to override the
default DMA mapping and unmapping routines.  In general, these shouldn't
be necessary unless the host controller has special DMA requirements,
such as alignment contraints.  If these are not specified, the
general usb_hcd_(un)?map_urb_for_dma functions will be used instead.
Also, pass the status to unmap_urb_for_dma so it can know whether the
DMA buffer has been overwritten.

Finally, add a flag to be used by these implementations if they
allocated a temporary buffer so it can be freed properly when unmapping.

Signed-off-by: Robert Morell <rmorell@nvidia.com>
---
 drivers/usb/core/hcd.c  |   22 ++++++++++++++++++++--
 include/linux/usb.h     |    1 +
 include/linux/usb/hcd.h |   15 +++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index cd7150a..6ed174b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1282,6 +1282,14 @@ void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *hcd, struct urb *urb)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_unmap_urb_setup_for_dma);
 
+static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+	if (hcd->driver->unmap_urb_for_dma)
+		hcd->driver->unmap_urb_for_dma(hcd, urb);
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
 void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
 	enum dma_data_direction dir;
@@ -1320,6 +1328,15 @@ EXPORT_SYMBOL_GPL(usb_hcd_unmap_urb_for_dma);
 static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			   gfp_t mem_flags)
 {
+	if (hcd->driver->map_urb_for_dma)
+		return hcd->driver->map_urb_for_dma(hcd, urb, mem_flags);
+	else
+		return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
+int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+			    gfp_t mem_flags)
+{
 	enum dma_data_direction dir;
 	int ret = 0;
 
@@ -1415,6 +1432,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 	}
 	return ret;
 }
+EXPORT_SYMBOL_GPL(usb_hcd_map_urb_for_dma);
 
 /*-------------------------------------------------------------------------*/
 
@@ -1452,7 +1470,7 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
 		if (likely(status == 0)) {
 			status = hcd->driver->urb_enqueue(hcd, urb, mem_flags);
 			if (unlikely(status))
-				usb_hcd_unmap_urb_for_dma(hcd, urb);
+				unmap_urb_for_dma(hcd, urb);
 		}
 	}
 
@@ -1558,7 +1576,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 			!status))
 		status = -EREMOTEIO;
 
-	usb_hcd_unmap_urb_for_dma(hcd, urb);
+	unmap_urb_for_dma(hcd, urb);
 	usbmon_urb_complete(&hcd->self, urb, status);
 	usb_unanchor_urb(urb);
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a28eb25..7a957f2 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -979,6 +979,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_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 5cee57f..d825816 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -233,6 +233,19 @@ struct hc_driver {
 	int	(*urb_dequeue)(struct usb_hcd *hcd,
 				struct urb *urb, int status);
 
+	/*
+	 * (optional) these hooks allow an HCD to override the default DMA
+	 * mapping and unmapping routines.  In general, they shouldn't be
+	 * necessary unless the host controller has special DMA requirements,
+	 * such as alignment contraints.  If these are not specified, the
+	 * general usb_hcd_(un)?map_urb_for_dma functions will be used instead
+	 * (and it may be a good idea to call these functions in your HCD
+	 * implementation)
+	 */
+	int	(*map_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				   gfp_t mem_flags);
+	void    (*unmap_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb);
+
 	/* hw synch, freeing endpoint resources that urb_dequeue can't */
 	void	(*endpoint_disable)(struct usb_hcd *hcd,
 			struct usb_host_endpoint *ep);
@@ -329,6 +342,8 @@ extern int usb_hcd_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb(struct urb *urb, int status);
 extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
+extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+		gfp_t mem_flags);
 extern void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *);
 extern void usb_hcd_unmap_urb_for_dma(struct usb_hcd *, struct urb *);
 extern void usb_hcd_flush_endpoint(struct usb_device *udev,
-- 
1.7.3.4

Re: [PATCH 2/3] USB: HCD: Add driver hooks for (un)?map_urb_for_dma

[Alan Stern]

On Wed, 26 Jan 2011, Robert Morell wrote:

> Provide optional hooks for the host controller driver to override the
> default DMA mapping and unmapping routines.  In general, these shouldn't
> be necessary unless the host controller has special DMA requirements,
> such as alignment contraints.  If these are not specified, the
> general usb_hcd_(un)?map_urb_for_dma functions will be used instead.
> Also, pass the status to unmap_urb_for_dma so it can know whether the
> DMA buffer has been overwritten.
> 
> Finally, add a flag to be used by these implementations if they
> allocated a temporary buffer so it can be freed properly when unmapping.
> 
> Signed-off-by: Robert Morell <rmorell@nvidia.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>

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

[Robert Morell]

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>
---
 drivers/usb/host/ehci-tegra.c |   90 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 2341904..cfb608e 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -32,6 +32,8 @@
 #define TEGRA_USB_USBMODE_HOST			(3 << 0)
 #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
 
+#define TEGRA_USB_DMA_ALIGN 32
+
 struct tegra_ehci_context {
 	bool valid;
 	u32 command;
@@ -461,6 +463,92 @@ 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;
+
+	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;
+
+	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",
@@ -476,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,
-- 
1.7.3.4

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

[Greg KH]

On Wed, Jan 26, 2011 at 07:06: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>
> ---
>  drivers/usb/host/ehci-tegra.c |   90 +++++++++++++++++++++++++++++++++++++++++

This file doesn't seem to be in any tree that I can find, including my
own, so I can't apply this patch.

What am I supposed to do with it?

confused,

greg k-h

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

[Olof Johansson]

On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Jan 26, 2011 at 07:06: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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/usb/host/ehci-tegra.c |   90 +++++++++++++++++++++++++++++++++++++++++
>
> This file doesn't seem to be in any tree that I can find, including my
> own, so I can't apply this patch.
>
> What am I supposed to do with it?

It hasn't been posted for upstream yet, so nothing for you to do. The
driver will be posted for review soon, hopefully in time for .39.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Olof Johansson]

On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg@kroah.com> wrote:
> On Wed, Jan 26, 2011 at 07:06: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>
>> ---
>>  drivers/usb/host/ehci-tegra.c |   90 +++++++++++++++++++++++++++++++++++++++++
>
> This file doesn't seem to be in any tree that I can find, including my
> own, so I can't apply this patch.
>
> What am I supposed to do with it?

It hasn't been posted for upstream yet, so nothing for you to do. The
driver will be posted for review soon, hopefully in time for .39.


-Olof

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

[Greg KH]

On Fri, Feb 04, 2011 at 12:14:54PM -0800, Olof Johansson wrote:
> On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> > On Wed, Jan 26, 2011 at 07:06: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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  drivers/usb/host/ehci-tegra.c |   90 +++++++++++++++++++++++++++++++++++++++++
> >
> > This file doesn't seem to be in any tree that I can find, including my
> > own, so I can't apply this patch.
> >
> > What am I supposed to do with it?
> 
> It hasn't been posted for upstream yet, so nothing for you to do. The
> driver will be posted for review soon, hopefully in time for .39.

Then why would someone send me a patch for it already?

Still confused,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg KH]

On Fri, Feb 04, 2011 at 12:14:54PM -0800, Olof Johansson wrote:
> On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg@kroah.com> wrote:
> > On Wed, Jan 26, 2011 at 07:06: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>
> >> ---
> >>  drivers/usb/host/ehci-tegra.c |   90 +++++++++++++++++++++++++++++++++++++++++
> >
> > This file doesn't seem to be in any tree that I can find, including my
> > own, so I can't apply this patch.
> >
> > What am I supposed to do with it?
> 
> It hasn't been posted for upstream yet, so nothing for you to do. The
> driver will be posted for review soon, hopefully in time for .39.

Then why would someone send me a patch for it already?

Still confused,

greg k-h

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

[rmorell-DDmLM1+adcrQT0dZR+AlfA]

On Fri, Feb 04, 2011 at 12:16:16PM -0800, Greg KH wrote:
> On Fri, Feb 04, 2011 at 12:14:54PM -0800, Olof Johansson wrote:
> > On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> > > This file doesn't seem to be in any tree that I can find, including my
> > > own, so I can't apply this patch.
> > >
> > > What am I supposed to do with it?
> > 
> > It hasn't been posted for upstream yet, so nothing for you to do. The
> > driver will be posted for review soon, hopefully in time for .39.
> 
> Then why would someone send me a patch for it already?

Sorry, this is my fault.  My immediate need is to get this merged into
our release linux-tegra-2.6.36 branch, but wanted to get broad review
for the change since it affects USB core code.  I figured that it will
be easier to push the full driver into the USB tree with this part out
of the way already, anyway.

Thanks,
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[rmorell]

On Fri, Feb 04, 2011 at 12:16:16PM -0800, Greg KH wrote:
> On Fri, Feb 04, 2011 at 12:14:54PM -0800, Olof Johansson wrote:
> > On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg@kroah.com> wrote:
> > > This file doesn't seem to be in any tree that I can find, including my
> > > own, so I can't apply this patch.
> > >
> > > What am I supposed to do with it?
> > 
> > It hasn't been posted for upstream yet, so nothing for you to do. The
> > driver will be posted for review soon, hopefully in time for .39.
> 
> Then why would someone send me a patch for it already?

Sorry, this is my fault.  My immediate need is to get this merged into
our release linux-tegra-2.6.36 branch, but wanted to get broad review
for the change since it affects USB core code.  I figured that it will
be easier to push the full driver into the USB tree with this part out
of the way already, anyway.

Thanks,
Robert

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

[Greg KH]

On Fri, Feb 04, 2011 at 12:26:40PM -0800, rmorell-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org wrote:
> On Fri, Feb 04, 2011 at 12:16:16PM -0800, Greg KH wrote:
> > On Fri, Feb 04, 2011 at 12:14:54PM -0800, Olof Johansson wrote:
> > > On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> > > > This file doesn't seem to be in any tree that I can find, including my
> > > > own, so I can't apply this patch.
> > > >
> > > > What am I supposed to do with it?
> > > 
> > > It hasn't been posted for upstream yet, so nothing for you to do. The
> > > driver will be posted for review soon, hopefully in time for .39.
> > 
> > Then why would someone send me a patch for it already?
> 
> Sorry, this is my fault.  My immediate need is to get this merged into
> our release linux-tegra-2.6.36 branch, but wanted to get broad review
> for the change since it affects USB core code.  I figured that it will
> be easier to push the full driver into the USB tree with this part out
> of the way already, anyway.

Ok, that makes more sense, but next time, please say something,
as I wasted a lot of time trying to figure out why this patch wasn't
applying correctly :(

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg KH]

On Fri, Feb 04, 2011 at 12:26:40PM -0800, rmorell@nvidia.com wrote:
> On Fri, Feb 04, 2011 at 12:16:16PM -0800, Greg KH wrote:
> > On Fri, Feb 04, 2011 at 12:14:54PM -0800, Olof Johansson wrote:
> > > On Fri, Feb 4, 2011 at 11:49 AM, Greg KH <greg@kroah.com> wrote:
> > > > This file doesn't seem to be in any tree that I can find, including my
> > > > own, so I can't apply this patch.
> > > >
> > > > What am I supposed to do with it?
> > > 
> > > It hasn't been posted for upstream yet, so nothing for you to do. The
> > > driver will be posted for review soon, hopefully in time for .39.
> > 
> > Then why would someone send me a patch for it already?
> 
> Sorry, this is my fault.  My immediate need is to get this merged into
> our release linux-tegra-2.6.36 branch, but wanted to get broad review
> for the change since it affects USB core code.  I figured that it will
> be easier to push the full driver into the USB tree with this part out
> of the way already, anyway.

Ok, that makes more sense, but next time, please say something,
as I wasted a lot of time trying to figure out why this patch wasn't
applying correctly :(

[PATCH 1/2] USB: HCD: Add driver hooks for (un)?map_urb_for_dma

[Robert Morell]

Provide optional hooks for the host controller driver to override the
default DMA mapping and unmapping routines.  In general, these shouldn't
be necessary unless the host controller has special DMA requirements,
such as alignment contraints.  If these are not specified, the
general usb_hcd_(un)?map_urb_for_dma functions will be used instead.
Also, pass the status to unmap_urb_for_dma so it can know whether the
DMA buffer has been overwritten.

Finally, add a flag to be used by these implementations if they
allocated a temporary buffer so it can be freed properly when unmapping.

Signed-off-by: Robert Morell <rmorell@nvidia.com>
---

The original patches renamed the exported symbols from
(un)?map_urb_setup_for_dma to usb_hcd_(un)?map_urb_setup_for_dma, and continued
to use the ones not prefixed by "usb_hcd" internally.  However, due to commit
1dae423dd9b which already exported unmap_urb_setup_for_dma as-is, I reworked
this to export both unchanged and switch to the hooked_ prefix internally.

 drivers/usb/core/hcd.c  |   29 ++++++++++++++++++++++++-----
 include/linux/usb.h     |    1 +
 include/linux/usb/hcd.h |   14 ++++++++++++++
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index ced846a..6fe339d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1282,6 +1282,15 @@ void unmap_urb_setup_for_dma(struct usb_hcd *hcd, struct urb *urb)
 }
 EXPORT_SYMBOL_GPL(unmap_urb_setup_for_dma);
 
+static void hooked_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+				     int status)
+{
+	if (hcd->driver->unmap_urb_for_dma)
+		hcd->driver->unmap_urb_for_dma(hcd, urb, status);
+	else
+		unmap_urb_for_dma(hcd, urb);
+}
+
 void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
 	enum dma_data_direction dir;
@@ -1317,8 +1326,17 @@ void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 }
 EXPORT_SYMBOL_GPL(unmap_urb_for_dma);
 
-static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
-			   gfp_t mem_flags)
+static int hooked_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+				  gfp_t mem_flags)
+{
+	if (hcd->driver->map_urb_for_dma)
+		return hcd->driver->map_urb_for_dma(hcd, urb, mem_flags);
+	else
+		return map_urb_for_dma(hcd, urb, mem_flags);
+}
+
+int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+		    gfp_t mem_flags)
 {
 	enum dma_data_direction dir;
 	int ret = 0;
@@ -1415,6 +1433,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 	}
 	return ret;
 }
+EXPORT_SYMBOL_GPL(map_urb_for_dma);
 
 /*-------------------------------------------------------------------------*/
 
@@ -1448,11 +1467,11 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
 	if (is_root_hub(urb->dev)) {
 		status = rh_urb_enqueue(hcd, urb);
 	} else {
-		status = map_urb_for_dma(hcd, urb, mem_flags);
+		status = hooked_map_urb_for_dma(hcd, urb, mem_flags);
 		if (likely(status == 0)) {
 			status = hcd->driver->urb_enqueue(hcd, urb, mem_flags);
 			if (unlikely(status))
-				unmap_urb_for_dma(hcd, urb);
+				hooked_unmap_urb_for_dma(hcd, urb, status);
 		}
 	}
 
@@ -1558,7 +1577,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 			!status))
 		status = -EREMOTEIO;
 
-	unmap_urb_for_dma(hcd, urb);
+	hooked_unmap_urb_for_dma(hcd, urb, status);
 	usbmon_urb_complete(&hcd->self, urb, status);
 	usb_unanchor_urb(urb);
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a28eb25..7a957f2 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -979,6 +979,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_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 0b6e751..dc487ff 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -233,6 +233,18 @@ struct hc_driver {
 	int	(*urb_dequeue)(struct usb_hcd *hcd,
 				struct urb *urb, int status);
 
+	/* (optional) these hooks allow an HCD to override the default DMA
+	 * mapping and unmapping routines.  In general, they shouldn't be
+	 * necessary unless the host controller has special DMA requirements,
+	 * such as alignment contraints.  If these are not specified, the
+         * general (un)?map_urb_for_dma functions will be used instead (and it
+         * may be a good idea to call these functions in your HCD
+         * implementation) */
+	int	(*map_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				   gfp_t mem_flags);
+	void    (*unmap_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				     int status);
+
 	/* hw synch, freeing endpoint resources that urb_dequeue can't */
 	void	(*endpoint_disable)(struct usb_hcd *hcd,
 			struct usb_host_endpoint *ep);
@@ -329,6 +341,8 @@ extern int usb_hcd_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb(struct urb *urb, int status);
 extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
+extern int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+		gfp_t mem_flags);
 extern void unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *);
 extern void unmap_urb_for_dma(struct usb_hcd *, struct urb *);
 extern void usb_hcd_flush_endpoint(struct usb_device *udev,
-- 
1.7.3.4

Re: [PATCH 1/2] USB: HCD: Add driver hooks for (un)?map_urb_for_dma

[Greg KH]

On Thu, Jan 20, 2011 at 01:41:54PM -0800, Robert Morell wrote:
> Provide optional hooks for the host controller driver to override the
> default DMA mapping and unmapping routines.  In general, these shouldn't
> be necessary unless the host controller has special DMA requirements,
> such as alignment contraints.  If these are not specified, the
> general usb_hcd_(un)?map_urb_for_dma functions will be used instead.
> Also, pass the status to unmap_urb_for_dma so it can know whether the
> DMA buffer has been overwritten.
> 
> Finally, add a flag to be used by these implementations if they
> allocated a temporary buffer so it can be freed properly when unmapping.
> 
> Signed-off-by: Robert Morell <rmorell@nvidia.com>

I'd like to get an ACK from Alan before applying this one, and ideally
also from David and Sarah as they know the HCD code also.

Anyone?

thanks,

greg k-h

Re: [PATCH 1/2] USB: HCD: Add driver hooks for (un)?map_urb_for_dma

[Alan Stern]

On Thu, 20 Jan 2011, Robert Morell wrote:

> Provide optional hooks for the host controller driver to override the
> default DMA mapping and unmapping routines.  In general, these shouldn't
> be necessary unless the host controller has special DMA requirements,
> such as alignment contraints.  If these are not specified, the
> general usb_hcd_(un)?map_urb_for_dma functions will be used instead.
> Also, pass the status to unmap_urb_for_dma so it can know whether the
> DMA buffer has been overwritten.
> 
> Finally, add a flag to be used by these implementations if they
> allocated a temporary buffer so it can be freed properly when unmapping.

Overall this looks fine.

> Signed-off-by: Robert Morell <rmorell@nvidia.com>
> ---
> 
> The original patches renamed the exported symbols from
> (un)?map_urb_setup_for_dma to usb_hcd_(un)?map_urb_setup_for_dma, and continued
> to use the ones not prefixed by "usb_hcd" internally.  However, due to commit
> 1dae423dd9b which already exported unmap_urb_setup_for_dma as-is, I reworked
> this to export both unchanged and switch to the hooked_ prefix internally.

In fact, it would be preferable to have the "usb_" or "usb_hcd_" prefix
on exported symbols.  Can you write a third patch to go before these
two, which changes the existing usages?  I think there aren't too many.

> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 0b6e751..dc487ff 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -233,6 +233,18 @@ struct hc_driver {
>  	int	(*urb_dequeue)(struct usb_hcd *hcd,
>  				struct urb *urb, int status);
>  
> +	/* (optional) these hooks allow an HCD to override the default DMA
> +	 * mapping and unmapping routines.  In general, they shouldn't be
> +	 * necessary unless the host controller has special DMA requirements,
> +	 * such as alignment contraints.  If these are not specified, the
> +         * general (un)?map_urb_for_dma functions will be used instead (and it
> +         * may be a good idea to call these functions in your HCD
> +         * implementation) */

Leading tabs vs. spaces.  Also, the style rules (not always rigorously
followed, but we may as well try) require the /* and */ to be on their
own lines.

Alan Stern

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

[Robert Morell]

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>
---
 drivers/usb/host/ehci-tegra.c |   92 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 2341904..7cdfc65 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -32,6 +32,8 @@
 #define TEGRA_USB_USBMODE_HOST			(3 << 0)
 #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
 
+#define TEGRA_USB_DMA_ALIGN 32
+
 struct tegra_ehci_context {
 	bool valid;
 	u32 command;
@@ -461,6 +463,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, int status)
+{
+	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 && !status)
+		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;
+
+	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 = map_urb_for_dma(hcd, urb, mem_flags);
+	if (ret)
+		free_temp_buffer(urb, ret);
+
+	return ret;
+}
+
+static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+					 int status)
+{
+	unmap_urb_for_dma(hcd, urb);
+	free_temp_buffer(urb, status);
+}
+
 static const struct hc_driver tegra_ehci_hc_driver = {
 	.description		= hcd_name,
 	.product_desc		= "Tegra EHCI Host Controller",
@@ -476,6 +566,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,
-- 
1.7.3.4

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

[Alan Stern]

On Thu, 20 Jan 2011, 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>
> ---
>  drivers/usb/host/ehci-tegra.c |   92 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 92 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 2341904..7cdfc65 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -32,6 +32,8 @@
>  #define TEGRA_USB_USBMODE_HOST			(3 << 0)
>  #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
>  
> +#define TEGRA_USB_DMA_ALIGN 32
> +
>  struct tegra_ehci_context {
>  	bool valid;
>  	u32 command;
> @@ -461,6 +463,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, int status)
> +{
> +	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 && !status)
> +		memcpy(temp->old_xfer_buffer, temp->data,
> +			urb->transfer_buffer_length);

Even if status is nonzero, there may be valid data in the buffer.  You 
should skip that test.

No other problems that I can see.

Alan Stern

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

[rmorell]

On Mon, Jan 24, 2011 at 08:36:55AM -0800, Alan Stern wrote:
> On Thu, 20 Jan 2011, 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>
> > ---
> >  drivers/usb/host/ehci-tegra.c |   92 +++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 92 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> > index 2341904..7cdfc65 100644
> > --- a/drivers/usb/host/ehci-tegra.c
> > +++ b/drivers/usb/host/ehci-tegra.c
> > @@ -32,6 +32,8 @@
> >  #define TEGRA_USB_USBMODE_HOST			(3 << 0)
> >  #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
> >  
> > +#define TEGRA_USB_DMA_ALIGN 32
> > +
> >  struct tegra_ehci_context {
> >  	bool valid;
> >  	u32 command;
> > @@ -461,6 +463,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, int status)
> > +{
> > +	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 && !status)
> > +		memcpy(temp->old_xfer_buffer, temp->data,
> > +			urb->transfer_buffer_length);
> 
> Even if status is nonzero, there may be valid data in the buffer.  You 
> should skip that test.

Thanks for looking, Alan.  I added that test based on earlier feedback.
I think the big concern here is security: if the URB fails in such a way
that the buffer is not overwritten, then we may copy out freed kernel
data to userspace.

Are there specific status codes that I can check for here?  I guess the
only other option is to remove the direction check from the alloc path
or alloc with GFP_ZERO.

Thanks,
Robert

> No other problems that I can see.
> 
> Alan Stern
> 
> 

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

[Alan Stern]

On Mon, 24 Jan 2011 rmorell@nvidia.com wrote:

> > > +	if (dir == DMA_FROM_DEVICE && !status)
> > > +		memcpy(temp->old_xfer_buffer, temp->data,
> > > +			urb->transfer_buffer_length);
> > 
> > Even if status is nonzero, there may be valid data in the buffer.  You 
> > should skip that test.
> 
> Thanks for looking, Alan.  I added that test based on earlier feedback.
> I think the big concern here is security: if the URB fails in such a way
> that the buffer is not overwritten, then we may copy out freed kernel
> data to userspace.

Not to userspace, only to the driver that submitted the URB.  That 
driver must be part of the kernel.  For URBs that _do_ originate in 
userspace, by way of usbfs, the usbfs code is responsible for not 
leaking any kernel data.

> Are there specific status codes that I can check for here?

No.

>  I guess the
> only other option is to remove the direction check from the alloc path
> or alloc with GFP_ZERO.

You don't have to worry about this; it isn't a security concern.

Alan Stern

[PATCH v4 1/2] USB: HCD: Add driver hooks for (un)?map_urb_for_dma

[Robert Morell]

Provide optional hooks for the host controller driver to override the
default DMA mapping and unmapping routines.  In general, these shouldn't
be necessary unless the host controller has special DMA requirements,
such as alignment contraints.  If these are not specified, the
general usb_hcd_(un)?map_urb_for_dma functions will be used instead.
Also, pass the status to unmap_urb_for_dma so it can know whether the
DMA buffer has been overwritten.

Finally, add a flag to be used by these implementations if they
allocated a temporary buffer so it can be freed properly when unmapping.

Signed-off-by: Robert Morell <rmorell@nvidia.com>
---
 drivers/usb/core/hcd.c  |   25 +++++++++++++++++++++----
 include/linux/usb.h     |    1 +
 include/linux/usb/hcd.h |   15 +++++++++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5cca00a..a1dd23e 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1263,7 +1263,15 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
 	*dma_handle = 0;
 }
 
-static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, int status)
+{
+	if (hcd->driver->unmap_urb_for_dma)
+		hcd->driver->unmap_urb_for_dma(hcd, urb, status);
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
+void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
 	enum dma_data_direction dir;
 
@@ -1311,6 +1319,15 @@ static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			   gfp_t mem_flags)
 {
+	if (hcd->driver->map_urb_for_dma)
+		return hcd->driver->map_urb_for_dma(hcd, urb, mem_flags);
+	else
+		return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
+int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+			    gfp_t mem_flags)
+{
 	enum dma_data_direction dir;
 	int ret = 0;
 
@@ -1400,7 +1417,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 		}
 		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
 				URB_SETUP_MAP_LOCAL)))
-			unmap_urb_for_dma(hcd, urb);
+			usb_hcd_unmap_urb_for_dma(hcd, urb);
 	}
 	return ret;
 }
@@ -1441,7 +1458,7 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
 		if (likely(status == 0)) {
 			status = hcd->driver->urb_enqueue(hcd, urb, mem_flags);
 			if (unlikely(status))
-				unmap_urb_for_dma(hcd, urb);
+				unmap_urb_for_dma(hcd, urb, status);
 		}
 	}
 
@@ -1547,7 +1564,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 			!status))
 		status = -EREMOTEIO;
 
-	unmap_urb_for_dma(hcd, urb);
+	unmap_urb_for_dma(hcd, urb, status);
 	usbmon_urb_complete(&hcd->self, urb, status);
 	usb_unanchor_urb(urb);
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 35fe6ab..6d4566e 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_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 3b571f1..fb3321d 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -233,6 +233,18 @@ struct hc_driver {
 	int	(*urb_dequeue)(struct usb_hcd *hcd,
 				struct urb *urb, int status);
 
+	/* (optional) these hooks allow an HCD to override the default DMA
+	 * mapping and unmapping routines.  In general, they shouldn't be
+	 * necessary unless the host controller has special DMA requirements,
+	 * such as alignment contraints.  If these are not specified, the
+	 * general usb_hcd_(un)?map_urb_for_dma functions will be used instead
+	 * (and it may be a good idea to call these functions in your HCD
+	 * implementation) */
+	int	(*map_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				   gfp_t mem_flags);
+	void    (*unmap_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				     int status);
+
 	/* hw synch, freeing endpoint resources that urb_dequeue can't */
 	void	(*endpoint_disable)(struct usb_hcd *hcd,
 			struct usb_host_endpoint *ep);
@@ -327,6 +339,9 @@ extern void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb);
 
 extern int usb_hcd_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb(struct urb *urb, int status);
+extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+		gfp_t mem_flags);
+extern void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb);
 extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
 extern void usb_hcd_flush_endpoint(struct usb_device *udev,
-- 
1.7.2.2

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

[Robert Morell]

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>
---
 drivers/usb/host/ehci-tegra.c |   92 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 21c58df..4aa484b 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -32,6 +32,8 @@
 #define TEGRA_USB_USBMODE_HOST			(3 << 0)
 #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
 
+#define TEGRA_USB_DMA_ALIGN 32
+
 struct tegra_ehci_context {
 	bool valid;
 	u32 command;
@@ -456,6 +458,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, int status)
+{
+	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 && !status)
+		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;
+
+	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, ret);
+
+	return ret;
+}
+
+static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+					 int status)
+{
+	usb_hcd_unmap_urb_for_dma(hcd, urb);
+	free_temp_buffer(urb, status);
+}
+
 static const struct hc_driver tegra_ehci_hc_driver = {
 	.description		= hcd_name,
 	.product_desc		= "Tegra EHCI Host Controller",
@@ -471,6 +561,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,
-- 
1.7.2.2

[PATCH 1/2] USB: Add driver hooks for (un)?map_urb_for_dma

[Robert Morell]

Provide optional hooks for the host controller driver to override the
default DMA mapping and unmapping routines.  In general, these shouldn't
be necessary unless the host controller has special DMA requirements,
such as alignment contraints.  If these are not specified, the
general usb_hcd_(un)?map_urb_for_dma functions will be used instead.

Also add a flag to be used by these implementations if they allocated
a temporary buffer so it can be freed properly when unmapping.

Signed-off-by: Robert Morell <rmorell@nvidia.com>
---
 drivers/usb/core/hcd.c  |   19 ++++++++++++++++++-
 include/linux/usb.h     |    1 +
 include/linux/usb/hcd.h |   14 ++++++++++++++
 3 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5cca00a..84fee0f 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1265,6 +1265,14 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
 
 static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
+	if (hcd->driver->unmap_urb_for_dma)
+		hcd->driver->unmap_urb_for_dma(hcd, urb);
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
+void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
 	enum dma_data_direction dir;
 
 	if (urb->transfer_flags & URB_SETUP_MAP_SINGLE)
@@ -1311,6 +1319,15 @@ static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			   gfp_t mem_flags)
 {
+	if (hcd->driver->map_urb_for_dma)
+		return hcd->driver->map_urb_for_dma(hcd, urb, mem_flags);
+	else
+		return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
+int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+			    gfp_t mem_flags)
+{
 	enum dma_data_direction dir;
 	int ret = 0;
 
@@ -1400,7 +1417,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 		}
 		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
 				URB_SETUP_MAP_LOCAL)))
-			unmap_urb_for_dma(hcd, urb);
+			usb_hcd_unmap_urb_for_dma(hcd, urb);
 	}
 	return ret;
 }
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 35fe6ab..6d4566e 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_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 3b571f1..1fa8e67 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -233,6 +233,17 @@ struct hc_driver {
 	int	(*urb_dequeue)(struct usb_hcd *hcd,
 				struct urb *urb, int status);
 
+	/* (optional) these hooks allow an HCD to override the default DMA
+	 * mapping and unmapping routines.  In general, they shouldn't be
+	 * necessary unless the host controller has special DMA requirements,
+	 * such as alignment contraints.  If these are not specified, the
+	 * general usb_hcd_(un)?map_urb_for_dma functions will be used instead
+	 * (and it may be a good idea to call these functions in your HCD
+	 * implementation) */
+	int	(*map_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				   gfp_t mem_flags);
+	void    (*unmap_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb);
+
 	/* hw synch, freeing endpoint resources that urb_dequeue can't */
 	void	(*endpoint_disable)(struct usb_hcd *hcd,
 			struct usb_host_endpoint *ep);
@@ -327,6 +338,9 @@ extern void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb);
 
 extern int usb_hcd_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb(struct urb *urb, int status);
+extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+		gfp_t mem_flags);
+extern void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb);
 extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
 extern void usb_hcd_flush_endpoint(struct usb_device *udev,
-- 
1.7.2.2

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

[Robert Morell]

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>
---
 drivers/usb/host/ehci-tegra.c |   91 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index d990c1c..9030fa5 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -32,6 +32,8 @@
 #define TEGRA_USB_USBMODE_HOST			(3 << 0)
 #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
 
+#define TEGRA_USB_DMA_ALIGN 32
+
 struct tegra_ehci_context {
 	bool valid;
 	u32 command;
@@ -457,6 +459,93 @@ 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;
+
+	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 +561,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,
-- 
1.7.2.2

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

[Oliver Neukum]

Am Sonntag, 19. Dezember 2010, 22:38:18 schrieb Robert Morell:
> +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);

Using free_temp_buffer() in the error case looks like a bad
idea, as you'd execute the memcpy if the direction tells you to.

	Regards
		Oliver

[PATCH v2 1/2] USB: Add driver hooks for (un)?map_urb_for_dma

[Robert Morell]

These are useful in case your device has quirks involving what memory
is and is not DMAable.

Signed-off-by: Robert Morell <rmorell@nvidia.com>
Reviewed-by: Scott Williams <scwilliams@nvidia.com>
Reviewed-by: Janne Hellsten <jhellsten@nvidia.com>
---
 drivers/usb/core/hcd.c  |   19 ++++++++++++++++++-
 include/linux/usb/hcd.h |    8 ++++++++
 2 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5cca00a..84fee0f 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1265,6 +1265,14 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
 
 static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 {
+	if (hcd->driver->unmap_urb_for_dma)
+		hcd->driver->unmap_urb_for_dma(hcd, urb);
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
+void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
 	enum dma_data_direction dir;
 
 	if (urb->transfer_flags & URB_SETUP_MAP_SINGLE)
@@ -1311,6 +1319,15 @@ static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			   gfp_t mem_flags)
 {
+	if (hcd->driver->map_urb_for_dma)
+		return hcd->driver->map_urb_for_dma(hcd, urb, mem_flags);
+	else
+		return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
+int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+			    gfp_t mem_flags)
+{
 	enum dma_data_direction dir;
 	int ret = 0;
 
@@ -1400,7 +1417,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 		}
 		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
 				URB_SETUP_MAP_LOCAL)))
-			unmap_urb_for_dma(hcd, urb);
+			usb_hcd_unmap_urb_for_dma(hcd, urb);
 	}
 	return ret;
 }
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 3b571f1..8aaf687 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -233,6 +233,11 @@ struct hc_driver {
 	int	(*urb_dequeue)(struct usb_hcd *hcd,
 				struct urb *urb, int status);
 
+	/* dma support */
+	int	(*map_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb,
+				   gfp_t mem_flags);
+	void    (*unmap_urb_for_dma)(struct usb_hcd *hcd, struct urb *urb);
+
 	/* hw synch, freeing endpoint resources that urb_dequeue can't */
 	void	(*endpoint_disable)(struct usb_hcd *hcd,
 			struct usb_host_endpoint *ep);
@@ -327,6 +332,9 @@ extern void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb);
 
 extern int usb_hcd_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb(struct urb *urb, int status);
+extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+		gfp_t mem_flags);
+extern void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb);
 extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
 extern void usb_hcd_flush_endpoint(struct usb_device *udev,
-- 
1.7.2.2

Re: [PATCH v2 1/2] USB: Add driver hooks for (un)?map_urb_for_dma

[Greg KH]

On Fri, Dec 17, 2010 at 05:49:30PM -0800, Robert Morell wrote:
> These are useful in case your device has quirks involving what memory
> is and is not DMAable.

What "device"?  Please be a whole lot more descriptive here both in the
changelog commit, and in the patch below in the comments for the hooks,
exactly what these hooks are for, who should use them, and why.

thanks,

greg k-h

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

[Robert Morell]

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>
---
 drivers/usb/host/ehci-tegra.c |   91 +++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h           |    1 +
 2 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index d990c1c..9030fa5 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -32,6 +32,8 @@
 #define TEGRA_USB_USBMODE_HOST			(3 << 0)
 #define TEGRA_USB_PORTSC1_PTC(x)		(((x) & 0xf) << 16)
 
+#define TEGRA_USB_DMA_ALIGN 32
+
 struct tegra_ehci_context {
 	bool valid;
 	u32 command;
@@ -457,6 +459,93 @@ 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;
+
+	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 +561,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..6d4566e 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_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
-- 
1.7.2.2

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

[Greg KH]

On Fri, Dec 17, 2010 at 05:49:31PM -0800, Robert Morell wrote:
> --- 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_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */

This should go in the previous patch, right?

thanks,

greg k-h