[PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Vikas Sajjan]

Does warm reset while activating SuperSpeed HUBs if the hub activate type
is HUB_RESET_RESUME.

When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
USB 3.0 device connected on 3.0 port, during resume I noticed that the
XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
This behaviour is inconsistent and the connection with connected USB 3.0 device
on 3.0 port was LOST.

Doing warm reset while activating SuperSpeed HUBs if the hub
activate type is HUB_RESET_RESUME, gets the connected device to the stable state.

Reviewed at https://chromium-review.googlesource.com/#/c/177132/

Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)

rebased on Greg Kroah-Hartman's usb-next
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git

Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
---
 drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a7c04e2..d8432b0 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -993,6 +993,21 @@ int usb_remove_device(struct usb_device *udev)
 	return 0;
 }
 
+#define PORT_RESET_TRIES	5
+#define SET_ADDRESS_TRIES	2
+#define GET_DESCRIPTOR_TRIES	2
+#define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
+#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
+
+#define HUB_ROOT_RESET_TIME	50	/* times are in msec */
+#define HUB_SHORT_RESET_TIME	10
+#define HUB_BH_RESET_TIME	50
+#define HUB_LONG_RESET_TIME	200
+#define HUB_RESET_TIMEOUT	800
+
+static int hub_port_reset(struct usb_hub *hub, int port1,
+			struct usb_device *udev, unsigned int delay, bool warm);
+
 enum hub_activation_type {
 	HUB_INIT, HUB_INIT2, HUB_INIT3,		/* INITs must come first */
 	HUB_POST_RESET, HUB_RESUME, HUB_RESET_RESUME,
@@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 		u16 portstatus, portchange;
 
 		portstatus = portchange = 0;
+
+		/* Some connected devices might be still in unknown state even
+		 * after reset-resume, a WARM_RESET gets the connected device
+		 * to the normal state.
+		 */
+		if (udev && hub_is_superspeed(hub->hdev) &&
+						type == HUB_RESET_RESUME)
+			hub_port_reset(hub, port1, NULL,
+						HUB_BH_RESET_TIME, true);
+
 		status = hub_port_status(hub, port1, &portstatus, &portchange);
 		if (udev || (portstatus & USB_PORT_STAT_CONNECTION))
 			dev_dbg(hub->intfdev,
@@ -2510,22 +2535,6 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
 	return hcd->wireless;
 }
 
-
-#define PORT_RESET_TRIES	5
-#define SET_ADDRESS_TRIES	2
-#define GET_DESCRIPTOR_TRIES	2
-#define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
-#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
-
-#define HUB_ROOT_RESET_TIME	50	/* times are in msec */
-#define HUB_SHORT_RESET_TIME	10
-#define HUB_BH_RESET_TIME	50
-#define HUB_LONG_RESET_TIME	200
-#define HUB_RESET_TIMEOUT	800
-
-static int hub_port_reset(struct usb_hub *hub, int port1,
-			struct usb_device *udev, unsigned int delay, bool warm);
-
 /* Is a USB 3.0 port in the Inactive or Complinance Mode state?
  * Port worm reset is required to recover
  */
-- 
1.7.9.5

[PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Vikas Sajjan]

Does warm reset while activating SuperSpeed HUBs if the hub activate type
is HUB_RESET_RESUME.

When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
USB 3.0 device connected on 3.0 port, during resume I noticed that the
XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
This behaviour is inconsistent and the connection with connected USB 3.0 device
on 3.0 port was LOST.

Doing warm reset while activating SuperSpeed HUBs if the hub
activate type is HUB_RESET_RESUME, gets the connected device to the stable state.

Reviewed at https://chromium-review.googlesource.com/#/c/177132/

Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)

rebased on Greg Kroah-Hartman's usb-next
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git

Signed-off-by: Vikas Sajjan <vikas.sajjan-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a7c04e2..d8432b0 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -993,6 +993,21 @@ int usb_remove_device(struct usb_device *udev)
 	return 0;
 }
 
+#define PORT_RESET_TRIES	5
+#define SET_ADDRESS_TRIES	2
+#define GET_DESCRIPTOR_TRIES	2
+#define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
+#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
+
+#define HUB_ROOT_RESET_TIME	50	/* times are in msec */
+#define HUB_SHORT_RESET_TIME	10
+#define HUB_BH_RESET_TIME	50
+#define HUB_LONG_RESET_TIME	200
+#define HUB_RESET_TIMEOUT	800
+
+static int hub_port_reset(struct usb_hub *hub, int port1,
+			struct usb_device *udev, unsigned int delay, bool warm);
+
 enum hub_activation_type {
 	HUB_INIT, HUB_INIT2, HUB_INIT3,		/* INITs must come first */
 	HUB_POST_RESET, HUB_RESUME, HUB_RESET_RESUME,
@@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 		u16 portstatus, portchange;
 
 		portstatus = portchange = 0;
+
+		/* Some connected devices might be still in unknown state even
+		 * after reset-resume, a WARM_RESET gets the connected device
+		 * to the normal state.
+		 */
+		if (udev && hub_is_superspeed(hub->hdev) &&
+						type == HUB_RESET_RESUME)
+			hub_port_reset(hub, port1, NULL,
+						HUB_BH_RESET_TIME, true);
+
 		status = hub_port_status(hub, port1, &portstatus, &portchange);
 		if (udev || (portstatus & USB_PORT_STAT_CONNECTION))
 			dev_dbg(hub->intfdev,
@@ -2510,22 +2535,6 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
 	return hcd->wireless;
 }
 
-
-#define PORT_RESET_TRIES	5
-#define SET_ADDRESS_TRIES	2
-#define GET_DESCRIPTOR_TRIES	2
-#define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
-#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
-
-#define HUB_ROOT_RESET_TIME	50	/* times are in msec */
-#define HUB_SHORT_RESET_TIME	10
-#define HUB_BH_RESET_TIME	50
-#define HUB_LONG_RESET_TIME	200
-#define HUB_RESET_TIMEOUT	800
-
-static int hub_port_reset(struct usb_hub *hub, int port1,
-			struct usb_device *udev, unsigned int delay, bool warm);

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Vivek Gautam]

Hi Vikas,


On Mon, Dec 9, 2013 at 5:59 PM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:

few minor nits here. ;-)

> Does warm reset while activating SuperSpeed HUBs if the hub activate type
> is HUB_RESET_RESUME.
>
> When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
> USB 3.0 device connected on 3.0 port, during resume I noticed that the
> XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
> This behaviour is inconsistent and the connection with connected USB 3.0 device
> on 3.0 port was LOST.
>
> Doing warm reset while activating SuperSpeed HUBs if the hub
> activate type is HUB_RESET_RESUME, gets the connected device to the stable state.
>
> Reviewed at https://chromium-review.googlesource.com/#/c/177132/
>
> Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)
>
> rebased on Greg Kroah-Hartman's usb-next
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git

Above two lines may not be required in the commit message.

>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
> ---
>  drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a7c04e2..d8432b0 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -993,6 +993,21 @@ int usb_remove_device(struct usb_device *udev)
>         return 0;
>  }
>
> +#define PORT_RESET_TRIES       5
> +#define SET_ADDRESS_TRIES      2
> +#define GET_DESCRIPTOR_TRIES   2
> +#define SET_CONFIG_TRIES       (2 * (use_both_schemes + 1))
> +#define USE_NEW_SCHEME(i)      ((i) / 2 == (int)old_scheme_first)
> +
> +#define HUB_ROOT_RESET_TIME    50      /* times are in msec */
> +#define HUB_SHORT_RESET_TIME   10
> +#define HUB_BH_RESET_TIME      50
> +#define HUB_LONG_RESET_TIME    200
> +#define HUB_RESET_TIMEOUT      800
> +
> +static int hub_port_reset(struct usb_hub *hub, int port1,
> +                       struct usb_device *udev, unsigned int delay, bool warm);
> +
>  enum hub_activation_type {
>         HUB_INIT, HUB_INIT2, HUB_INIT3,         /* INITs must come first */
>         HUB_POST_RESET, HUB_RESUME, HUB_RESET_RESUME,
> @@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>                 u16 portstatus, portchange;
>
>                 portstatus = portchange = 0;
> +
> +               /* Some connected devices might be still in unknown state even
Please take care of multiple line commenting style.

> +                * after reset-resume, a WARM_RESET gets the connected device
> +                * to the normal state.
> +                */
> +               if (udev && hub_is_superspeed(hub->hdev) &&
> +                                               type == HUB_RESET_RESUME)
> +                       hub_port_reset(hub, port1, NULL,
> +                                               HUB_BH_RESET_TIME, true);
> +
>                 status = hub_port_status(hub, port1, &portstatus, &portchange);
>                 if (udev || (portstatus & USB_PORT_STAT_CONNECTION))
>                         dev_dbg(hub->intfdev,
> @@ -2510,22 +2535,6 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>         return hcd->wireless;
>  }
>
> -
> -#define PORT_RESET_TRIES       5
> -#define SET_ADDRESS_TRIES      2
> -#define GET_DESCRIPTOR_TRIES   2
> -#define SET_CONFIG_TRIES       (2 * (use_both_schemes + 1))
> -#define USE_NEW_SCHEME(i)      ((i) / 2 == (int)old_scheme_first)
> -
> -#define HUB_ROOT_RESET_TIME    50      /* times are in msec */
> -#define HUB_SHORT_RESET_TIME   10
> -#define HUB_BH_RESET_TIME      50
> -#define HUB_LONG_RESET_TIME    200
> -#define HUB_RESET_TIMEOUT      800
> -
> -static int hub_port_reset(struct usb_hub *hub, int port1,
> -                       struct usb_device *udev, unsigned int delay, bool warm);
> -
>  /* Is a USB 3.0 port in the Inactive or Complinance Mode state?
>   * Port worm reset is required to recover
>   */
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Vivek Gautam]

Hi Vikas,


On Mon, Dec 9, 2013 at 5:59 PM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:

few minor nits here. ;-)

> Does warm reset while activating SuperSpeed HUBs if the hub activate type
> is HUB_RESET_RESUME.
>
> When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
> USB 3.0 device connected on 3.0 port, during resume I noticed that the
> XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
> This behaviour is inconsistent and the connection with connected USB 3.0 device
> on 3.0 port was LOST.
>
> Doing warm reset while activating SuperSpeed HUBs if the hub
> activate type is HUB_RESET_RESUME, gets the connected device to the stable state.
>
> Reviewed at https://chromium-review.googlesource.com/#/c/177132/
>
> Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)
>
> rebased on Greg Kroah-Hartman's usb-next
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git

Above two lines may not be required in the commit message.

>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
> ---
>  drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a7c04e2..d8432b0 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -993,6 +993,21 @@ int usb_remove_device(struct usb_device *udev)
>         return 0;
>  }
>
> +#define PORT_RESET_TRIES       5
> +#define SET_ADDRESS_TRIES      2
> +#define GET_DESCRIPTOR_TRIES   2
> +#define SET_CONFIG_TRIES       (2 * (use_both_schemes + 1))
> +#define USE_NEW_SCHEME(i)      ((i) / 2 == (int)old_scheme_first)
> +
> +#define HUB_ROOT_RESET_TIME    50      /* times are in msec */
> +#define HUB_SHORT_RESET_TIME   10
> +#define HUB_BH_RESET_TIME      50
> +#define HUB_LONG_RESET_TIME    200
> +#define HUB_RESET_TIMEOUT      800
> +
> +static int hub_port_reset(struct usb_hub *hub, int port1,
> +                       struct usb_device *udev, unsigned int delay, bool warm);
> +
>  enum hub_activation_type {
>         HUB_INIT, HUB_INIT2, HUB_INIT3,         /* INITs must come first */
>         HUB_POST_RESET, HUB_RESUME, HUB_RESET_RESUME,
> @@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>                 u16 portstatus, portchange;
>
>                 portstatus = portchange = 0;
> +
> +               /* Some connected devices might be still in unknown state even
Please take care of multiple line commenting style.

> +                * after reset-resume, a WARM_RESET gets the connected device
> +                * to the normal state.
> +                */
> +               if (udev && hub_is_superspeed(hub->hdev) &&
> +                                               type == HUB_RESET_RESUME)
> +                       hub_port_reset(hub, port1, NULL,
> +                                               HUB_BH_RESET_TIME, true);
> +
>                 status = hub_port_status(hub, port1, &portstatus, &portchange);
>                 if (udev || (portstatus & USB_PORT_STAT_CONNECTION))
>                         dev_dbg(hub->intfdev,
> @@ -2510,22 +2535,6 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>         return hcd->wireless;
>  }
>
> -
> -#define PORT_RESET_TRIES       5
> -#define SET_ADDRESS_TRIES      2
> -#define GET_DESCRIPTOR_TRIES   2
> -#define SET_CONFIG_TRIES       (2 * (use_both_schemes + 1))
> -#define USE_NEW_SCHEME(i)      ((i) / 2 == (int)old_scheme_first)
> -
> -#define HUB_ROOT_RESET_TIME    50      /* times are in msec */
> -#define HUB_SHORT_RESET_TIME   10
> -#define HUB_BH_RESET_TIME      50
> -#define HUB_LONG_RESET_TIME    200
> -#define HUB_RESET_TIMEOUT      800
> -
> -static int hub_port_reset(struct usb_hub *hub, int port1,
> -                       struct usb_device *udev, unsigned int delay, bool warm);
> -
>  /* Is a USB 3.0 port in the Inactive or Complinance Mode state?
>   * Port worm reset is required to recover
>   */
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Alan Stern]

On Mon, 9 Dec 2013, Vikas Sajjan wrote:

> Does warm reset while activating SuperSpeed HUBs if the hub activate type
> is HUB_RESET_RESUME.
> 
> When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
> USB 3.0 device connected on 3.0 port, during resume I noticed that the
> XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
> This behaviour is inconsistent and the connection with connected USB 3.0 device
> on 3.0 port was LOST.
> 
> Doing warm reset while activating SuperSpeed HUBs if the hub
> activate type is HUB_RESET_RESUME, gets the connected device to the stable state.
> 
> Reviewed at https://chromium-review.googlesource.com/#/c/177132/
> 
> Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)
> 
> rebased on Greg Kroah-Hartman's usb-next
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
> ---
>  drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a7c04e2..d8432b0 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

> @@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  		u16 portstatus, portchange;
>  
>  		portstatus = portchange = 0;
> +
> +		/* Some connected devices might be still in unknown state even
> +		 * after reset-resume, a WARM_RESET gets the connected device
> +		 * to the normal state.
> +		 */
> +		if (udev && hub_is_superspeed(hub->hdev) &&
> +						type == HUB_RESET_RESUME)
> +			hub_port_reset(hub, port1, NULL,
> +						HUB_BH_RESET_TIME, true);

Please don't do this all the time to every attached port.  Do it only 
when it is really needed.

Shouldn't you pass udev as the third argument?  If not, please explain
why not.

Finally, I don't see why you put this in hub_activate().  Isn't it more 
closely connected with the reset-resume procedure for the child device?

Alan Stern

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Alan Stern]

On Mon, 9 Dec 2013, Vikas Sajjan wrote:

> Does warm reset while activating SuperSpeed HUBs if the hub activate type
> is HUB_RESET_RESUME.
> 
> When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
> USB 3.0 device connected on 3.0 port, during resume I noticed that the
> XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
> This behaviour is inconsistent and the connection with connected USB 3.0 device
> on 3.0 port was LOST.
> 
> Doing warm reset while activating SuperSpeed HUBs if the hub
> activate type is HUB_RESET_RESUME, gets the connected device to the stable state.
> 
> Reviewed at https://chromium-review.googlesource.com/#/c/177132/
> 
> Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)
> 
> rebased on Greg Kroah-Hartman's usb-next
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
> ---
>  drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a7c04e2..d8432b0 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

> @@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  		u16 portstatus, portchange;
>  
>  		portstatus = portchange = 0;
> +
> +		/* Some connected devices might be still in unknown state even
> +		 * after reset-resume, a WARM_RESET gets the connected device
> +		 * to the normal state.
> +		 */
> +		if (udev && hub_is_superspeed(hub->hdev) &&
> +						type == HUB_RESET_RESUME)
> +			hub_port_reset(hub, port1, NULL,
> +						HUB_BH_RESET_TIME, true);

Please don't do this all the time to every attached port.  Do it only 
when it is really needed.

Shouldn't you pass udev as the third argument?  If not, please explain
why not.

Finally, I don't see why you put this in hub_activate().  Isn't it more 
closely connected with the reset-resume procedure for the child device?

Alan Stern

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Sarah Sharp]

On Mon, Dec 09, 2013 at 10:24:52AM -0500, Alan Stern wrote:
> On Mon, 9 Dec 2013, Vikas Sajjan wrote:
> 
> > Does warm reset while activating SuperSpeed HUBs if the hub activate type
> > is HUB_RESET_RESUME.
> > 
> > When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
> > USB 3.0 device connected on 3.0 port, during resume I noticed that the
> > XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
> > This behaviour is inconsistent and the connection with connected USB 3.0 device
> > on 3.0 port was LOST.

Does the device eventually re-connect on the USB port?  Or is warm reset
necessary to make the device connect?

Does the xHCI register restore complete after resume from S3, or is
power lost?  I'm trying to figure out whether xhci_reset is called
before your issue is triggered.

> > Doing warm reset while activating SuperSpeed HUBs if the hub
> > activate type is HUB_RESET_RESUME, gets the connected device to the stable state.
> > 
> > Reviewed at https://chromium-review.googlesource.com/#/c/177132/
> > 
> > Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)

Is this issue specific to the particular USB device manufacturer
(Transcend)?  Does the same device lose connection on resume from S3
with other host controller vendors?  Have you seen this issue when the
USB 3.0 device is behind a USB 3.0 hub?

I ask because this sounds like a low-level link training issue that's
specific to the exynos host or USB device.  I would rather track down
which hardware is to blame than generically add a warm reset for all USB
3.0 devices.

> > rebased on Greg Kroah-Hartman's usb-next
> > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> > 
> > Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
> > ---
> >  drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
> >  1 file changed, 25 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index a7c04e2..d8432b0 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> 
> > @@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
> >  		u16 portstatus, portchange;
> >  
> >  		portstatus = portchange = 0;
> > +
> > +		/* Some connected devices might be still in unknown state even
> > +		 * after reset-resume, a WARM_RESET gets the connected device
> > +		 * to the normal state.
> > +		 */
> > +		if (udev && hub_is_superspeed(hub->hdev) &&
> > +						type == HUB_RESET_RESUME)
> > +			hub_port_reset(hub, port1, NULL,
> > +						HUB_BH_RESET_TIME, true);
> 
> Please don't do this all the time to every attached port.  Do it only 
> when it is really needed.

Agreed.  Can we at least limit the warm reset to devices directly
attached to roothubs?  You can also change this code to get the port
status and only do the warm reset if the port link state is
USB_SS_PORT_LS_POLLING, USB_SS_PORT_LS_RX_DETECT, or
USB_SS_PORT_LS_SS_INACTIVE.

> Shouldn't you pass udev as the third argument?  If not, please explain
> why not.
> 
> Finally, I don't see why you put this in hub_activate().  Isn't it more 
> closely connected with the reset-resume procedure for the child device?

Sarah Sharp

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Vikas Sajjan]

Hi Sarah,

On Mon, Dec 9, 2013 at 11:54 PM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Mon, Dec 09, 2013 at 10:24:52AM -0500, Alan Stern wrote:
>> On Mon, 9 Dec 2013, Vikas Sajjan wrote:
>>
>> > Does warm reset while activating SuperSpeed HUBs if the hub activate type
>> > is HUB_RESET_RESUME.
>> >
>> > When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
>> > USB 3.0 device connected on 3.0 port, during resume I noticed that the
>> > XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
>> > This behaviour is inconsistent and the connection with connected USB 3.0 device
>> > on 3.0 port was LOST.
>
> Does the device eventually re-connect on the USB port?  Or is warm reset
> necessary to make the device connect?

Yes, warm reset was necesssary, without which the device was NOT reconnecting.

>
> Does the xHCI register restore complete after resume from S3, or is
> power lost?  I'm trying to figure out whether xhci_reset is called
> before your issue is triggered.


The reason why I came up with this solution is during xhci_resume(),
it enters below condition and marks the reset_resume flag for the
ROOT_HUB as 1

/* If restore operation fails, re-initialize the HC during resume */
        if ((temp & STS_SRE) || hibernated) {
                /* Let the USB core know _both_ roothubs lost power. */
                 usb_root_hub_lost_power(xhci->main_hcd->self.root_hub);
                 usb_root_hub_lost_power(xhci->shared_hcd->self.root_hub);


>
>> > Doing warm reset while activating SuperSpeed HUBs if the hub
>> > activate type is HUB_RESET_RESUME, gets the connected device to the stable state.
>> >
>> > Reviewed at https://chromium-review.googlesource.com/#/c/177132/
>> >
>> > Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)
>
> Is this issue specific to the particular USB device manufacturer
> (Transcend)?  Does the same device lose connection on resume from S3
> with other host controller vendors?  Have you seen this issue when the
> USB 3.0 device is behind a USB 3.0 hub?


This issue was specific to this paritcular make of Transcend.

we saw this on our chromebook. I did try Suspend-to-RAM with the same
device on Intel machine running Ubuntu.
It had worked fine without any issue.

Interestingly, if I connect with analyser, Suspend-to-RAM works fine
and USB re-enumerates successfully.
so connecting Suspend-to-RAM for debugging was not helping, as it works fine.
I did put prints in multiple places to get port status, and i see that
port is in sometimes RECOVERY, POLLING or In active STATE.
The behaviour was inconsistent.


>
> I ask because this sounds like a low-level link training issue that's
> specific to the exynos host or USB device.  I would rather track down
> which hardware is to blame than generically add a warm reset for all USB
> 3.0 devices.
>
>> > rebased on Greg Kroah-Hartman's usb-next
>> > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
>> >
>> > Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
>> > ---
>> >  drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
>> >  1 file changed, 25 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> > index a7c04e2..d8432b0 100644
>> > --- a/drivers/usb/core/hub.c
>> > +++ b/drivers/usb/core/hub.c
>>
>> > @@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>> >             u16 portstatus, portchange;
>> >
>> >             portstatus = portchange = 0;
>> > +
>> > +           /* Some connected devices might be still in unknown state even
>> > +            * after reset-resume, a WARM_RESET gets the connected device
>> > +            * to the normal state.
>> > +            */
>> > +           if (udev && hub_is_superspeed(hub->hdev) &&
>> > +                                           type == HUB_RESET_RESUME)
>> > +                   hub_port_reset(hub, port1, NULL,
>> > +                                           HUB_BH_RESET_TIME, true);
>>
>> Please don't do this all the time to every attached port.  Do it only
>> when it is really needed.
>
> Agreed.  Can we at least limit the warm reset to devices directly
> attached to roothubs?  You can also change this code to get the port
> status and only do the warm reset if the port link state is
> USB_SS_PORT_LS_POLLING, USB_SS_PORT_LS_RX_DETECT, or
> USB_SS_PORT_LS_SS_INACTIVE.
>
>> Shouldn't you pass udev as the third argument?  If not, please explain
>> why not.
>>
>> Finally, I don't see why you put this in hub_activate().  Isn't it more
>> closely connected with the reset-resume procedure for the child device?
>
> Sarah Sharp
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Vikas Sajjan]

Hi Alan,

On Mon, Dec 9, 2013 at 8:54 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 9 Dec 2013, Vikas Sajjan wrote:
>
>> Does warm reset while activating SuperSpeed HUBs if the hub activate type
>> is HUB_RESET_RESUME.
>>
>> When we do Suspend-to-RAM with (any one of the 16, 32, 64 Jetflash) transcend
>> USB 3.0 device connected on 3.0 port, during resume I noticed that the
>> XHCI controller has moved to sometimes RECOVERY, POLLING or INACTIVE STATE.
>> This behaviour is inconsistent and the connection with connected USB 3.0 device
>> on 3.0 port was LOST.
>>
>> Doing warm reset while activating SuperSpeed HUBs if the hub
>> activate type is HUB_RESET_RESUME, gets the connected device to the stable state.
>>
>> Reviewed at https://chromium-review.googlesource.com/#/c/177132/
>>
>> Tested on exynos5420 and exynos5250 with Transcend Jetflash USB 3.0 device (8564:1000)
>>
>> rebased on Greg Kroah-Hartman's usb-next
>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
>>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
>> ---
>>  drivers/usb/core/hub.c |   41 +++++++++++++++++++++++++----------------
>>  1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index a7c04e2..d8432b0 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>
>> @@ -1093,6 +1108,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>>               u16 portstatus, portchange;
>>
>>               portstatus = portchange = 0;
>> +
>> +             /* Some connected devices might be still in unknown state even
>> +              * after reset-resume, a WARM_RESET gets the connected device
>> +              * to the normal state.
>> +              */
>> +             if (udev && hub_is_superspeed(hub->hdev) &&
>> +                                             type == HUB_RESET_RESUME)
>> +                     hub_port_reset(hub, port1, NULL,
>> +                                             HUB_BH_RESET_TIME, true);
>
> Please don't do this all the time to every attached port.  Do it only
> when it is really needed.
>
> Shouldn't you pass udev as the third argument?  If not, please explain
> why not.

yea, I have NOT tried passing udev as the third argument.


>
> Finally, I don't see why you put this in hub_activate().  Isn't it more
> closely connected with the reset-resume procedure for the child device?


I was trying to add a FIX in usb_port_resume(), but in our case we
have CONFIG_USB_DEFAULT_PERSIST disabled.

Interestingly, if I disable the CONFIG_USB_DEFAULT_PERSIST, then the
function usb_port_resume() will never be called and transcend Jetflash
device Suspend-to-RAM fails.

In normal scenario (ie., CONFIG_USB_DEFAULT_PERSIST=y) the sequence is:
===========================================================================
Step 1: For Root HUB :
usb_resume_both() ---> usb_resume_device() -> generic_resume() -->
hcd_bus_resume() --> xhci_bus_resume().
                              |
                              |-->usb_resume_interface() --->
hub_reset_resume() -->  xhci_update_hub_device()

Step 2:  For the Device connected
usb_resume_both() ---> usb_resume_device() ->
generic_resume()-->usb_port_resume()-->hub_port_logical_disconnect()
--> hub_port_disable() --> hub_usb3_port_disable().


In our scenario (ie., CONFIG_USB_DEFAULT_PERSIST=N) the sequence is:
===============================================================================
Step 1: For Root HUB
usb_resume_both() ---> usb_resume_device() -> generic_resume() -->
hcd_bus_resume() --> xhci_bus_resume().
                              |
                              |-->usb_resume_interface() --->
hub_reset_resume() -->  xhci_update_hub_device()

Step 2 :  Never occurs

So Suspend-to-RAM fails.

Hence i added a FIX in  hub_reset_resume().

Let me know if I am wrong.

>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Alan Stern]

On Tue, 10 Dec 2013, Vikas Sajjan wrote:

> > Finally, I don't see why you put this in hub_activate().  Isn't it more
> > closely connected with the reset-resume procedure for the child device?
> 
> 
> I was trying to add a FIX in usb_port_resume(), but in our case we
> have CONFIG_USB_DEFAULT_PERSIST disabled.
> 
> Interestingly, if I disable the CONFIG_USB_DEFAULT_PERSIST, then the
> function usb_port_resume() will never be called and transcend Jetflash
> device Suspend-to-RAM fails.

I don't know what you mean by "fails".  The system goes to sleep and 
then later on wakes up, doesn't it?

Do you mean that the Jetflash device gets disconnected when the system
wakes up?  That's _supposed_ to happen under those circumstances.  
When hub_activate() sees HUB_RESET_RESUME, all child devices get
disconnected except those where udev->persist_enabled is set.

> In normal scenario (ie., CONFIG_USB_DEFAULT_PERSIST=y) the sequence is:
> ===========================================================================
> Step 1: For Root HUB :
> usb_resume_both() ---> usb_resume_device() -> generic_resume() -->
> hcd_bus_resume() --> xhci_bus_resume().
>                               |
>                               |-->usb_resume_interface() --->
> hub_reset_resume() -->  xhci_update_hub_device()
> 
> Step 2:  For the Device connected
> usb_resume_both() ---> usb_resume_device() ->
> generic_resume()-->usb_port_resume()-->hub_port_logical_disconnect()

You lost me there.  Why does usb_port_resume call 
hub_port_logical_disconnect?  Does this happen because 
check_port_resume_type returns an error code?  What are the values of 
the portchange and portstatus arguments to check_port_resume_type?

> --> hub_port_disable() --> hub_usb3_port_disable().
> 
> 
> In our scenario (ie., CONFIG_USB_DEFAULT_PERSIST=N) the sequence is:
> ===============================================================================
> Step 1: For Root HUB
> usb_resume_both() ---> usb_resume_device() -> generic_resume() -->
> hcd_bus_resume() --> xhci_bus_resume().
>                               |
>                               |-->usb_resume_interface() --->
> hub_reset_resume() -->  xhci_update_hub_device()
> 
> Step 2 :  Never occurs

That's exactly right.

> So Suspend-to-RAM fails.

No, it succeeds in behaving the way it is intended to behave.

> Hence i added a FIX in  hub_reset_resume().
> 
> Let me know if I am wrong.

I can't tell at this point.  It depends on the reason why 
hub_port_logical_disconnect got called.

Alan Stern

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Alan Stern]

On Tue, 10 Dec 2013, Vikas Sajjan wrote:

> > Finally, I don't see why you put this in hub_activate().  Isn't it more
> > closely connected with the reset-resume procedure for the child device?
> 
> 
> I was trying to add a FIX in usb_port_resume(), but in our case we
> have CONFIG_USB_DEFAULT_PERSIST disabled.
> 
> Interestingly, if I disable the CONFIG_USB_DEFAULT_PERSIST, then the
> function usb_port_resume() will never be called and transcend Jetflash
> device Suspend-to-RAM fails.

I don't know what you mean by "fails".  The system goes to sleep and 
then later on wakes up, doesn't it?

Do you mean that the Jetflash device gets disconnected when the system
wakes up?  That's _supposed_ to happen under those circumstances.  
When hub_activate() sees HUB_RESET_RESUME, all child devices get
disconnected except those where udev->persist_enabled is set.

> In normal scenario (ie., CONFIG_USB_DEFAULT_PERSIST=y) the sequence is:
> ===========================================================================
> Step 1: For Root HUB :
> usb_resume_both() ---> usb_resume_device() -> generic_resume() -->
> hcd_bus_resume() --> xhci_bus_resume().
>                               |
>                               |-->usb_resume_interface() --->
> hub_reset_resume() -->  xhci_update_hub_device()
> 
> Step 2:  For the Device connected
> usb_resume_both() ---> usb_resume_device() ->
> generic_resume()-->usb_port_resume()-->hub_port_logical_disconnect()

You lost me there.  Why does usb_port_resume call 
hub_port_logical_disconnect?  Does this happen because 
check_port_resume_type returns an error code?  What are the values of 
the portchange and portstatus arguments to check_port_resume_type?

> --> hub_port_disable() --> hub_usb3_port_disable().
> 
> 
> In our scenario (ie., CONFIG_USB_DEFAULT_PERSIST=N) the sequence is:
> ===============================================================================
> Step 1: For Root HUB
> usb_resume_both() ---> usb_resume_device() -> generic_resume() -->
> hcd_bus_resume() --> xhci_bus_resume().
>                               |
>                               |-->usb_resume_interface() --->
> hub_reset_resume() -->  xhci_update_hub_device()
> 
> Step 2 :  Never occurs

That's exactly right.

> So Suspend-to-RAM fails.

No, it succeeds in behaving the way it is intended to behave.

> Hence i added a FIX in  hub_reset_resume().
> 
> Let me know if I am wrong.

I can't tell at this point.  It depends on the reason why 
hub_port_logical_disconnect got called.

Alan Stern

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Julius Werner]

> I don't know what you mean by "fails".  The system goes to sleep and
> then later on wakes up, doesn't it?
>
> Do you mean that the Jetflash device gets disconnected when the system
> wakes up?  That's _supposed_ to happen under those circumstances.
> When hub_activate() sees HUB_RESET_RESUME, all child devices get
> disconnected except those where udev->persist_enabled is set.

This patch was written in response to the same bug as my "usb: hub:
Use correct reset for wedged USB3 devices that are NOTATTACHED"
submission. My patch only helps when the port gets stuck in Compliance
Mode, but Vikas reports that he can sometimes see it stuck in Polling
or Recovery states as well.

The underlying issue is a deadlock in the USB 3.0 link training state
machine when the host controller is unilaterally reset on resume
(without driving a reset on the bus). The host port starts out back in
Rx.Detect without remembering anything about its previous state, but
the device is still in U3. The host detects Rx terminations, moves to
Polling and starts sending LFPS link training packets, but the device
doesn't expect those and interprets them as link problems (moving to
Recovery). What happens next seems to be device specific, but
apparently the device can end up in SS.Inactive while the host port
gets stuck in Polling or Recovery (or some kind of livelock between
those).

This patch tries to warm reset all USB 3.0 ports on reset-resume
(after xhci_reset() was called) that had devices connected to them
before suspend. This seems to be the only way to ensure the devices'
state machines get back to a well-defined state that the host can work
with. I don't think this is a specific hardware bug, it's just an
unfortunate design flaw that the USB 3.0 spec doesn't account for a
root hub port being reset independently of its connected device. I
think Sarah is correct that it could be limited to root hubs, though.

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Sarah Sharp]

On Wed, Dec 11, 2013 at 11:00:13AM -0800, Julius Werner wrote:
> > I don't know what you mean by "fails".  The system goes to sleep and
> > then later on wakes up, doesn't it?
> >
> > Do you mean that the Jetflash device gets disconnected when the system
> > wakes up?  That's _supposed_ to happen under those circumstances.
> > When hub_activate() sees HUB_RESET_RESUME, all child devices get
> > disconnected except those where udev->persist_enabled is set.
> 
> This patch was written in response to the same bug as my "usb: hub:
> Use correct reset for wedged USB3 devices that are NOTATTACHED"
> submission. My patch only helps when the port gets stuck in Compliance
> Mode, but Vikas reports that he can sometimes see it stuck in Polling
> or Recovery states as well.
> 
> The underlying issue is a deadlock in the USB 3.0 link training state
> machine when the host controller is unilaterally reset on resume
> (without driving a reset on the bus).

The xHCI spec requires that when the xHCI host is reset, a USB reset is
driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
to warm reset.  See table 32 in the xHCI spec, in the definition of
HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
ports at all on host controller reset?

> The host port starts out back in
> Rx.Detect without remembering anything about its previous state, but
> the device is still in U3. The host detects Rx terminations, moves to
> Polling and starts sending LFPS link training packets, but the device
> doesn't expect those and interprets them as link problems (moving to
> Recovery). What happens next seems to be device specific, but
> apparently the device can end up in SS.Inactive while the host port
> gets stuck in Polling or Recovery (or some kind of livelock between
> those).
> 
> This patch tries to warm reset all USB 3.0 ports on reset-resume
> (after xhci_reset() was called) that had devices connected to them
> before suspend. This seems to be the only way to ensure the devices'
> state machines get back to a well-defined state that the host can work
> with. I don't think this is a specific hardware bug, it's just an
> unfortunate design flaw that the USB 3.0 spec doesn't account for a
> root hub port being reset independently of its connected device. I
> think Sarah is correct that it could be limited to root hubs, though.

Sarah Sharp

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Sarah Sharp]

On Wed, Dec 11, 2013 at 11:00:13AM -0800, Julius Werner wrote:
> > I don't know what you mean by "fails".  The system goes to sleep and
> > then later on wakes up, doesn't it?
> >
> > Do you mean that the Jetflash device gets disconnected when the system
> > wakes up?  That's _supposed_ to happen under those circumstances.
> > When hub_activate() sees HUB_RESET_RESUME, all child devices get
> > disconnected except those where udev->persist_enabled is set.
> 
> This patch was written in response to the same bug as my "usb: hub:
> Use correct reset for wedged USB3 devices that are NOTATTACHED"
> submission. My patch only helps when the port gets stuck in Compliance
> Mode, but Vikas reports that he can sometimes see it stuck in Polling
> or Recovery states as well.
> 
> The underlying issue is a deadlock in the USB 3.0 link training state
> machine when the host controller is unilaterally reset on resume
> (without driving a reset on the bus).

The xHCI spec requires that when the xHCI host is reset, a USB reset is
driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
to warm reset.  See table 32 in the xHCI spec, in the definition of
HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
ports at all on host controller reset?

> The host port starts out back in
> Rx.Detect without remembering anything about its previous state, but
> the device is still in U3. The host detects Rx terminations, moves to
> Polling and starts sending LFPS link training packets, but the device
> doesn't expect those and interprets them as link problems (moving to
> Recovery). What happens next seems to be device specific, but
> apparently the device can end up in SS.Inactive while the host port
> gets stuck in Polling or Recovery (or some kind of livelock between
> those).
> 
> This patch tries to warm reset all USB 3.0 ports on reset-resume
> (after xhci_reset() was called) that had devices connected to them
> before suspend. This seems to be the only way to ensure the devices'
> state machines get back to a well-defined state that the host can work
> with. I don't think this is a specific hardware bug, it's just an
> unfortunate design flaw that the USB 3.0 spec doesn't account for a
> root hub port being reset independently of its connected device. I
> think Sarah is correct that it could be limited to root hubs, though.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Dan Williams]

[-- Attachment #1: Type: text/plain, Size: 9158 bytes --]

On Wed, Dec 11, 2013 at 11:36 AM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Wed, Dec 11, 2013 at 11:00:13AM -0800, Julius Werner wrote:
>> > I don't know what you mean by "fails".  The system goes to sleep and
>> > then later on wakes up, doesn't it?
>> >
>> > Do you mean that the Jetflash device gets disconnected when the system
>> > wakes up?  That's _supposed_ to happen under those circumstances.
>> > When hub_activate() sees HUB_RESET_RESUME, all child devices get
>> > disconnected except those where udev->persist_enabled is set.
>>
>> This patch was written in response to the same bug as my "usb: hub:
>> Use correct reset for wedged USB3 devices that are NOTATTACHED"
>> submission. My patch only helps when the port gets stuck in Compliance
>> Mode, but Vikas reports that he can sometimes see it stuck in Polling
>> or Recovery states as well.
>>
>> The underlying issue is a deadlock in the USB 3.0 link training state
>> machine when the host controller is unilaterally reset on resume
>> (without driving a reset on the bus).
>
> The xHCI spec requires that when the xHCI host is reset, a USB reset is
> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
> to warm reset.  See table 32 in the xHCI spec, in the definition of
> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
> ports at all on host controller reset?

...although, the spec says that it does not wait for the port resets
to complete.  As far as I can see re-issuing a warm reset and waiting
is the only way to guarantee the core times the recovery.  Presumably
the portstatus debounce in hub_activate() mitigates this, but that
100ms is less than a full reset timeout.  I have something similar in
the port power rework patches [1], but I think something like the
following (untested) is more generic, it arranges for reset_resume to
start with a warm reset if necessary.

(also attached)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a7c04e24ca48..30ce237569dd 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2783,8 +2783,14 @@ static int check_port_resume_type(struct
usb_device *udev,
                struct usb_hub *hub, int port1,
                int status, unsigned portchange, unsigned portstatus)
 {
+       /* Did the port go SS.Inactive?  Even if ->persist_enabled is
cleared the
+        * device won't come back until a warm reset completes
+        */
+       if (hub_port_warm_reset_required(hub, portstatus)) {
+               udev->reset_resume = 1;
+               udev->reset_resume_warm = 1;
        /* Is the device still present? */
-       if (status || port_is_suspended(hub, portstatus) ||
+       } else if (status || port_is_suspended(hub, portstatus) ||
                        !port_is_power_on(hub, portstatus) ||
                        !(portstatus & USB_PORT_STAT_CONNECTION)) {
                if (status >= 0)
@@ -4022,7 +4028,8 @@ hub_port_init (struct usb_hub *hub, struct
usb_device *udev, int port1,

        /* Reset the device; full speed may morph to high speed */
        /* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
-       retval = hub_port_reset(hub, port1, udev, delay, false);
+       retval = hub_port_reset(hub, port1, udev, delay,
+                               udev->reset_resume_warm);
        if (retval < 0)         /* error or disconnect */
                goto fail;
        /* success, speed is known */
@@ -4730,7 +4737,8 @@ static void hub_events(void)

                /* deal with port status changes */
                for (i = 1; i <= hdev->maxchild; i++) {
-                       if (test_bit(i, hub->busy_bits))
+                       if (test_bit(i, hub->busy_bits) ||
+                           test_bit(i, hub->delayed_change_bits))
                                continue;
                        connect_change = test_bit(i, hub->change_bits);
                        wakeup_change = test_and_clear_bit(i, hub->wakeup_bits);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7454865ad148..ff1b6fe4a0ff 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -572,6 +572,7 @@ struct usb_device {

        unsigned do_remote_wakeup:1;
        unsigned reset_resume:1;
+       unsigned reset_resume_warm:1;
        unsigned port_is_suspended:1;
 #endif
        struct wusb_dev *wusb_dev;

>
>> The host port starts out back in
>> Rx.Detect without remembering anything about its previous state, but
>> the device is still in U3. The host detects Rx terminations, moves to
>> Polling and starts sending LFPS link training packets, but the device
>> doesn't expect those and interprets them as link problems (moving to
>> Recovery). What happens next seems to be device specific, but
>> apparently the device can end up in SS.Inactive while the host port
>> gets stuck in Polling or Recovery (or some kind of livelock between
>> those).

In testing the port power patches I see this particular device give up
on its superspeed connection if it sees too many link failures and
fallsback to connecting via its high speed connection.

>> This patch tries to warm reset all USB 3.0 ports on reset-resume
>> (after xhci_reset() was called) that had devices connected to them
>> before suspend. This seems to be the only way to ensure the devices'
>> state machines get back to a well-defined state that the host can work
>> with. I don't think this is a specific hardware bug, it's just an
>> unfortunate design flaw that the USB 3.0 spec doesn't account for a
>> root hub port being reset independently of its connected device. I
>> think Sarah is correct that it could be limited to root hubs, though.
>

I think we also need something like the following to hold off khubd
while further resume actions may be taking place.  Again we're
mitigated by the debounce delay in most cases, but if a warm reset
recovery is going to take longer than that khubd needs to be held off
for the given port.

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 30ce237569dd..a99e3eb28aa5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1173,22 +1173,25 @@ static void hub_activate(struct usb_hub *hub,
enum hub_activation_type type)
                                                port_resumed))
                                set_bit(port1, hub->change_bits);

-               } else if (udev->persist_enabled) {
+               } else {
                        struct usb_port *port_dev = hub->ports[port1 - 1];

+                       if (udev->persist_enabled) {
 #ifdef CONFIG_PM
-                       udev->reset_resume = 1;
+                               udev->reset_resume = 1;
 #endif
-                       /* Don't set the change_bits when the device
-                        * was powered off.
-                        */
-                       if (port_dev->power_is_on)
-                               set_bit(port1, hub->change_bits);
+                               /* Don't set the change_bits when the device
+                                * was powered off.
+                                */
+                               if (port_dev->power_is_on)
+                                       set_bit(port1, hub->change_bits);
+                       }

-               } else {
-                       /* The power session is gone; tell khubd */
-                       usb_set_device_state(udev, USB_STATE_NOTATTACHED);
-                       set_bit(port1, hub->change_bits);
+                       /* The power session may be gone; wait for port
+                        * recovery before letting khubd take action on
+                        * this port
+                        */
+                       set_bit(port1, hub->delayed_change_bits);
                }
        }

@@ -3285,6 +3288,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
                usb_unlocked_enable_lpm(udev);
        }

+       if (test_and_clear_bit(port1, hub->delayed_change_bits)) {
+               set_bit(port1, hub->change_bits);
+               kick_khubd(hub);
+       }
+
        return status;
 }

diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 4e4790dea343..74e87c0380f8 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -45,8 +45,9 @@ struct usb_hub {
        unsigned long           event_bits[1];  /* status change bitmask */
        unsigned long           change_bits[1]; /* ports with logical connect
                                                        status change */
-       unsigned long           busy_bits[1];   /* ports being reset or
-                                                       resumed */
+       unsigned long           busy_bits[1];   /* ports being reset */
+       unsigned long           delayed_change_bits[1]; /* ports awaiting
+                                                          recovery */
        unsigned long           removed_bits[1]; /* ports with a "removed"
                                                        device present */
        unsigned long           wakeup_bits[1]; /* ports that have signaled

[-- Attachment #2: warm-reset-resume --]
[-- Type: application/octet-stream, Size: 2493 bytes --]

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 8d989b1d3dc5..327b927afb52 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1333,8 +1333,10 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
 
  done:
 	dev_vdbg(&udev->dev, "%s: status %d\n", __func__, status);
-	if (!status)
+	if (!status) {
 		udev->reset_resume = 0;
+		udev->reset_resume_warm = 0;
+	}
 	return status;
 }
 
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a7c04e24ca48..30ce237569dd 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2783,8 +2783,14 @@ static int check_port_resume_type(struct usb_device *udev,
 		struct usb_hub *hub, int port1,
 		int status, unsigned portchange, unsigned portstatus)
 {
+	/* Did the port go SS.Inactive?  Even if ->persist_enabled is cleared the
+	 * device won't come back until a warm reset completes
+	 */
+	if (hub_port_warm_reset_required(hub, portstatus)) {
+		udev->reset_resume = 1;
+		udev->reset_resume_warm = 1;
 	/* Is the device still present? */
-	if (status || port_is_suspended(hub, portstatus) ||
+	} else if (status || port_is_suspended(hub, portstatus) ||
 			!port_is_power_on(hub, portstatus) ||
 			!(portstatus & USB_PORT_STAT_CONNECTION)) {
 		if (status >= 0)
@@ -4022,7 +4028,8 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
 
 	/* Reset the device; full speed may morph to high speed */
 	/* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
-	retval = hub_port_reset(hub, port1, udev, delay, false);
+	retval = hub_port_reset(hub, port1, udev, delay,
+				udev->reset_resume_warm);
 	if (retval < 0)		/* error or disconnect */
 		goto fail;
 	/* success, speed is known */
@@ -4730,7 +4737,8 @@ static void hub_events(void)
 
 		/* deal with port status changes */
 		for (i = 1; i <= hdev->maxchild; i++) {
-			if (test_bit(i, hub->busy_bits))
+			if (test_bit(i, hub->busy_bits) ||
+			    test_bit(i, hub->delayed_change_bits))
 				continue;
 			connect_change = test_bit(i, hub->change_bits);
 			wakeup_change = test_and_clear_bit(i, hub->wakeup_bits);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7454865ad148..ff1b6fe4a0ff 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -572,6 +572,7 @@ struct usb_device {
 
 	unsigned do_remote_wakeup:1;
 	unsigned reset_resume:1;
+	unsigned reset_resume_warm:1;
 	unsigned port_is_suspended:1;
 #endif
 	struct wusb_dev *wusb_dev;

[-- Attachment #3: delay-khubd --]
[-- Type: application/octet-stream, Size: 2173 bytes --]

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 30ce237569dd..a99e3eb28aa5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1173,22 +1173,25 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 						port_resumed))
 				set_bit(port1, hub->change_bits);
 
-		} else if (udev->persist_enabled) {
+		} else {
 			struct usb_port *port_dev = hub->ports[port1 - 1];
 
+			if (udev->persist_enabled) {
 #ifdef CONFIG_PM
-			udev->reset_resume = 1;
+				udev->reset_resume = 1;
 #endif
-			/* Don't set the change_bits when the device
-			 * was powered off.
-			 */
-			if (port_dev->power_is_on)
-				set_bit(port1, hub->change_bits);
+				/* Don't set the change_bits when the device
+				 * was powered off.
+				 */
+				if (port_dev->power_is_on)
+					set_bit(port1, hub->change_bits);
+			}
 
-		} else {
-			/* The power session is gone; tell khubd */
-			usb_set_device_state(udev, USB_STATE_NOTATTACHED);
-			set_bit(port1, hub->change_bits);
+			/* The power session may be gone; wait for port
+			 * recovery before letting khubd take action on
+			 * this port
+			 */
+			set_bit(port1, hub->delayed_change_bits);
 		}
 	}
 
@@ -3285,6 +3288,11 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 		usb_unlocked_enable_lpm(udev);
 	}
 
+	if (test_and_clear_bit(port1, hub->delayed_change_bits)) {
+		set_bit(port1, hub->change_bits);
+		kick_khubd(hub);
+	}
+
 	return status;
 }
 
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 4e4790dea343..74e87c0380f8 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -45,8 +45,9 @@ struct usb_hub {
 	unsigned long		event_bits[1];	/* status change bitmask */
 	unsigned long		change_bits[1];	/* ports with logical connect
 							status change */
-	unsigned long		busy_bits[1];	/* ports being reset or
-							resumed */
+	unsigned long		busy_bits[1];	/* ports being reset */
+	unsigned long		delayed_change_bits[1]; /* ports awaiting
+							   recovery */
 	unsigned long		removed_bits[1]; /* ports with a "removed"
 							device present */
 	unsigned long		wakeup_bits[1];	/* ports that have signaled

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Dan Williams]

On Wed, Dec 11, 2013 at 2:51 PM, Dan Williams <dan.j.williams@gmail.com> wrote:
> On Wed, Dec 11, 2013 at 11:36 AM, Sarah Sharp
> <sarah.a.sharp@linux.intel.com> wrote:
>> On Wed, Dec 11, 2013 at 11:00:13AM -0800, Julius Werner wrote:
>>> > I don't know what you mean by "fails".  The system goes to sleep and
>>> > then later on wakes up, doesn't it?
>>> >
>>> > Do you mean that the Jetflash device gets disconnected when the system
>>> > wakes up?  That's _supposed_ to happen under those circumstances.
>>> > When hub_activate() sees HUB_RESET_RESUME, all child devices get
>>> > disconnected except those where udev->persist_enabled is set.
>>>
>>> This patch was written in response to the same bug as my "usb: hub:
>>> Use correct reset for wedged USB3 devices that are NOTATTACHED"
>>> submission. My patch only helps when the port gets stuck in Compliance
>>> Mode, but Vikas reports that he can sometimes see it stuck in Polling
>>> or Recovery states as well.
>>>
>>> The underlying issue is a deadlock in the USB 3.0 link training state
>>> machine when the host controller is unilaterally reset on resume
>>> (without driving a reset on the bus).
>>
>> The xHCI spec requires that when the xHCI host is reset, a USB reset is
>> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
>> to warm reset.  See table 32 in the xHCI spec, in the definition of
>> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
>> ports at all on host controller reset?
>
> ...although, the spec says that it does not wait for the port resets
> to complete.  As far as I can see re-issuing a warm reset and waiting
> is the only way to guarantee the core times the recovery.  Presumably
> the portstatus debounce in hub_activate() mitigates this, but that
> 100ms is less than a full reset timeout.  I have something similar in
> the port power rework patches [1], but I think something like the
> following (untested) is more generic, it arranges for reset_resume to
> start with a warm reset if necessary.
>
> (also attached)
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a7c04e24ca48..30ce237569dd 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2783,8 +2783,14 @@ static int check_port_resume_type(struct
> usb_device *udev,
>                 struct usb_hub *hub, int port1,
>                 int status, unsigned portchange, unsigned portstatus)
>  {
> +       /* Did the port go SS.Inactive?  Even if ->persist_enabled is
> cleared the
> +        * device won't come back until a warm reset completes
> +        */
> +       if (hub_port_warm_reset_required(hub, portstatus)) {
> +               udev->reset_resume = 1;
> +               udev->reset_resume_warm = 1;

Also need to set 'status' to 0 here.

If it's truly just a case of waiting for port warm resets to complete
it might be better to inject additional debounce delay here, but the
spec seems to indicate that there is no way to know that escalated
warm resets are in progress.  4.19.5.1 says "The Port Reset (PR) flag
shall be ‘1’ while Hot or Warm Reset is being executed. The Port Reset
Change (PRC) flag shall be set (‘1’) when the reset execution is
complete and PR transitions to ‘0’."... but that is only if software
initiated the warm reset.  When the warm reset was the result of an
HCRST we hit "Note, the completion of the xHC reset process is not
gated by the Root Hub port reset process."

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Julius Werner]

>> ...although, the spec says that it does not wait for the port resets
>> to complete.  As far as I can see re-issuing a warm reset and waiting
>> is the only way to guarantee the core times the recovery.  Presumably
>> the portstatus debounce in hub_activate() mitigates this, but that
>> 100ms is less than a full reset timeout.

It's definitely not just a timing issue for us. I can't reproduce all
the same cases as Vikas, but when I attach a USB analyzer to the ones
I do see the host controller doesn't even start sending a reset.

>>> The xHCI spec requires that when the xHCI host is reset, a USB reset is
>>> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
>>> to warm reset.  See table 32 in the xHCI spec, in the definition of
>>> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
>>> ports at all on host controller reset?

Oh, interesting, I hadn't seen that yet. So I guess the spec itself is
fine if it were followed to the letter.

I did some more tests about this on my Exynos machine: when I put a
device to autosuspend (U3) and manually poke the xHC reset bit, I do
see an automatic warm reset on the analyzer and the ports manage to
retrain to U0. But after a system suspend/resume which calls
xhci_reset() in the process, there is no reset on the wire. I also
noticed that it doesn't drive a reset (even after manual poking) when
there is no device connected on the other end of the analyzer.

So this might be our problem: maybe these host controllers (Synopsys
DesignWare) issue the spec-mandated warm reset only on ports where
they think there is a device attached. But after a system
suspend/resume (where the whole IP block on the SoC was powered down),
the host controller cannot know that there is still a device with an
active power session attached, and therefore doesn't drive the reset
on its own.

Even though this is a host controller bug, we still have to deal with
it somehow. I guess we could move the code into xhci_plat_resume() and
hide it behind a quirk to lessen the impact. But since reset_resume is
not a common case for most host controllers, it's hard to say if this
is DesignWare specific or a more widespread implementation mistake.

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Alan Stern]

On Wed, 11 Dec 2013, Julius Werner wrote:

> >> ...although, the spec says that it does not wait for the port resets
> >> to complete.  As far as I can see re-issuing a warm reset and waiting
> >> is the only way to guarantee the core times the recovery.  Presumably
> >> the portstatus debounce in hub_activate() mitigates this, but that
> >> 100ms is less than a full reset timeout.
> 
> It's definitely not just a timing issue for us. I can't reproduce all
> the same cases as Vikas, but when I attach a USB analyzer to the ones
> I do see the host controller doesn't even start sending a reset.
> 
> >>> The xHCI spec requires that when the xHCI host is reset, a USB reset is
> >>> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
> >>> to warm reset.  See table 32 in the xHCI spec, in the definition of
> >>> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
> >>> ports at all on host controller reset?
> 
> Oh, interesting, I hadn't seen that yet. So I guess the spec itself is
> fine if it were followed to the letter.
> 
> I did some more tests about this on my Exynos machine: when I put a
> device to autosuspend (U3) and manually poke the xHC reset bit, I do
> see an automatic warm reset on the analyzer and the ports manage to
> retrain to U0. But after a system suspend/resume which calls
> xhci_reset() in the process, there is no reset on the wire. I also
> noticed that it doesn't drive a reset (even after manual poking) when
> there is no device connected on the other end of the analyzer.
> 
> So this might be our problem: maybe these host controllers (Synopsys
> DesignWare) issue the spec-mandated warm reset only on ports where
> they think there is a device attached. But after a system
> suspend/resume (where the whole IP block on the SoC was powered down),
> the host controller cannot know that there is still a device with an
> active power session attached, and therefore doesn't drive the reset
> on its own.
> 
> Even though this is a host controller bug, we still have to deal with
> it somehow. I guess we could move the code into xhci_plat_resume() and
> hide it behind a quirk to lessen the impact. But since reset_resume is
> not a common case for most host controllers, it's hard to say if this
> is DesignWare specific or a more widespread implementation mistake.

I was going to suggest something along these lines too.  This seems to 
be a bug in xHCI.  Therefore the fix belongs in xhci-hcd, not in the 
hub driver.

Alan Stern

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Sarah Sharp]

On Thu, Dec 12, 2013 at 11:05:04AM -0500, Alan Stern wrote:
> On Wed, 11 Dec 2013, Julius Werner wrote:
> 
> > >> ...although, the spec says that it does not wait for the port resets
> > >> to complete.  As far as I can see re-issuing a warm reset and waiting
> > >> is the only way to guarantee the core times the recovery.  Presumably
> > >> the portstatus debounce in hub_activate() mitigates this, but that
> > >> 100ms is less than a full reset timeout.
> > 
> > It's definitely not just a timing issue for us. I can't reproduce all
> > the same cases as Vikas, but when I attach a USB analyzer to the ones
> > I do see the host controller doesn't even start sending a reset.
> > 
> > >>> The xHCI spec requires that when the xHCI host is reset, a USB reset is
> > >>> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
> > >>> to warm reset.  See table 32 in the xHCI spec, in the definition of
> > >>> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
> > >>> ports at all on host controller reset?
> > 
> > Oh, interesting, I hadn't seen that yet. So I guess the spec itself is
> > fine if it were followed to the letter.
> > 
> > I did some more tests about this on my Exynos machine: when I put a
> > device to autosuspend (U3) and manually poke the xHC reset bit, I do
> > see an automatic warm reset on the analyzer and the ports manage to
> > retrain to U0. But after a system suspend/resume which calls
> > xhci_reset() in the process, there is no reset on the wire. I also
> > noticed that it doesn't drive a reset (even after manual poking) when
> > there is no device connected on the other end of the analyzer.
> > 
> > So this might be our problem: maybe these host controllers (Synopsys
> > DesignWare) issue the spec-mandated warm reset only on ports where
> > they think there is a device attached. But after a system
> > suspend/resume (where the whole IP block on the SoC was powered down),
> > the host controller cannot know that there is still a device with an
> > active power session attached, and therefore doesn't drive the reset
> > on its own.

Ok, that makes some sense.  I could see why host controllers wouldn't
want to drive reset on an unconnected port.

> > Even though this is a host controller bug, we still have to deal with
> > it somehow. I guess we could move the code into xhci_plat_resume() and
> > hide it behind a quirk to lessen the impact. But since reset_resume is
> > not a common case for most host controllers, it's hard to say if this
> > is DesignWare specific or a more widespread implementation mistake.
> 
> I was going to suggest something along these lines too.  This seems to 
> be a bug in xHCI.  Therefore the fix belongs in xhci-hcd, not in the 
> hub driver.

I agree.  Is there a chance that the Synopsys DesignWare will be a PCI
device instead of a platform device?  If so, it would be better to put
the code into xhci_resume instead of xhci_plat_resume.  That also allows
you to only issue the warm reset when the register restore state command
fails, after the xhci_reset call.

Also, I assume that other systems with the Synopsys DesignWare IP will
experience this issue?  I know of at least two other chipsets that will
include that IP, and it would be good to find a way to trigger on the
Synopsys IP, rather than off xHCI PCI vendor and device ID.  Otherwise
we'll be adding PCI IDs to the xHCI driver quirks for many many kernels
to come.

I'm actually leaning towards enabling the check for warm reset broadly.
It seems like it wouldn't hurt to issue a warm reset on the USB 3.0
ports if they're in compliance, poll, or rx.detect.  So, let's enable
this broadly in xhci_resume, mark the patch for stable, but ask for the
backport to be delayed until 3.13.3 is out, to allow for more testing.
If anyone complains of xHCI behavior changes, we'll change the code to
add a quirk.

Sarah Sharp

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Dan Williams]

On Fri, Dec 13, 2013 at 9:48 AM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Thu, Dec 12, 2013 at 11:05:04AM -0500, Alan Stern wrote:
>> On Wed, 11 Dec 2013, Julius Werner wrote:
>>
>> > >> ...although, the spec says that it does not wait for the port resets
>> > >> to complete.  As far as I can see re-issuing a warm reset and waiting
>> > >> is the only way to guarantee the core times the recovery.  Presumably
>> > >> the portstatus debounce in hub_activate() mitigates this, but that
>> > >> 100ms is less than a full reset timeout.
>> >
>> > It's definitely not just a timing issue for us. I can't reproduce all
>> > the same cases as Vikas, but when I attach a USB analyzer to the ones
>> > I do see the host controller doesn't even start sending a reset.
>> >
>> > >>> The xHCI spec requires that when the xHCI host is reset, a USB reset is
>> > >>> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
>> > >>> to warm reset.  See table 32 in the xHCI spec, in the definition of
>> > >>> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
>> > >>> ports at all on host controller reset?
>> >
>> > Oh, interesting, I hadn't seen that yet. So I guess the spec itself is
>> > fine if it were followed to the letter.
>> >
>> > I did some more tests about this on my Exynos machine: when I put a
>> > device to autosuspend (U3) and manually poke the xHC reset bit, I do
>> > see an automatic warm reset on the analyzer and the ports manage to
>> > retrain to U0. But after a system suspend/resume which calls
>> > xhci_reset() in the process, there is no reset on the wire. I also
>> > noticed that it doesn't drive a reset (even after manual poking) when
>> > there is no device connected on the other end of the analyzer.
>> >
>> > So this might be our problem: maybe these host controllers (Synopsys
>> > DesignWare) issue the spec-mandated warm reset only on ports where
>> > they think there is a device attached. But after a system
>> > suspend/resume (where the whole IP block on the SoC was powered down),
>> > the host controller cannot know that there is still a device with an
>> > active power session attached, and therefore doesn't drive the reset
>> > on its own.
>
> Ok, that makes some sense.  I could see why host controllers wouldn't
> want to drive reset on an unconnected port.
>
>> > Even though this is a host controller bug, we still have to deal with
>> > it somehow. I guess we could move the code into xhci_plat_resume() and
>> > hide it behind a quirk to lessen the impact. But since reset_resume is
>> > not a common case for most host controllers, it's hard to say if this
>> > is DesignWare specific or a more widespread implementation mistake.
>>
>> I was going to suggest something along these lines too.  This seems to
>> be a bug in xHCI.  Therefore the fix belongs in xhci-hcd, not in the
>> hub driver.
>
> I agree.  Is there a chance that the Synopsys DesignWare will be a PCI
> device instead of a platform device?  If so, it would be better to put
> the code into xhci_resume instead of xhci_plat_resume.  That also allows
> you to only issue the warm reset when the register restore state command
> fails, after the xhci_reset call.
>
> Also, I assume that other systems with the Synopsys DesignWare IP will
> experience this issue?  I know of at least two other chipsets that will
> include that IP, and it would be good to find a way to trigger on the
> Synopsys IP, rather than off xHCI PCI vendor and device ID.  Otherwise
> we'll be adding PCI IDs to the xHCI driver quirks for many many kernels
> to come.
>
> I'm actually leaning towards enabling the check for warm reset broadly.
> It seems like it wouldn't hurt to issue a warm reset on the USB 3.0
> ports if they're in compliance, poll, or rx.detect.  So, let's enable
> this broadly in xhci_resume, mark the patch for stable, but ask for the
> backport to be delayed until 3.13.3 is out, to allow for more testing.
> If anyone complains of xHCI behavior changes, we'll change the code to
> add a quirk.

Is there a clean way to make this per-port rather than globally at
xhci_resume()?  I am looking to hook into this for port power recovery
as Tianyu's testing encountered "warm reset required" conditions at
runtime_resume.  I'm still on the hunt for a solid reproducer, but it
indicates this is a more general quirk with power session recovery.

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[David Cohen]

Hi Sarah,

On Fri, Dec 13, 2013 at 09:48:15AM -0800, Sarah Sharp wrote:
> On Thu, Dec 12, 2013 at 11:05:04AM -0500, Alan Stern wrote:
> > On Wed, 11 Dec 2013, Julius Werner wrote:
> > 
> > > >> ...although, the spec says that it does not wait for the port resets
> > > >> to complete.  As far as I can see re-issuing a warm reset and waiting
> > > >> is the only way to guarantee the core times the recovery.  Presumably
> > > >> the portstatus debounce in hub_activate() mitigates this, but that
> > > >> 100ms is less than a full reset timeout.
> > > 
> > > It's definitely not just a timing issue for us. I can't reproduce all
> > > the same cases as Vikas, but when I attach a USB analyzer to the ones
> > > I do see the host controller doesn't even start sending a reset.
> > > 
> > > >>> The xHCI spec requires that when the xHCI host is reset, a USB reset is
> > > >>> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
> > > >>> to warm reset.  See table 32 in the xHCI spec, in the definition of
> > > >>> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
> > > >>> ports at all on host controller reset?
> > > 
> > > Oh, interesting, I hadn't seen that yet. So I guess the spec itself is
> > > fine if it were followed to the letter.
> > > 
> > > I did some more tests about this on my Exynos machine: when I put a
> > > device to autosuspend (U3) and manually poke the xHC reset bit, I do
> > > see an automatic warm reset on the analyzer and the ports manage to
> > > retrain to U0. But after a system suspend/resume which calls
> > > xhci_reset() in the process, there is no reset on the wire. I also
> > > noticed that it doesn't drive a reset (even after manual poking) when
> > > there is no device connected on the other end of the analyzer.
> > > 
> > > So this might be our problem: maybe these host controllers (Synopsys
> > > DesignWare) issue the spec-mandated warm reset only on ports where
> > > they think there is a device attached. But after a system
> > > suspend/resume (where the whole IP block on the SoC was powered down),
> > > the host controller cannot know that there is still a device with an
> > > active power session attached, and therefore doesn't drive the reset
> > > on its own.
> 
> Ok, that makes some sense.  I could see why host controllers wouldn't
> want to drive reset on an unconnected port.
> 
> > > Even though this is a host controller bug, we still have to deal with
> > > it somehow. I guess we could move the code into xhci_plat_resume() and
> > > hide it behind a quirk to lessen the impact. But since reset_resume is
> > > not a common case for most host controllers, it's hard to say if this
> > > is DesignWare specific or a more widespread implementation mistake.
> > 
> > I was going to suggest something along these lines too.  This seems to 
> > be a bug in xHCI.  Therefore the fix belongs in xhci-hcd, not in the 
> > hub driver.
> 
> I agree.  Is there a chance that the Synopsys DesignWare will be a PCI
> device instead of a platform device?  If so, it would be better to put
> the code into xhci_resume instead of xhci_plat_resume.  That also allows

DWC3 on Intel Baytrail and Merrifield is PCI device.

Br, David

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Alan Stern]

On Fri, 13 Dec 2013, Dan Williams wrote:

> > I'm actually leaning towards enabling the check for warm reset broadly.
> > It seems like it wouldn't hurt to issue a warm reset on the USB 3.0
> > ports if they're in compliance, poll, or rx.detect.  So, let's enable
> > this broadly in xhci_resume, mark the patch for stable, but ask for the
> > backport to be delayed until 3.13.3 is out, to allow for more testing.
> > If anyone complains of xHCI behavior changes, we'll change the code to
> > add a quirk.
> 
> Is there a clean way to make this per-port rather than globally at
> xhci_resume()?  I am looking to hook into this for port power recovery
> as Tianyu's testing encountered "warm reset required" conditions at
> runtime_resume.  I'm still on the hunt for a solid reproducer, but it
> indicates this is a more general quirk with power session recovery.

There's no reason you can't do per-port testing inside xhci_resume
(assuming you know what to test for) as well as putting a warm reset in
the port-power handler of xhci_hub_control.

Of course, doing simultaneous warm resets on multiple ports will use 
less time than resetting each port individually, in sequence.

Alan Stern

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[Dan Williams]

On Fri, Dec 13, 2013 at 10:15 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 13 Dec 2013, Dan Williams wrote:
>
>> > I'm actually leaning towards enabling the check for warm reset broadly.
>> > It seems like it wouldn't hurt to issue a warm reset on the USB 3.0
>> > ports if they're in compliance, poll, or rx.detect.  So, let's enable
>> > this broadly in xhci_resume, mark the patch for stable, but ask for the
>> > backport to be delayed until 3.13.3 is out, to allow for more testing.
>> > If anyone complains of xHCI behavior changes, we'll change the code to
>> > add a quirk.
>>
>> Is there a clean way to make this per-port rather than globally at
>> xhci_resume()?  I am looking to hook into this for port power recovery
>> as Tianyu's testing encountered "warm reset required" conditions at
>> runtime_resume.  I'm still on the hunt for a solid reproducer, but it
>> indicates this is a more general quirk with power session recovery.
>
> There's no reason you can't do per-port testing inside xhci_resume
> (assuming you know what to test for) as well as putting a warm reset in
> the port-power handler of xhci_hub_control.

I'm just uneasy putting the recovery there as we lose the context of
why the port was powered-on.  For example we don't want to
pre-empt/duplicate a reset in xhci_hub_control() that is already
specified in hub_events().

> Of course, doing simultaneous warm resets on multiple ports will use
> less time than resetting each port individually, in sequence.
>

For the hub resume case, yes.  For pm_runtime_resume of an individual
port I believe it needs to be synchronous.

Re: [PATCH] USB: core: Add warm reset while reset-resuming SuperSpeed HUBs

[David Cohen]

On Fri, Dec 13, 2013 at 10:00:28AM -0800, David Cohen wrote:
> Hi Sarah,
> 
> On Fri, Dec 13, 2013 at 09:48:15AM -0800, Sarah Sharp wrote:
> > On Thu, Dec 12, 2013 at 11:05:04AM -0500, Alan Stern wrote:
> > > On Wed, 11 Dec 2013, Julius Werner wrote:
> > > 
> > > > >> ...although, the spec says that it does not wait for the port resets
> > > > >> to complete.  As far as I can see re-issuing a warm reset and waiting
> > > > >> is the only way to guarantee the core times the recovery.  Presumably
> > > > >> the portstatus debounce in hub_activate() mitigates this, but that
> > > > >> 100ms is less than a full reset timeout.
> > > > 
> > > > It's definitely not just a timing issue for us. I can't reproduce all
> > > > the same cases as Vikas, but when I attach a USB analyzer to the ones
> > > > I do see the host controller doesn't even start sending a reset.
> > > > 
> > > > >>> The xHCI spec requires that when the xHCI host is reset, a USB reset is
> > > > >>> driven down the USB 3.0 ports.  If hot reset fails, the port may migrate
> > > > >>> to warm reset.  See table 32 in the xHCI spec, in the definition of
> > > > >>> HCRST.  It sounds like this host doesn't drive a USB reset down USB 3.0
> > > > >>> ports at all on host controller reset?
> > > > 
> > > > Oh, interesting, I hadn't seen that yet. So I guess the spec itself is
> > > > fine if it were followed to the letter.
> > > > 
> > > > I did some more tests about this on my Exynos machine: when I put a
> > > > device to autosuspend (U3) and manually poke the xHC reset bit, I do
> > > > see an automatic warm reset on the analyzer and the ports manage to
> > > > retrain to U0. But after a system suspend/resume which calls
> > > > xhci_reset() in the process, there is no reset on the wire. I also
> > > > noticed that it doesn't drive a reset (even after manual poking) when
> > > > there is no device connected on the other end of the analyzer.
> > > > 
> > > > So this might be our problem: maybe these host controllers (Synopsys
> > > > DesignWare) issue the spec-mandated warm reset only on ports where
> > > > they think there is a device attached. But after a system
> > > > suspend/resume (where the whole IP block on the SoC was powered down),
> > > > the host controller cannot know that there is still a device with an
> > > > active power session attached, and therefore doesn't drive the reset
> > > > on its own.
> > 
> > Ok, that makes some sense.  I could see why host controllers wouldn't
> > want to drive reset on an unconnected port.
> > 
> > > > Even though this is a host controller bug, we still have to deal with
> > > > it somehow. I guess we could move the code into xhci_plat_resume() and
> > > > hide it behind a quirk to lessen the impact. But since reset_resume is
> > > > not a common case for most host controllers, it's hard to say if this
> > > > is DesignWare specific or a more widespread implementation mistake.
> > > 
> > > I was going to suggest something along these lines too.  This seems to 
> > > be a bug in xHCI.  Therefore the fix belongs in xhci-hcd, not in the 
> > > hub driver.
> > 
> > I agree.  Is there a chance that the Synopsys DesignWare will be a PCI
> > device instead of a platform device?  If so, it would be better to put
> > the code into xhci_resume instead of xhci_plat_resume.  That also allows
> 
> DWC3 on Intel Baytrail and Merrifield is PCI device.

But it actually registers xHCI's platform device to probe it. So,
nevermind.

Br, David