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 for Android: free password hash cracker in your pocket

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

Message-ID: <8e300b0b-91f8-4003-a1b9-0f22869ae6e1@rowland.harvard.edu>
Date: Fri, 26 Jul 2024 13:48:45 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Paul Menzel <pmenzel@...gen.mpg.de>
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

On Wed, Jul 24, 2024 at 11:00:42PM +0200, Paul Menzel wrote:
> Dear Alan,
> 
> 
> 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.

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

Alan Stern