Re: [PATCH] USB: core: hub_port_reset: Remove extra 40 ms reset recovery time

[Paul Menzel <pmenzel@molgen.mpg.de>]

[To: +Nicolas, Cc: -Heikki]


Dear Alan, dear Nicolas,


Am 04.08.24 um 09:15 schrieb Paul Menzel:

> Am 26.07.24 um 19:48 schrieb Alan Stern:
>> On Wed, Jul 24, 2024 at 11:00:42PM +0200, Paul Menzel wrote:
> 
>>> Am 24.07.24 um 20:52 schrieb Alan Stern:
>>>> On Wed, Jul 24, 2024 at 08:14:34PM +0200, Paul Menzel wrote:
>>>
>>> […]
>>>
>>>>> Am 24.07.24 um 16:10 schrieb Alan Stern:
>>>>>> On Wed, Jul 24, 2024 at 01:15:23PM +0200, Paul Menzel wrote:
>>>>>>> This basically reverts commit b789696af8b4102b7cc26dec30c2c51ce51ee18b
>>>>>>> ("[PATCH] USB: relax usbcore reset timings") from 2005.
>>>>>>>
>>>>>>> This adds unneeded 40 ms during resume from suspend on a majority of
>>>>>>
>>>>>> Wrong.  It adds 40 ms to the recovery time from a port reset -- see the
>>>>>> commit's title.  Suspend and resume do not in general involve port
>>>>>> resets (although sometimes they do).

[…]

>>>>>>> devices, where it’s not needed, like the Dell XPS 13 9360/0596KF, 
>>>>>>> BIOS 2.21.0 06/02/2022 with
>>>>>>
>>>>>>> The commit messages unfortunately does not list the devices 
>>>>>>> needing this.
>>>>>>> Should they surface again, these should be added to the quirk 
>>>>>>> list for
>>>>>>> USB_QUIRK_HUB_SLOW_RESET.
>>>>>>
>>>>>> This quirk applies to hubs that need extra time when one of their 
>>>>>> ports
>>>>>> gets reset.  However, it seems likely that the patch you are 
>>>>>> reverting
>>>>>> was meant to help the device attached to the port, not the hub 
>>>>>> itself.
>>>>>> Which would mean that the adding hubs to the quirk list won't help
>>>>>> unless every hub is added -- in which case there's no point reverting
>>>>>> the patch.
>>>>>>
>>>>>> Furthermore, should any of these bad hubs or devices still be in use,
>>>>>> your change would cause them to stop working reliably.  It would be a
>>>>>> regression.
>>>>>>
>>>>>> A better approach would be to add a sysfs boolean attribute to the 
>>>>>> hub
>>>>>> driver to enable the 40-ms reset-recovery delay, and make it 
>>>>>> default to
>>>>>> True.  Then people who don't need the delay could disable it from
>>>>>> userspace, say by a udev rule.
>>>>>
>>>>> How would you name it?
>>>>
>>>> You could call it "long_reset_recovery".  Anything like that would be
>>>> okay.
>>>
>>> Would it be useful to makes it an integer instead of a boolean, and 
>>> allow to configure the delay: `extra_reset_recovery_delay_ms`?
>>
>> Sure, why not?  Just so long as the default value matches the current
>> behavior.
> 
> I hope, I am going to find time to take a stab at it.

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4b93c0bd1d4b..72dd16eaa73a 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -120,9 +120,16 @@ MODULE_PARM_DESC(use_both_schemes,
                 "try the other device initialization scheme if the "
                 "first one fails");

+static int extra_reset_recovery_delay_ms = 40;
+module_param(extra_reset_recovery_delay_ms, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(extra_reset_recovery_delay_ms,
+               "extra recovery delay for USB devices after reset in 
milliseconds "
+               "(default 40 ms");
+
  /* Mutual exclusion for EHCI CF initialization.  This interferes with
   * port reset on some companion controllers.
   */
+
  DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
  EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);

@@ -3110,7 +3117,7 @@ static int hub_port_reset(struct usb_hub *hub, int 
port1,
                         usleep_range(10000, 12000);
                 else {
                         /* TRSTRCY = 10 ms; plus some extra */
-                       reset_recovery_time = 10 + 40;
+                       reset_recovery_time = 10 + 
extra_reset_recovery_delay_ms;

                         /* Hub needs extra delay after resetting its 
port. */
                         if (hub->hdev->quirks & USB_QUIRK_HUB_SLOW_RESET)

The if condition above

		if (port_dev->quirks & USB_PORT_QUIRK_FAST_ENUM)
			usleep_range(10000, 12000);

is from Nicholas’ commit aa071a92bbf0 (usb: hub: Per-port setting to 
reduce TRSTRCY to 10 ms) from 2018 [2] adding the port quirk 
`USB_PORT_QUIRK_FAST_ENUM`.

> urrently, the USB hub core waits for 50 ms after enumerating the
> device. This was added to help "some high speed devices" to
> enumerate (b789696af8 "[PATCH] USB: relax usbcore reset timings").
> 
> On some devices, the time-to-active is important, so we provide
> a per-port option to reduce the time to what the USB specification
> requires: 10 ms.

Nicholas, do you have field data from ChromeOS if the 40 ms delay is 
needed, and do you apply the quirk to all ports?

Being ignorant about USB in general, does this quirk make my patch 
obsolete, or should I just send a patch with the diff above?


Kind regards,

Paul


> [1]: https://lore.kernel.org/all/f1e2e2b1-b83c-4105-b62c-a053d18c2985@molgen.mpg.de/
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aa071a92bbf09d993ff0dbf3b1f2b53ac93ad654