[PATCH v3] common: usb-hub: Reset hub port before scanning

[Shantur Rathore]

Hi Dragan and Andre,

On Sat, Feb 3, 2024 at 10:39 AM Dragan Simic <dsimic at manjaro.org> wrote:
>
> Hello Andre and Shantur,
>
> On 2024-02-02 12:28, Andre Przywara wrote:
> > On Fri, 02 Feb 2024 03:35:24 +0100 Dragan Simic <dsimic at manjaro.org>
> > wrote:
> >> On 2024-02-02 01:12, Andre Przywara wrote:
> >> > On Thu, 1 Feb 2024 18:35:28 +0000 Shantur Rathore <i at shantur.com>
> >> > wrote:
> >> >> On Thu, Feb 1, 2024 at 4:46 PM Andre Przywara <andre.przywara at arm.com>
> >> >> wrote:
> >> >> > On Thu, 1 Feb 2024 09:39:54 +0000 Shantur Rathore <i at shantur.com> wrote:
> >> >> > > Which USB disk are you using? Is it a USB 2.0 disk ?
> >> >> > > Can you try with any other USB disk to confirm?
> >> >> >
> >> >> > Yes, this was some USB 2.0 memory stick, on an USB 2.0 port.
> >> >> > Most sunxi boards only have USB 2.0, but there is one SoC with USB 3.0, I
> >> >> > will try some combinations later tonight.
> >> >>
> >> >> That would be awesome.
> >> >> We need to test
> >> >>
> >> >> USB 3.0 and 2.0 ports with USB 3.0 and 2.0 drives.
> >> >
> >> > So I had some mixed and apparently inconsistent results:
> >> > - On the Pine-H64 (H6 SoC with USB 3.0 + USB 2.0) it worked with
> >> >   mainline, but only after I applied some pending sunxi USB PHY
> >> >   regulator patches. This is an unrelated issue, those patches
> >> >   are needed to enable USB 3.0 support on that board (shared regulator
> >> >   conflict).
> >> >   I tested a USB 2.0 and a USB 3.0 drive in both kind of slots, with
> >> >   and without the patch.
> >> > - On the OrangePi Zero 3 (H616 SoC with USB 2.0 only), the USB 3.0
> >> >   drive would not work with mainline. A USB 2.0 drive was fine.
> >> >   Applying your patch below fixed that problem, now both drives worked.
> >>
> >> Let me interject, please...
> >>
> >> Huh, this (mih)behavior with the tested OrangePi Zero 3 is really
> >> strange.  As we know, a USB 3.0 drive plugged into a USB 2.0 port
> >> should behave just like a USB 2.0 drive, using the separate set
> >> of pins inside the connector.
> >
> > Yes, I found this odd as well, hence saying inconsistent above.
> >
> >> If possible, performing some additional testing with another USB
> >> 3.0 drive would be quite interesting.  Could it be that something
> >> is wrong with the USB 2.0 receptacle on your OrangePi Zero 3,
> >> mechanically or electrically?
> >
> > The receptacle on the OPi Zero3 is a standard USB 2.0 Type-A socket,
> > with clearly only 4 pins. The USB 3.0 stick in question has all 9 pins.
> > So indeed there should be little difference to a USB 2.0 stick.
>
> Ah, sorry, I wasn't clear enough...  I actually had some possible
> damage to the Zero 3's USB 2.0 receptacle in mind, or some results
> of the regular wear and tear, which could've possibly caused some
> intermittent issues.
>
> > And I just tested the same board with some other (cheap) USB 2.0 stick:
> > it's the same situation as with the USB 3.0 drive: it does not work
> > with
> > mainline, but works with the patch below. So the USB 3.0 device here
> > might
> > be a red herring, and it's actually more up to an individual device?
> > Or maybe it's like: *Some* USB devices (in Hi-Speed mode?) do not like
> > it
> > when their port is reset before it's powered on? And the root hub
> > implementation also plays a role, which is why we only got reports
> > about
> > Allwinner boards?
>
> Hmm, we've got quite a few unknowns there.  Thank you for
> performing the additional testing!
>
> > Also I think Marek said that Linux does the reset only when using
> > SuperSpeed (hence the patch below), so maybe it's something in the USB
> > 3.0
> > spec that changed the requirements?
>
> I just spent about an hour and a half going through the USB 2.0
> and USB 3.0 specifications.  From what I understood, resetting
> a USB 3.0 port should be required when a USB device transitions
> between different states, such as when it resumes from suspend,
> but maybe not necessarily upon the initial device attachment.
>
> Though, the Linux kernel USB driver seems to be doing additional
> USB 3.0 port resets upon the initial USB device attachment,
> presumably to ensure proper state of the USB 3.0 hub port and
> proper USB mode negotiation.
>
> Here are the links to a couple of screenshots from the USB 3.0
> specification, for future reference: https://i.imgur.com/cMaBdq2.png
> and https://i.imgur.com/PDciRjP.png .
>
> Moreover, such USB port resets don't seem to exist for USB 2.0
> hubs, or at least I didn't see them in the USB 2.0 specification.
> The resets seem to be added to the USB 3.0 specification as part
> of the port and device mode negotiation.
>
> Thus, the additional fix that Shantur sent, available below, looks
> to be the right way to handle it.  Thus, Shantur, AFAICT it's fine
> to submit this fix as a separate "Fixes" patch.
>

Thanks for confirmation.
I will submit a patch soon.

> >> > - On the OrangePi Zero (H3 with USB 2.0 only), it worked with and
> >> >   without the patch below, with both the USB 2.0 and USB 3.0 drive.
> >> > - On the BananaPi M64 (A64 with USB 2.0 only and an onboard USB 2.0
> >> >   hub) it also worked in all cases: with and without the patch, USB 2.0
> >> >   and USB 3.0 drive.
> >> >
> >> > So the good news is: with the patch below it worked in all cases, with
> >> > all drives in all slots, on all boards. With current mainline some
> >> > drives don't work at least on the board with the H616 SoC.
> >> >
> >> > I cannot really say if that makes sense, and the results above maybe
> >> > more per board than per SoC (different VBUS supply), but would
> >> > pragmatically tend to use the patch, feel free to add my Tested-by:
> >> > when sending:
> >> >
> >> > Tested-by: Andre Przywara <andre.przywara at arm.com>
> >> >
> >> > Just one tiny thing:
> >> >
> >> >> Once you have tested it, can you please try the patch below
> >> >>
> >> >>         debug("enabling power on all ports\n");
> >> >>         for (i = 0; i < dev->maxchild; i++) {
> >> >> -               usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> >> >> -               debug("Reset : port %d returns %lX\n", i + 1,
> >> >> dev->status);
> >> >> +               if (usb_hub_is_superspeed(hub)) {
> >> >
> >> > this must be: usb_hub_is_superspeed(dev)

No, this is to check the hub. If the hub is USB 3.0 then we can try reset.
The device isn't identified yet.

> >> >
> >> > So many thanks for that patch, that seem to have fixed it for me - even
> >> > though I don't understand why. But then again I only have superficial
> >> > USB knowledge.
> >> >
> >> >> +                       usb_set_port_feature(dev, i + 1,
> >> >> USB_PORT_FEAT_RESET);
> >> >> +                       debug("Reset : port %d returns %lX\n", i + 1,
> >> >> dev->status);
> >> >> +               }
> >> >>                 usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
> >> >>                 debug("PowerOn : port %d returns %lX\n", i + 1,
> >> >> dev->status);
> >> >>         }