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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f1e2e2b1-b83c-4105-b62c-a053d18c2985@molgen.mpg.de>
Date: Wed, 24 Jul 2024 20:14:34 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 David Brownell <david-b@...bell.net>,
 Kai-Heng Feng <kai.heng.feng@...onical.com>,
 Hans de Goede <hdegoede@...hat.com>, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: core: hub_port_reset: Remove extra 40 ms reset
 recovery time

[Cc: -gregkh@...e.de]

Dear Alan,


Thank you for your reply.

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).

It looks like on my system the ports are reset:

```
$ grep suspend-240501-063619/hub_port_reset abreu_mem_ftrace.txt
  6416.257589 |   3)  kworker-9023  |               | 
hub_port_reset [usbcore]() {
  6416.387182 |   2)  kworker-9023  |   129593.0 us |                  } 
/* hub_port_reset [usbcore] */
  6416.387380 |   2)  kworker-9023  |               | 
hub_port_reset [usbcore]() {
  6416.513458 |   3)  kworker-9023  |   126078.4 us |                  } 
/* hub_port_reset [usbcore] */
  6416.537813 |   2)  kworker-9844  |               | 
hub_port_reset [usbcore]() {
  6416.666142 |   3)  kworker-9844  |   128328.5 us |                  } 
/* hub_port_reset [usbcore] */
  6416.666429 |   3)  kworker-9844  |               | 
hub_port_reset [usbcore]() {
  6416.793315 |   1)  kworker-9844  |   126885.9 us |                  } 
/* hub_port_reset [usbcore] */
  6416.813559 |   1)  kworker-9849  |               | 
hub_port_reset [usbcore]() {
  6416.941882 |   2)  kworker-9849  |   128322.4 us |                  } 
/* hub_port_reset [usbcore] */
  6416.942633 |   2)  kworker-9849  |               | 
hub_port_reset [usbcore]() {
  6417.069205 |   3)  kworker-9849  |   126572.4 us |                  } 
/* hub_port_reset [usbcore] */
```

>> 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?


Kind regards,

Paul


>> Fixes: b789696af8b4 ("[PATCH] USB: relax usbcore reset timings")
>> Cc: Kai-Heng Feng <kai.heng.feng@...onical.com>
>> Cc: Hans de Goede <hdegoede@...hat.com>
>> Cc: David Brownell <david-b@...bell.net>
>> Signed-off-by: Paul Menzel <pmenzel@...gen.mpg.de>
>> ---
>>   drivers/usb/core/hub.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 4b93c0bd1d4b..487d5fe60f0c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -3110,7 +3110,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;
>>   
>>   			/* Hub needs extra delay after resetting its port. */
>>   			if (hub->hdev->quirks & USB_QUIRK_HUB_SLOW_RESET)
View attachment "abreu_mem_dmesg.txt" of type "text/plain" (5196 bytes)