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(-)
--
2.0.5
Make sure only to copy any actual data rather than the whole buffer,
when releasing the temporary buffer used for unaligned non-isochronous
transfers.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/musb/musb_host.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index c3d5fc9dfb5b..e1fb5d885c18 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -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);
--
2.0.5
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(-)
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index ff9af29b4e9f..4031b372008e 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -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);
--
2.0.5
Make 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.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/staging/octeon-usb/octeon-hcd.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index 1daeb3125a1f..e03873c343b1 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -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);
--
2.0.5
On Thu, Apr 23, 2015 at 4:06 PM, Johan Hovold <[email protected]> wrote:
> 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(-)
>
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index ff9af29b4e9f..4031b372008e 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -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);
Out of curiosity: any reason not to declare that length variable
inside this new compound?
Thanks,
Frans
On Thu, Apr 23, 2015 at 04:31:51PM +0200, Frans Klaver wrote:
> On Thu, Apr 23, 2015 at 4:06 PM, Johan Hovold <[email protected]> wrote:
> > 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);
>
> Out of curiosity: any reason not to declare that length variable
> inside this new compound?
Just my style preference.
Johan
On Thu, 23 Apr 2015, Johan Hovold wrote:
> 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(-)
>
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index ff9af29b4e9f..4031b372008e 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -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);
Acked-by: Alan Stern <[email protected]>
On 04/23/2015 08:06 AM, Johan Hovold wrote:
> 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)
On Thu, Apr 23, 2015 at 02:32:26PM -0600, Stephen Warren wrote:
> On 04/23/2015 08:06 AM, Johan Hovold wrote:
> > 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