These patches fix an issue I ran into into where the Beaglebone Black
would lock up on disconnect.
Turns out this 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 could lead starvation of the hub
work queue when using large-enough buffers.
Included is also a fix for the same issue 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, appears to already get this right.
Johan
Johan Hovold (2):
USB: musb: fix inefficient copy of unaligned buffers
USB: ehci-tegra: fix inefficient copy of unaligned buffers
drivers/usb/host/ehci-tegra.c | 2 +-
drivers/usb/musb/musb_host.c | 2 +-
2 files changed, 2 insertions(+), 2 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 transfer buffers.
Note that this also fixes a lockup on disconnect, where repeated failed
transfers would starve the hub workqueue from processing the disconnect,
which would have prevented the urbs from being resubmitted. In this case
there is no data to forward, but the full buffer length was being copied
nonetheless.
The disconnect lockup can easily be reproduced using four unaligned
4k-bulk-in urbs on Beaglebone Black.
Fixes: 8408fd1d83e3 ("usb: musb: implement (un)map_urb_for_dma hooks")
Cc: stable <[email protected]> # v3.10
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/musb/musb_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index c3d5fc9dfb5b..d8327ec19a1f 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2523,7 +2523,7 @@ static void musb_free_temp_buffer(struct urb *urb)
if (dir == DMA_FROM_DEVICE) {
memcpy(temp->old_xfer_buffer, temp->data,
- urb->transfer_buffer_length);
+ urb->actual_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 transfer buffers.
Note that the corresponding fix of musb also fixes a lockup on
disconnect, where repeated failed transfers would starve the hub
workqueue from processing the disconnect, which would have prevented the
urbs from being resubmitted. In this case there is no data to forward,
but the full buffer length was being copied nonetheless.
Compile-tested only.
Fixes: fbf9865c6d96 ("USB: ehci: tegra: Align DMA transfers to 32 bytes")
Cc: stable <[email protected]> # v2.6.39
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/host/ehci-tegra.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index ff9af29b4e9f..dfe2a7abc36d 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -313,7 +313,7 @@ static void free_dma_aligned_buffer(struct urb *urb)
if (usb_urb_dir_in(urb))
memcpy(temp->old_xfer_buffer, temp->data,
- urb->transfer_buffer_length);
+ urb->actual_length);
urb->transfer_buffer = temp->old_xfer_buffer;
kfree(temp->kmalloc_ptr);
--
2.0.5
On Tue, 7 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 transfer buffers.
>
> Note that the corresponding fix of musb also fixes a lockup on
> disconnect, where repeated failed transfers would starve the hub
> workqueue from processing the disconnect, which would have prevented the
> urbs from being resubmitted. In this case there is no data to forward,
> but the full buffer length was being copied nonetheless.
This is wrong for isochronous transfers, because the transfer data
generally isn't contiguous in memory.
It would be okay to do this for other transfer types, though.
Alan Stern
On Tue, Apr 07, 2015 at 11:30:15AM -0400, Alan Stern wrote:
> On Tue, 7 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 transfer buffers.
> >
> > Note that the corresponding fix of musb also fixes a lockup on
> > disconnect, where repeated failed transfers would starve the hub
> > workqueue from processing the disconnect, which would have prevented the
> > urbs from being resubmitted. In this case there is no data to forward,
> > but the full buffer length was being copied nonetheless.
>
> This is wrong for isochronous transfers, because the transfer data
> generally isn't contiguous in memory.
>
> It would be okay to do this for other transfer types, though.
Thanks for pointing that out. I'll re-spin the series tomorrow.
Johan