[PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

When a USB device attached to a hub got disconnected, MUSB controller
generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.

Currently the MUSB host driver returns -EPROTO in current URB, then the
USB device driver could immediately resubmit the URB which causes MUSB
generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
storm then the hub never got a chance to report the USB device detach.

To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
immediately resubmit the URB.

Signed-off-by: Bin Liu <b-liu@ti.com>
---
 drivers/usb/musb/musb_host.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 8736f4251a22..e41d910dcad1 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1774,9 +1774,9 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 		status = -EPIPE;
 
 	} else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-		musb_dbg(musb, "end %d RX proto error", epnum);
+		dev_err(musb->controller, "ep%d RX three-strikes error", epnum);
 
-		status = -EPROTO;
+		status = -ESHUTDOWN;
 		musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
 		rx_csr &= ~MUSB_RXCSR_H_ERROR;
-- 
2.17.1

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Alan Stern]

On Wed, May 13, 2020 at 04:36:20PM -0500, Bin Liu wrote:
> When a USB device attached to a hub got disconnected, MUSB controller
> generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.
> 
> Currently the MUSB host driver returns -EPROTO in current URB, then the
> USB device driver could immediately resubmit the URB which causes MUSB
> generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
> storm then the hub never got a chance to report the USB device detach.
> 
> To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
> MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
> immediately resubmit the URB.
> 
> Signed-off-by: Bin Liu <b-liu@ti.com>

Strictly speaking, this is not the right thing to do.  It goes against 
the API described in error-codes.rst.  A better approach would be to fix 
the drivers that immediately resubmit an URB after getting a -EPROTO 
error.  After all, that is the wrong thing to do no matter what sort of 
host controller the device is attached to.

Alan Stern

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Wed, May 13, 2020 at 09:32:05PM -0400, Alan Stern wrote:
> On Wed, May 13, 2020 at 04:36:20PM -0500, Bin Liu wrote:
> > When a USB device attached to a hub got disconnected, MUSB controller
> > generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.
> > 
> > Currently the MUSB host driver returns -EPROTO in current URB, then the
> > USB device driver could immediately resubmit the URB which causes MUSB
> > generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
> > storm then the hub never got a chance to report the USB device detach.
> > 
> > To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
> > MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
> > immediately resubmit the URB.
> > 
> > Signed-off-by: Bin Liu <b-liu@ti.com>
> 
> Strictly speaking, this is not the right thing to do.  It goes against 
> the API described in error-codes.rst.  A better approach would be to fix 

error-codes.rst says:

-ESHUTDOWN              The device or host controller has been
			disabled due to some problem that could not
			be worked around, such as a physical
			disconnect.

So -ESHUTDOWN is applicable in this case - the device is disconnected
behind a hub.

> the drivers that immediately resubmit an URB after getting a -EPROTO 

This has been discussed before [1]. And John indicated there are many
device drivers do not delay when resubmitting after getting -EPROTO [2],
and it is quite bit of work to fix it in all device drivers [3]. So
better to solve the issue in HCD.

> error.  After all, that is the wrong thing to do no matter what sort of 
> host controller the device is attached to.

[1] https://www.spinics.net/lists/linux-usb/msg175292.html
[2] https://www.spinics.net/lists/linux-usb/msg176327.html
[3] https://www.spinics.net/lists/linux-usb/msg176375.html

-Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Alan Stern]

On Thu, May 14, 2020 at 09:28:03AM -0500, Bin Liu wrote:
> On Wed, May 13, 2020 at 09:32:05PM -0400, Alan Stern wrote:
> > On Wed, May 13, 2020 at 04:36:20PM -0500, Bin Liu wrote:
> > > When a USB device attached to a hub got disconnected, MUSB controller
> > > generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.
> > > 
> > > Currently the MUSB host driver returns -EPROTO in current URB, then the
> > > USB device driver could immediately resubmit the URB which causes MUSB
> > > generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
> > > storm then the hub never got a chance to report the USB device detach.
> > > 
> > > To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
> > > MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
> > > immediately resubmit the URB.
> > > 
> > > Signed-off-by: Bin Liu <b-liu@ti.com>
> > 
> > Strictly speaking, this is not the right thing to do.  It goes against 
> > the API described in error-codes.rst.  A better approach would be to fix 
> 
> error-codes.rst says:
> 
> -ESHUTDOWN              The device or host controller has been
> 			disabled due to some problem that could not
> 			be worked around, such as a physical
> 			disconnect.
> 
> So -ESHUTDOWN is applicable in this case - the device is disconnected
> behind a hub.

Yes, but you don't _know_ that the device was disconnected.  All you 
know is that there was a 3-strikes error.  Other problems can cause such 
errors (noise, for example).

Alan Stern

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Thu, May 14, 2020 at 10:40:53AM -0400, Alan Stern wrote:
> On Thu, May 14, 2020 at 09:28:03AM -0500, Bin Liu wrote:
> > On Wed, May 13, 2020 at 09:32:05PM -0400, Alan Stern wrote:
> > > On Wed, May 13, 2020 at 04:36:20PM -0500, Bin Liu wrote:
> > > > When a USB device attached to a hub got disconnected, MUSB controller
> > > > generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.
> > > > 
> > > > Currently the MUSB host driver returns -EPROTO in current URB, then the
> > > > USB device driver could immediately resubmit the URB which causes MUSB
> > > > generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
> > > > storm then the hub never got a chance to report the USB device detach.
> > > > 
> > > > To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
> > > > MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
> > > > immediately resubmit the URB.
> > > > 
> > > > Signed-off-by: Bin Liu <b-liu@ti.com>
> > > 
> > > Strictly speaking, this is not the right thing to do.  It goes against 
> > > the API described in error-codes.rst.  A better approach would be to fix 
> > 
> > error-codes.rst says:
> > 
> > -ESHUTDOWN              The device or host controller has been
> > 			disabled due to some problem that could not
> > 			be worked around, such as a physical
> > 			disconnect.
> > 
> > So -ESHUTDOWN is applicable in this case - the device is disconnected
> > behind a hub.
> 
> Yes, but you don't _know_ that the device was disconnected.  All you 
> know is that there was a 3-strikes error.  Other problems can cause such 
> errors (noise, for example).

Yes, I know this. But we don't have a solution then. I cannot add
resubmit delay in those ~500 device drivers.

-Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Thu, May 14, 2020 at 10:02:59AM -0500, Bin Liu wrote:
> On Thu, May 14, 2020 at 10:40:53AM -0400, Alan Stern wrote:
> > On Thu, May 14, 2020 at 09:28:03AM -0500, Bin Liu wrote:
> > > On Wed, May 13, 2020 at 09:32:05PM -0400, Alan Stern wrote:
> > > > On Wed, May 13, 2020 at 04:36:20PM -0500, Bin Liu wrote:
> > > > > When a USB device attached to a hub got disconnected, MUSB controller
> > > > > generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.
> > > > > 
> > > > > Currently the MUSB host driver returns -EPROTO in current URB, then the
> > > > > USB device driver could immediately resubmit the URB which causes MUSB
> > > > > generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
> > > > > storm then the hub never got a chance to report the USB device detach.
> > > > > 
> > > > > To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
> > > > > MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
> > > > > immediately resubmit the URB.
> > > > > 
> > > > > Signed-off-by: Bin Liu <b-liu@ti.com>
> > > > 
> > > > Strictly speaking, this is not the right thing to do.  It goes against 
> > > > the API described in error-codes.rst.  A better approach would be to fix 
> > > 
> > > error-codes.rst says:
> > > 
> > > -ESHUTDOWN              The device or host controller has been
> > > 			disabled due to some problem that could not
> > > 			be worked around, such as a physical
> > > 			disconnect.
> > > 
> > > So -ESHUTDOWN is applicable in this case - the device is disconnected
> > > behind a hub.
> > 
> > Yes, but you don't _know_ that the device was disconnected.  All you 
> > know is that there was a 3-strikes error.  Other problems can cause such 
> > errors (noise, for example).
> 
> Yes, I know this. But we don't have a solution then. I cannot add
> resubmit delay in those ~500 device drivers.

By the way I don't think noise could last long enough to cause 3-strikes
error. A shortest USB packet is about 3-bytes long, a noise should be
just a glitch, it won't last at least 3-bytes long to supress the bus
and 3 times on the exact timing when the host expecting a response
packet. I cannot think of any other reason which can cause the 3-strikes
error other than the device is off the bus.

-Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Alan Stern]

On Thu, May 14, 2020 at 10:37:31AM -0500, Bin Liu wrote:
> On Thu, May 14, 2020 at 10:02:59AM -0500, Bin Liu wrote:
> > On Thu, May 14, 2020 at 10:40:53AM -0400, Alan Stern wrote:
> > > On Thu, May 14, 2020 at 09:28:03AM -0500, Bin Liu wrote:
> > > > On Wed, May 13, 2020 at 09:32:05PM -0400, Alan Stern wrote:
> > > > > On Wed, May 13, 2020 at 04:36:20PM -0500, Bin Liu wrote:
> > > > > > When a USB device attached to a hub got disconnected, MUSB controller
> > > > > > generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.
> > > > > > 
> > > > > > Currently the MUSB host driver returns -EPROTO in current URB, then the
> > > > > > USB device driver could immediately resubmit the URB which causes MUSB
> > > > > > generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
> > > > > > storm then the hub never got a chance to report the USB device detach.
> > > > > > 
> > > > > > To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
> > > > > > MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
> > > > > > immediately resubmit the URB.
> > > > > > 
> > > > > > Signed-off-by: Bin Liu <b-liu@ti.com>
> > > > > 
> > > > > Strictly speaking, this is not the right thing to do.  It goes against 
> > > > > the API described in error-codes.rst.  A better approach would be to fix 
> > > > 
> > > > error-codes.rst says:
> > > > 
> > > > -ESHUTDOWN              The device or host controller has been
> > > > 			disabled due to some problem that could not
> > > > 			be worked around, such as a physical
> > > > 			disconnect.
> > > > 
> > > > So -ESHUTDOWN is applicable in this case - the device is disconnected
> > > > behind a hub.
> > > 
> > > Yes, but you don't _know_ that the device was disconnected.  All you 
> > > know is that there was a 3-strikes error.  Other problems can cause such 
> > > errors (noise, for example).
> > 
> > Yes, I know this. But we don't have a solution then. I cannot add
> > resubmit delay in those ~500 device drivers.
> 
> By the way I don't think noise could last long enough to cause 3-strikes
> error. A shortest USB packet is about 3-bytes long, a noise should be
> just a glitch, it won't last at least 3-bytes long to supress the bus
> and 3 times on the exact timing when the host expecting a response
> packet. I cannot think of any other reason which can cause the 3-strikes
> error other than the device is off the bus.

Heh.  I heard from somebody (many years ago) about a setup where one of 
his USB devices stopped working whenever he turned on the fluorescent 
lights.

Yes, I agree that noise is pretty uncommon, and the vast majority of 
3-strikes errors are caused by disconnection or device firmware bugs.  
That's why I didn't NAK this patch.

Still, it's worth pointing out that this change abuses the API (perhaps 
mentioning it in a comment).  And it still would be preferable to fix 
the drivers in question, impractical though that may be.  (I have a hard 
time believing there are really 500 of them getting this wrong...)

Alan Stern

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Thu, May 14, 2020 at 12:26:04PM -0400, Alan Stern wrote:
> On Thu, May 14, 2020 at 10:37:31AM -0500, Bin Liu wrote:
> > On Thu, May 14, 2020 at 10:02:59AM -0500, Bin Liu wrote:
> > > On Thu, May 14, 2020 at 10:40:53AM -0400, Alan Stern wrote:
> > > > On Thu, May 14, 2020 at 09:28:03AM -0500, Bin Liu wrote:
> > > > > On Wed, May 13, 2020 at 09:32:05PM -0400, Alan Stern wrote:
> > > > > > On Wed, May 13, 2020 at 04:36:20PM -0500, Bin Liu wrote:
> > > > > > > When a USB device attached to a hub got disconnected, MUSB controller
> > > > > > > generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.
> > > > > > > 
> > > > > > > Currently the MUSB host driver returns -EPROTO in current URB, then the
> > > > > > > USB device driver could immediately resubmit the URB which causes MUSB
> > > > > > > generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
> > > > > > > storm then the hub never got a chance to report the USB device detach.
> > > > > > > 
> > > > > > > To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
> > > > > > > MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
> > > > > > > immediately resubmit the URB.
> > > > > > > 
> > > > > > > Signed-off-by: Bin Liu <b-liu@ti.com>
> > > > > > 
> > > > > > Strictly speaking, this is not the right thing to do.  It goes against 
> > > > > > the API described in error-codes.rst.  A better approach would be to fix 
> > > > > 
> > > > > error-codes.rst says:
> > > > > 
> > > > > -ESHUTDOWN              The device or host controller has been
> > > > > 			disabled due to some problem that could not
> > > > > 			be worked around, such as a physical
> > > > > 			disconnect.
> > > > > 
> > > > > So -ESHUTDOWN is applicable in this case - the device is disconnected
> > > > > behind a hub.
> > > > 
> > > > Yes, but you don't _know_ that the device was disconnected.  All you 
> > > > know is that there was a 3-strikes error.  Other problems can cause such 
> > > > errors (noise, for example).
> > > 
> > > Yes, I know this. But we don't have a solution then. I cannot add
> > > resubmit delay in those ~500 device drivers.
> > 
> > By the way I don't think noise could last long enough to cause 3-strikes
> > error. A shortest USB packet is about 3-bytes long, a noise should be
> > just a glitch, it won't last at least 3-bytes long to supress the bus
> > and 3 times on the exact timing when the host expecting a response
> > packet. I cannot think of any other reason which can cause the 3-strikes
> > error other than the device is off the bus.
> 
> Heh.  I heard from somebody (many years ago) about a setup where one of 
> his USB devices stopped working whenever he turned on the fluorescent 
> lights.

This could happen, noise comming in from the power supply, cheap power
supply and poor board design.

> Yes, I agree that noise is pretty uncommon, and the vast majority of 
> 3-strikes errors are caused by disconnection or device firmware bugs.  

I wasn't mean noise is uncommon, it is just it won't cause 3-strikes
error because the glitch is very short. I have seen many cases that
noise causing issues on MUSB due to poor board design especially on
those USB modem or WIFI projects.

> That's why I didn't NAK this patch.

Thanks.

> Still, it's worth pointing out that this change abuses the API (perhaps 
> mentioning it in a comment).  And it still would be preferable to fix 

Okay, I will add those notes in comment in v2.

> the drivers in question, impractical though that may be.  (I have a hard 
> time believing there are really 500 of them getting this wrong...)

I am not sure about it either, but yeah it is not practical to fix the
issue in device drivers. So far I have seen 3 reports of this issue:

1. with FTDI usb-serial adapter, the issue is in the usb serial generic
driver;

2. with a modem, the issue is in usb wwan driver as I fixed in the patch
I sent yesterday;

2. another modem, before I was able to locate the offending device
driver, the guy who reported the issue disappeared, and not responding
me.

-Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Alan Stern]

On Thu, May 14, 2020 at 12:00:23PM -0500, Bin Liu wrote:

> > Still, it's worth pointing out that this change abuses the API (perhaps 
> > mentioning it in a comment).  And it still would be preferable to fix 
> 
> Okay, I will add those notes in comment in v2.
> 
> > the drivers in question, impractical though that may be.  (I have a hard 
> > time believing there are really 500 of them getting this wrong...)
> 
> I am not sure about it either, but yeah it is not practical to fix the
> issue in device drivers. So far I have seen 3 reports of this issue:
> 
> 1. with FTDI usb-serial adapter, the issue is in the usb serial generic
> driver;

It shouldn't be hard to fix this.  We don't even need a delayed retry; 
the driver can simply treat -EPROTO as an unrecoverable error, just like 
-ESHUTDOWN.

Alan Stern

> 2. with a modem, the issue is in usb wwan driver as I fixed in the patch
> I sent yesterday;
> 
> 2. another modem, before I was able to locate the offending device
> driver, the guy who reported the issue disappeared, and not responding
> me.
> 
> -Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Thu, May 14, 2020 at 02:55:49PM -0400, Alan Stern wrote:
> On Thu, May 14, 2020 at 12:00:23PM -0500, Bin Liu wrote:
> 
> > > Still, it's worth pointing out that this change abuses the API (perhaps 
> > > mentioning it in a comment).  And it still would be preferable to fix 
> > 
> > Okay, I will add those notes in comment in v2.
> > 
> > > the drivers in question, impractical though that may be.  (I have a hard 
> > > time believing there are really 500 of them getting this wrong...)
> > 
> > I am not sure about it either, but yeah it is not practical to fix the
> > issue in device drivers. So far I have seen 3 reports of this issue:
> > 
> > 1. with FTDI usb-serial adapter, the issue is in the usb serial generic
> > driver;
> 
> It shouldn't be hard to fix this.  We don't even need a delayed retry; 
> the driver can simply treat -EPROTO as an unrecoverable error, just like 
> -ESHUTDOWN.

This is just an example showing that it is not practical to fix the
problem in device drivers.

BTY, I just briefly searched the drivers/ folder, there are only three
drivers handle -EPROTO specifically in rx callback and bail out. So
seems most drivers do not care about -EPROTO.

-Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Alan Stern]

On Tue, May 19, 2020 at 12:12:55PM -0500, Bin Liu wrote:
> On Thu, May 14, 2020 at 02:55:49PM -0400, Alan Stern wrote:
> > On Thu, May 14, 2020 at 12:00:23PM -0500, Bin Liu wrote:
> > 
> > > > Still, it's worth pointing out that this change abuses the API (perhaps 
> > > > mentioning it in a comment).  And it still would be preferable to fix 
> > > 
> > > Okay, I will add those notes in comment in v2.
> > > 
> > > > the drivers in question, impractical though that may be.  (I have a hard 
> > > > time believing there are really 500 of them getting this wrong...)
> > > 
> > > I am not sure about it either, but yeah it is not practical to fix the
> > > issue in device drivers. So far I have seen 3 reports of this issue:
> > > 
> > > 1. with FTDI usb-serial adapter, the issue is in the usb serial generic
> > > driver;
> > 
> > It shouldn't be hard to fix this.  We don't even need a delayed retry; 
> > the driver can simply treat -EPROTO as an unrecoverable error, just like 
> > -ESHUTDOWN.
> 
> This is just an example showing that it is not practical to fix the
> problem in device drivers.
> 
> BTY, I just briefly searched the drivers/ folder, there are only three
> drivers handle -EPROTO specifically in rx callback and bail out. So
> seems most drivers do not care about -EPROTO.

Then do you think we should change the API?

Right now it's a little too complicated.  There's no real reason for 
EPROTO, EILSEQ, and ETIME to be different codes.  And now you seem to be 
suggesting that we should fold them all into ESHUTDOWN.

Alan Stern

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Tue, May 19, 2020 at 04:01:58PM -0400, Alan Stern wrote:
> On Tue, May 19, 2020 at 12:12:55PM -0500, Bin Liu wrote:
> > On Thu, May 14, 2020 at 02:55:49PM -0400, Alan Stern wrote:
> > > On Thu, May 14, 2020 at 12:00:23PM -0500, Bin Liu wrote:
> > > 
> > > > > Still, it's worth pointing out that this change abuses the API (perhaps 
> > > > > mentioning it in a comment).  And it still would be preferable to fix 
> > > > 
> > > > Okay, I will add those notes in comment in v2.
> > > > 
> > > > > the drivers in question, impractical though that may be.  (I have a hard 
> > > > > time believing there are really 500 of them getting this wrong...)
> > > > 
> > > > I am not sure about it either, but yeah it is not practical to fix the
> > > > issue in device drivers. So far I have seen 3 reports of this issue:
> > > > 
> > > > 1. with FTDI usb-serial adapter, the issue is in the usb serial generic
> > > > driver;
> > > 
> > > It shouldn't be hard to fix this.  We don't even need a delayed retry; 
> > > the driver can simply treat -EPROTO as an unrecoverable error, just like 
> > > -ESHUTDOWN.
> > 
> > This is just an example showing that it is not practical to fix the
> > problem in device drivers.
> > 
> > BTY, I just briefly searched the drivers/ folder, there are only three
> > drivers handle -EPROTO specifically in rx callback and bail out. So
> > seems most drivers do not care about -EPROTO.
> 
> Then do you think we should change the API?
> 
> Right now it's a little too complicated.  There's no real reason for 
> EPROTO, EILSEQ, and ETIME to be different codes.  And now you seem to be 
> suggesting that we should fold them all into ESHUTDOWN.

I haven't studied all the USB error code enough to suggest this. I am
only changing to ESHUTDOWN in musb driver because I know this 3-strikes
error should only be caused by USB device disabled, which is the
definition of ESHUTDOWN in error-code.rst.

-Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Alan Stern]

> > Then do you think we should change the API?
> > 
> > Right now it's a little too complicated.  There's no real reason for 
> > EPROTO, EILSEQ, and ETIME to be different codes.  And now you seem to be 
> > suggesting that we should fold them all into ESHUTDOWN.
> 
> I haven't studied all the USB error code enough to suggest this. I am
> only changing to ESHUTDOWN in musb driver because I know this 3-strikes
> error should only be caused by USB device disabled, which is the
> definition of ESHUTDOWN in error-code.rst.

You should spend a little time studying the error-codes document.  
EPROTO, EILSEQ, and ETIME describe different kinds of errors that all 
fall into the 3-strikes category.

Alan Stern

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Wed, May 20, 2020 at 12:40:55PM -0400, Alan Stern wrote:
> > > Then do you think we should change the API?
> > > 
> > > Right now it's a little too complicated.  There's no real reason for 
> > > EPROTO, EILSEQ, and ETIME to be different codes.  And now you seem to be 
> > > suggesting that we should fold them all into ESHUTDOWN.
> > 
> > I haven't studied all the USB error code enough to suggest this. I am
> > only changing to ESHUTDOWN in musb driver because I know this 3-strikes
> > error should only be caused by USB device disabled, which is the
> > definition of ESHUTDOWN in error-code.rst.
> 
> You should spend a little time studying the error-codes document.  
> EPROTO, EILSEQ, and ETIME describe different kinds of errors that all 
> fall into the 3-strikes category.

But not all device drivers, except the 3 I found, treat EPROTO as fatal,
and I cannot fix it in all the device drivers. The only thing I can do
at this moment is changing the MUSB HCD driver to return ESHUTDOWN which
still follows the document, kind of.

-Bin.

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Alan Stern]

On Wed, May 20, 2020 at 01:05:50PM -0500, Bin Liu wrote:
> On Wed, May 20, 2020 at 12:40:55PM -0400, Alan Stern wrote:
> > > > Then do you think we should change the API?
> > > > 
> > > > Right now it's a little too complicated.  There's no real reason for 
> > > > EPROTO, EILSEQ, and ETIME to be different codes.  And now you seem to be 
> > > > suggesting that we should fold them all into ESHUTDOWN.
> > > 
> > > I haven't studied all the USB error code enough to suggest this. I am
> > > only changing to ESHUTDOWN in musb driver because I know this 3-strikes
> > > error should only be caused by USB device disabled, which is the
> > > definition of ESHUTDOWN in error-code.rst.
> > 
> > You should spend a little time studying the error-codes document.  
> > EPROTO, EILSEQ, and ETIME describe different kinds of errors that all 
> > fall into the 3-strikes category.
> 
> But not all device drivers, except the 3 I found, treat EPROTO as fatal,
> and I cannot fix it in all the device drivers. The only thing I can do
> at this moment is changing the MUSB HCD driver to return ESHUTDOWN which
> still follows the document, kind of.

Think long-term.  Yes, for now you're focused on changing the MUSB HCD.  
But what should happen in the future?

At some point we all need to stop reacting and start making some 
deliberate plans.  This issue is bigger than one patch or one driver.

Alan Stern

Re: [PATCH] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

On Wed, May 20, 2020 at 02:25:36PM -0400, Alan Stern wrote:
> On Wed, May 20, 2020 at 01:05:50PM -0500, Bin Liu wrote:
> > On Wed, May 20, 2020 at 12:40:55PM -0400, Alan Stern wrote:
> > > > > Then do you think we should change the API?
> > > > > 
> > > > > Right now it's a little too complicated.  There's no real reason for 
> > > > > EPROTO, EILSEQ, and ETIME to be different codes.  And now you seem to be 
> > > > > suggesting that we should fold them all into ESHUTDOWN.
> > > > 
> > > > I haven't studied all the USB error code enough to suggest this. I am
> > > > only changing to ESHUTDOWN in musb driver because I know this 3-strikes
> > > > error should only be caused by USB device disabled, which is the
> > > > definition of ESHUTDOWN in error-code.rst.
> > > 
> > > You should spend a little time studying the error-codes document.  
> > > EPROTO, EILSEQ, and ETIME describe different kinds of errors that all 
> > > fall into the 3-strikes category.
> > 
> > But not all device drivers, except the 3 I found, treat EPROTO as fatal,
> > and I cannot fix it in all the device drivers. The only thing I can do
> > at this moment is changing the MUSB HCD driver to return ESHUTDOWN which
> > still follows the document, kind of.
> 
> Think long-term.  Yes, for now you're focused on changing the MUSB HCD.  
> But what should happen in the future?
> 
> At some point we all need to stop reacting and start making some 
> deliberate plans.  This issue is bigger than one patch or one driver.

Yes, the issue big, not all device drivers treat the urb error status
properly. As the case #3 I mentioned earlier, even MUSB HCD returns
ESHUTDOWN, the interrupt storm still happens. Unfortunately before I
located the offending device driver, the remote setup is gone...

I agree with you, we probably should simply the number of the error code
based on the real usecases and the handlings in device drivers.

-Bin.

[PATCH v2] usb: musb: return -ESHUTDOWN in urb when three-strikes error happened

[Bin Liu]

When a USB device attached to a hub got disconnected, MUSB controller
generates RXCSR_RX_ERROR interrupt for the 3-strikes-out error.

Currently the MUSB host driver returns -EPROTO in current URB, then the
USB device driver could immediately resubmit the URB which causes MUSB
generate RXCSR_RX_ERROR interrupt again. This circle causes interrupt
storm then the hub never got a chance to report the USB device detach.

To fix the interrupt storm, change the URB return code to -ESHUTDOWN for
MUSB_RXCSR_H_ERROR interrupt, so that the USB device driver will not
immediately resubmit the URB.

Signed-off-by: Bin Liu <b-liu@ti.com>
---
v2: added inline comments

 drivers/usb/musb/musb_host.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 8736f4251a22..8b7d22a0c0fb 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1774,9 +1774,15 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 		status = -EPIPE;
 
 	} else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-		musb_dbg(musb, "end %d RX proto error", epnum);
+		dev_err(musb->controller, "ep%d RX three-strikes error", epnum);
 
-		status = -EPROTO;
+		/*
+		 * The three-strikes error could only happen when the USB
+		 * device is not accessible, for example detached or powered
+		 * off. So return the fatal error -ESHUTDOWN so hopefully the
+		 * USB device drivers won't immediately resubmit the same URB.
+		 */
+		status = -ESHUTDOWN;
 		musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
 		rx_csr &= ~MUSB_RXCSR_H_ERROR;
-- 
2.17.1