usb: hub: avoid warm port reset during USB3 disconnect

During disconnect USB-3 ports often go via SS.Inactive link error state before the missing terminations are noticed, and link finally goes to RxDetect state

Avoid immediately warm-resetting ports in SS.Inactive state. Let ports settle for a while and re-read the link status a few times 20ms apart to see if the ports transitions out of SS.Inactive.

According to USB 3.x spec 7.5.2, a port in SS.Inactive should automatically check for missing far-end receiver termination every 12 ms (SSInactiveQuietTimeout)

The futile multiple warm reset retries of a disconnected device takes a lot of time, also the resetting of a removed devices has caused cases where the reset bit got stuck for a long time on xHCI roothub. This lead to issues in detecting new devices connected to the same port shortly after.

Tested-by Mark Pearson <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Quoted 18 lines · Mathias Nyman

During disconnect USB-3 ports often go via SS.Inactive link error state before the missing terminations are noticed, and link finally goes to RxDetect state

Avoid immediately warm-resetting ports in SS.Inactive state. Let ports settle for a while and re-read the link status a few times 20ms apart to see if the ports transitions out of SS.Inactive.

According to USB 3.x spec 7.5.2, a port in SS.Inactive should automatically check for missing far-end receiver termination every 12 ms (SSInactiveQuietTimeout)

The futile multiple warm reset retries of a disconnected device takes a lot of time, also the resetting of a removed devices has caused cases where the reset bit got stuck for a long time on xHCI roothub. This lead to issues in detecting new devices connected to the same port shortly after.

Quoted 5 lines

Tested-by Mark Pearson <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Does this fix a specific commit, or has it always been this way? And is

this for 5.16-final or 5.17-rc1 and/or stable trees?

During disconnect USB-3 ports often go via SS.Inactive link error state before the missing terminations are noticed, and link finally goes to RxDetect state

Avoid immediately warm-resetting ports in SS.Inactive state. Let ports settle for a while and re-read the link status a few times 20ms apart to see if the ports transitions out of SS.Inactive.

According to USB 3.x spec 7.5.2, a port in SS.Inactive should automatically check for missing far-end receiver termination every 12 ms (SSInactiveQuietTimeout)

The futile multiple warm reset retries of a disconnected device takes a lot of time, also the resetting of a removed devices has caused cases where the reset bit got stuck for a long time on xHCI roothub. This lead to issues in detecting new devices connected to the same port shortly after.

Tested-by Mark Pearson <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

drivers/usb/core/hub.c +7 -3

index 00070a8a6507..e907dfa0ca6d 100644
@@ -2777,6 +2777,8 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
 #define PORT_INIT_TRIES		4
 #endif	/* CONFIG_USB_FEW_INIT_RETRIES */
 
+#define DETECT_DISCONNECT_TRIES 5
+
 #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
 #define HUB_SHORT_RESET_TIME	10
 #define HUB_BH_RESET_TIME	50

@@ -5543,6 +5545,7 @@ static void port_event(struct usb_hub *hub, int port1)
 	struct usb_device *udev = port_dev->child;
 	struct usb_device *hdev = hub->hdev;
 	u16 portstatus, portchange;
+	int i = 0;
 
 	connect_change = test_bit(port1, hub->change_bits);
 	clear_bit(port1, hub->event_bits);

@@ -5619,17 +5622,27 @@ static void port_event(struct usb_hub *hub, int port1)
 		connect_change = 1;
 
 	/*
-	 * Warm reset a USB3 protocol port if it's in
-	 * SS.Inactive state.
+	 * Avoid trying to recover a USB3 SS.Inactive port with a warm reset if
+	 * the device was disconnected. A 12ms disconnect detect timer in
+	 * SS.Inactive state transitions the port to RxDetect automatically.
+	 * SS.Inactive link error state is common during device disconnect.
 	 */
-	if (hub_port_warm_reset_required(hub, port1, portstatus)) {

drivers/usb/core/hub.c +4 -0

+		} else if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
 				|| udev->state == USB_STATE_NOTATTACHED) {
+			dev_dbg(&port_dev->dev, "do warm reset, port only\n");
 			if (hub_port_reset(hub, port1, NULL,
 					HUB_BH_RESET_TIME, true) < 0)
 				hub_port_disable(hub, port1, 1);
 		} else {
+			dev_dbg(&port_dev->dev, "do warm reset, full device\n");
 			usb_unlock_port(port_dev);
 			usb_lock_device(udev);
 			usb_reset_device(udev);

@@ -5637,6 +5650,7 @@ static void port_event(struct usb_hub *hub, int port1)
 			usb_lock_port(port_dev);
 			connect_change = 0;
 		}
+		break;
 	}
 
 	if (connect_change)

Signature · 1 line

2.25.1

Quoted 57 lines · Mathias Nyman

During disconnect USB-3 ports often go via SS.Inactive link error state before the missing terminations are noticed, and link finally goes to RxDetect state

Avoid immediately warm-resetting ports in SS.Inactive state. Let ports settle for a while and re-read the link status a few times 20ms apart to see if the ports transitions out of SS.Inactive.

According to USB 3.x spec 7.5.2, a port in SS.Inactive should automatically check for missing far-end receiver termination every 12 ms (SSInactiveQuietTimeout)

The futile multiple warm reset retries of a disconnected device takes a lot of time, also the resetting of a removed devices has caused cases where the reset bit got stuck for a long time on xHCI roothub. This lead to issues in detecting new devices connected to the same port shortly after.

Tested-by Mark Pearson <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

drivers/usb/core/hub.c +7 -3

index 00070a8a6507..e907dfa0ca6d 100644
@@ -2777,6 +2777,8 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
 #define PORT_INIT_TRIES		4
 #endif	/* CONFIG_USB_FEW_INIT_RETRIES */
 
+#define DETECT_DISCONNECT_TRIES 5
+
 #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
 #define HUB_SHORT_RESET_TIME	10
 #define HUB_BH_RESET_TIME	50

@@ -5543,6 +5545,7 @@ static void port_event(struct usb_hub *hub, int port1)
 	struct usb_device *udev = port_dev->child;
 	struct usb_device *hdev = hub->hdev;
 	u16 portstatus, portchange;
+	int i = 0;
 
 	connect_change = test_bit(port1, hub->change_bits);
 	clear_bit(port1, hub->event_bits);

@@ -5619,17 +5622,27 @@ static void port_event(struct usb_hub *hub, int port1)
 		connect_change = 1;
 
 	/*
-	 * Warm reset a USB3 protocol port if it's in
-	 * SS.Inactive state.
+	 * Avoid trying to recover a USB3 SS.Inactive port with a warm reset if
+	 * the device was disconnected. A 12ms disconnect detect timer in
+	 * SS.Inactive state transitions the port to RxDetect automatically.
+	 * SS.Inactive link error state is common during device disconnect.
 	 */
-	if (hub_port_warm_reset_required(hub, port1, portstatus)) {

drivers/usb/core/hub.c static void port_event(struct usb_hub *hub, int port1) · line 5637

Quoted 10 lines

drivers/usb/core/hub.c +8 -2

-		dev_dbg(&port_dev->dev, "do warm reset\n");
-		if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
+	while (hub_port_warm_reset_required(hub, port1, portstatus)) {
+		if ((i++ < DETECT_DISCONNECT_TRIES) && udev) {
+			u16 unused;
+
+			msleep(20);
+			hub_port_status(hub, port1, &portstatus, &unused);
+			dev_dbg(&port_dev->dev, "Wait for inactive link disconnect detect\n");
+			continue;

This may be bikeshedding, and you should feel free to ignore the following suggestion if you dislike it.

Don't you think it would be a lot clearer if the new "while" loop covered only the code above, and the two sections below (port-only or full-device warm reset) came after the end of the loop? I had to reread the patch a few times to figure out what it was really doing.

Alan Stern

During disconnect USB-3 ports often go via SS.Inactive link error state before the missing terminations are noticed, and link finally goes to RxDetect state

Avoid immediately warm-resetting ports in SS.Inactive state. Let ports settle for a while and re-read the link status a few times 20ms apart to see if the ports transitions out of SS.Inactive.

According to USB 3.x spec 7.5.2, a port in SS.Inactive should automatically check for missing far-end receiver termination every 12 ms (SSInactiveQuietTimeout)

The futile multiple warm reset retries of a disconnected device takes a lot of time, also the resetting of a removed devices has caused cases where the reset bit got stuck for a long time on xHCI roothub. This lead to issues in detecting new devices connected to the same port shortly after.

Tested-by Mark Pearson <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
  drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
  1 file changed, 19 insertions(+), 5 deletions(-)

drivers/usb/core/hub.c +19 -5

index 00070a8a6507..e907dfa0ca6d 100644
@@ -2777,6 +2777,8 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
  #define PORT_INIT_TRIES		4
  #endif	/* CONFIG_USB_FEW_INIT_RETRIES */
 
+#define DETECT_DISCONNECT_TRIES 5
+
  #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
  #define HUB_SHORT_RESET_TIME	10
  #define HUB_BH_RESET_TIME	50

@@ -5543,6 +5545,7 @@ static void port_event(struct usb_hub *hub, int port1)
  	struct usb_device *udev = port_dev->child;
  	struct usb_device *hdev = hub->hdev;
  	u16 portstatus, portchange;
+	int i = 0;
 
  	connect_change = test_bit(port1, hub->change_bits);
  	clear_bit(port1, hub->event_bits);

@@ -5619,17 +5622,27 @@ static void port_event(struct usb_hub *hub, int port1)
  		connect_change = 1;
 
  	/*
-	 * Warm reset a USB3 protocol port if it's in
-	 * SS.Inactive state.
+	 * Avoid trying to recover a USB3 SS.Inactive port with a warm reset if
+	 * the device was disconnected. A 12ms disconnect detect timer in
+	 * SS.Inactive state transitions the port to RxDetect automatically.
+	 * SS.Inactive link error state is common during device disconnect.
  	 */
-	if (hub_port_warm_reset_required(hub, port1, portstatus)) {
-		dev_dbg(&port_dev->dev, "do warm reset\n");
-		if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
+	while (hub_port_warm_reset_required(hub, port1, portstatus)) {
+		if ((i++ < DETECT_DISCONNECT_TRIES) && udev) {
+			u16 unused;
+
+			msleep(20);
+			hub_port_status(hub, port1, &portstatus, &unused);
+			dev_dbg(&port_dev->dev, "Wait for inactive link disconnect detect\n");
+			continue;
+		} else if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
  				|| udev->state == USB_STATE_NOTATTACHED) {
+			dev_dbg(&port_dev->dev, "do warm reset, port only\n");
  			if (hub_port_reset(hub, port1, NULL,
  					HUB_BH_RESET_TIME, true) < 0)
  				hub_port_disable(hub, port1, 1);
  		} else {
+			dev_dbg(&port_dev->dev, "do warm reset, full device\n");
  			usb_unlock_port(port_dev);
  			usb_lock_device(udev);
  			usb_reset_device(udev);

@@ -5637,6 +5650,7 @@ static void port_event(struct usb_hub *hub, int port1)
  			usb_lock_port(port_dev);
  			connect_change = 0;
  		}
+		break;
  	}
 
  	if (connect_change)