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

[Shantur Rathore]

On Wed, Feb 7, 2024 at 9:22 AM Shantur Rathore <i at shantur.com> wrote:
>
> 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.
>

Take my words back. Didn't realise the dev is a hub device.

> > >> >
> > >> > 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);
> > >> >>         }