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

[Dragan Simic]

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.

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