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

[Dragan Simic <dsimic@manjaro.org>]

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.