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@xxxxxxxxxx>
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> ---
> 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
>
