USB: fix inefficient copy of unaligned buffers
Subsystems (5)
v2 cover USB: fix inefficient copy of unaligned buffers
These patches (for 4.1) make sure that only the received data is copied from the temporary buffers used for unaligned transfers.
I discovered this when debugging an issue where the Beaglebone Black would lock up on disconnect.
Turns out it was related to the transfer_buffers not being properly aligned, causing musb to use temporary buffers for the transfers. On transfer errors (e.g. during disconnect), the full buffer content was still being copied, something which would alter timings enough to prevent the disconnect from being detected and processed.
The first patch in the series works around the problem in that particular set up, but obviously does not solve the underlying issue, which needs to be analysed further.
I also included a corresponding patch for ehci-tegra that has been compile tested only.
Note that the octeon-hcd driver in staging, which also uses temporary buffers for unaligned transfers, was broken for isochronous transfers. The third patch fixes that, but is also untested due to lack of hardware.
Johan
v2:
- Make sure to copy the full buffer for isochronous transfers, in which case the received data is not necessarily contiguous (thanks Alan).
- Drop stable tags as this is really just an optimisation.
Johan Hovold (3):
USB: musb: fix inefficient copy of unaligned buffers
USB: ehci-tegra: fix inefficient copy of unaligned buffers
staging: octeon-usb: fix unaligned isochronous transfers
drivers/staging/octeon-usb/octeon-hcd.c | 12 +++++++++--- drivers/usb/host/ehci-tegra.c | 12 +++++++++--- drivers/usb/musb/musb_host.c | 9 +++++++-- 3 files changed, 25 insertions(+), 8 deletions(-)
Signature · 1 line
2.0.5
PATCH v2 1/3 USB: musb: fix inefficient copy of unaligned buffers
Make sure only to copy any actual data rather than the whole buffer, when releasing the temporary buffer used for unaligned non-isochronous transfers.
--- drivers/usb/musb/musb_host.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/usb/musb/musb_host.c +7 -2
@@ -2512,6 +2512,7 @@ static void musb_free_temp_buffer(struct urb *urb) {enum dma_data_direction dir;struct musb_temp_buffer *temp; +size_t length;if (!(urb->transfer_flags &URB_ALIGNED_TEMP_BUFFER ))return ;
@@ -2522,8 +2523,12 @@ static void musb_free_temp_buffer(struct urb *urb) data);if (dir ==DMA_FROM_DEVICE ) { -memcpy (temp->old_xfer_buffer, temp->data, - urb->transfer_buffer_length); +if (usb_pipeisoc (urb->pipe)) + length = urb->transfer_buffer_length; +else + length = urb->actual_length; + +memcpy (temp->old_xfer_buffer, temp->data, length); } urb->transfer_buffer = temp->old_xfer_buffer;kfree (temp->kmalloc_ptr);
Signature · 1 line
2.0.5
PATCH v2 2/3 USB: ehci-tegra: fix inefficient copy of unaligned buffers
In reply to Johan HovoldMake sure only to copy any actual data rather than the whole buffer, when releasing the temporary buffer used for unaligned non-isochronous transfers.
Compile-tested only.
--- drivers/usb/host/ehci-tegra.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/usb/host/ehci-tegra.c +9 -3
@@ -304,6 +304,7 @@ struct dma_aligned_buffer {static void free_dma_aligned_buffer (struct urb *urb) {struct dma_aligned_buffer *temp; +size_t length;if (!(urb->transfer_flags &URB_ALIGNED_TEMP_BUFFER ))return ;
@@ -311,9 +312,14 @@ static void free_dma_aligned_buffer(struct urb *urb) temp =container_of (urb->transfer_buffer, struct dma_aligned_buffer, data); -if (usb_urb_dir_in (urb)) -memcpy (temp->old_xfer_buffer, temp->data, - urb->transfer_buffer_length); +if (usb_urb_dir_in (urb)) { +if (usb_pipeisoc (urb->pipe)) + length = urb->transfer_buffer_length; +else + length = urb->actual_length; + +memcpy (temp->old_xfer_buffer, temp->data, length); + } urb->transfer_buffer = temp->old_xfer_buffer;kfree (temp->kmalloc_ptr);
Signature · 6 lines
2.0.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers
Quoted 30 lines · Johan Hovold
Make sure only to copy any actual data rather than the whole buffer, when releasing the temporary buffer used for unaligned non-isochronous transfers.
Compile-tested only.
Signed-off-by Johan Hovold <[email protected]>--- drivers/usb/host/ehci-tegra.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/usb/host/ehci-tegra.c +1
-3@@ -304,6 +304,7 @@ struct dma_aligned_buffer {static void free_dma_aligned_buffer (struct urb *urb) {struct dma_aligned_buffer *temp; +size_t length;if (!(urb->transfer_flags &URB_ALIGNED_TEMP_BUFFER ))return ;@@ -311,9 +312,14 @@ static void free_dma_aligned_buffer(struct urb *urb) temp =container_of (urb->transfer_buffer, struct dma_aligned_buffer, data); -if (usb_urb_dir_in (urb)) -memcpy (temp->old_xfer_buffer, temp->data, - urb->transfer_buffer_length);
drivers/usb/host/ehci-tegra.c static void free_dma_aligned_buffer(struct urb *urb) · line 324
Quoted 10 lines
drivers/usb/host/ehci-tegra.c +8
-0+if (usb_urb_dir_in (urb)) { +if (usb_pipeisoc (urb->pipe)) + length = urb->transfer_buffer_length; +else + length = urb->actual_length; + +memcpy (temp->old_xfer_buffer, temp->data, length); + } urb->transfer_buffer = temp->old_xfer_buffer;kfree (temp->kmalloc_ptr);
Out of curiosity: any reason not to declare that length variable inside this new compound?
Thanks,
Frans
Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers
Quoted 13 lines · Frans Klaver
Quoted 11 lines · Johan Hovold
static void
free_dma_aligned_buffer(struct urb *urb){ struct dma_aligned_buffer *temp;drivers/usb/host/ehci-tegra.c +1
-0+size_t length;if (!(urb->transfer_flags &URB_ALIGNED_TEMP_BUFFER ))return ;@@ -311,9 +312,14 @@ static void free_dma_aligned_buffer(struct urb *urb) temp =container_of (urb->transfer_buffer, struct dma_aligned_buffer, data);
Quoted 16 lines
drivers/usb/host/ehci-tegra.c static void free_dma_aligned_buffer(struct urb *urb) · line 324
Quoted 13 lines
drivers/usb/host/ehci-tegra.c +8
-3-if (usb_urb_dir_in (urb)) -memcpy (temp->old_xfer_buffer, temp->data, - urb->transfer_buffer_length); +if (usb_urb_dir_in (urb)) { +if (usb_pipeisoc (urb->pipe)) + length = urb->transfer_buffer_length; +else + length = urb->actual_length; + +memcpy (temp->old_xfer_buffer, temp->data, length); + } urb->transfer_buffer = temp->old_xfer_buffer;kfree (temp->kmalloc_ptr);Out of curiosity: any reason not to declare that length variable inside this new compound?
Just my style preference.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers
In reply to Johan HovoldQuoted 30 lines · Johan Hovold
Make sure only to copy any actual data rather than the whole buffer, when releasing the temporary buffer used for unaligned non-isochronous transfers.
Compile-tested only.
Signed-off-by Johan Hovold <[email protected]>--- drivers/usb/host/ehci-tegra.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/usb/host/ehci-tegra.c +1
-3@@ -304,6 +304,7 @@ struct dma_aligned_buffer {static void free_dma_aligned_buffer (struct urb *urb) {struct dma_aligned_buffer *temp; +size_t length;if (!(urb->transfer_flags &URB_ALIGNED_TEMP_BUFFER ))return ;@@ -311,9 +312,14 @@ static void free_dma_aligned_buffer(struct urb *urb) temp =container_of (urb->transfer_buffer, struct dma_aligned_buffer, data); -if (usb_urb_dir_in (urb)) -memcpy (temp->old_xfer_buffer, temp->data, - urb->transfer_buffer_length);
drivers/usb/host/ehci-tegra.c static void free_dma_aligned_buffer(struct urb *urb) · line 324
Quoted 10 lines
drivers/usb/host/ehci-tegra.c +8
-0+if (usb_urb_dir_in (urb)) { +if (usb_pipeisoc (urb->pipe)) + length = urb->transfer_buffer_length; +else + length = urb->actual_length; + +memcpy (temp->old_xfer_buffer, temp->data, length); + } urb->transfer_buffer = temp->old_xfer_buffer;kfree (temp->kmalloc_ptr);
Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers
In reply to Johan HovoldQuoted 5 lines · Johan Hovold
Make sure only to copy any actual data rather than the whole buffer, when releasing the temporary buffer used for unaligned non-isochronous transfers.
Compile-tested only.
(Tested a USB network device attached to Jetson TK1, with a large download, and ping packets of various sizes from another host)
Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers
Quoted 11 lines · Stephen Warren
Quoted 5 lines · Johan Hovold
Make sure only to copy any actual data rather than the whole buffer, when releasing the temporary buffer used for unaligned non-isochronous transfers.
Compile-tested only.
Tested-by Stephen Warren <[email protected]>(Tested a USB network device attached to Jetson TK1, with a large download, and ping packets of various sizes from another host)
Thanks for testing, Stephen!
Johan
PATCH v2 3/3 staging: octeon-usb: fix unaligned isochronous transfers
In reply to Johan HovoldMake sure to copy the whole transfer buffer when releasing the temporary buffer used for unaligned isochronous transfers as the data is not necessarily contiguous in that case.
--- drivers/staging/octeon-usb/octeon-hcd.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +9 -3
@@ -509,15 +509,21 @@ static int octeon_alloc_temp_buffer(struct urb *urb, gfp_t mem_flags)static void octeon_free_temp_buffer (struct urb *urb) {struct octeon_temp_buffer *temp; +size_t length;if (!(urb->transfer_flags &URB_ALIGNED_TEMP_BUFFER ))return ; temp = container_of(urb->transfer_buffer, struct octeon_temp_buffer, data); -if (usb_urb_dir_in (urb)) -memcpy (temp->orig_buffer, urb->transfer_buffer, - urb->actual_length); +if (usb_urb_dir_in (urb)) { +if (usb_pipeisoc (urb->pipe)) + length = urb->transfer_buffer_length; +else + length = urb->actual_length; + +memcpy (temp->orig_buffer, urb->transfer_buffer, length); + } urb->transfer_buffer = temp->orig_buffer; urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER ;kfree (temp->temp_buffer);
Signature · 1 line
2.0.5