[PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[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 <markpearson@lenovo.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 00070a8a6507..e907dfa0ca6d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -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)
-- 
2.25.1

Re: [PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[Greg KH]

On Fri, Dec 10, 2021 at 01:16:53PM +0200, Mathias Nyman wrote:
> 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 <markpearson@lenovo.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  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?

thanks,

greg k-h

Re: [PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[Mathias Nyman]

On 10.12.2021 14.07, Greg KH wrote:
> On Fri, Dec 10, 2021 at 01:16:53PM +0200, Mathias Nyman wrote:
>> 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 <markpearson@lenovo.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>  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

Has been this way for 10 years. A guess would be since kernel v3.0:
5e467f6ebab1 usbcore: warm reset USB3 port in SS.Inactive state
But that code has been altered a few times since then.

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

maybe 5.17-rc1. 
Skipping stable until shown to resolve more critical issues, and has been
sitting in upstream for a while.

Thanks
-Mathias

Re: [PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[Alan Stern]

On Fri, Dec 10, 2021 at 01:16:53PM +0200, Mathias Nyman wrote:
> 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 <markpearson@lenovo.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 00070a8a6507..e907dfa0ca6d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -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;

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

> +		} 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)
> -- 
> 2.25.1
> 

Re: [PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[Mathias Nyman]

On 10.12.2021 21.12, Alan Stern wrote:
> On Fri, Dec 10, 2021 at 01:16:53PM +0200, Mathias Nyman wrote:
>> 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 <markpearson@lenovo.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>  drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 00070a8a6507..e907dfa0ca6d 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -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;
> 
> 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
>

I did first write this as:

while (hub_port_warm_reset_required(portstatus) && dev && still_retry--) {
	msleep();
	hub_port_status(&portstatus);
}

if (hub_port_warm_reset_required(portstatus)) {
	if (!dev || ...)
		hub_port_reset()
	else
		usb_reset_device()
}

But then decided it might be clearer to check if warm reset is required
in just one place.
Maybe it got a bit unnecessary complex to read inside that while loop,
even if this patch doesn't add any more code than the other way.

That said, this patch was tested by other external teams than just
Mark Pearson, all giving their thumbs up.
Not sure I can get the same amount of testing on a new version, and for
that reason, if there are no stronger objections, I'd like to keep this
as is. 

Thanks
-Mathias

Re: [PATCH] usb: hub: avoid warm port reset during USB3 disconnect

[Wohl, Tobias]

Hi Mathias,

I have encountered the problem that my device (zynq ultrascale+ platform) ends up
performing warm resets when disconnecting USB3 devices and this patch helped with that.
Nevertheless a much higher value for the define DETECT_DISCONNECT_TRIES was needed to detect
the disconnect status early:

[  211.847722] xhci-hcd xhci-hcd.0.auto: Port change event, 2-1, id 2, portsc: 0x4012c1
[  211.847739] xhci-hcd xhci-hcd.0.auto: handle_port_status: starting port polling.
[  211.847767] hub 2-0:1.0: state 7 ports 1 chg 0000 evt 0002
[  211.847786] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x4012c1, return 0x4002c1
[  211.847805] usb usb2-port1: link state change
[  211.847817] xhci-hcd xhci-hcd.0.auto: clear port1 link state change, portsc: 0x12c1
[  211.872383] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  211.872415] usb usb2-port1: Wait for inactive link disconnect detect
[  211.900342] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  211.900372] usb usb2-port1: Wait for inactive link disconnect detect
[  211.928342] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  211.928374] usb usb2-port1: Wait for inactive link disconnect detect
[  211.948323] xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
[  211.956338] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  211.956383] usb usb2-port1: Wait for inactive link disconnect detect
[  211.984345] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  211.984378] usb usb2-port1: Wait for inactive link disconnect detect
[  212.012406] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.012442] usb usb2-port1: Wait for inactive link disconnect detect
[  212.040542] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.040586] usb usb2-port1: Wait for inactive link disconnect detect
[  212.068343] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.068391] usb usb2-port1: Wait for inactive link disconnect detect
[  212.096344] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.096376] usb usb2-port1: Wait for inactive link disconnect detect
[  212.124343] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.124373] usb usb2-port1: Wait for inactive link disconnect detect
[  212.152352] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.152391] usb usb2-port1: Wait for inactive link disconnect detect
[  212.180341] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.180371] usb usb2-port1: Wait for inactive link disconnect detect
[  212.208366] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.208748] usb usb2-port1: Wait for inactive link disconnect detect
[  212.224375] xhci-hcd xhci-hcd.0.auto: Can't queue urb, port error, link inactive
[  212.236342] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x12c1, return 0x2c1
[  212.236373] usb usb2-port1: Wait for inactive link disconnect detect
[  212.251650] xhci-hcd xhci-hcd.0.auto: Port change event, 2-1, id 2, portsc: 0x202a0
[  212.251661] xhci-hcd xhci-hcd.0.auto: handle_port_status: starting port polling.
[  212.264374] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x202a0, return 0x102a0
[  212.264400] usb usb2-port1: Wait for inactive link disconnect detect
[  212.264442] xhci-hcd xhci-hcd.0.auto: Can't queue urb, port error, link inactive
[  212.264451] usb 2-1: Disable of device-initiated U1 failed.
[  212.264477] xhci-hcd xhci-hcd.0.auto: Can't queue urb, port error, link inactive
[  212.264484] usb 2-1: Disable of device-initiated U2 failed.
[  212.264493] xhci-hcd xhci-hcd.0.auto: Set up evaluate context for LPM MEL change.
[  212.264504] xhci-hcd xhci-hcd.0.auto: // Ding dong!
[  212.264529] xhci-hcd xhci-hcd.0.auto: Successful evaluate context command
[  212.264545] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x202a0, return 0x102a0
[  212.264570] xhci-hcd xhci-hcd.0.auto: set port reset, actual port 2-1 status  = 0x202b0
[  212.264599] hub 2-0:1.0: state 7 ports 1 chg 0000 evt 0002
[  212.332342] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x2a02a0, return 0x3102a0
[  212.332390] usb usb2-port1: not reset yet, waiting 60ms
[  212.400355] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x2a02a0, return 0x3102a0
[  212.400395] usb usb2-port1: not reset yet, waiting 200ms
[  212.416387] usb 1-1.2-port2: indicator off status 0
[  212.416470] usb 1-1.2-port3: indicator green status 0
[  212.416535] usb 1-1.2-port4: indicator green status 0
[  212.416617] usb 1-1.2-port5: indicator off status 0
[  212.416682] usb 1-1.2-port6: indicator off status 0
[  212.608338] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x2a02a0, return 0x3102a0
[  212.608376] usb usb2-port1: not reset yet, waiting 200ms
[  212.816348] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x2a02a0, return 0x3102a0
[  212.816404] usb usb2-port1: not reset yet, waiting 200ms
[  213.024340] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x2a02a0, return 0x3102a0
[  213.024376] usb usb2-port1: not reset yet, waiting 200ms
[  213.024388] xhci-hcd xhci-hcd.0.auto: clear port1 reset change, portsc: 0xa02a0
[  213.024411] xhci-hcd xhci-hcd.0.auto: clear port1 warm(BH) reset change, portsc: 0x202a0
[  213.024434] xhci-hcd xhci-hcd.0.auto: clear port1 link state change, portsc: 0x202a0
[  213.024456] xhci-hcd xhci-hcd.0.auto: clear port1 connect change, portsc: 0x2a0
[  213.024479] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x2a0, return 0x2a0
[  213.024506] xhci-hcd xhci-hcd.0.auto: CTRL: TypeReq=0x2303 val=0x5 idx=0x301 len=0 ==> -19
[  213.024527] usb usb2-port1: logical disconnect
[  213.024537] xhci-hcd xhci-hcd.0.auto: CTRL: TypeReq=0x2303 val=0x5 idx=0x301 len=0 ==> -19
[  213.024583] xhci-hcd xhci-hcd.0.auto: Get port status 2-1 read: 0x2a0, return 0x2a0
[  213.024636] usb usb2-port1: status 02a0, change 0000, 5.0 Gb/s
[  213.024645] usb 2-1: USB disconnect, device number 5

In some rare cases even

#define DETECT_DISCONNECT_TRIES 20

was not enough. As I want to make sure that the warm resets will never be performed for
a disconnected device (as this takes a long time), I want to enhance the value for
DETECT_DISCONNECT_TRIES for my application.

Are there any issues to expect when setting a too high value for DETECT_DISCONNECT_TRIES?

Thanks,

Tobias

On 10.12.2021 12:16, Mathias Nyman wrote:

> 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 <markpearson@lenovo.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>   drivers/usb/core/hub.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 00070a8a6507..e907dfa0ca6d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -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)