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

[Guenter Roeck]

On 11/05/2017 10:41 AM, 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.

We have seen this problem with Ethernet dongles left in a bad/hung state during
a previous boot, so it definitely does happen. This happened, for example, with
the r8152 driver with upstream commit 2f25abe6bac ("r8152: prevent the driver
from transmitting packets with carrier off") missing.

Problem though, as mentioned, is that many hubs don't implement this feature,
and if I understand correctly root hubs don't implement it either.

Guenter

> > > 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?
>
>
> > > Â #define USE_NEW_SCHEME(i)ÂÂÂ ((i) / 2 == (int)old_scheme_first)
> > > Â #define HUB_ROOT_RESET_TIMEÂÂÂ 60ÂÂÂ /* times are in msec */
> > > @@ -4805,7 +4805,6 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> > > ÂÂÂÂÂ status = 0;
> > > ÂÂÂÂÂ for (i = 0; i < SET_CONFIG_TRIES; i++) {
> > > -
> >
> > Gratuitous whitespace change.
> >
> > > ÂÂÂÂÂÂÂÂÂ /* reallocate for each attempt, since references
> > > ÂÂÂÂÂÂÂÂÂÂ * to the previous one can escape in various ways
> > > ÂÂÂÂÂÂÂÂÂÂ */
> > > @@ -4935,6 +4934,15 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> > > ÂÂÂÂÂÂÂÂÂ usb_put_dev(udev);
> > > ÂÂÂÂÂÂÂÂÂ if ((status == -ENOTCONN) || (status == -ENOTSUPP))
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
> > > +
> > > +ÂÂÂÂÂÂÂ /* When halfway through our retry count, power-cycle the port */
> > > +ÂÂÂÂÂÂÂ if (i == (SET_CONFIG_TRIES / 2) - 1) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ dev_info(&port_dev->dev, "attempt power cycle\n");
> > > +ÂÂÂÂÂÂÂÂÂÂÂ usb_hub_set_port_power(hdev, hub, port1, false);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ msleep(800);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ usb_hub_set_port_power(hdev, hub, port1, true);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ msleep(hub_power_on_good_delay(hub));
> > > +ÂÂÂÂÂÂÂ }
> > > ÂÂÂÂÂ }
> > > ÂÂÂÂÂ if (hub->hdev->parent ||
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ !hcd->driver->port_handed_over ||
> > > @@ -5476,7 +5484,6 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> > > ÂÂÂÂÂ udev->bos = NULL;
> > > ÂÂÂÂÂ for (i = 0; i < SET_CONFIG_TRIES; ++i) {
> > > -
> >
> > Another gratuitous change.
> >
> > > ÂÂÂÂÂÂÂÂÂ /* ep0 maxpacket size may change; let the HCD know about it.
> > > ÂÂÂÂÂÂÂÂÂÂ * Other endpoints will be handled by re-enumeration. */
> > > ÂÂÂÂÂÂÂÂÂ usb_ep0_reinit(udev);
> >
> > Alan Stern