Re: [PATCH][RFC] usb: hub: Cycle HUB power when initialization fails

[Alan Stern]

On Sun, 5 Nov 2017, Mike Looijmans wrote:

> ïOn 03-11-17 18:27, Alan Stern wrote:
> > On Fri, 3 Nov 2017, Mike Looijmans wrote:
> > 
> >> Sometimes the USB device gets confused about the state of the initialization and
> >> the connection fails. In particular, the device thinks that it's already set up
> >> and running while the host thinks the device still needs to be configured. To
> > 
> > How do you know that this is really the issue?  How can the device
> > think it's already set if it doesn't have an assigned address?
> 
> It seems to me that the device just doesn't react at all on the host 
> requests to assign an address. I've seen this happen with various custom 
> mass-storage like appliances, but also DVB tuners and such. The device 
> won't return to a working state until you unplug it and put it back, or, 
> and that's what the patch does, just power-cycle the USB port, which has 
> the same effect.
> 
> >> work around this issue, power-cycle the hub's output to issue a sort of "reset"
> >> to the device. This makes the device restart its state machine and then the
> >> initialization succeeds.
> >>
> >> This fixes problems where the kernel reports a list of errors like this:
> >>
> >> usb 1-1.3: device not accepting address 19, error -71
> >>
> >> The end result is a non-functioning device. After this patch, the sequence
> >> becomes like this:
> >>
> >> usb 1-1.3: new high-speed USB device number 18 using ci_hdrc
> >> usb 1-1.3: device not accepting address 18, error -71
> >> usb 1-1.3: new high-speed USB device number 19 using ci_hdrc
> >> usb 1-1.3: device not accepting address 19, error -71
> >> usb 1-1-port3: attempt power cycle
> >> usb 1-1.3: new high-speed USB device number 21 using ci_hdrc
> >> usb-storage 1-1.3:1.2: USB Mass Storage device detected
> >>
> >> Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
> >> ---
> >> This is a fix I did for a customer which might be appropriate for upstream. What do you think?
> >>
> >>   drivers/usb/core/hub.c | 13 ++++++++++---
> >>   1 file changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> >> index e9ce6bb..a30c1e7 100644
> >> --- a/drivers/usb/core/hub.c
> >> +++ b/drivers/usb/core/hub.c
> >> @@ -2611,7 +2611,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
> >>   #define PORT_RESET_TRIES	5
> >>   #define SET_ADDRESS_TRIES	2
> >>   #define GET_DESCRIPTOR_TRIES	2
> >> -#define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
> >> +#define SET_CONFIG_TRIES	(4 * (use_both_schemes + 1))
> > 
> > We already have too many retry loops.  I am not keen on the idea of
> > adding even more.  How about leaving this value the same and adding the
> > power cycle?
> 
> I'm fine with that as well.
> 
> > The ideal, however, would be to find out what is wrong with the device
> > and see what needs to be done to fix it properly.  This change won't
> > work on many computers (desktops and laptops) because they don't have
> > real USB port-power switching.  A lot of hubs don't have it either.
> 
> I'm pretty sure it's the device's fault, but they're out there and 
> probably not upgradable anyway if you could get the manufacturer to pick 
> up the phone.
> 
> On desktop/laptop machines the problem isn't as pressing since there's a 
> often a user there who can unplug the thing. It's really nasty on 
> embedded systems, and they tend to have the USB power wired through a 
> supply limiter/switch.
> I'm thinking worst that could happen on desktops is that the patch won't 
> have any effect at all.
> 
> So what do you think, is this worth a v2 patch for general consumption 
> or should I keep this a "private" patch for systems that have 
> demonstrated to benefit from it?

We might as well apply a v2 patch, since it will help on some systems.

Alan stern