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: <c7c299e7-605c-4bd6-afad-dfbfe266aa7e@rowland.harvard.edu>
Date: Wed, 24 Jul 2024 10:10:48 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Paul Menzel <pmenzel@...gen.mpg.de>
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>,
	Greg Kroah-Hartman <gregkh@...e.de>, 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 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.

Alan Stern

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