linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: hub: fix detection of high tier USB3 devices behind suspended hubs
@ 2025-06-09 12:20 Mathias Nyman
  2025-06-09 13:57 ` Alan Stern
  2025-06-10  9:34 ` Oliver Neukum
  0 siblings, 2 replies; 4+ messages in thread
From: Mathias Nyman @ 2025-06-09 12:20 UTC (permalink / raw)
  To: gregkh; +Cc: linux-usb, stern, Mathias Nyman, stable

USB3 devices connected behind several external suspended hubs may not
be detected when plugged in due to aggressive hub runtime pm suspend.

The hub driver immediately runtime-suspends hubs if there are no
active children or port activity.

There is a delay between the wake signal causing hub resume, and driver
visible port activity on the hub downstream facing ports.
Most of the LFPS handshake, resume signaling and link training done
on the downstream ports is not visible to the hub driver until completed,
when device then will appear fully enabled and running on the port.

This delay between wake signal and detectable port change is even more
significant with chained suspended hubs where the wake signal will
propagate upstream first. Suspended hubs will only start resuming
downstream ports after upstream facing port resumes.

The hub driver may resume a USB3 hub, read status of all ports, not
yet see any activity, and runtime suspend back the hub before any
port activity is visible.

This exact case was seen when conncting USB3 devices to a suspended
Thunderbolt dock.

USB3 specification defines a 100ms tU3WakeupRetryDelay, indicating
USB3 devices expect to be resumed within 100ms after signaling wake.
if not then device will resend the wake signal.

Give the USB3 hubs twice this time (200ms) to detect any port
changes after resume, before allowing hub to runtime suspend again.

Cc: stable@vger.kernel.org
Fixes: 596d789a211d ("USB: set hub's default autosuspend delay as 0")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hub.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 770d1e91183c..5c12dfdef569 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -68,6 +68,12 @@
  */
 #define USB_SHORT_SET_ADDRESS_REQ_TIMEOUT	500  /* ms */
 
+/*
+ * Give SS hubs 200ms time after wake to train downstream links before
+ * assuming no port activity and allowing hub to runtime suspend back.
+ */
+#define USB_SS_PORT_U0_WAKE_TIME	200  /* ms */
+
 /* Protect struct usb_device->state and ->children members
  * Note: Both are also protected by ->dev.sem, except that ->state can
  * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */
@@ -1068,11 +1074,12 @@ int usb_remove_device(struct usb_device *udev)
 
 enum hub_activation_type {
 	HUB_INIT, HUB_INIT2, HUB_INIT3,		/* INITs must come first */
-	HUB_POST_RESET, HUB_RESUME, HUB_RESET_RESUME,
+	HUB_POST_RESET, HUB_RESUME, HUB_RESET_RESUME, HUB_POST_RESUME,
 };
 
 static void hub_init_func2(struct work_struct *ws);
 static void hub_init_func3(struct work_struct *ws);
+static void hub_post_resume(struct work_struct *ws);
 
 static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 {
@@ -1095,6 +1102,13 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 			goto init2;
 		goto init3;
 	}
+
+	if (type == HUB_POST_RESUME) {
+		usb_autopm_put_interface_async(to_usb_interface(hub->intfdev));
+		hub_put(hub);
+		return;
+	}
+
 	hub_get(hub);
 
 	/* The superspeed hub except for root hub has to use Hub Depth
@@ -1343,6 +1357,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 		device_unlock(&hdev->dev);
 	}
 
+	if (type == HUB_RESUME && hub_is_superspeed(hub->hdev)) {
+		/* give usb3 downstream links training time after hub resume */
+		INIT_DELAYED_WORK(&hub->init_work, hub_post_resume);
+		queue_delayed_work(system_power_efficient_wq, &hub->init_work,
+				   msecs_to_jiffies(USB_SS_PORT_U0_WAKE_TIME));
+		usb_autopm_get_interface_no_resume(
+			to_usb_interface(hub->intfdev));
+		return;
+	}
+
 	hub_put(hub);
 }
 
@@ -1361,6 +1385,13 @@ static void hub_init_func3(struct work_struct *ws)
 	hub_activate(hub, HUB_INIT3);
 }
 
+static void hub_post_resume(struct work_struct *ws)
+{
+	struct usb_hub *hub = container_of(ws, struct usb_hub, init_work.work);
+
+	hub_activate(hub, HUB_POST_RESUME);
+}
+
 enum hub_quiescing_type {
 	HUB_DISCONNECT, HUB_PRE_RESET, HUB_SUSPEND
 };
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] usb: hub: fix detection of high tier USB3 devices behind suspended hubs
  2025-06-09 12:20 [PATCH] usb: hub: fix detection of high tier USB3 devices behind suspended hubs Mathias Nyman
@ 2025-06-09 13:57 ` Alan Stern
  2025-06-10  9:34 ` Oliver Neukum
  1 sibling, 0 replies; 4+ messages in thread
From: Alan Stern @ 2025-06-09 13:57 UTC (permalink / raw)
  To: gregkh, linux-usb, stable

On Mon, Jun 09, 2025 at 03:20:47PM +0300, Mathias Nyman wrote:
> USB3 devices connected behind several external suspended hubs may not
> be detected when plugged in due to aggressive hub runtime pm suspend.
> 
> The hub driver immediately runtime-suspends hubs if there are no
> active children or port activity.
> 
> There is a delay between the wake signal causing hub resume, and driver
> visible port activity on the hub downstream facing ports.
> Most of the LFPS handshake, resume signaling and link training done
> on the downstream ports is not visible to the hub driver until completed,
> when device then will appear fully enabled and running on the port.
> 
> This delay between wake signal and detectable port change is even more
> significant with chained suspended hubs where the wake signal will
> propagate upstream first. Suspended hubs will only start resuming
> downstream ports after upstream facing port resumes.
> 
> The hub driver may resume a USB3 hub, read status of all ports, not
> yet see any activity, and runtime suspend back the hub before any
> port activity is visible.
> 
> This exact case was seen when conncting USB3 devices to a suspended
> Thunderbolt dock.
> 
> USB3 specification defines a 100ms tU3WakeupRetryDelay, indicating
> USB3 devices expect to be resumed within 100ms after signaling wake.
> if not then device will resend the wake signal.
> 
> Give the USB3 hubs twice this time (200ms) to detect any port
> changes after resume, before allowing hub to runtime suspend again.
> 
> Cc: stable@vger.kernel.org
> Fixes: 596d789a211d ("USB: set hub's default autosuspend delay as 0")
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] usb: hub: fix detection of high tier USB3 devices behind suspended hubs
  2025-06-09 12:20 [PATCH] usb: hub: fix detection of high tier USB3 devices behind suspended hubs Mathias Nyman
  2025-06-09 13:57 ` Alan Stern
@ 2025-06-10  9:34 ` Oliver Neukum
  2025-06-10 10:43   ` Mathias Nyman
  1 sibling, 1 reply; 4+ messages in thread
From: Oliver Neukum @ 2025-06-10  9:34 UTC (permalink / raw)
  To: Mathias Nyman, gregkh; +Cc: linux-usb, stern, stable

On 09.06.25 14:20, Mathias Nyman wrote:

> 
> Cc: stable@vger.kernel.org
> Fixes: 596d789a211d ("USB: set hub's default autosuspend delay as 0")

Is that the correct breaker commit? It seems to me that it marks
only the commit which turned the problem into the default. It
was always possible.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] usb: hub: fix detection of high tier USB3 devices behind suspended hubs
  2025-06-10  9:34 ` Oliver Neukum
@ 2025-06-10 10:43   ` Mathias Nyman
  0 siblings, 0 replies; 4+ messages in thread
From: Mathias Nyman @ 2025-06-10 10:43 UTC (permalink / raw)
  To: Oliver Neukum, gregkh; +Cc: linux-usb, stern, stable

On 10.6.2025 12.34, Oliver Neukum wrote:
> On 09.06.25 14:20, Mathias Nyman wrote:
> 
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 596d789a211d ("USB: set hub's default autosuspend delay as 0")
> 
> Is that the correct breaker commit? It seems to me that it marks
> only the commit which turned the problem into the default. It
> was always possible.

True, user could trigger the issue by manually setting autosuspend delay to 0
before that patch,

Maybe a better Fixes commit would be:
2839f5bcfcfc ("USB: Turn on auto-suspend for USB 3.0 hubs.")

Both are from 2012 so not sure it really matters anymore
  
("USB: Turn on auto-suspend for USB 3.0 hubs.") was added to v3.4 kernel
("USB: set hub's default autosuspend delay as 0") was added to v3.8 kernel

Thanks
-Mathias


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-06-10 10:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-09 12:20 [PATCH] usb: hub: fix detection of high tier USB3 devices behind suspended hubs Mathias Nyman
2025-06-09 13:57 ` Alan Stern
2025-06-10  9:34 ` Oliver Neukum
2025-06-10 10:43   ` Mathias Nyman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).