Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756640Ab0LQWnY (ORCPT ); Fri, 17 Dec 2010 17:43:24 -0500 Received: from hqemgate04.nvidia.com ([216.228.121.35]:7543 "EHLO hqemgate04.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756088Ab0LQWnW (ORCPT ); Fri, 17 Dec 2010 17:43:22 -0500 X-PGP-Universal: processed; by hqnvupgp05.nvidia.com on Fri, 17 Dec 2010 14:35:32 -0800 Date: Fri, 17 Dec 2010 14:42:21 -0800 From: rmorell@nvidia.com To: Greg KH Cc: David Brownell , Benoit Goby , Alan Stern , Sarah Sharp , Matthew Wilcox , Ming Lei , Jacob Pan , Olof Johansson , Erik Gilling , Colin Cross , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 2/2] USB: ehci: tegra: Align DMA transfers to 32 bytes Message-ID: <20101217224221.GE25105@morell.nvidia.com> References: <1292623129-26361-1-git-send-email-rmorell@nvidia.com> <1292623129-26361-3-git-send-email-rmorell@nvidia.com> <20101217223502.GB19418@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20101217223502.GB19418@suse.de> X-NVConfidentiality: public User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5978 Lines: 173 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 > > Reviewed-by: Scott Williams > > Reviewed-by: Janne Hellsten > > --- > > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/