Re: [PATCH 1/2] usb: hub: use shorter 120ms post resume hold for SS root hubs

[Mathias Nyman <mathias.nyman@linux.intel.com>]

Hi

Some minor nitpicks/personal preferences if you are sending a v2.

I don't really object taking this as is either.

On 8/31/26 22:03, Lovekesh Solanki wrote:
> Commit 8f5b7e2bec1c ("usb: hub: fix detection of high tier USB3 devices
> behind suspended hubs") holds a runtime PM reference for
> USB_SS_PORT_U0_WAKE_TIME (200ms) after every SS hub resume, so
> downstream links have time to finish trianing before the hub can
> suspend again.
> 
> But 200ms is excessive for root hubs since a root hub has no upstream
> suspended hub whose wake propagation needs to be accounted for.
> The root hub still needs some time for USB3 link training to complete,
> And root hubs also default to zero auto suspend delay and get
> periodically resumed by userspace USB enumeration opening and
> closing their usbfs nodes (adb, libusb).
> This extra 200ms hold stretches this resume into a suspend/resume
> cycle of roughly once per second, which on idle AMD systems have
> been reported to cause system freezes.

I'd highlight the fact that this patch mitigates the AMD platform issue.

Maybe begin commit message with something like

"Holding a USB3 hub PM runtime reference for 200ms at hub resume triggers a
  AMD platform issue. Users running Android adb report crashes after adb has
  been polling and waking up the USB subsystem once a second for some time.
  Vendors are working on a solution. Disabling USB runtime PM is one way to
  prevent this issue, but it is also proven that reducing the hold time to 120ms in
  resume also mitigates it. See Link for more details.

  Reducing the hold time to 120ms for the USB3 roothub is in itself a valid change
  and optimization as the current 200ms is excessive...
"

> 
> Shorten the post resume hold to 120ms only for root hubs but
> keep the 200ms for external hubs that the original commit is
> intended for.
> 
> Reported-by: Mathieu Fluhr <mathieu.fluhr@gmail.com>
> Link: https://lore.kernel.org/all/CAPyJwA_D9qw0T72f8zwM1yKjP+To=maVANbcdsWM7yRmbBxYvw@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Fixes: 8f5b7e2bec1c ("usb: hub: fix detection of high tier USB3 devices behind suspended hubs")
> Signed-off-by: Lovekesh Solanki <lovekeshsolanki00@gmail.com>
> ---
>   drivers/usb/core/hub.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3345b3298daf..e1415ad5f908 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -76,6 +76,12 @@
>    */
>   #define USB_SS_PORT_U0_WAKE_TIME	200  /* ms */
>   
> +/*
> + * Root hubs have no upstream hub whose wake propagation needs to be
> + * accounted for, but still needs time for USB3 link training to complete.
> + */
> +#define USB_SS_ROOT_HUB_U0_WAKE_TIME 120 /* ms */

I'd do minor changes to the original code comment above the first #define,
and just add the second define right after it

/*
  * modified original comment
  */
#define USB_SS_PORT_U0_WAKE_TIME	200  /* ms */
#define USB_SS_RH_PORT_U0_WAKE_TIME	120  /* ms */

Thanks
Mathias