[FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Dragan Simic
dsimic at manjaro.org
Tue Feb 13 04:59:21 CET 2024
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 at shantur.com>
>>>> wrote:
>>>>> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic at 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 at 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 at 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 at arm.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Shantur Rathore <i at 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 at shantur.com>
>>>>>> Helped-by: Dragan Simic <dsimic at manjaro.org>
>>>>>> Tested-by: Andre Przywara <andre.przywara at arm.com>
>>>>>> Reviewed-by: Dragan Simic <dsimic at 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.
More information about the U-Boot
mailing list