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: <712dee24-e939-4b1b-b2ea-0c0c12891a62@molgen.mpg.de>
Date: Sun, 4 Aug 2024 09:15:34 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Alan Stern <stern@...land.harvard.edu>,
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 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

[To: +Heikki]


Dear Alan, dear Heikki,


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).
>>>>
>>>> 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] */
>>>
>>>> ```
>>>
>>> It depends on the hardware and the kind of suspend.
>>
>> It is ACPI S3 suspend. Can I find out, why the ports are reset? Not
>> resetting the ports would be even better to reduce the resume time.
> 
> It's probably an xHCI thing -- the hardware may stop providing power to
> the ports during S3 suspend, or something like that.  The xHCI people
> may have a better idea of what's going on.

Heikki, can you confirm this. I am attaching the logs with

     echo 'file drivers/usb/* +p' | sudo tee 
/sys/kernel/debug/dynamic_debug/control

The logs have:

     [  150.414450] xhci_hcd 0000:00:14.0: clear port3 reset change, 
portsc: 0x603
     [  150.474375] xhci_hcd 0000:00:14.0: Resetting device with slot ID 1
     [  150.474378] xhci_hcd 0000:00:14.0: // Ding dong!
     [  150.474533] xhci_hcd 0000:00:14.0: Completed reset device command.
     [  150.474537] xhci_hcd 0000:00:14.0: Successful reset device command.
     [  150.474599] usb 1-3: reset full-speed USB device number 2 using 
xhci_hcd

(The list thread also has more logs [1].)

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


Kind regards,

Paul


[1]: 
https://lore.kernel.org/all/f1e2e2b1-b83c-4105-b62c-a053d18c2985@molgen.mpg.de/
View attachment "20240804--linux-6.10-rc1+--usb-dynamic-debug.txt" of type "text/plain" (124075 bytes)