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

[Andre Przywara]

On Fri, 02 Feb 2024 03:35:24 +0100
Dragan Simic <dsimic at manjaro.org> wrote:

Hi,

> Hello Andre,
> 
> 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:  
> >> > > On Tue, Jan 30, 2024 at 2:01 PM Andre Przywara <andre.przywara at arm.com> wrote:  
> >> > > > On Sat,  9 Dec 2023 18:10:56 +0000 Shantur Rathore <i at shantur.com> wrote:  
> >> > > > > Currently when a hub is turned on, all the ports are powered on.
> >> > > > > This works well for hubs which have individual power control.
> >> > > > >
> >> > > > > For the hubs without individual power control this has no effect.
> >> > > > > Mostly in these scenarios the hub port is powered before the USB
> >> > > > > controller is enabled, this can lead to some devices in unexpected
> >> > > > > state.
> >> > > > >
> >> > > > > With this patch, we explicitly reset the port while powering up hub
> >> > > > > This resets the port for hubs without port power control and has
> >> > > > > no effect on hubs with port power control as the port is still off.
> >> > > > >
> >> > > > > Before this patch AMicro AM8180 based NVME to USB adapter won't be
> >> > > > > detected as a USB3.0 Mass Storage device but with this it works as
> >> > > > > expected.
> >> > > > >
> >> > > > > Tested working after this patch:
> >> > > > > 1. AMicro AM8180 based NVME to USB Adapter
> >> > > > > 2. Kingston DataTraveler 3.0
> >> > > > > 3. GenesysLogic USB3.0 Hub
> >> > > > >
> >> > > > > The drives were tested while connected directly and via the hub.  
> >> > > >
> >> > > > so this broke USB operation on some Allwinner boards. The ports still
> >> > > > enumerate correctly, but no devices are detected. With mainline
> >> > > > (containing this patch), and a USB stick connected:
> >> > > > starting USB...
> >> > > > Bus usb at 5200000: USB EHCI 1.00
> >> > > > Bus usb at 5200400: USB OHCI 1.0
> >> > > > scanning bus usb at 5200000 for devices... 1 USB Device(s) found
> >> > > > scanning bus usb at 5200400 for devices... 1 USB Device(s) found
> >> > > >        scanning usb for storage devices... 0 Storage Device(s) found
> >> > > >
> >> > > > With this patch here reverted:
> >> > > > starting USB...
> >> > > > Bus usb at 5200000: USB EHCI 1.00
> >> > > > Bus usb at 5200400: USB OHCI 1.0
> >> > > > scanning bus usb at 5200000 for devices... 2 USB Device(s) found
> >> > > > scanning bus usb at 5200400 for devices... 1 USB Device(s) found
> >> > > >        scanning usb for storage devices... 1 Storage Device(s) found
> >> > > >
> >> > > > I have no clue why this happens, there is no discrete hub anywhere in this
> >> > > > setup, so I guess the code runs for the root hub here?
> >> > > > I am attaching a log with DEBUG enabled for common/usb_hub.c, for both
> >> > > > cases.
> >> > > >
> >> > > > Any clues, debug suggestions, or even a fix are warmly welcome.
> >> > > >  
> >> > >
> >> > > 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.
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?
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?

Cheers,
Andre

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