Skip to contentSkip to conversation

MUSB interrupt storm on device removal

DISCUSSION Johan Hovold 1 participant 1 message Jan 23, 2019 USB

lore
Share & tools
lore
#0 lore raw
Quoted 76 lines · Greg Kroah-Hartman
Quoted 66 lines · Bin Liu

Hi Johan,

Quoted 63 lines · Bin Liu
Quoted 38 lines · Måns Rullgård
Quoted 27 lines · Bin Liu
Quoted 17 lines · Måns Rullgård
Quoted 15 lines · Bin Liu
Quoted 13 lines · Måns Rullgård
Quoted 11 lines · Bin Liu
Quoted 10 lines · Måns Rullgård
Quoted 8 lines · Bin Liu
Quoted 7 lines · Måns Rullgård
Quoted 5 lines · Bin Liu

Hi,

On Mon, Dec 17, 2018 at 03:13:12PM +0000, Måns Rullgård wrote:
I have a strange problem with the musb driver in host mode on AM3358
(beaglebone) hardware. If I connect a multi-port serial adapter and

Quoted 8 lines
Quoted 1 line

Is this on beaglebone or beagleboneblack?

We've seen it with Beaglebone Enhanced [1] and BeagleCore [2] based devices. Both are based on the AM3358 and mostly compatible with the Beaglebone Black.

[1] https://elinux.org/SanCloud_BeagleBoneEnhanced
[2] http://beaglecore.com/products/bcm1str.html

Quoted 8 lines

Okay, now I see you have a hub on the board. I just reproduced the console lockup after added a hub in my setup. I will find some time to debug this.

FYI, there was a similar issue in a few years ago, which was that the hub routine never got called during the storm then the device disconnect event never got populated from the hub driver. I am wondering if this is the same issue pops up again...

Quoted 4 lines

Now that you mentioned the hub, I tried connecting the serial adapter to the OTG port (in host mode) on the Beaglebone Enhanced (it's not normally used in this product). When connected directly, the disconnect indeed works properly. With a hub in between, it breaks.

Please use the following hack as a workaround for now. I will think more about it.

Regards,
-Bin.

--------8<----------
diff git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c index b59ce9ad14ce..3719f6d9b26c 100644

drivers/usb/musb/musb_host.c +1 -1
@@ -1811,7 +1811,7 @@ void musb_host_rx(struct musb *musb, u8 epnum)
        } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
                musb_dbg(musb, "end %d RX proto error", epnum);
 
-               status = -EPROTO;
+               status = -EPIPE;
                musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
Quoted 8 lines

drivers/usb/musb/musb_host.c void musb_host_rx(struct musb *musb, u8 epnum) · line 1817

Quoted 1 line
drivers/usb/musb/musb_host.c +0 -0
                rx_csr &= ~MUSB_RXCSR_H_ERROR;

When a usb serial device is detached, the musb controller driver returns -EPROTO in urb giveback, then usb_serial_generic_read_bulk_callback() re-submit the urb, this circle causes interrupt storm, and the hub driver workqueue never got a chance to run to populate the device disconnect event, the whole console locks up. Do you think usb_serial_generic_read_bulk_callback() should handle -EPROTO as well?

Quoted 8 lines

Why is musb returning that error? According to the documentation we have about this type of thing (Documentation/driver-api/usb/error-codes.rst), that should only happen if one of the following just occured:
- bitstuff error
- no response packet received within the prescribed bus turn-around time
- unknown USB error

But we also have in that file:

This is also one of several codes that different kinds of host controller use to indicate a transfer has failed because of device disconnect. In the interval before the hub driver starts disconnect processing, devices may receive such fault reports for every request.

Quoted 4 lines

That's not what any other host controller returns when a device is removed, so either you are going to have to fix all USB drives for this issue, or you need to fix the musb driver to not send this error for when a device is removed (hint, do the latter...)

Right, this needs to be handle at the HCD level.

dwc2 fixed a similar lockup issue due to retried NAKed transaction by not retrying immediately:

38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away")

Note that there are some later follow-up patches to that one. Might give some inspiration though.

Johan