[PATCH v2 0/3] USB: fix inefficient copy of unaligned buffers

[PATCH v2 0/3] USB: fix inefficient copy of unaligned buffers

[Johan Hovold]

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

[PATCH v2 0/3] USB: fix inefficient copy of unaligned buffers

[Johan Hovold]

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

[PATCH v2 1/3] USB: musb: fix inefficient copy of unaligned buffers

[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.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 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

[PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[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 <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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

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

[PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[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 <johan@kernel.org>
---
 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

[PATCH v2 3/3] staging: octeon-usb: fix unaligned isochronous transfers

[Johan Hovold]

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 <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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

[PATCH v2 3/3] staging: octeon-usb: fix unaligned isochronous transfers

[Johan Hovold]

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 <johan@kernel.org>
---
 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

Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Frans Klaver]

On Thu, Apr 23, 2015 at 4:06 PM, Johan Hovold <johan@kernel.org> 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 <johan@kernel.org>
> ---
>  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

Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Johan Hovold]

On Thu, Apr 23, 2015 at 04:31:51PM +0200, Frans Klaver wrote:
> On Thu, Apr 23, 2015 at 4:06 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Johan Hovold]

On Thu, Apr 23, 2015 at 04:31:51PM +0200, Frans Klaver wrote:
> On Thu, Apr 23, 2015 at 4:06 PM, Johan Hovold <johan@kernel.org> 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

Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Alan Stern]

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 <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  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 <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Alan Stern]

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 <johan@kernel.org>
> ---
>  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 <stern@rowland.harvard.edu>

Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Stephen Warren]

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 <swarren@nvidia.com>

(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

[Johan Hovold]

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 <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> (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

Re: [PATCH v2 2/3] USB: ehci-tegra: fix inefficient copy of unaligned buffers

[Johan Hovold]

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 <swarren@nvidia.com>
> 
> (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