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.)
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 <[email protected]>
Reviewed-by: Scott Williams <[email protected]>
Reviewed-by: Janne Hellsten <[email protected]>
---
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
These are useful in case your device has quirks involving what memory
is and is not DMAable.
Signed-off-by: Robert Morell <[email protected]>
Reviewed-by: Scott Williams <[email protected]>
Reviewed-by: Janne Hellsten <[email protected]>
---
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
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
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 <[email protected]>
> Reviewed-by: Scott Williams <[email protected]>
> Reviewed-by: Janne Hellsten <[email protected]>
> ---
> 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
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 <[email protected]>
> > Reviewed-by: Scott Williams <[email protected]>
> > Reviewed-by: Janne Hellsten <[email protected]>
> > ---
> > 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
>
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
>
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.
On Fri, Dec 17, 2010 at 02:44:29PM -0800, [email protected] 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
On Fri, Dec 17, 2010 at 02:42:21PM -0800, [email protected] 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 <[email protected]>
> > > Reviewed-by: Scott Williams <[email protected]>
> > > Reviewed-by: Janne Hellsten <[email protected]>
> > > ---
> > > 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
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
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
On Fri, Dec 17, 2010 at 03:09:40PM -0800, Greg KH wrote:
> On Fri, Dec 17, 2010 at 02:42:21PM -0800, [email protected] 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
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
>
On Fri, Dec 17, 2010 at 03:40:08PM -0800, [email protected] wrote:
> On Fri, Dec 17, 2010 at 03:09:40PM -0800, Greg KH wrote:
> > On Fri, Dec 17, 2010 at 02:42:21PM -0800, [email protected] 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
On Fri, Dec 17, 2010 at 04:37:47PM -0800, Greg KH wrote:
> On Fri, Dec 17, 2010 at 03:40:08PM -0800, [email protected] wrote:
> > On Fri, Dec 17, 2010 at 03:09:40PM -0800, Greg KH wrote:
> > > On Fri, Dec 17, 2010 at 02:42:21PM -0800, [email protected] 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
These are useful in case your device has quirks involving what memory
is and is not DMAable.
Signed-off-by: Robert Morell <[email protected]>
Reviewed-by: Scott Williams <[email protected]>
Reviewed-by: Janne Hellsten <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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