[FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Shantur Rathore]

USB 3.0 spec requires hub to reset device while
enumeration. Some USB 2.0 hubs / devices don't
handle this well and after implementation of
reset some USB 2.0 disks weren't detected on
Allwinner based boards.

Resetting only when hub is USB 3.0 fixes it.

Tested-by: Andre Przywara <andre.przywara@arm.com>

Signed-off-by: Shantur Rathore <i@shantur.com>
---

 common/usb_hub.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 3fb7e14d10..2e054eb935 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 
 	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(dev)) {
+			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);
 	}
-- 
2.40.1

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Marek Vasut]

On 2/7/24 11:23, Shantur Rathore wrote:
> USB 3.0 spec requires hub to reset device while
> enumeration. Some USB 2.0 hubs / devices don't
> handle this well and after implementation of
> reset some USB 2.0 disks weren't detected on
> Allwinner based boards.
> 
> Resetting only when hub is USB 3.0 fixes it.

It would be good to include as many details about the faulty hardware in 
the commit message as possible, so that when someone else runs into 
this, they would have all that information available.

> Tested-by: Andre Przywara <andre.przywara@arm.com>
> 
> Signed-off-by: Shantur Rathore <i@shantur.com>
> ---
> 
>   common/usb_hub.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 3fb7e14d10..2e054eb935 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>   
>   	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(dev)) {

Should this condition be "all which are lower than superspeed" instead , 
so when the next generation of USB comes, this problem won't trigger ?

What does Linux do btw ?

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Shantur Rathore]

Hi Marek,

On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/7/24 11:23, Shantur Rathore wrote:
> > USB 3.0 spec requires hub to reset device while
> > enumeration. Some USB 2.0 hubs / devices don't
> > handle this well and after implementation of
> > reset some USB 2.0 disks weren't detected on
> > Allwinner based boards.
> >
> > Resetting only when hub is USB 3.0 fixes it.
>
> It would be good to include as many details about the faulty hardware in
> the commit message as possible, so that when someone else runs into
> this, they would have all that information available.
>
> > Tested-by: Andre Przywara <andre.przywara@arm.com>
> >
> > Signed-off-by: Shantur Rathore <i@shantur.com>
> > ---
> >
> >   common/usb_hub.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/usb_hub.c b/common/usb_hub.c
> > index 3fb7e14d10..2e054eb935 100644
> > --- a/common/usb_hub.c
> > +++ b/common/usb_hub.c
> > @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
> >
> >       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(dev)) {
>
> Should this condition be "all which are lower than superspeed" instead ,
> so when the next generation of USB comes, this problem won't trigger ?
>
> What does Linux do btw ?

As of now Linux checks if the hub is superspeed
https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859

which is
 return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; // USB_HUB_PR_SS = 3

This holds true for newer SuperSpeedPlus hubs as well.
https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155

We can change the check to be  bDeviceProtocol > 2 but who knows if
things change in the newer version of spec.
I am open to suggestions.

Kind regards,
Shantur

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Marek Vasut]

On 2/8/24 12:30, Shantur Rathore wrote:
> Hi Marek,
> 
> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/7/24 11:23, Shantur Rathore wrote:
>>> USB 3.0 spec requires hub to reset device while
>>> enumeration. Some USB 2.0 hubs / devices don't
>>> handle this well and after implementation of
>>> reset some USB 2.0 disks weren't detected on
>>> Allwinner based boards.
>>>
>>> Resetting only when hub is USB 3.0 fixes it.
>>
>> It would be good to include as many details about the faulty hardware in
>> the commit message as possible, so that when someone else runs into
>> this, they would have all that information available.
>>
>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>> ---
>>>
>>>    common/usb_hub.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> index 3fb7e14d10..2e054eb935 100644
>>> --- a/common/usb_hub.c
>>> +++ b/common/usb_hub.c
>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>>>
>>>        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(dev)) {
>>
>> Should this condition be "all which are lower than superspeed" instead ,
>> so when the next generation of USB comes, this problem won't trigger ?
>>
>> What does Linux do btw ?
> 
> As of now Linux checks if the hub is superspeed
> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> 
> which is
>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; // USB_HUB_PR_SS = 3
> 
> This holds true for newer SuperSpeedPlus hubs as well.
> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> 
> We can change the check to be  bDeviceProtocol > 2 but who knows if
> things change in the newer version of spec.
> I am open to suggestions.

Please just include the ^ in the commit description. Use link to 
git.kernel.org , not some mirror . This is extremely useful information 
and, well, you already wrote the V2 commit message addition in this answer.

Thanks

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

On 2024-02-08 14:33, Marek Vasut wrote:
> On 2/8/24 12:30, Shantur Rathore wrote:
>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>>> USB 3.0 spec requires hub to reset device while
>>>> enumeration. Some USB 2.0 hubs / devices don't
>>>> handle this well and after implementation of
>>>> reset some USB 2.0 disks weren't detected on
>>>> Allwinner based boards.
>>>> 
>>>> Resetting only when hub is USB 3.0 fixes it.
>>> 
>>> It would be good to include as many details about the faulty hardware 
>>> in
>>> the commit message as possible, so that when someone else runs into
>>> this, they would have all that information available.
>>> 
>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>> 
>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>> ---
>>>> 
>>>>    common/usb_hub.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>> index 3fb7e14d10..2e054eb935 100644
>>>> --- a/common/usb_hub.c
>>>> +++ b/common/usb_hub.c
>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct 
>>>> usb_hub_device *hub)
>>>> 
>>>>        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(dev)) {
>>> 
>>> Should this condition be "all which are lower than superspeed" 
>>> instead ,
>>> so when the next generation of USB comes, this problem won't trigger 
>>> ?
>>> 
>>> What does Linux do btw ?
>> 
>> As of now Linux checks if the hub is superspeed
>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>> 
>> which is
>>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; // 
>> USB_HUB_PR_SS = 3
>> 
>> This holds true for newer SuperSpeedPlus hubs as well.
>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>> 
>> We can change the check to be  bDeviceProtocol > 2 but who knows if
>> things change in the newer version of spec.
>> I am open to suggestions.
> 
> Please just include the ^ in the commit description. Use link to
> git.kernel.org , not some mirror . This is extremely useful
> information and, well, you already wrote the V2 commit message
> addition in this answer.

Shantur, if that would be easier or quicker for you, I can write
a quite detailed patch description for you, in exchange for a
"Helped-by" tag in the v2 patch submission. :)

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Shantur Rathore]

On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2024-02-08 14:33, Marek Vasut wrote:
> > On 2/8/24 12:30, Shantur Rathore wrote:
> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
> >>> On 2/7/24 11:23, Shantur Rathore wrote:
> >>>> USB 3.0 spec requires hub to reset device while
> >>>> enumeration. Some USB 2.0 hubs / devices don't
> >>>> handle this well and after implementation of
> >>>> reset some USB 2.0 disks weren't detected on
> >>>> Allwinner based boards.
> >>>>
> >>>> Resetting only when hub is USB 3.0 fixes it.
> >>>
> >>> It would be good to include as many details about the faulty hardware
> >>> in
> >>> the commit message as possible, so that when someone else runs into
> >>> this, they would have all that information available.
> >>>
> >>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> >>>>
> >>>> Signed-off-by: Shantur Rathore <i@shantur.com>
> >>>> ---
> >>>>
> >>>>    common/usb_hub.c | 6 ++++--
> >>>>    1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >>>> index 3fb7e14d10..2e054eb935 100644
> >>>> --- a/common/usb_hub.c
> >>>> +++ b/common/usb_hub.c
> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> >>>> usb_hub_device *hub)
> >>>>
> >>>>        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(dev)) {
> >>>
> >>> Should this condition be "all which are lower than superspeed"
> >>> instead ,
> >>> so when the next generation of USB comes, this problem won't trigger
> >>> ?
> >>>
> >>> What does Linux do btw ?
> >>
> >> As of now Linux checks if the hub is superspeed
> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> >>
> >> which is
> >>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> >> USB_HUB_PR_SS = 3
> >>
> >> This holds true for newer SuperSpeedPlus hubs as well.
> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> >>
> >> We can change the check to be  bDeviceProtocol > 2 but who knows if
> >> things change in the newer version of spec.
> >> I am open to suggestions.
> >
> > Please just include the ^ in the commit description. Use link to
> > git.kernel.org , not some mirror . This is extremely useful
> > information and, well, you already wrote the V2 commit message
> > addition in this answer.
>
> Shantur, if that would be easier or quicker for you, I can write
> a quite detailed patch description for you, in exchange for a
> "Helped-by" tag in the v2 patch submission. :)

That would be really kind of you Dragan.
I am down with the flu so that would really help me as my brain is
working at 15% capacity.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

On 2024-02-08 15:10, Shantur Rathore wrote:
> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org> wrote:
>> On 2024-02-08 14:33, Marek Vasut wrote:
>> > On 2/8/24 12:30, Shantur Rathore wrote:
>> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>> >>> On 2/7/24 11:23, Shantur Rathore wrote:
>> >>>> USB 3.0 spec requires hub to reset device while
>> >>>> enumeration. Some USB 2.0 hubs / devices don't
>> >>>> handle this well and after implementation of
>> >>>> reset some USB 2.0 disks weren't detected on
>> >>>> Allwinner based boards.
>> >>>>
>> >>>> Resetting only when hub is USB 3.0 fixes it.
>> >>>
>> >>> It would be good to include as many details about the faulty hardware
>> >>> in
>> >>> the commit message as possible, so that when someone else runs into
>> >>> this, they would have all that information available.
>> >>>
>> >>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> >>>>
>> >>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>> >>>> ---
>> >>>>
>> >>>>    common/usb_hub.c | 6 ++++--
>> >>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>> >>>>
>> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> >>>> index 3fb7e14d10..2e054eb935 100644
>> >>>> --- a/common/usb_hub.c
>> >>>> +++ b/common/usb_hub.c
>> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>> >>>> usb_hub_device *hub)
>> >>>>
>> >>>>        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(dev)) {
>> >>>
>> >>> Should this condition be "all which are lower than superspeed"
>> >>> instead ,
>> >>> so when the next generation of USB comes, this problem won't trigger
>> >>> ?
>> >>>
>> >>> What does Linux do btw ?
>> >>
>> >> As of now Linux checks if the hub is superspeed
>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>> >>
>> >> which is
>> >>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>> >> USB_HUB_PR_SS = 3
>> >>
>> >> This holds true for newer SuperSpeedPlus hubs as well.
>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>> >>
>> >> We can change the check to be  bDeviceProtocol > 2 but who knows if
>> >> things change in the newer version of spec.
>> >> I am open to suggestions.
>> >
>> > Please just include the ^ in the commit description. Use link to
>> > git.kernel.org , not some mirror . This is extremely useful
>> > information and, well, you already wrote the V2 commit message
>> > addition in this answer.
>> 
>> Shantur, if that would be easier or quicker for you, I can write
>> a quite detailed patch description for you, in exchange for a
>> "Helped-by" tag in the v2 patch submission. :)
> 
> That would be really kind of you Dragan.

Sure, I'll write the summary and send it over.

> I am down with the flu so that would really help me as my brain is
> working at 15% capacity.

Oh, I'm really sorry to hear that. :(  I hope you'll get better
soon, and I know very well what's it like;  I've also been sick
recently, as a result of some kind of flu that unfortunately found
its way into my lungs, and it took me about a month to get back
to about 90% of my usual mental capacity.  I'm still not back to
exactly 100%. :/

I really hope you'll recover much faster.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

Hello Shantur,

On 2024-02-08 15:17, Dragan Simic wrote:
> On 2024-02-08 15:10, Shantur Rathore wrote:
>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>>> On 2024-02-08 14:33, Marek Vasut wrote:
>>> > On 2/8/24 12:30, Shantur Rathore wrote:
>>> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>>> >>> On 2/7/24 11:23, Shantur Rathore wrote:
>>> >>>> USB 3.0 spec requires hub to reset device while
>>> >>>> enumeration. Some USB 2.0 hubs / devices don't
>>> >>>> handle this well and after implementation of
>>> >>>> reset some USB 2.0 disks weren't detected on
>>> >>>> Allwinner based boards.
>>> >>>>
>>> >>>> Resetting only when hub is USB 3.0 fixes it.
>>> >>>
>>> >>> It would be good to include as many details about the faulty hardware
>>> >>> in
>>> >>> the commit message as possible, so that when someone else runs into
>>> >>> this, they would have all that information available.
>>> >>>
>>> >>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>> >>>>
>>> >>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>> >>>> ---
>>> >>>>
>>> >>>>    common/usb_hub.c | 6 ++++--
>>> >>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>> >>>>
>>> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> >>>> index 3fb7e14d10..2e054eb935 100644
>>> >>>> --- a/common/usb_hub.c
>>> >>>> +++ b/common/usb_hub.c
>>> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>>> >>>> usb_hub_device *hub)
>>> >>>>
>>> >>>>        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(dev)) {
>>> >>>
>>> >>> Should this condition be "all which are lower than superspeed"
>>> >>> instead ,
>>> >>> so when the next generation of USB comes, this problem won't trigger
>>> >>> ?
>>> >>>
>>> >>> What does Linux do btw ?
>>> >>
>>> >> As of now Linux checks if the hub is superspeed
>>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>>> >>
>>> >> which is
>>> >>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>>> >> USB_HUB_PR_SS = 3
>>> >>
>>> >> This holds true for newer SuperSpeedPlus hubs as well.
>>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>>> >>
>>> >> We can change the check to be  bDeviceProtocol > 2 but who knows if
>>> >> things change in the newer version of spec.
>>> >> I am open to suggestions.
>>> >
>>> > Please just include the ^ in the commit description. Use link to
>>> > git.kernel.org , not some mirror . This is extremely useful
>>> > information and, well, you already wrote the V2 commit message
>>> > addition in this answer.
>>> 
>>> Shantur, if that would be easier or quicker for you, I can write
>>> a quite detailed patch description for you, in exchange for a
>>> "Helped-by" tag in the v2 patch submission. :)
>> 
>> That would be really kind of you Dragan.
> 
> Sure, I'll write the summary and send it over.
> 
>> I am down with the flu so that would really help me as my brain is
>> working at 15% capacity.
> 
> Oh, I'm really sorry to hear that. :(  I hope you'll get better
> soon, and I know very well what's it like;  I've also been sick
> recently, as a result of some kind of flu that unfortunately found
> its way into my lungs, and it took me about a month to get back
> to about 90% of my usual mental capacity.  I'm still not back to
> exactly 100%. :/
> 
> I really hope you'll recover much faster.

I hope you're feeling better.

Below are the patch subject and description that I prepared for you,
please have a look.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------
[PATCH v2] common: usb-hub: Reset USB 3.0 hubs only

Additional testing of the changes introduced in commit 33e06dcbe57a 
("common:
usb-hub: Reset hub port before scanning") revealed that some USB 3.0 
flash
drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0 
only.
More precisely, some tested USB 3.0 flash drives failed to be detected 
and
work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB 
2.0
only, while the same USB flash drives worked just fine on a Pine64 H64 
with
Allwinner H6 SoC, which supports both USB 2.0 and 3.0.

Resetting USB 3.0 hubs only has been tested to work as expected, 
resolving
the previous issues on the Allwinner H616, while not introducing any new
issues on other Allwinner SoCs.  Thus, let's fix it that way.

According to the USB 3.0 specification, resetting a USB 3.0 port is 
required
when an attached USB device transitions between different states, such 
as
when it resumes from suspend.  Though, the Linux kernel performs 
additional
USB 3.0 port resets upon initial USB device attachment, presumably to 
ensure
proper state of the USB 3.0 hub port and proper USB mode negotiation 
during
the initial USB device attachment and enumeration.

Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.

The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as 
visible
in file drivers/usb/core/hub.c in the kernel source, line 2859.  This 
check
also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well, 
which
hopefully makes it future proof.

Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
Link: 
https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
Link: 
https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
Signed-off-by: Shantur Rathore <i@shantur.com>
Helped-by: Dragan Simic <dsimic@manjaro.org>
Tested-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Dragan Simic <dsimic@manjaro.org>
------- >8 ------------- >8 ------------- >8 ------------- >8 -------

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Shantur Rathore]

Thanks Dragan,

On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Shantur,
>
> On 2024-02-08 15:17, Dragan Simic wrote:
> > On 2024-02-08 15:10, Shantur Rathore wrote:
> >> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
> >> wrote:
> >>> On 2024-02-08 14:33, Marek Vasut wrote:
> >>> > On 2/8/24 12:30, Shantur Rathore wrote:
> >>> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
> >>> >>> On 2/7/24 11:23, Shantur Rathore wrote:
> >>> >>>> USB 3.0 spec requires hub to reset device while
> >>> >>>> enumeration. Some USB 2.0 hubs / devices don't
> >>> >>>> handle this well and after implementation of
> >>> >>>> reset some USB 2.0 disks weren't detected on
> >>> >>>> Allwinner based boards.
> >>> >>>>
> >>> >>>> Resetting only when hub is USB 3.0 fixes it.
> >>> >>>
> >>> >>> It would be good to include as many details about the faulty hardware
> >>> >>> in
> >>> >>> the commit message as possible, so that when someone else runs into
> >>> >>> this, they would have all that information available.
> >>> >>>
> >>> >>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> >>> >>>>
> >>> >>>> Signed-off-by: Shantur Rathore <i@shantur.com>
> >>> >>>> ---
> >>> >>>>
> >>> >>>>    common/usb_hub.c | 6 ++++--
> >>> >>>>    1 file changed, 4 insertions(+), 2 deletions(-)
> >>> >>>>
> >>> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >>> >>>> index 3fb7e14d10..2e054eb935 100644
> >>> >>>> --- a/common/usb_hub.c
> >>> >>>> +++ b/common/usb_hub.c
> >>> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> >>> >>>> usb_hub_device *hub)
> >>> >>>>
> >>> >>>>        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(dev)) {
> >>> >>>
> >>> >>> Should this condition be "all which are lower than superspeed"
> >>> >>> instead ,
> >>> >>> so when the next generation of USB comes, this problem won't trigger
> >>> >>> ?
> >>> >>>
> >>> >>> What does Linux do btw ?
> >>> >>
> >>> >> As of now Linux checks if the hub is superspeed
> >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> >>> >>
> >>> >> which is
> >>> >>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> >>> >> USB_HUB_PR_SS = 3
> >>> >>
> >>> >> This holds true for newer SuperSpeedPlus hubs as well.
> >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> >>> >>
> >>> >> We can change the check to be  bDeviceProtocol > 2 but who knows if
> >>> >> things change in the newer version of spec.
> >>> >> I am open to suggestions.
> >>> >
> >>> > Please just include the ^ in the commit description. Use link to
> >>> > git.kernel.org , not some mirror . This is extremely useful
> >>> > information and, well, you already wrote the V2 commit message
> >>> > addition in this answer.
> >>>
> >>> Shantur, if that would be easier or quicker for you, I can write
> >>> a quite detailed patch description for you, in exchange for a
> >>> "Helped-by" tag in the v2 patch submission. :)
> >>
> >> That would be really kind of you Dragan.
> >
> > Sure, I'll write the summary and send it over.
> >
> >> I am down with the flu so that would really help me as my brain is
> >> working at 15% capacity.
> >
> > Oh, I'm really sorry to hear that. :(  I hope you'll get better
> > soon, and I know very well what's it like;  I've also been sick
> > recently, as a result of some kind of flu that unfortunately found
> > its way into my lungs, and it took me about a month to get back
> > to about 90% of my usual mental capacity.  I'm still not back to
> > exactly 100%. :/
> >
> > I really hope you'll recover much faster.
>
> I hope you're feeling better.
>
> Below are the patch subject and description that I prepared for you,
> please have a look.
>
> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>
> Additional testing of the changes introduced in commit 33e06dcbe57a
> ("common:
> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
> flash

I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.

> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
> only.
> More precisely, some tested USB 3.0 flash drives failed to be detected
> and
> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
> 2.0
> only, while the same USB flash drives worked just fine on a Pine64 H64
> with
> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>
> Resetting USB 3.0 hubs only has been tested to work as expected,
> resolving
> the previous issues on the Allwinner H616, while not introducing any new
> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>
> According to the USB 3.0 specification, resetting a USB 3.0 port is
> required
> when an attached USB device transitions between different states, such
> as
> when it resumes from suspend.  Though, the Linux kernel performs
> additional
> USB 3.0 port resets upon initial USB device attachment, presumably to
> ensure
> proper state of the USB 3.0 hub port and proper USB mode negotiation
> during
> the initial USB device attachment and enumeration.
>
> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
>
> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
> visible
> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
> check
> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
> which
> hopefully makes it future proof.
>
> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> Link:
> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
> Link:
> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
> Signed-off-by: Shantur Rathore <i@shantur.com>
> Helped-by: Dragan Simic <dsimic@manjaro.org>
> Tested-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------

Regards,
Shantur

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Shantur Rathore]

On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>
> Thanks Dragan,
>
> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > Hello Shantur,
> >
> > On 2024-02-08 15:17, Dragan Simic wrote:
> > > On 2024-02-08 15:10, Shantur Rathore wrote:
> > >> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
> > >> wrote:
> > >>> On 2024-02-08 14:33, Marek Vasut wrote:
> > >>> > On 2/8/24 12:30, Shantur Rathore wrote:
> > >>> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
> > >>> >>> On 2/7/24 11:23, Shantur Rathore wrote:
> > >>> >>>> USB 3.0 spec requires hub to reset device while
> > >>> >>>> enumeration. Some USB 2.0 hubs / devices don't
> > >>> >>>> handle this well and after implementation of
> > >>> >>>> reset some USB 2.0 disks weren't detected on
> > >>> >>>> Allwinner based boards.
> > >>> >>>>
> > >>> >>>> Resetting only when hub is USB 3.0 fixes it.
> > >>> >>>
> > >>> >>> It would be good to include as many details about the faulty hardware
> > >>> >>> in
> > >>> >>> the commit message as possible, so that when someone else runs into
> > >>> >>> this, they would have all that information available.
> > >>> >>>
> > >>> >>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> > >>> >>>>
> > >>> >>>> Signed-off-by: Shantur Rathore <i@shantur.com>
> > >>> >>>> ---
> > >>> >>>>
> > >>> >>>>    common/usb_hub.c | 6 ++++--
> > >>> >>>>    1 file changed, 4 insertions(+), 2 deletions(-)
> > >>> >>>>
> > >>> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> > >>> >>>> index 3fb7e14d10..2e054eb935 100644
> > >>> >>>> --- a/common/usb_hub.c
> > >>> >>>> +++ b/common/usb_hub.c
> > >>> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> > >>> >>>> usb_hub_device *hub)
> > >>> >>>>
> > >>> >>>>        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(dev)) {
> > >>> >>>
> > >>> >>> Should this condition be "all which are lower than superspeed"
> > >>> >>> instead ,
> > >>> >>> so when the next generation of USB comes, this problem won't trigger
> > >>> >>> ?
> > >>> >>>
> > >>> >>> What does Linux do btw ?
> > >>> >>
> > >>> >> As of now Linux checks if the hub is superspeed
> > >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> > >>> >>
> > >>> >> which is
> > >>> >>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> > >>> >> USB_HUB_PR_SS = 3
> > >>> >>
> > >>> >> This holds true for newer SuperSpeedPlus hubs as well.
> > >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> > >>> >>
> > >>> >> We can change the check to be  bDeviceProtocol > 2 but who knows if
> > >>> >> things change in the newer version of spec.
> > >>> >> I am open to suggestions.
> > >>> >
> > >>> > Please just include the ^ in the commit description. Use link to
> > >>> > git.kernel.org , not some mirror . This is extremely useful
> > >>> > information and, well, you already wrote the V2 commit message
> > >>> > addition in this answer.
> > >>>
> > >>> Shantur, if that would be easier or quicker for you, I can write
> > >>> a quite detailed patch description for you, in exchange for a
> > >>> "Helped-by" tag in the v2 patch submission. :)
> > >>
> > >> That would be really kind of you Dragan.
> > >
> > > Sure, I'll write the summary and send it over.
> > >
> > >> I am down with the flu so that would really help me as my brain is
> > >> working at 15% capacity.
> > >
> > > Oh, I'm really sorry to hear that. :(  I hope you'll get better
> > > soon, and I know very well what's it like;  I've also been sick
> > > recently, as a result of some kind of flu that unfortunately found
> > > its way into my lungs, and it took me about a month to get back
> > > to about 90% of my usual mental capacity.  I'm still not back to
> > > exactly 100%. :/
> > >
> > > I really hope you'll recover much faster.
> >
> > I hope you're feeling better.
> >
> > Below are the patch subject and description that I prepared for you,
> > please have a look.
> >
> > ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> > [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
> >
> > Additional testing of the changes introduced in commit 33e06dcbe57a
> > ("common:
> > usb-hub: Reset hub port before scanning") revealed that some USB 3.0
> > flash
>
> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>
> > drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
> > only.
> > More precisely, some tested USB 3.0 flash drives failed to be detected
> > and
> > work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
> > 2.0
> > only, while the same USB flash drives worked just fine on a Pine64 H64
> > with
> > Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
> >
> > Resetting USB 3.0 hubs only has been tested to work as expected,
> > resolving
> > the previous issues on the Allwinner H616, while not introducing any new
> > issues on other Allwinner SoCs.  Thus, let's fix it that way.
> >
> > According to the USB 3.0 specification, resetting a USB 3.0 port is
> > required
> > when an attached USB device transitions between different states, such
> > as
> > when it resumes from suspend.  Though, the Linux kernel performs
> > additional
> > USB 3.0 port resets upon initial USB device attachment, presumably to
> > ensure
> > proper state of the USB 3.0 hub port and proper USB mode negotiation
> > during
> > the initial USB device attachment and enumeration.
> >
> > Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
> >
> > The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
> > visible
> > in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
> > check
> > also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
> > which
> > hopefully makes it future proof.
> >
> > Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> > Link:
> > https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
> > Link:
> > https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
> > Signed-off-by: Shantur Rathore <i@shantur.com>
> > Helped-by: Dragan Simic <dsimic@manjaro.org>
> > Tested-by: Andre Przywara <andre.przywara@arm.com>
> > Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> > ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>
> Regards,
> Shantur

Is this description acceptable to you Marek.

Thanks,
Shantur

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Marek Vasut]

On 2/12/24 14:41, Shantur Rathore wrote:
> On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>>
>> Thanks Dragan,
>>
>> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
>>>
>>> Hello Shantur,
>>>
>>> On 2024-02-08 15:17, Dragan Simic wrote:
>>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>>>>> wrote:
>>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>>>>>>>>> USB 3.0 spec requires hub to reset device while
>>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>>>>>>>>>> handle this well and after implementation of
>>>>>>>>>> reset some USB 2.0 disks weren't detected on
>>>>>>>>>> Allwinner based boards.
>>>>>>>>>>
>>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>>>>>>>>>
>>>>>>>>> It would be good to include as many details about the faulty hardware
>>>>>>>>> in
>>>>>>>>> the commit message as possible, so that when someone else runs into
>>>>>>>>> this, they would have all that information available.
>>>>>>>>>
>>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>>     common/usb_hub.c | 6 ++++--
>>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>>>>>>>>>> --- a/common/usb_hub.c
>>>>>>>>>> +++ b/common/usb_hub.c
>>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>>>>>>>>>> usb_hub_device *hub)
>>>>>>>>>>
>>>>>>>>>>         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(dev)) {
>>>>>>>>>
>>>>>>>>> Should this condition be "all which are lower than superspeed"
>>>>>>>>> instead ,
>>>>>>>>> so when the next generation of USB comes, this problem won't trigger
>>>>>>>>> ?
>>>>>>>>>
>>>>>>>>> What does Linux do btw ?
>>>>>>>>
>>>>>>>> As of now Linux checks if the hub is superspeed
>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>>>>>>>>
>>>>>>>> which is
>>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>>>>>>>> USB_HUB_PR_SS = 3
>>>>>>>>
>>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>>>>>>>>
>>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who knows if
>>>>>>>> things change in the newer version of spec.
>>>>>>>> I am open to suggestions.
>>>>>>>
>>>>>>> Please just include the ^ in the commit description. Use link to
>>>>>>> git.kernel.org , not some mirror . This is extremely useful
>>>>>>> information and, well, you already wrote the V2 commit message
>>>>>>> addition in this answer.
>>>>>>
>>>>>> Shantur, if that would be easier or quicker for you, I can write
>>>>>> a quite detailed patch description for you, in exchange for a
>>>>>> "Helped-by" tag in the v2 patch submission. :)
>>>>>
>>>>> That would be really kind of you Dragan.
>>>>
>>>> Sure, I'll write the summary and send it over.
>>>>
>>>>> I am down with the flu so that would really help me as my brain is
>>>>> working at 15% capacity.
>>>>
>>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>>>> soon, and I know very well what's it like;  I've also been sick
>>>> recently, as a result of some kind of flu that unfortunately found
>>>> its way into my lungs, and it took me about a month to get back
>>>> to about 90% of my usual mental capacity.  I'm still not back to
>>>> exactly 100%. :/
>>>>
>>>> I really hope you'll recover much faster.
>>>
>>> I hope you're feeling better.
>>>
>>> Below are the patch subject and description that I prepared for you,
>>> please have a look.
>>>
>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>>>
>>> Additional testing of the changes introduced in commit 33e06dcbe57a
>>> ("common:
>>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>>> flash
>>
>> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>>
>>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
>>> only.
>>> More precisely, some tested USB 3.0 flash drives failed to be detected
>>> and
>>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
>>> 2.0
>>> only, while the same USB flash drives worked just fine on a Pine64 H64
>>> with
>>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>>>
>>> Resetting USB 3.0 hubs only has been tested to work as expected,
>>> resolving
>>> the previous issues on the Allwinner H616, while not introducing any new
>>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>>>
>>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>>> required
>>> when an attached USB device transitions between different states, such
>>> as
>>> when it resumes from suspend.  Though, the Linux kernel performs
>>> additional
>>> USB 3.0 port resets upon initial USB device attachment, presumably to
>>> ensure
>>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>>> during
>>> the initial USB device attachment and enumeration.
>>>
>>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
>>>
>>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>>> visible
>>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
>>> check
>>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>>> which
>>> hopefully makes it future proof.
>>>
>>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
>>> Link:
>>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>>> Link:
>>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>
>> Regards,
>> Shantur
> 
> Is this description acceptable to you Marek.

Please send a V2 patch . If possible, include the device information as 
reported by Andre, esp. which USB stick triggered it, including USB IDs, 
this is important for future reference and in case someone has similar 
failure.

Please don't use "in the kernel source, line 2859", considering the rate 
of change of the Linux kernel, it is best to also include exact commit 
ID as of which this is a line 2859 and spell out this is referring to 
Linux kernel.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

Hello Marek, Andre and Shantur,

On 2024-02-12 21:19, Marek Vasut wrote:
> On 2/12/24 14:41, Shantur Rathore wrote:
>> On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>>> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> 
>>> wrote:
>>>> On 2024-02-08 15:17, Dragan Simic wrote:
>>>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>>>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>>>>>> wrote:
>>>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>>>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>>>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> 
>>>>>>>>> wrote:
>>>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>>>>>>>>>> USB 3.0 spec requires hub to reset device while
>>>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>>>>>>>>>>> handle this well and after implementation of
>>>>>>>>>>> reset some USB 2.0 disks weren't detected on
>>>>>>>>>>> Allwinner based boards.
>>>>>>>>>>> 
>>>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>>>>>>>>>> 
>>>>>>>>>> It would be good to include as many details about the faulty 
>>>>>>>>>> hardware
>>>>>>>>>> in
>>>>>>>>>> the commit message as possible, so that when someone else runs 
>>>>>>>>>> into
>>>>>>>>>> this, they would have all that information available.
>>>>>>>>>> 
>>>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>>>>>>>> 
>>>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>>>>>>>> ---
>>>>>>>>>>> 
>>>>>>>>>>>     common/usb_hub.c | 6 ++++--
>>>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>>>>> 
>>>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>>>>>>>>>>> --- a/common/usb_hub.c
>>>>>>>>>>> +++ b/common/usb_hub.c
>>>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>>>>>>>>>>> usb_hub_device *hub)
>>>>>>>>>>> 
>>>>>>>>>>>         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(dev)) {
>>>>>>>>>> 
>>>>>>>>>> Should this condition be "all which are lower than superspeed"
>>>>>>>>>> instead ,
>>>>>>>>>> so when the next generation of USB comes, this problem won't 
>>>>>>>>>> trigger
>>>>>>>>>> ?
>>>>>>>>>> 
>>>>>>>>>> What does Linux do btw ?
>>>>>>>>> 
>>>>>>>>> As of now Linux checks if the hub is superspeed
>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>>>>>>>>> 
>>>>>>>>> which is
>>>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>>>>>>>>> USB_HUB_PR_SS = 3
>>>>>>>>> 
>>>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>>>>>>>>> 
>>>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who 
>>>>>>>>> knows if
>>>>>>>>> things change in the newer version of spec.
>>>>>>>>> I am open to suggestions.
>>>>>>>> 
>>>>>>>> Please just include the ^ in the commit description. Use link to
>>>>>>>> git.kernel.org , not some mirror . This is extremely useful
>>>>>>>> information and, well, you already wrote the V2 commit message
>>>>>>>> addition in this answer.
>>>>>>> 
>>>>>>> Shantur, if that would be easier or quicker for you, I can write
>>>>>>> a quite detailed patch description for you, in exchange for a
>>>>>>> "Helped-by" tag in the v2 patch submission. :)
>>>>>> 
>>>>>> That would be really kind of you Dragan.
>>>>> 
>>>>> Sure, I'll write the summary and send it over.
>>>>> 
>>>>>> I am down with the flu so that would really help me as my brain is
>>>>>> working at 15% capacity.
>>>>> 
>>>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>>>>> soon, and I know very well what's it like;  I've also been sick
>>>>> recently, as a result of some kind of flu that unfortunately found
>>>>> its way into my lungs, and it took me about a month to get back
>>>>> to about 90% of my usual mental capacity.  I'm still not back to
>>>>> exactly 100%. :/
>>>>> 
>>>>> I really hope you'll recover much faster.
>>>> 
>>>> I hope you're feeling better.
>>>> 
>>>> Below are the patch subject and description that I prepared for you,
>>>> please have a look.
>>>> 
>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>> -------
>>>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>>>> 
>>>> Additional testing of the changes introduced in commit 33e06dcbe57a
>>>> ("common:
>>>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>>>> flash
>>> 
>>> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>>> 
>>>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 
>>>> 2.0
>>>> only.
>>>> More precisely, some tested USB 3.0 flash drives failed to be 
>>>> detected
>>>> and
>>>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports 
>>>> USB
>>>> 2.0
>>>> only, while the same USB flash drives worked just fine on a Pine64 
>>>> H64
>>>> with
>>>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>>>> 
>>>> Resetting USB 3.0 hubs only has been tested to work as expected,
>>>> resolving
>>>> the previous issues on the Allwinner H616, while not introducing any 
>>>> new
>>>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>>>> 
>>>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>>>> required
>>>> when an attached USB device transitions between different states, 
>>>> such
>>>> as
>>>> when it resumes from suspend.  Though, the Linux kernel performs
>>>> additional
>>>> USB 3.0 port resets upon initial USB device attachment, presumably 
>>>> to
>>>> ensure
>>>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>>>> during
>>>> the initial USB device attachment and enumeration.
>>>> 
>>>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 
>>>> 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.
>>>> 
>>>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>>>> visible
>>>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  
>>>> This
>>>> check
>>>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>>>> which
>>>> hopefully makes it future proof.
>>>> 
>>>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before 
>>>> scanning")
>>>> Link:
>>>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>>>> Link:
>>>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>> -------
>>> 
>>> Regards,
>>> Shantur
>> 
>> Is this description acceptable to you Marek.
> 
> Please send a V2 patch . If possible, include the device information
> as reported by Andre, esp. which USB stick triggered it, including USB
> IDs, this is important for future reference and in case someone has
> similar failure.

If Andre can supply the USB IDs, I'm willing to help Shantur by
adjusting the wording of the patch description to include them.

> Please don't use "in the kernel source, line 2859", considering the
> rate of change of the Linux kernel, it is best to also include exact
> commit ID as of which this is a line 2859 and spell out this is
> referring to Linux kernel.

Good point.  I'm willing to adjust the wording.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Marek Vasut]

On 2/12/24 22:10, Dragan Simic wrote:
> Hello Marek, Andre and Shantur,
> 
> On 2024-02-12 21:19, Marek Vasut wrote:
>> On 2/12/24 14:41, Shantur Rathore wrote:
>>> On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>>>> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> 
>>>> wrote:
>>>>> On 2024-02-08 15:17, Dragan Simic wrote:
>>>>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>>>>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>>>>>>> wrote:
>>>>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>>>>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>>>>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>>>>>>>>>>> USB 3.0 spec requires hub to reset device while
>>>>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>>>>>>>>>>>> handle this well and after implementation of
>>>>>>>>>>>> reset some USB 2.0 disks weren't detected on
>>>>>>>>>>>> Allwinner based boards.
>>>>>>>>>>>>
>>>>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>>>>>>>>>>>
>>>>>>>>>>> It would be good to include as many details about the faulty 
>>>>>>>>>>> hardware
>>>>>>>>>>> in
>>>>>>>>>>> the commit message as possible, so that when someone else 
>>>>>>>>>>> runs into
>>>>>>>>>>> this, they would have all that information available.
>>>>>>>>>>>
>>>>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>>     common/usb_hub.c | 6 ++++--
>>>>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>>>>>>>>>>>> --- a/common/usb_hub.c
>>>>>>>>>>>> +++ b/common/usb_hub.c
>>>>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>>>>>>>>>>>> usb_hub_device *hub)
>>>>>>>>>>>>
>>>>>>>>>>>>         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(dev)) {
>>>>>>>>>>>
>>>>>>>>>>> Should this condition be "all which are lower than superspeed"
>>>>>>>>>>> instead ,
>>>>>>>>>>> so when the next generation of USB comes, this problem won't 
>>>>>>>>>>> trigger
>>>>>>>>>>> ?
>>>>>>>>>>>
>>>>>>>>>>> What does Linux do btw ?
>>>>>>>>>>
>>>>>>>>>> As of now Linux checks if the hub is superspeed
>>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>>>>>>>>>>
>>>>>>>>>> which is
>>>>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>>>>>>>>>> USB_HUB_PR_SS = 3
>>>>>>>>>>
>>>>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>>>>>>>>>>
>>>>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who 
>>>>>>>>>> knows if
>>>>>>>>>> things change in the newer version of spec.
>>>>>>>>>> I am open to suggestions.
>>>>>>>>>
>>>>>>>>> Please just include the ^ in the commit description. Use link to
>>>>>>>>> git.kernel.org , not some mirror . This is extremely useful
>>>>>>>>> information and, well, you already wrote the V2 commit message
>>>>>>>>> addition in this answer.
>>>>>>>>
>>>>>>>> Shantur, if that would be easier or quicker for you, I can write
>>>>>>>> a quite detailed patch description for you, in exchange for a
>>>>>>>> "Helped-by" tag in the v2 patch submission. :)
>>>>>>>
>>>>>>> That would be really kind of you Dragan.
>>>>>>
>>>>>> Sure, I'll write the summary and send it over.
>>>>>>
>>>>>>> I am down with the flu so that would really help me as my brain is
>>>>>>> working at 15% capacity.
>>>>>>
>>>>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>>>>>> soon, and I know very well what's it like;  I've also been sick
>>>>>> recently, as a result of some kind of flu that unfortunately found
>>>>>> its way into my lungs, and it took me about a month to get back
>>>>>> to about 90% of my usual mental capacity.  I'm still not back to
>>>>>> exactly 100%. :/
>>>>>>
>>>>>> I really hope you'll recover much faster.
>>>>>
>>>>> I hope you're feeling better.
>>>>>
>>>>> Below are the patch subject and description that I prepared for you,
>>>>> please have a look.
>>>>>
>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>>>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>>>>>
>>>>> Additional testing of the changes introduced in commit 33e06dcbe57a
>>>>> ("common:
>>>>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>>>>> flash
>>>>
>>>> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>>>>
>>>>> drives didn't work in U-Boot on some Allwinner SoCs that support 
>>>>> USB 2.0
>>>>> only.
>>>>> More precisely, some tested USB 3.0 flash drives failed to be detected
>>>>> and
>>>>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
>>>>> 2.0
>>>>> only, while the same USB flash drives worked just fine on a Pine64 H64
>>>>> with
>>>>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>>>>>
>>>>> Resetting USB 3.0 hubs only has been tested to work as expected,
>>>>> resolving
>>>>> the previous issues on the Allwinner H616, while not introducing 
>>>>> any new
>>>>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>>>>>
>>>>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>>>>> required
>>>>> when an attached USB device transitions between different states, such
>>>>> as
>>>>> when it resumes from suspend.  Though, the Linux kernel performs
>>>>> additional
>>>>> USB 3.0 port resets upon initial USB device attachment, presumably to
>>>>> ensure
>>>>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>>>>> during
>>>>> the initial USB device attachment and enumeration.
>>>>>
>>>>> Such USB port resets don't seem to exist for USB 2.0 hubs, 
>>>>> according 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.
>>>>>
>>>>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>>>>> visible
>>>>> in file drivers/usb/core/hub.c in the kernel source, line 2859. This
>>>>> check
>>>>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>>>>> which
>>>>> hopefully makes it future proof.
>>>>>
>>>>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before 
>>>>> scanning")
>>>>> Link:
>>>>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>>>>> Link:
>>>>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>>>
>>>> Regards,
>>>> Shantur
>>>
>>> Is this description acceptable to you Marek.
>>
>> Please send a V2 patch . If possible, include the device information
>> as reported by Andre, esp. which USB stick triggered it, including USB
>> IDs, this is important for future reference and in case someone has
>> similar failure.
> 
> If Andre can supply the USB IDs, I'm willing to help Shantur by
> adjusting the wording of the patch description to include them.

That's what I'm hoping is gonna happen , thanks !

>> Please don't use "in the kernel source, line 2859", considering the
>> rate of change of the Linux kernel, it is best to also include exact
>> commit ID as of which this is a line 2859 and spell out this is
>> referring to Linux kernel.
> 
> Good point.  I'm willing to adjust the wording.

Thanks .

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

On 2024-02-13 00:33, Marek Vasut wrote:
> On 2/12/24 22:10, Dragan Simic wrote:
>> On 2024-02-12 21:19, Marek Vasut wrote:
>>> On 2/12/24 14:41, Shantur Rathore wrote:
>>>> On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> 
>>>> wrote:
>>>>> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> 
>>>>> wrote:
>>>>>> On 2024-02-08 15:17, Dragan Simic wrote:
>>>>>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>>>>>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>>>>>>>> wrote:
>>>>>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>>>>>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>>>>>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> 
>>>>>>>>>>> wrote:
>>>>>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>>>>>>>>>>>> USB 3.0 spec requires hub to reset device while
>>>>>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>>>>>>>>>>>>> handle this well and after implementation of
>>>>>>>>>>>>> reset some USB 2.0 disks weren't detected on
>>>>>>>>>>>>> Allwinner based boards.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>>>>>>>>>>>> 
>>>>>>>>>>>> It would be good to include as many details about the faulty 
>>>>>>>>>>>> hardware
>>>>>>>>>>>> in
>>>>>>>>>>>> the commit message as possible, so that when someone else 
>>>>>>>>>>>> runs into
>>>>>>>>>>>> this, they would have all that information available.
>>>>>>>>>>>> 
>>>>>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> 
>>>>>>>>>>>>>     common/usb_hub.c | 6 ++++--
>>>>>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>>>>>>> 
>>>>>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>>>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>>>>>>>>>>>>> --- a/common/usb_hub.c
>>>>>>>>>>>>> +++ b/common/usb_hub.c
>>>>>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>>>>>>>>>>>>> usb_hub_device *hub)
>>>>>>>>>>>>> 
>>>>>>>>>>>>>         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(dev)) {
>>>>>>>>>>>> 
>>>>>>>>>>>> Should this condition be "all which are lower than 
>>>>>>>>>>>> superspeed"
>>>>>>>>>>>> instead ,
>>>>>>>>>>>> so when the next generation of USB comes, this problem won't 
>>>>>>>>>>>> trigger
>>>>>>>>>>>> ?
>>>>>>>>>>>> 
>>>>>>>>>>>> What does Linux do btw ?
>>>>>>>>>>> 
>>>>>>>>>>> As of now Linux checks if the hub is superspeed
>>>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>>>>>>>>>>> 
>>>>>>>>>>> which is
>>>>>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; 
>>>>>>>>>>> //
>>>>>>>>>>> USB_HUB_PR_SS = 3
>>>>>>>>>>> 
>>>>>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>>>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>>>>>>>>>>> 
>>>>>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who 
>>>>>>>>>>> knows if
>>>>>>>>>>> things change in the newer version of spec.
>>>>>>>>>>> I am open to suggestions.
>>>>>>>>>> 
>>>>>>>>>> Please just include the ^ in the commit description. Use link 
>>>>>>>>>> to
>>>>>>>>>> git.kernel.org , not some mirror . This is extremely useful
>>>>>>>>>> information and, well, you already wrote the V2 commit message
>>>>>>>>>> addition in this answer.
>>>>>>>>> 
>>>>>>>>> Shantur, if that would be easier or quicker for you, I can 
>>>>>>>>> write
>>>>>>>>> a quite detailed patch description for you, in exchange for a
>>>>>>>>> "Helped-by" tag in the v2 patch submission. :)
>>>>>>>> 
>>>>>>>> That would be really kind of you Dragan.
>>>>>>> 
>>>>>>> Sure, I'll write the summary and send it over.
>>>>>>> 
>>>>>>>> I am down with the flu so that would really help me as my brain 
>>>>>>>> is
>>>>>>>> working at 15% capacity.
>>>>>>> 
>>>>>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>>>>>>> soon, and I know very well what's it like;  I've also been sick
>>>>>>> recently, as a result of some kind of flu that unfortunately 
>>>>>>> found
>>>>>>> its way into my lungs, and it took me about a month to get back
>>>>>>> to about 90% of my usual mental capacity.  I'm still not back to
>>>>>>> exactly 100%. :/
>>>>>>> 
>>>>>>> I really hope you'll recover much faster.
>>>>>> 
>>>>>> I hope you're feeling better.
>>>>>> 
>>>>>> Below are the patch subject and description that I prepared for 
>>>>>> you,
>>>>>> please have a look.
>>>>>> 
>>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>>>> -------
>>>>>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>>>>>> 
>>>>>> Additional testing of the changes introduced in commit 
>>>>>> 33e06dcbe57a
>>>>>> ("common:
>>>>>> usb-hub: Reset hub port before scanning") revealed that some USB 
>>>>>> 3.0
>>>>>> flash
>>>>> 
>>>>> I think it was a USB 2.0 drive that didn't work on the USB 2.0 
>>>>> port.
>>>>> 
>>>>>> drives didn't work in U-Boot on some Allwinner SoCs that support 
>>>>>> USB 2.0
>>>>>> only.
>>>>>> More precisely, some tested USB 3.0 flash drives failed to be 
>>>>>> detected
>>>>>> and
>>>>>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports 
>>>>>> USB
>>>>>> 2.0
>>>>>> only, while the same USB flash drives worked just fine on a Pine64 
>>>>>> H64
>>>>>> with
>>>>>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>>>>>> 
>>>>>> Resetting USB 3.0 hubs only has been tested to work as expected,
>>>>>> resolving
>>>>>> the previous issues on the Allwinner H616, while not introducing 
>>>>>> any new
>>>>>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>>>>>> 
>>>>>> According to the USB 3.0 specification, resetting a USB 3.0 port 
>>>>>> is
>>>>>> required
>>>>>> when an attached USB device transitions between different states, 
>>>>>> such
>>>>>> as
>>>>>> when it resumes from suspend.  Though, the Linux kernel performs
>>>>>> additional
>>>>>> USB 3.0 port resets upon initial USB device attachment, presumably 
>>>>>> to
>>>>>> ensure
>>>>>> proper state of the USB 3.0 hub port and proper USB mode 
>>>>>> negotiation
>>>>>> during
>>>>>> the initial USB device attachment and enumeration.
>>>>>> 
>>>>>> Such USB port resets don't seem to exist for USB 2.0 hubs, 
>>>>>> according 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.
>>>>>> 
>>>>>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, 
>>>>>> as
>>>>>> visible
>>>>>> in file drivers/usb/core/hub.c in the kernel source, line 2859. 
>>>>>> This
>>>>>> check
>>>>>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as 
>>>>>> well,
>>>>>> which
>>>>>> hopefully makes it future proof.
>>>>>> 
>>>>>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before 
>>>>>> scanning")
>>>>>> Link:
>>>>>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>>>>>> Link:
>>>>>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>>>> -------
>>>>> 
>>>>> Regards,
>>>>> Shantur
>>>> 
>>>> Is this description acceptable to you Marek.
>>> 
>>> Please send a V2 patch . If possible, include the device information
>>> as reported by Andre, esp. which USB stick triggered it, including 
>>> USB
>>> IDs, this is important for future reference and in case someone has
>>> similar failure.
>> 
>> If Andre can supply the USB IDs, I'm willing to help Shantur by
>> adjusting the wording of the patch description to include them.
> 
> That's what I'm hoping is gonna happen , thanks !
> 
>>> Please don't use "in the kernel source, line 2859", considering the
>>> rate of change of the Linux kernel, it is best to also include exact
>>> commit ID as of which this is a line 2859 and spell out this is
>>> referring to Linux kernel.
>> 
>> Good point.  I'm willing to adjust the wording.
> 
> Thanks .

I'm glad to help, and I'll be happy to see this patch accepted, with
properly detailed description for future reference.

I've already adjusted the wording of the description, and I'll send
the adjusted version over after Andre provides the USB IDs of the USB
flash drives that initially failed to work.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Marek Vasut]

On 2/13/24 04:59, Dragan Simic wrote:
> On 2024-02-13 00:33, Marek Vasut wrote:
>> On 2/12/24 22:10, Dragan Simic wrote:
>>> On 2024-02-12 21:19, Marek Vasut wrote:
>>>> On 2/12/24 14:41, Shantur Rathore wrote:
>>>>> On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>>>>>> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> 
>>>>>> wrote:
>>>>>>> On 2024-02-08 15:17, Dragan Simic wrote:
>>>>>>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>>>>>>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>>>>>>>>> wrote:
>>>>>>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>>>>>>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>>>>>>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> 
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>>>>>>>>>>>>> USB 3.0 spec requires hub to reset device while
>>>>>>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>>>>>>>>>>>>>> handle this well and after implementation of
>>>>>>>>>>>>>> reset some USB 2.0 disks weren't detected on
>>>>>>>>>>>>>> Allwinner based boards.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It would be good to include as many details about the 
>>>>>>>>>>>>> faulty hardware
>>>>>>>>>>>>> in
>>>>>>>>>>>>> the commit message as possible, so that when someone else 
>>>>>>>>>>>>> runs into
>>>>>>>>>>>>> this, they would have all that information available.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>     common/usb_hub.c | 6 ++++--
>>>>>>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>>>>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>>>>>>>>>>>>>> --- a/common/usb_hub.c
>>>>>>>>>>>>>> +++ b/common/usb_hub.c
>>>>>>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>>>>>>>>>>>>>> usb_hub_device *hub)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>         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(dev)) {
>>>>>>>>>>>>>
>>>>>>>>>>>>> Should this condition be "all which are lower than superspeed"
>>>>>>>>>>>>> instead ,
>>>>>>>>>>>>> so when the next generation of USB comes, this problem 
>>>>>>>>>>>>> won't trigger
>>>>>>>>>>>>> ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> What does Linux do btw ?
>>>>>>>>>>>>
>>>>>>>>>>>> As of now Linux checks if the hub is superspeed
>>>>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>>>>>>>>>>>>
>>>>>>>>>>>> which is
>>>>>>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>>>>>>>>>>>> USB_HUB_PR_SS = 3
>>>>>>>>>>>>
>>>>>>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>>>>>>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>>>>>>>>>>>>
>>>>>>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who 
>>>>>>>>>>>> knows if
>>>>>>>>>>>> things change in the newer version of spec.
>>>>>>>>>>>> I am open to suggestions.
>>>>>>>>>>>
>>>>>>>>>>> Please just include the ^ in the commit description. Use link to
>>>>>>>>>>> git.kernel.org , not some mirror . This is extremely useful
>>>>>>>>>>> information and, well, you already wrote the V2 commit message
>>>>>>>>>>> addition in this answer.
>>>>>>>>>>
>>>>>>>>>> Shantur, if that would be easier or quicker for you, I can write
>>>>>>>>>> a quite detailed patch description for you, in exchange for a
>>>>>>>>>> "Helped-by" tag in the v2 patch submission. :)
>>>>>>>>>
>>>>>>>>> That would be really kind of you Dragan.
>>>>>>>>
>>>>>>>> Sure, I'll write the summary and send it over.
>>>>>>>>
>>>>>>>>> I am down with the flu so that would really help me as my brain is
>>>>>>>>> working at 15% capacity.
>>>>>>>>
>>>>>>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>>>>>>>> soon, and I know very well what's it like;  I've also been sick
>>>>>>>> recently, as a result of some kind of flu that unfortunately found
>>>>>>>> its way into my lungs, and it took me about a month to get back
>>>>>>>> to about 90% of my usual mental capacity.  I'm still not back to
>>>>>>>> exactly 100%. :/
>>>>>>>>
>>>>>>>> I really hope you'll recover much faster.
>>>>>>>
>>>>>>> I hope you're feeling better.
>>>>>>>
>>>>>>> Below are the patch subject and description that I prepared for you,
>>>>>>> please have a look.
>>>>>>>
>>>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>>>>> -------
>>>>>>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>>>>>>>
>>>>>>> Additional testing of the changes introduced in commit 33e06dcbe57a
>>>>>>> ("common:
>>>>>>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>>>>>>> flash
>>>>>>
>>>>>> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>>>>>>
>>>>>>> drives didn't work in U-Boot on some Allwinner SoCs that support 
>>>>>>> USB 2.0
>>>>>>> only.
>>>>>>> More precisely, some tested USB 3.0 flash drives failed to be 
>>>>>>> detected
>>>>>>> and
>>>>>>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which 
>>>>>>> supports USB
>>>>>>> 2.0
>>>>>>> only, while the same USB flash drives worked just fine on a 
>>>>>>> Pine64 H64
>>>>>>> with
>>>>>>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>>>>>>>
>>>>>>> Resetting USB 3.0 hubs only has been tested to work as expected,
>>>>>>> resolving
>>>>>>> the previous issues on the Allwinner H616, while not introducing 
>>>>>>> any new
>>>>>>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>>>>>>>
>>>>>>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>>>>>>> required
>>>>>>> when an attached USB device transitions between different states, 
>>>>>>> such
>>>>>>> as
>>>>>>> when it resumes from suspend.  Though, the Linux kernel performs
>>>>>>> additional
>>>>>>> USB 3.0 port resets upon initial USB device attachment, 
>>>>>>> presumably to
>>>>>>> ensure
>>>>>>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>>>>>>> during
>>>>>>> the initial USB device attachment and enumeration.
>>>>>>>
>>>>>>> Such USB port resets don't seem to exist for USB 2.0 hubs, 
>>>>>>> according 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.
>>>>>>>
>>>>>>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>>>>>>> visible
>>>>>>> in file drivers/usb/core/hub.c in the kernel source, line 2859. This
>>>>>>> check
>>>>>>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>>>>>>> which
>>>>>>> hopefully makes it future proof.
>>>>>>>
>>>>>>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before 
>>>>>>> scanning")
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>>>>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>>>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>>>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>>>>> -------
>>>>>>
>>>>>> Regards,
>>>>>> Shantur
>>>>>
>>>>> Is this description acceptable to you Marek.
>>>>
>>>> Please send a V2 patch . If possible, include the device information
>>>> as reported by Andre, esp. which USB stick triggered it, including USB
>>>> IDs, this is important for future reference and in case someone has
>>>> similar failure.
>>>
>>> If Andre can supply the USB IDs, I'm willing to help Shantur by
>>> adjusting the wording of the patch description to include them.
>>
>> That's what I'm hoping is gonna happen , thanks !
>>
>>>> Please don't use "in the kernel source, line 2859", considering the
>>>> rate of change of the Linux kernel, it is best to also include exact
>>>> commit ID as of which this is a line 2859 and spell out this is
>>>> referring to Linux kernel.
>>>
>>> Good point.  I'm willing to adjust the wording.
>>
>> Thanks .
> 
> I'm glad to help, and I'll be happy to see this patch accepted, with
> properly detailed description for future reference.
> 
> I've already adjusted the wording of the description, and I'll send
> the adjusted version over after Andre provides the USB IDs of the USB
> flash drives that initially failed to work.

Thanks !

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Andre Przywara]

On Mon, 12 Feb 2024 21:19:13 +0100
Marek Vasut <marex@denx.de> wrote:

> On 2/12/24 14:41, Shantur Rathore wrote:
> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:  
> >>
> >> Thanks Dragan,
> >>
> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:  
> >>>
> >>> Hello Shantur,
> >>>
> >>> On 2024-02-08 15:17, Dragan Simic wrote:  
> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:  
> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
> >>>>> wrote:  
> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:  
> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:  
> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:  
> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:  
> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
> >>>>>>>>>> handle this well and after implementation of
> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
> >>>>>>>>>> Allwinner based boards.
> >>>>>>>>>>
> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.  
> >>>>>>>>>
> >>>>>>>>> It would be good to include as many details about the faulty hardware
> >>>>>>>>> in
> >>>>>>>>> the commit message as possible, so that when someone else runs into
> >>>>>>>>> this, they would have all that information available.
> >>>>>>>>>  
> >>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>>     common/usb_hub.c | 6 ++++--
> >>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
> >>>>>>>>>> --- a/common/usb_hub.c
> >>>>>>>>>> +++ b/common/usb_hub.c
> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> >>>>>>>>>> usb_hub_device *hub)
> >>>>>>>>>>
> >>>>>>>>>>         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(dev)) {  
> >>>>>>>>>
> >>>>>>>>> Should this condition be "all which are lower than superspeed"
> >>>>>>>>> instead ,
> >>>>>>>>> so when the next generation of USB comes, this problem won't trigger
> >>>>>>>>> ?
> >>>>>>>>>
> >>>>>>>>> What does Linux do btw ?  
> >>>>>>>>
> >>>>>>>> As of now Linux checks if the hub is superspeed
> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> >>>>>>>>
> >>>>>>>> which is
> >>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> >>>>>>>> USB_HUB_PR_SS = 3
> >>>>>>>>
> >>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> >>>>>>>>
> >>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who knows if
> >>>>>>>> things change in the newer version of spec.
> >>>>>>>> I am open to suggestions.  
> >>>>>>>
> >>>>>>> Please just include the ^ in the commit description. Use link to
> >>>>>>> git.kernel.org , not some mirror . This is extremely useful
> >>>>>>> information and, well, you already wrote the V2 commit message
> >>>>>>> addition in this answer.  
> >>>>>>
> >>>>>> Shantur, if that would be easier or quicker for you, I can write
> >>>>>> a quite detailed patch description for you, in exchange for a
> >>>>>> "Helped-by" tag in the v2 patch submission. :)  
> >>>>>
> >>>>> That would be really kind of you Dragan.  
> >>>>
> >>>> Sure, I'll write the summary and send it over.
> >>>>  
> >>>>> I am down with the flu so that would really help me as my brain is
> >>>>> working at 15% capacity.  
> >>>>
> >>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
> >>>> soon, and I know very well what's it like;  I've also been sick
> >>>> recently, as a result of some kind of flu that unfortunately found
> >>>> its way into my lungs, and it took me about a month to get back
> >>>> to about 90% of my usual mental capacity.  I'm still not back to
> >>>> exactly 100%. :/
> >>>>
> >>>> I really hope you'll recover much faster.  
> >>>
> >>> I hope you're feeling better.
> >>>
> >>> Below are the patch subject and description that I prepared for you,
> >>> please have a look.
> >>>  
> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------  
> >>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
> >>>
> >>> Additional testing of the changes introduced in commit 33e06dcbe57a
> >>> ("common:
> >>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
> >>> flash  
> >>
> >> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
> >>  
> >>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
> >>> only.
> >>> More precisely, some tested USB 3.0 flash drives failed to be detected
> >>> and
> >>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
> >>> 2.0
> >>> only, while the same USB flash drives worked just fine on a Pine64 H64
> >>> with
> >>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
> >>>
> >>> Resetting USB 3.0 hubs only has been tested to work as expected,
> >>> resolving
> >>> the previous issues on the Allwinner H616, while not introducing any new
> >>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
> >>>
> >>> According to the USB 3.0 specification, resetting a USB 3.0 port is
> >>> required
> >>> when an attached USB device transitions between different states, such
> >>> as
> >>> when it resumes from suspend.  Though, the Linux kernel performs
> >>> additional
> >>> USB 3.0 port resets upon initial USB device attachment, presumably to
> >>> ensure
> >>> proper state of the USB 3.0 hub port and proper USB mode negotiation
> >>> during
> >>> the initial USB device attachment and enumeration.
> >>>
> >>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
> >>>
> >>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
> >>> visible
> >>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
> >>> check
> >>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
> >>> which
> >>> hopefully makes it future proof.
> >>>
> >>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> >>> Link:
> >>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
> >>> Link:
> >>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
> >>> Signed-off-by: Shantur Rathore <i@shantur.com>
> >>> Helped-by: Dragan Simic <dsimic@manjaro.org>
> >>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> >>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>  
> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------  
> >>
> >> Regards,
> >> Shantur  
> > 
> > Is this description acceptable to you Marek.  
> 
> Please send a V2 patch . If possible, include the device information as 
> reported by Andre, esp. which USB stick triggered it, including USB IDs, 
> this is important for future reference and in case someone has similar 
> failure.

So the USB 2.0 stick is some no-name, unlabelled and super cheap one, I
think we bought a pack of it, just for boot-strapping machines. The USB
ID of "abcd:1234" kind of gives away that this is bogus AF.
The USB 3.0 stick is a PNY 32GB one, the USB ID is:
1f75:0917 Innostor Technology Corporation IS917 Mass storage

Hope that helps.

Cheers,
Andre

> Please don't use "in the kernel source, line 2859", considering the rate 
> of change of the Linux kernel, it is best to also include exact commit 
> ID as of which this is a line 2859 and spell out this is referring to 
> Linux kernel.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

Hello Andre,

On 2024-02-14 03:04, Andre Przywara wrote:
> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut <marex@denx.de> wrote:
>> On 2/12/24 14:41, Shantur Rathore wrote:
>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
>> >>> On 2024-02-08 15:17, Dragan Simic wrote:
>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>> >>>>> wrote:
>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>> >>>>>>>>>> handle this well and after implementation of
>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
>> >>>>>>>>>> Allwinner based boards.
>> >>>>>>>>>>
>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>> >>>>>>>>>
>> >>>>>>>>> It would be good to include as many details about the faulty hardware
>> >>>>>>>>> in
>> >>>>>>>>> the commit message as possible, so that when someone else runs into
>> >>>>>>>>> this, they would have all that information available.
>> >>>>>>>>>
>> >>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> >>>>>>>>>>
>> >>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>> >>>>>>>>>> ---
>> >>>>>>>>>>
>> >>>>>>>>>>     common/usb_hub.c | 6 ++++--
>> >>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>> >>>>>>>>>>
>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>> >>>>>>>>>> --- a/common/usb_hub.c
>> >>>>>>>>>> +++ b/common/usb_hub.c
>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>> >>>>>>>>>> usb_hub_device *hub)
>> >>>>>>>>>>
>> >>>>>>>>>>         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(dev)) {
>> >>>>>>>>>
>> >>>>>>>>> Should this condition be "all which are lower than superspeed"
>> >>>>>>>>> instead ,
>> >>>>>>>>> so when the next generation of USB comes, this problem won't trigger
>> >>>>>>>>> ?
>> >>>>>>>>>
>> >>>>>>>>> What does Linux do btw ?
>> >>>>>>>>
>> >>>>>>>> As of now Linux checks if the hub is superspeed
>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>> >>>>>>>>
>> >>>>>>>> which is
>> >>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>> >>>>>>>> USB_HUB_PR_SS = 3
>> >>>>>>>>
>> >>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>> >>>>>>>>
>> >>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who knows if
>> >>>>>>>> things change in the newer version of spec.
>> >>>>>>>> I am open to suggestions.
>> >>>>>>>
>> >>>>>>> Please just include the ^ in the commit description. Use link to
>> >>>>>>> git.kernel.org , not some mirror . This is extremely useful
>> >>>>>>> information and, well, you already wrote the V2 commit message
>> >>>>>>> addition in this answer.
>> >>>>>>
>> >>>>>> Shantur, if that would be easier or quicker for you, I can write
>> >>>>>> a quite detailed patch description for you, in exchange for a
>> >>>>>> "Helped-by" tag in the v2 patch submission. :)
>> >>>>>
>> >>>>> That would be really kind of you Dragan.
>> >>>>
>> >>>> Sure, I'll write the summary and send it over.
>> >>>>
>> >>>>> I am down with the flu so that would really help me as my brain is
>> >>>>> working at 15% capacity.
>> >>>>
>> >>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>> >>>> soon, and I know very well what's it like;  I've also been sick
>> >>>> recently, as a result of some kind of flu that unfortunately found
>> >>>> its way into my lungs, and it took me about a month to get back
>> >>>> to about 90% of my usual mental capacity.  I'm still not back to
>> >>>> exactly 100%. :/
>> >>>>
>> >>>> I really hope you'll recover much faster.
>> >>>
>> >>> I hope you're feeling better.
>> >>>
>> >>> Below are the patch subject and description that I prepared for you,
>> >>> please have a look.
>> >>>
>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> >>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>> >>>
>> >>> Additional testing of the changes introduced in commit 33e06dcbe57a
>> >>> ("common:
>> >>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>> >>> flash
>> >>
>> >> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>> >>
>> >>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
>> >>> only.
>> >>> More precisely, some tested USB 3.0 flash drives failed to be detected
>> >>> and
>> >>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
>> >>> 2.0
>> >>> only, while the same USB flash drives worked just fine on a Pine64 H64
>> >>> with
>> >>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>> >>>
>> >>> Resetting USB 3.0 hubs only has been tested to work as expected,
>> >>> resolving
>> >>> the previous issues on the Allwinner H616, while not introducing any new
>> >>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>> >>>
>> >>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>> >>> required
>> >>> when an attached USB device transitions between different states, such
>> >>> as
>> >>> when it resumes from suspend.  Though, the Linux kernel performs
>> >>> additional
>> >>> USB 3.0 port resets upon initial USB device attachment, presumably to
>> >>> ensure
>> >>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>> >>> during
>> >>> the initial USB device attachment and enumeration.
>> >>>
>> >>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
>> >>>
>> >>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>> >>> visible
>> >>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
>> >>> check
>> >>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>> >>> which
>> >>> hopefully makes it future proof.
>> >>>
>> >>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
>> >>> Link:
>> >>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>> >>> Link:
>> >>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>> >>> Signed-off-by: Shantur Rathore <i@shantur.com>
>> >>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>> >>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> >>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> >>
>> >> Regards,
>> >> Shantur
>> >
>> > Is this description acceptable to you Marek.
>> 
>> Please send a V2 patch . If possible, include the device information 
>> as
>> reported by Andre, esp. which USB stick triggered it, including USB 
>> IDs,
>> this is important for future reference and in case someone has similar
>> failure.
> 
> So the USB 2.0 stick is some no-name, unlabelled and super cheap one, I
> think we bought a pack of it, just for boot-strapping machines. The USB
> ID of "abcd:1234" kind of gives away that this is bogus AF.
> The USB 3.0 stick is a PNY 32GB one, the USB ID is:
> 1f75:0917 Innostor Technology Corporation IS917 Mass storage
> 
> Hope that helps.

Thank you for replying.  I'll include the available information into
the revised commit description and send it over a bit later.

>> Please don't use "in the kernel source, line 2859", considering the 
>> rate
>> of change of the Linux kernel, it is best to also include exact commit
>> ID as of which this is a line 2859 and spell out this is referring to
>> Linux kernel.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

Hello Shantur and Marek,

On 2024-02-14 04:18, Dragan Simic wrote:
> On 2024-02-14 03:04, Andre Przywara wrote:
>> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut <marex@denx.de> wrote:
>>> On 2/12/24 14:41, Shantur Rathore wrote:
>>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
>>> >>> On 2024-02-08 15:17, Dragan Simic wrote:
>>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>>> >>>>> wrote:
>>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
>>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>>> >>>>>>>>>> handle this well and after implementation of
>>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
>>> >>>>>>>>>> Allwinner based boards.
>>> >>>>>>>>>>
>>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>>> >>>>>>>>>
>>> >>>>>>>>> It would be good to include as many details about the faulty hardware
>>> >>>>>>>>> in
>>> >>>>>>>>> the commit message as possible, so that when someone else runs into
>>> >>>>>>>>> this, they would have all that information available.
>>> >>>>>>>>>
>>> >>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>> >>>>>>>>>>
>>> >>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>> >>>>>>>>>> ---
>>> >>>>>>>>>>
>>> >>>>>>>>>>     common/usb_hub.c | 6 ++++--
>>> >>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>> >>>>>>>>>>
>>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>>> >>>>>>>>>> --- a/common/usb_hub.c
>>> >>>>>>>>>> +++ b/common/usb_hub.c
>>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>>> >>>>>>>>>> usb_hub_device *hub)
>>> >>>>>>>>>>
>>> >>>>>>>>>>         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(dev)) {
>>> >>>>>>>>>
>>> >>>>>>>>> Should this condition be "all which are lower than superspeed"
>>> >>>>>>>>> instead ,
>>> >>>>>>>>> so when the next generation of USB comes, this problem won't trigger
>>> >>>>>>>>> ?
>>> >>>>>>>>>
>>> >>>>>>>>> What does Linux do btw ?
>>> >>>>>>>>
>>> >>>>>>>> As of now Linux checks if the hub is superspeed
>>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>>> >>>>>>>>
>>> >>>>>>>> which is
>>> >>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>>> >>>>>>>> USB_HUB_PR_SS = 3
>>> >>>>>>>>
>>> >>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>>> >>>>>>>>
>>> >>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who knows if
>>> >>>>>>>> things change in the newer version of spec.
>>> >>>>>>>> I am open to suggestions.
>>> >>>>>>>
>>> >>>>>>> Please just include the ^ in the commit description. Use link to
>>> >>>>>>> git.kernel.org , not some mirror . This is extremely useful
>>> >>>>>>> information and, well, you already wrote the V2 commit message
>>> >>>>>>> addition in this answer.
>>> >>>>>>
>>> >>>>>> Shantur, if that would be easier or quicker for you, I can write
>>> >>>>>> a quite detailed patch description for you, in exchange for a
>>> >>>>>> "Helped-by" tag in the v2 patch submission. :)
>>> >>>>>
>>> >>>>> That would be really kind of you Dragan.
>>> >>>>
>>> >>>> Sure, I'll write the summary and send it over.
>>> >>>>
>>> >>>>> I am down with the flu so that would really help me as my brain is
>>> >>>>> working at 15% capacity.
>>> >>>>
>>> >>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>>> >>>> soon, and I know very well what's it like;  I've also been sick
>>> >>>> recently, as a result of some kind of flu that unfortunately found
>>> >>>> its way into my lungs, and it took me about a month to get back
>>> >>>> to about 90% of my usual mental capacity.  I'm still not back to
>>> >>>> exactly 100%. :/
>>> >>>>
>>> >>>> I really hope you'll recover much faster.
>>> >>>
>>> >>> I hope you're feeling better.
>>> >>>
>>> >>> Below are the patch subject and description that I prepared for you,
>>> >>> please have a look.
>>> >>>
>>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>> >>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>>> >>>
>>> >>> Additional testing of the changes introduced in commit 33e06dcbe57a
>>> >>> ("common:
>>> >>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>>> >>> flash
>>> >>
>>> >> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>>> >>
>>> >>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
>>> >>> only.
>>> >>> More precisely, some tested USB 3.0 flash drives failed to be detected
>>> >>> and
>>> >>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
>>> >>> 2.0
>>> >>> only, while the same USB flash drives worked just fine on a Pine64 H64
>>> >>> with
>>> >>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>>> >>>
>>> >>> Resetting USB 3.0 hubs only has been tested to work as expected,
>>> >>> resolving
>>> >>> the previous issues on the Allwinner H616, while not introducing any new
>>> >>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>>> >>>
>>> >>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>>> >>> required
>>> >>> when an attached USB device transitions between different states, such
>>> >>> as
>>> >>> when it resumes from suspend.  Though, the Linux kernel performs
>>> >>> additional
>>> >>> USB 3.0 port resets upon initial USB device attachment, presumably to
>>> >>> ensure
>>> >>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>>> >>> during
>>> >>> the initial USB device attachment and enumeration.
>>> >>>
>>> >>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
>>> >>>
>>> >>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>>> >>> visible
>>> >>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
>>> >>> check
>>> >>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>>> >>> which
>>> >>> hopefully makes it future proof.
>>> >>>
>>> >>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
>>> >>> Link:
>>> >>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>>> >>> Link:
>>> >>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>>> >>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>> >>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>>> >>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>> >>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>> >>
>>> >> Regards,
>>> >> Shantur
>>> >
>>> > Is this description acceptable to you Marek.
>>> 
>>> Please send a V2 patch . If possible, include the device information 
>>> as
>>> reported by Andre, esp. which USB stick triggered it, including USB 
>>> IDs,
>>> this is important for future reference and in case someone has 
>>> similar
>>> failure.
>> 
>> So the USB 2.0 stick is some no-name, unlabelled and super cheap one, 
>> I
>> think we bought a pack of it, just for boot-strapping machines. The 
>> USB
>> ID of "abcd:1234" kind of gives away that this is bogus AF.
>> The USB 3.0 stick is a PNY 32GB one, the USB ID is:
>> 1f75:0917 Innostor Technology Corporation IS917 Mass storage
>> 
>> Hope that helps.
> 
> Thank you for replying.  I'll include the available information into
> the revised commit description and send it over a bit later.

Here are the revised commit subject and description, please have a look
and let me know if further improvements are needed.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------
[PATCH v2] common: usb-hub: Reset USB 3.0 hubs only

Additional testing of the changes introduced in commit 33e06dcbe57a 
("common:
usb-hub: Reset hub port before scanning") revealed that some USB 2.0 and 
3.0
flash drives didn't work in U-Boot on some Allwinner SoCs that support 
USB
2.0 interfaces only.  More precisely, some of the tested USB 2.0 and 3.0
flash drives failed to be detected and work on an OrangePi Zero 3, based 
on
the Allwinner H616 SoC that supports USB 2.0 only, while the same USB 
flash
drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC 
that
supports both USB 2.0 and USB 3.0 interfaces.

The USB ID of the above-mentioned USB 3.0 flash drive that failed to 
work is
1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32 
GB
in size and sold under the PNY brand.  The mentioned USB 2.0 drive is 
some
inexpensive no-name drive with an invalid USB ID.

Resetting USB 3.0 hubs only, which this patch introduces to the USB hub
resets, has been tested to work as expected, resolving the identified 
issues
on the Allwinner H616, while not introducing any new issues on other 
tested
Allwinner SoCs.  Thus, let's fix it that way.

According to the USB 3.0 specification, resetting a USB 3.0 port is 
required
when an attached USB device transitions between different states, such 
as
when it resumes from suspend.  Though, the Linux kernel performs 
additional
USB 3.0 port resets upon initial USB device attachment, as visible in 
commit
07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the 
kernel
source, to ensure proper state of the USB 3.0 hub port and proper USB 
mode
negotiation during the initial USB device attachment and enumeration.

These additional types of USB port resets don't exist for USB 2.0 hubs,
according 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.

The Linux kernel resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible 
in
commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm 
reset.")
in the kernel source.  The check for SuperSpeed hubs is performed in a 
way
that also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as 
well,
which hopefully makes it future proof.

Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
Link: 
https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
Link: 
https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
Signed-off-by: Shantur Rathore <i@shantur.com>
Helped-by: Dragan Simic <dsimic@manjaro.org>
Tested-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Dragan Simic <dsimic@manjaro.org>
------- >8 ------------- >8 ------------- >8 ------------- >8 -------

Shantur, I hope you're feeling better.

>>> Please don't use "in the kernel source, line 2859", considering the 
>>> rate
>>> of change of the Linux kernel, it is best to also include exact 
>>> commit
>>> ID as of which this is a line 2859 and spell out this is referring to
>>> Linux kernel.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Shantur Rathore]

On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Shantur and Marek,
>
> On 2024-02-14 04:18, Dragan Simic wrote:
> > On 2024-02-14 03:04, Andre Przywara wrote:
> >> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut <marex@denx.de> wrote:
> >>> On 2/12/24 14:41, Shantur Rathore wrote:
> >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
> >>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
> >>> >>> On 2024-02-08 15:17, Dragan Simic wrote:
> >>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:
> >>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
> >>> >>>>> wrote:
> >>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
> >>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
> >>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
> >>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
> >>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
> >>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
> >>> >>>>>>>>>> handle this well and after implementation of
> >>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
> >>> >>>>>>>>>> Allwinner based boards.
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
> >>> >>>>>>>>>
> >>> >>>>>>>>> It would be good to include as many details about the faulty hardware
> >>> >>>>>>>>> in
> >>> >>>>>>>>> the commit message as possible, so that when someone else runs into
> >>> >>>>>>>>> this, they would have all that information available.
> >>> >>>>>>>>>
> >>> >>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
> >>> >>>>>>>>>> ---
> >>> >>>>>>>>>>
> >>> >>>>>>>>>>     common/usb_hub.c | 6 ++++--
> >>> >>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
> >>> >>>>>>>>>> --- a/common/usb_hub.c
> >>> >>>>>>>>>> +++ b/common/usb_hub.c
> >>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> >>> >>>>>>>>>> usb_hub_device *hub)
> >>> >>>>>>>>>>
> >>> >>>>>>>>>>         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(dev)) {
> >>> >>>>>>>>>
> >>> >>>>>>>>> Should this condition be "all which are lower than superspeed"
> >>> >>>>>>>>> instead ,
> >>> >>>>>>>>> so when the next generation of USB comes, this problem won't trigger
> >>> >>>>>>>>> ?
> >>> >>>>>>>>>
> >>> >>>>>>>>> What does Linux do btw ?
> >>> >>>>>>>>
> >>> >>>>>>>> As of now Linux checks if the hub is superspeed
> >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> >>> >>>>>>>>
> >>> >>>>>>>> which is
> >>> >>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> >>> >>>>>>>> USB_HUB_PR_SS = 3
> >>> >>>>>>>>
> >>> >>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
> >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> >>> >>>>>>>>
> >>> >>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who knows if
> >>> >>>>>>>> things change in the newer version of spec.
> >>> >>>>>>>> I am open to suggestions.
> >>> >>>>>>>
> >>> >>>>>>> Please just include the ^ in the commit description. Use link to
> >>> >>>>>>> git.kernel.org , not some mirror . This is extremely useful
> >>> >>>>>>> information and, well, you already wrote the V2 commit message
> >>> >>>>>>> addition in this answer.
> >>> >>>>>>
> >>> >>>>>> Shantur, if that would be easier or quicker for you, I can write
> >>> >>>>>> a quite detailed patch description for you, in exchange for a
> >>> >>>>>> "Helped-by" tag in the v2 patch submission. :)
> >>> >>>>>
> >>> >>>>> That would be really kind of you Dragan.
> >>> >>>>
> >>> >>>> Sure, I'll write the summary and send it over.
> >>> >>>>
> >>> >>>>> I am down with the flu so that would really help me as my brain is
> >>> >>>>> working at 15% capacity.
> >>> >>>>
> >>> >>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
> >>> >>>> soon, and I know very well what's it like;  I've also been sick
> >>> >>>> recently, as a result of some kind of flu that unfortunately found
> >>> >>>> its way into my lungs, and it took me about a month to get back
> >>> >>>> to about 90% of my usual mental capacity.  I'm still not back to
> >>> >>>> exactly 100%. :/
> >>> >>>>
> >>> >>>> I really hope you'll recover much faster.
> >>> >>>
> >>> >>> I hope you're feeling better.
> >>> >>>
> >>> >>> Below are the patch subject and description that I prepared for you,
> >>> >>> please have a look.
> >>> >>>
> >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> >>> >>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
> >>> >>>
> >>> >>> Additional testing of the changes introduced in commit 33e06dcbe57a
> >>> >>> ("common:
> >>> >>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
> >>> >>> flash
> >>> >>
> >>> >> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
> >>> >>
> >>> >>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
> >>> >>> only.
> >>> >>> More precisely, some tested USB 3.0 flash drives failed to be detected
> >>> >>> and
> >>> >>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
> >>> >>> 2.0
> >>> >>> only, while the same USB flash drives worked just fine on a Pine64 H64
> >>> >>> with
> >>> >>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
> >>> >>>
> >>> >>> Resetting USB 3.0 hubs only has been tested to work as expected,
> >>> >>> resolving
> >>> >>> the previous issues on the Allwinner H616, while not introducing any new
> >>> >>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
> >>> >>>
> >>> >>> According to the USB 3.0 specification, resetting a USB 3.0 port is
> >>> >>> required
> >>> >>> when an attached USB device transitions between different states, such
> >>> >>> as
> >>> >>> when it resumes from suspend.  Though, the Linux kernel performs
> >>> >>> additional
> >>> >>> USB 3.0 port resets upon initial USB device attachment, presumably to
> >>> >>> ensure
> >>> >>> proper state of the USB 3.0 hub port and proper USB mode negotiation
> >>> >>> during
> >>> >>> the initial USB device attachment and enumeration.
> >>> >>>
> >>> >>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
> >>> >>>
> >>> >>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
> >>> >>> visible
> >>> >>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
> >>> >>> check
> >>> >>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
> >>> >>> which
> >>> >>> hopefully makes it future proof.
> >>> >>>
> >>> >>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> >>> >>> Link:
> >>> >>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
> >>> >>> Link:
> >>> >>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
> >>> >>> Signed-off-by: Shantur Rathore <i@shantur.com>
> >>> >>> Helped-by: Dragan Simic <dsimic@manjaro.org>
> >>> >>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> >>> >>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> >>> >>
> >>> >> Regards,
> >>> >> Shantur
> >>> >
> >>> > Is this description acceptable to you Marek.
> >>>
> >>> Please send a V2 patch . If possible, include the device information
> >>> as
> >>> reported by Andre, esp. which USB stick triggered it, including USB
> >>> IDs,
> >>> this is important for future reference and in case someone has
> >>> similar
> >>> failure.
> >>
> >> So the USB 2.0 stick is some no-name, unlabelled and super cheap one,
> >> I
> >> think we bought a pack of it, just for boot-strapping machines. The
> >> USB
> >> ID of "abcd:1234" kind of gives away that this is bogus AF.
> >> The USB 3.0 stick is a PNY 32GB one, the USB ID is:
> >> 1f75:0917 Innostor Technology Corporation IS917 Mass storage
> >>
> >> Hope that helps.
> >
> > Thank you for replying.  I'll include the available information into
> > the revised commit description and send it over a bit later.
>
> Here are the revised commit subject and description, please have a look
> and let me know if further improvements are needed.
>
> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>
> Additional testing of the changes introduced in commit 33e06dcbe57a
> ("common:
> usb-hub: Reset hub port before scanning") revealed that some USB 2.0 and
> 3.0
> flash drives didn't work in U-Boot on some Allwinner SoCs that support
> USB
> 2.0 interfaces only.  More precisely, some of the tested USB 2.0 and 3.0
> flash drives failed to be detected and work on an OrangePi Zero 3, based
> on
> the Allwinner H616 SoC that supports USB 2.0 only, while the same USB
> flash
> drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC
> that
> supports both USB 2.0 and USB 3.0 interfaces.
>
> The USB ID of the above-mentioned USB 3.0 flash drive that failed to
> work is
> 1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32
> GB
> in size and sold under the PNY brand.  The mentioned USB 2.0 drive is
> some
> inexpensive no-name drive with an invalid USB ID.
>
> Resetting USB 3.0 hubs only, which this patch introduces to the USB hub
> resets, has been tested to work as expected, resolving the identified
> issues
> on the Allwinner H616, while not introducing any new issues on other
> tested
> Allwinner SoCs.  Thus, let's fix it that way.
>
> According to the USB 3.0 specification, resetting a USB 3.0 port is
> required
> when an attached USB device transitions between different states, such
> as
> when it resumes from suspend.  Though, the Linux kernel performs
> additional
> USB 3.0 port resets upon initial USB device attachment, as visible in
> commit
> 07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the
> kernel
> source, to ensure proper state of the USB 3.0 hub port and proper USB
> mode
> negotiation during the initial USB device attachment and enumeration.
>
> These additional types of USB port resets don't exist for USB 2.0 hubs,
> according 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.
>
> The Linux kernel resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible
> in
> commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm
> reset.")
> in the kernel source.  The check for SuperSpeed hubs is performed in a
> way
> that also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as
> well,
> which hopefully makes it future proof.
>
> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> Link:
> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
> Link:
> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
> Signed-off-by: Shantur Rathore <i@shantur.com>
> Helped-by: Dragan Simic <dsimic@manjaro.org>
> Tested-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>
> Shantur, I hope you're feeling better.

Thanks Dragan, V2 is submitted.
I am feeling much better, thanks for asking.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Shantur Rathore]

On Wed, Feb 14, 2024 at 9:56 AM Shantur Rathore <i@shantur.com> wrote:
>
> On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > Hello Shantur and Marek,
> >
> > On 2024-02-14 04:18, Dragan Simic wrote:
> > > On 2024-02-14 03:04, Andre Przywara wrote:
> > >> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut <marex@denx.de> wrote:
> > >>> On 2/12/24 14:41, Shantur Rathore wrote:
> > >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
> > >>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
> > >>> >>> On 2024-02-08 15:17, Dragan Simic wrote:
> > >>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:
> > >>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
> > >>> >>>>> wrote:
> > >>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
> > >>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
> > >>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
> > >>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
> > >>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
> > >>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
> > >>> >>>>>>>>>> handle this well and after implementation of
> > >>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
> > >>> >>>>>>>>>> Allwinner based boards.
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
> > >>> >>>>>>>>>
> > >>> >>>>>>>>> It would be good to include as many details about the faulty hardware
> > >>> >>>>>>>>> in
> > >>> >>>>>>>>> the commit message as possible, so that when someone else runs into
> > >>> >>>>>>>>> this, they would have all that information available.
> > >>> >>>>>>>>>
> > >>> >>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
> > >>> >>>>>>>>>> ---
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>>     common/usb_hub.c | 6 ++++--
> > >>> >>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> > >>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
> > >>> >>>>>>>>>> --- a/common/usb_hub.c
> > >>> >>>>>>>>>> +++ b/common/usb_hub.c
> > >>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> > >>> >>>>>>>>>> usb_hub_device *hub)
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>>         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(dev)) {
> > >>> >>>>>>>>>
> > >>> >>>>>>>>> Should this condition be "all which are lower than superspeed"
> > >>> >>>>>>>>> instead ,
> > >>> >>>>>>>>> so when the next generation of USB comes, this problem won't trigger
> > >>> >>>>>>>>> ?
> > >>> >>>>>>>>>
> > >>> >>>>>>>>> What does Linux do btw ?
> > >>> >>>>>>>>
> > >>> >>>>>>>> As of now Linux checks if the hub is superspeed
> > >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> > >>> >>>>>>>>
> > >>> >>>>>>>> which is
> > >>> >>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> > >>> >>>>>>>> USB_HUB_PR_SS = 3
> > >>> >>>>>>>>
> > >>> >>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
> > >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> > >>> >>>>>>>>
> > >>> >>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who knows if
> > >>> >>>>>>>> things change in the newer version of spec.
> > >>> >>>>>>>> I am open to suggestions.
> > >>> >>>>>>>
> > >>> >>>>>>> Please just include the ^ in the commit description. Use link to
> > >>> >>>>>>> git.kernel.org , not some mirror . This is extremely useful
> > >>> >>>>>>> information and, well, you already wrote the V2 commit message
> > >>> >>>>>>> addition in this answer.
> > >>> >>>>>>
> > >>> >>>>>> Shantur, if that would be easier or quicker for you, I can write
> > >>> >>>>>> a quite detailed patch description for you, in exchange for a
> > >>> >>>>>> "Helped-by" tag in the v2 patch submission. :)
> > >>> >>>>>
> > >>> >>>>> That would be really kind of you Dragan.
> > >>> >>>>
> > >>> >>>> Sure, I'll write the summary and send it over.
> > >>> >>>>
> > >>> >>>>> I am down with the flu so that would really help me as my brain is
> > >>> >>>>> working at 15% capacity.
> > >>> >>>>
> > >>> >>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
> > >>> >>>> soon, and I know very well what's it like;  I've also been sick
> > >>> >>>> recently, as a result of some kind of flu that unfortunately found
> > >>> >>>> its way into my lungs, and it took me about a month to get back
> > >>> >>>> to about 90% of my usual mental capacity.  I'm still not back to
> > >>> >>>> exactly 100%. :/
> > >>> >>>>
> > >>> >>>> I really hope you'll recover much faster.
> > >>> >>>
> > >>> >>> I hope you're feeling better.
> > >>> >>>
> > >>> >>> Below are the patch subject and description that I prepared for you,
> > >>> >>> please have a look.
> > >>> >>>
> > >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> > >>> >>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
> > >>> >>>
> > >>> >>> Additional testing of the changes introduced in commit 33e06dcbe57a
> > >>> >>> ("common:
> > >>> >>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
> > >>> >>> flash
> > >>> >>
> > >>> >> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
> > >>> >>
> > >>> >>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
> > >>> >>> only.
> > >>> >>> More precisely, some tested USB 3.0 flash drives failed to be detected
> > >>> >>> and
> > >>> >>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
> > >>> >>> 2.0
> > >>> >>> only, while the same USB flash drives worked just fine on a Pine64 H64
> > >>> >>> with
> > >>> >>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
> > >>> >>>
> > >>> >>> Resetting USB 3.0 hubs only has been tested to work as expected,
> > >>> >>> resolving
> > >>> >>> the previous issues on the Allwinner H616, while not introducing any new
> > >>> >>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
> > >>> >>>
> > >>> >>> According to the USB 3.0 specification, resetting a USB 3.0 port is
> > >>> >>> required
> > >>> >>> when an attached USB device transitions between different states, such
> > >>> >>> as
> > >>> >>> when it resumes from suspend.  Though, the Linux kernel performs
> > >>> >>> additional
> > >>> >>> USB 3.0 port resets upon initial USB device attachment, presumably to
> > >>> >>> ensure
> > >>> >>> proper state of the USB 3.0 hub port and proper USB mode negotiation
> > >>> >>> during
> > >>> >>> the initial USB device attachment and enumeration.
> > >>> >>>
> > >>> >>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
> > >>> >>>
> > >>> >>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
> > >>> >>> visible
> > >>> >>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
> > >>> >>> check
> > >>> >>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
> > >>> >>> which
> > >>> >>> hopefully makes it future proof.
> > >>> >>>
> > >>> >>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> > >>> >>> Link:
> > >>> >>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
> > >>> >>> Link:
> > >>> >>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
> > >>> >>> Signed-off-by: Shantur Rathore <i@shantur.com>
> > >>> >>> Helped-by: Dragan Simic <dsimic@manjaro.org>
> > >>> >>> Tested-by: Andre Przywara <andre.przywara@arm.com>
> > >>> >>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> > >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> > >>> >>
> > >>> >> Regards,
> > >>> >> Shantur
> > >>> >
> > >>> > Is this description acceptable to you Marek.
> > >>>
> > >>> Please send a V2 patch . If possible, include the device information
> > >>> as
> > >>> reported by Andre, esp. which USB stick triggered it, including USB
> > >>> IDs,
> > >>> this is important for future reference and in case someone has
> > >>> similar
> > >>> failure.
> > >>
> > >> So the USB 2.0 stick is some no-name, unlabelled and super cheap one,
> > >> I
> > >> think we bought a pack of it, just for boot-strapping machines. The
> > >> USB
> > >> ID of "abcd:1234" kind of gives away that this is bogus AF.
> > >> The USB 3.0 stick is a PNY 32GB one, the USB ID is:
> > >> 1f75:0917 Innostor Technology Corporation IS917 Mass storage
> > >>
> > >> Hope that helps.
> > >
> > > Thank you for replying.  I'll include the available information into
> > > the revised commit description and send it over a bit later.
> >
> > Here are the revised commit subject and description, please have a look
> > and let me know if further improvements are needed.
> >
> > ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> > [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
> >
> > Additional testing of the changes introduced in commit 33e06dcbe57a
> > ("common:
> > usb-hub: Reset hub port before scanning") revealed that some USB 2.0 and
> > 3.0
> > flash drives didn't work in U-Boot on some Allwinner SoCs that support
> > USB
> > 2.0 interfaces only.  More precisely, some of the tested USB 2.0 and 3.0
> > flash drives failed to be detected and work on an OrangePi Zero 3, based
> > on
> > the Allwinner H616 SoC that supports USB 2.0 only, while the same USB
> > flash
> > drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC
> > that
> > supports both USB 2.0 and USB 3.0 interfaces.
> >
> > The USB ID of the above-mentioned USB 3.0 flash drive that failed to
> > work is
> > 1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32
> > GB
> > in size and sold under the PNY brand.  The mentioned USB 2.0 drive is
> > some
> > inexpensive no-name drive with an invalid USB ID.
> >
> > Resetting USB 3.0 hubs only, which this patch introduces to the USB hub
> > resets, has been tested to work as expected, resolving the identified
> > issues
> > on the Allwinner H616, while not introducing any new issues on other
> > tested
> > Allwinner SoCs.  Thus, let's fix it that way.
> >
> > According to the USB 3.0 specification, resetting a USB 3.0 port is
> > required
> > when an attached USB device transitions between different states, such
> > as
> > when it resumes from suspend.  Though, the Linux kernel performs
> > additional
> > USB 3.0 port resets upon initial USB device attachment, as visible in
> > commit
> > 07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the
> > kernel
> > source, to ensure proper state of the USB 3.0 hub port and proper USB
> > mode
> > negotiation during the initial USB device attachment and enumeration.
> >
> > These additional types of USB port resets don't exist for USB 2.0 hubs,
> > according 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.
> >
> > The Linux kernel resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible
> > in
> > commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm
> > reset.")
> > in the kernel source.  The check for SuperSpeed hubs is performed in a
> > way
> > that also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as
> > well,
> > which hopefully makes it future proof.
> >
> > Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> > Link:
> > https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
> > Link:
> > https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
> > Signed-off-by: Shantur Rathore <i@shantur.com>
> > Helped-by: Dragan Simic <dsimic@manjaro.org>
> > Tested-by: Andre Przywara <andre.przywara@arm.com>
> > Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> > ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> >
> > Shantur, I hope you're feeling better.
>
> Thanks Dragan, V2 is submitted.
> I am feeling much better, thanks for asking.

PS : I don't think I could ever explain as well as you did Dragan.
Thanks a lot for the help.

I will bring another pending matter to your attention on the Linux list now ;P

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

On 2024-02-14 11:01, Shantur Rathore wrote:
> On Wed, Feb 14, 2024 at 9:56 AM Shantur Rathore <i@shantur.com> wrote:
>> On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>> > On 2024-02-14 04:18, Dragan Simic wrote:
>> > > On 2024-02-14 03:04, Andre Przywara wrote:
>> > >> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut <marex@denx.de> wrote:
>> > >>> On 2/12/24 14:41, Shantur Rathore wrote:
>> > >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> wrote:
>> > >>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> wrote:
>> > >>> >>> On 2024-02-08 15:17, Dragan Simic wrote:
>> > >>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>> > >>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>> > >>> >>>>> wrote:
>> > >>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
>> > >>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
>> > >>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>> > >>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
>> > >>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
>> > >>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
>> > >>> >>>>>>>>>> handle this well and after implementation of
>> > >>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
>> > >>> >>>>>>>>>> Allwinner based boards.
>> > >>> >>>>>>>>>>
>> > >>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
>> > >>> >>>>>>>>>
>> > >>> >>>>>>>>> It would be good to include as many details about the faulty hardware
>> > >>> >>>>>>>>> in
>> > >>> >>>>>>>>> the commit message as possible, so that when someone else runs into
>> > >>> >>>>>>>>> this, they would have all that information available.
>> > >>> >>>>>>>>>
>> > >>> >>>>>>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> > >>> >>>>>>>>>>
>> > >>> >>>>>>>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>> > >>> >>>>>>>>>> ---
>> > >>> >>>>>>>>>>
>> > >>> >>>>>>>>>>     common/usb_hub.c | 6 ++++--
>> > >>> >>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>> > >>> >>>>>>>>>>
>> > >>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> > >>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
>> > >>> >>>>>>>>>> --- a/common/usb_hub.c
>> > >>> >>>>>>>>>> +++ b/common/usb_hub.c
>> > >>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>> > >>> >>>>>>>>>> usb_hub_device *hub)
>> > >>> >>>>>>>>>>
>> > >>> >>>>>>>>>>         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(dev)) {
>> > >>> >>>>>>>>>
>> > >>> >>>>>>>>> Should this condition be "all which are lower than superspeed"
>> > >>> >>>>>>>>> instead ,
>> > >>> >>>>>>>>> so when the next generation of USB comes, this problem won't trigger
>> > >>> >>>>>>>>> ?
>> > >>> >>>>>>>>>
>> > >>> >>>>>>>>> What does Linux do btw ?
>> > >>> >>>>>>>>
>> > >>> >>>>>>>> As of now Linux checks if the hub is superspeed
>> > >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>> > >>> >>>>>>>>
>> > >>> >>>>>>>> which is
>> > >>> >>>>>>>>    return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>> > >>> >>>>>>>> USB_HUB_PR_SS = 3
>> > >>> >>>>>>>>
>> > >>> >>>>>>>> This holds true for newer SuperSpeedPlus hubs as well.
>> > >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>> > >>> >>>>>>>>
>> > >>> >>>>>>>> We can change the check to be  bDeviceProtocol > 2 but who knows if
>> > >>> >>>>>>>> things change in the newer version of spec.
>> > >>> >>>>>>>> I am open to suggestions.
>> > >>> >>>>>>>
>> > >>> >>>>>>> Please just include the ^ in the commit description. Use link to
>> > >>> >>>>>>> git.kernel.org , not some mirror . This is extremely useful
>> > >>> >>>>>>> information and, well, you already wrote the V2 commit message
>> > >>> >>>>>>> addition in this answer.
>> > >>> >>>>>>
>> > >>> >>>>>> Shantur, if that would be easier or quicker for you, I can write
>> > >>> >>>>>> a quite detailed patch description for you, in exchange for a
>> > >>> >>>>>> "Helped-by" tag in the v2 patch submission. :)
>> > >>> >>>>>
>> > >>> >>>>> That would be really kind of you Dragan.
>> > >>> >>>>
>> > >>> >>>> Sure, I'll write the summary and send it over.
>> > >>> >>>>
>> > >>> >>>>> I am down with the flu so that would really help me as my brain is
>> > >>> >>>>> working at 15% capacity.
>> > >>> >>>>
>> > >>> >>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>> > >>> >>>> soon, and I know very well what's it like;  I've also been sick
>> > >>> >>>> recently, as a result of some kind of flu that unfortunately found
>> > >>> >>>> its way into my lungs, and it took me about a month to get back
>> > >>> >>>> to about 90% of my usual mental capacity.  I'm still not back to
>> > >>> >>>> exactly 100%. :/
>> > >>> >>>>
>> > >>> >>>> I really hope you'll recover much faster.
>> > >>> >>>
>> > >>> >>> I hope you're feeling better.
>> > >>> >>>
>> > >>> >>> Below are the patch subject and description that I prepared for you,
>> > >>> >>> please have a look.
>> > >>> >>>
>> > >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> > >>> >>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>> > >>> >>>
>> > >>> >>> Additional testing of the changes introduced in commit 33e06dcbe57a
>> > >>> >>> ("common:
>> > >>> >>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>> > >>> >>> flash
>> > >>> >>
>> > >>> >> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>> > >>> >>
>> > >>> >>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0
>> > >>> >>> only.
>> > >>> >>> More precisely, some tested USB 3.0 flash drives failed to be detected
>> > >>> >>> and
>> > >>> >>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
>> > >>> >>> 2.0
>> > >>> >>> only, while the same USB flash drives worked just fine on a Pine64 H64
>> > >>> >>> with
>> > >>> >>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>> > >>> >>>
>> > >>> >>> Resetting USB 3.0 hubs only has been tested to work as expected,
>> > >>> >>> resolving
>> > >>> >>> the previous issues on the Allwinner H616, while not introducing any new
>> > >>> >>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>> > >>> >>>
>> > >>> >>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>> > >>> >>> required
>> > >>> >>> when an attached USB device transitions between different states, such
>> > >>> >>> as
>> > >>> >>> when it resumes from suspend.  Though, the Linux kernel performs
>> > >>> >>> additional
>> > >>> >>> USB 3.0 port resets upon initial USB device attachment, presumably to
>> > >>> >>> ensure
>> > >>> >>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>> > >>> >>> during
>> > >>> >>> the initial USB device attachment and enumeration.
>> > >>> >>>
>> > >>> >>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 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.
>> > >>> >>>
>> > >>> >>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>> > >>> >>> visible
>> > >>> >>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
>> > >>> >>> check
>> > >>> >>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>> > >>> >>> which
>> > >>> >>> hopefully makes it future proof.
>> > >>> >>>
>> > >>> >>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
>> > >>> >>> Link:
>> > >>> >>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>> > >>> >>> Link:
>> > >>> >>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>> > >>> >>> Signed-off-by: Shantur Rathore <i@shantur.com>
>> > >>> >>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>> > >>> >>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> > >>> >>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>> > >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> > >>> >>
>> > >>> >> Regards,
>> > >>> >> Shantur
>> > >>> >
>> > >>> > Is this description acceptable to you Marek.
>> > >>>
>> > >>> Please send a V2 patch . If possible, include the device information
>> > >>> as
>> > >>> reported by Andre, esp. which USB stick triggered it, including USB
>> > >>> IDs,
>> > >>> this is important for future reference and in case someone has
>> > >>> similar
>> > >>> failure.
>> > >>
>> > >> So the USB 2.0 stick is some no-name, unlabelled and super cheap one,
>> > >> I
>> > >> think we bought a pack of it, just for boot-strapping machines. The
>> > >> USB
>> > >> ID of "abcd:1234" kind of gives away that this is bogus AF.
>> > >> The USB 3.0 stick is a PNY 32GB one, the USB ID is:
>> > >> 1f75:0917 Innostor Technology Corporation IS917 Mass storage
>> > >>
>> > >> Hope that helps.
>> > >
>> > > Thank you for replying.  I'll include the available information into
>> > > the revised commit description and send it over a bit later.
>> >
>> > Here are the revised commit subject and description, please have a look
>> > and let me know if further improvements are needed.
>> >
>> > ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> > [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>> >
>> > Additional testing of the changes introduced in commit 33e06dcbe57a
>> > ("common:
>> > usb-hub: Reset hub port before scanning") revealed that some USB 2.0 and
>> > 3.0
>> > flash drives didn't work in U-Boot on some Allwinner SoCs that support
>> > USB
>> > 2.0 interfaces only.  More precisely, some of the tested USB 2.0 and 3.0
>> > flash drives failed to be detected and work on an OrangePi Zero 3, based
>> > on
>> > the Allwinner H616 SoC that supports USB 2.0 only, while the same USB
>> > flash
>> > drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC
>> > that
>> > supports both USB 2.0 and USB 3.0 interfaces.
>> >
>> > The USB ID of the above-mentioned USB 3.0 flash drive that failed to
>> > work is
>> > 1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32
>> > GB
>> > in size and sold under the PNY brand.  The mentioned USB 2.0 drive is
>> > some
>> > inexpensive no-name drive with an invalid USB ID.
>> >
>> > Resetting USB 3.0 hubs only, which this patch introduces to the USB hub
>> > resets, has been tested to work as expected, resolving the identified
>> > issues
>> > on the Allwinner H616, while not introducing any new issues on other
>> > tested
>> > Allwinner SoCs.  Thus, let's fix it that way.
>> >
>> > According to the USB 3.0 specification, resetting a USB 3.0 port is
>> > required
>> > when an attached USB device transitions between different states, such
>> > as
>> > when it resumes from suspend.  Though, the Linux kernel performs
>> > additional
>> > USB 3.0 port resets upon initial USB device attachment, as visible in
>> > commit
>> > 07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the
>> > kernel
>> > source, to ensure proper state of the USB 3.0 hub port and proper USB
>> > mode
>> > negotiation during the initial USB device attachment and enumeration.
>> >
>> > These additional types of USB port resets don't exist for USB 2.0 hubs,
>> > according 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.
>> >
>> > The Linux kernel resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible
>> > in
>> > commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm
>> > reset.")
>> > in the kernel source.  The check for SuperSpeed hubs is performed in a
>> > way
>> > that also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as
>> > well,
>> > which hopefully makes it future proof.
>> >
>> > Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
>> > Link:
>> > https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>> > Link:
>> > https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>> > Signed-off-by: Shantur Rathore <i@shantur.com>
>> > Helped-by: Dragan Simic <dsimic@manjaro.org>
>> > Tested-by: Andre Przywara <andre.przywara@arm.com>
>> > Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>> > ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> >
>> > Shantur, I hope you're feeling better.
>> 
>> Thanks Dragan, V2 is submitted.
>> I am feeling much better, thanks for asking.

It's good to hear that you're feeling better!

> PS : I don't think I could ever explain as well as you did Dragan.
> Thanks a lot for the help.

I'm glad to help. :)  We're here to work together by combining
our skills and strengths.

> I will bring another pending matter to your attention on the Linux list 
> now ;P

Is it about the USB regulators on the RockPro64? :)  I haven't
forgotten that, and I expect to work on that rather soon.

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

Hello Shantur,

On 2024-02-12 14:40, Shantur Rathore wrote:
> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-02-08 15:17, Dragan Simic wrote:
>> > On 2024-02-08 15:10, Shantur Rathore wrote:
>> >> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic@manjaro.org>
>> >> wrote:
>> >>> On 2024-02-08 14:33, Marek Vasut wrote:
>> >>> > On 2/8/24 12:30, Shantur Rathore wrote:
>> >>> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>> >>> >>> On 2/7/24 11:23, Shantur Rathore wrote:
>> >>> >>>> USB 3.0 spec requires hub to reset device while
>> >>> >>>> enumeration. Some USB 2.0 hubs / devices don't
>> >>> >>>> handle this well and after implementation of
>> >>> >>>> reset some USB 2.0 disks weren't detected on
>> >>> >>>> Allwinner based boards.
>> >>> >>>>
>> >>> >>>> Resetting only when hub is USB 3.0 fixes it.
>> >>> >>>
>> >>> >>> It would be good to include as many details about the faulty hardware
>> >>> >>> in
>> >>> >>> the commit message as possible, so that when someone else runs into
>> >>> >>> this, they would have all that information available.
>> >>> >>>
>> >>> >>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> >>> >>>>
>> >>> >>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>> >>> >>>> ---
>> >>> >>>>
>> >>> >>>>    common/usb_hub.c | 6 ++++--
>> >>> >>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>> >>> >>>>
>> >>> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> >>> >>>> index 3fb7e14d10..2e054eb935 100644
>> >>> >>>> --- a/common/usb_hub.c
>> >>> >>>> +++ b/common/usb_hub.c
>> >>> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
>> >>> >>>> usb_hub_device *hub)
>> >>> >>>>
>> >>> >>>>        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(dev)) {
>> >>> >>>
>> >>> >>> Should this condition be "all which are lower than superspeed"
>> >>> >>> instead ,
>> >>> >>> so when the next generation of USB comes, this problem won't trigger
>> >>> >>> ?
>> >>> >>>
>> >>> >>> What does Linux do btw ?
>> >>> >>
>> >>> >> As of now Linux checks if the hub is superspeed
>> >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>> >>> >>
>> >>> >> which is
>> >>> >>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
>> >>> >> USB_HUB_PR_SS = 3
>> >>> >>
>> >>> >> This holds true for newer SuperSpeedPlus hubs as well.
>> >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>> >>> >>
>> >>> >> We can change the check to be  bDeviceProtocol > 2 but who knows if
>> >>> >> things change in the newer version of spec.
>> >>> >> I am open to suggestions.
>> >>> >
>> >>> > Please just include the ^ in the commit description. Use link to
>> >>> > git.kernel.org , not some mirror . This is extremely useful
>> >>> > information and, well, you already wrote the V2 commit message
>> >>> > addition in this answer.
>> >>>
>> >>> Shantur, if that would be easier or quicker for you, I can write
>> >>> a quite detailed patch description for you, in exchange for a
>> >>> "Helped-by" tag in the v2 patch submission. :)
>> >>
>> >> That would be really kind of you Dragan.
>> >
>> > Sure, I'll write the summary and send it over.
>> >
>> >> I am down with the flu so that would really help me as my brain is
>> >> working at 15% capacity.
>> >
>> > Oh, I'm really sorry to hear that. :(  I hope you'll get better
>> > soon, and I know very well what's it like;  I've also been sick
>> > recently, as a result of some kind of flu that unfortunately found
>> > its way into my lungs, and it took me about a month to get back
>> > to about 90% of my usual mental capacity.  I'm still not back to
>> > exactly 100%. :/
>> >
>> > I really hope you'll recover much faster.
>> 
>> I hope you're feeling better.
>> 
>> Below are the patch subject and description that I prepared for you,
>> please have a look.
>> 
>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>> 
>> Additional testing of the changes introduced in commit 33e06dcbe57a
>> ("common:
>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>> flash

s/some USB 3.0 flash/some USB 2.0 and 3.0 flash/

> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.

As visible in the message linked below, there was one USB 3.0 flash
drive that didn't work in a USB 2.0 port.  Though, you're right, there
was also a USB 2.0 drive that didn't work.  Thus, it's perhaps best to
adjust the wording as I suggested above and below.

https://lore.kernel.org/u-boot/20240202001218.63b4e9c3@minigeek.lan/

>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 
>> 2.0
>> only.
>> More precisely, some tested USB 3.0 flash drives failed to be detected
>> and

s/some tested USB 3.0 flash drives/some tested USB 2.0 and 3.0 flash 
drives/

>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB
>> 2.0
>> only, while the same USB flash drives worked just fine on a Pine64 H64
>> with
>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>> 
>> Resetting USB 3.0 hubs only has been tested to work as expected,
>> resolving
>> the previous issues on the Allwinner H616, while not introducing any 
>> new
>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>> 
>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>> required
>> when an attached USB device transitions between different states, such
>> as
>> when it resumes from suspend.  Though, the Linux kernel performs
>> additional
>> USB 3.0 port resets upon initial USB device attachment, presumably to
>> ensure
>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>> during
>> the initial USB device attachment and enumeration.
>> 
>> Such USB port resets don't seem to exist for USB 2.0 hubs, according 
>> 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.
>> 
>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>> visible
>> in file drivers/usb/core/hub.c in the kernel source, line 2859.  This
>> check
>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>> which
>> hopefully makes it future proof.
>> 
>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before 
>> scanning")
>> Link:
>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>> Link:
>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>> Signed-off-by: Shantur Rathore <i@shantur.com>
>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

Hello Shantur,

Please see my notes below.

On 2024-02-07 11:23, Shantur Rathore wrote:
> USB 3.0 spec requires hub to reset device while
> enumeration. Some USB 2.0 hubs / devices don't
> handle this well and after implementation of
> reset some USB 2.0 disks weren't detected on
> Allwinner based boards.
> 
> Resetting only when hub is USB 3.0 fixes it.
> 
> Tested-by: Andre Przywara <andre.przywara@arm.com>
> 
> Signed-off-by: Shantur Rathore <i@shantur.com>

Please, add the "Fixes" line available below, reformat the patch
description to the usual 78-character width, and remove unnecessary
"FIX " from the patch subject prefix.

Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")

As a side note, such tags can be produced very easily by running
"git log --max-count=1 --pretty=fixes <commit>".  I have an alias
like that defined in my ~/.gitconfig.

With these changes applied, please feel free to also add:

Reviewed-by: Dragan Simic <dsimic@anjaro.org>

> ---
> 
>  common/usb_hub.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 3fb7e14d10..2e054eb935 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device 
> *hub)
> 
>  	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(dev)) {
> +			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);
>  	}

Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic]

On 2024-02-07 14:14, Dragan Simic wrote:
> Hello Shantur,
> 
> Please see my notes below.
> 
> On 2024-02-07 11:23, Shantur Rathore wrote:
>> USB 3.0 spec requires hub to reset device while
>> enumeration. Some USB 2.0 hubs / devices don't
>> handle this well and after implementation of
>> reset some USB 2.0 disks weren't detected on
>> Allwinner based boards.
>> 
>> Resetting only when hub is USB 3.0 fixes it.
>> 
>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> 
>> Signed-off-by: Shantur Rathore <i@shantur.com>
> 
> Please, add the "Fixes" line available below, reformat the patch
> description to the usual 78-character width, and remove unnecessary
> "FIX " from the patch subject prefix.
> 
> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
> 
> As a side note, such tags can be produced very easily by running
> "git log --max-count=1 --pretty=fixes <commit>".  I have an alias
> like that defined in my ~/.gitconfig.
> 
> With these changes applied, please feel free to also add:
> 
> Reviewed-by: Dragan Simic <dsimic@anjaro.org>

Oops, sorry for the typo...  s/anjaro/manjaro/

>> ---
>> 
>>  common/usb_hub.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index 3fb7e14d10..2e054eb935 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct 
>> usb_hub_device *hub)
>> 
>>  	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(dev)) {
>> +			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);
>>  	}