[PATCH] usb: Retry port status check on resume to work around RH bugs

[PATCH] usb: Retry port status check on resume to work around RH bugs

[Julius Werner]

The EHCI controller on the RK3288 SoC is violating basic parts of the
USB spec and thereby unable to properly resume a suspended port. It does
not start SOF generation within 3ms of finishing resume signaling, so
the attached device will drop off the bus again. This is a particular
problem with runtime PM, where accessing the device will trigger a
resume that immediately makes it unavailabe (and reenumerate with a new
handle).

Thankfully, the persist feature is generally able to work around stuff
like that. Unfortunately, it doesn't quite work in this particular case
because the controller will turn off the CurrentConnectStatus bit for an
instant while the device is reconnecting, which causes the kernel to
conclude that it permanently disappeared. This patch adds a tiny retry
mechanism to the core port resume code which will catch this case and
shouldn't have any notable impact on other controllers.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/core/hub.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aeb50bb..d1d0a66 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub, unsigned portstatus)
  */
 static int check_port_resume_type(struct usb_device *udev,
 		struct usb_hub *hub, int port1,
-		int status, unsigned portchange, unsigned portstatus)
+		int status, u16 portchange, u16 portstatus)
 {
 	struct usb_port *port_dev = hub->ports[port1 - 1];
+	int retries = 3;
 
+retry:
 	/* Is a warm reset needed to recover the connection? */
 	if (status == 0 && udev->reset_resume
 		&& hub_port_warm_reset_required(hub, port1, portstatus)) {
@@ -2909,8 +2911,15 @@ static int check_port_resume_type(struct usb_device *udev,
 	else if (status || port_is_suspended(hub, portstatus) ||
 			!port_is_power_on(hub, portstatus) ||
 			!(portstatus & USB_PORT_STAT_CONNECTION)) {
-		if (status >= 0)
+		if (status >= 0) {
+			if (retries--) {
+				udelay(200);
+				status = hub_port_status(hub, port1,
+						&portstatus, &portchange);
+				goto retry;
+			}
 			status = -ENODEV;
+		}
 	}
 
 	/* Can't do a normal resume if the port isn't enabled,
-- 
2.1.2

Re: [PATCH] usb: Retry port status check on resume to work around RH bugs

[Alan Stern]

On Fri, 23 Jan 2015, Julius Werner wrote:

> The EHCI controller on the RK3288 SoC is violating basic parts of the
> USB spec and thereby unable to properly resume a suspended port. It does
> not start SOF generation within 3ms of finishing resume signaling, so
> the attached device will drop off the bus again. This is a particular
> problem with runtime PM, where accessing the device will trigger a
> resume that immediately makes it unavailabe (and reenumerate with a new
> handle).
> 
> Thankfully, the persist feature is generally able to work around stuff
> like that. Unfortunately, it doesn't quite work in this particular case
> because the controller will turn off the CurrentConnectStatus bit for an
> instant while the device is reconnecting, which causes the kernel to
> conclude that it permanently disappeared. This patch adds a tiny retry
> mechanism to the core port resume code which will catch this case and
> shouldn't have any notable impact on other controllers.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/usb/core/hub.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index aeb50bb..d1d0a66 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub, unsigned portstatus)
>   */
>  static int check_port_resume_type(struct usb_device *udev,
>  		struct usb_hub *hub, int port1,
> -		int status, unsigned portchange, unsigned portstatus)
> +		int status, u16 portchange, u16 portstatus)
>  {
>  	struct usb_port *port_dev = hub->ports[port1 - 1];
> +	int retries = 3;
>  
> +retry:
>  	/* Is a warm reset needed to recover the connection? */
>  	if (status == 0 && udev->reset_resume
>  		&& hub_port_warm_reset_required(hub, port1, portstatus)) {
> @@ -2909,8 +2911,15 @@ static int check_port_resume_type(struct usb_device *udev,
>  	else if (status || port_is_suspended(hub, portstatus) ||
>  			!port_is_power_on(hub, portstatus) ||
>  			!(portstatus & USB_PORT_STAT_CONNECTION)) {
> -		if (status >= 0)
> +		if (status >= 0) {
> +			if (retries--) {
> +				udelay(200);

You should use a sleeping function call, not a delay.

> +				status = hub_port_status(hub, port1,
> +						&portstatus, &portchange);
> +				goto retry;
> +			}
>  			status = -ENODEV;
> +		}
>  	}
>  
>  	/* Can't do a normal resume if the port isn't enabled,

Otherwise this is okay.

Alan Stern

Re: [PATCH] usb: Retry port status check on resume to work around RH bugs

[Julius Werner]

> You should use a sleeping function call, not a delay.

Whoops... yes, of course. Guess too much firmware work made me forget
what multithreading is there for a moment. Will send a v2.

[PATCH v2] usb: Retry port status check on resume to work around RH bugs

[Julius Werner]

The EHCI controller on the RK3288 SoC is violating basic parts of the
USB spec and thereby unable to properly resume a suspended port. It does
not start SOF generation within 3ms of finishing resume signaling, so
the attached device will drop off the bus again. This is a particular
problem with runtime PM, where accessing the device will trigger a
resume that immediately makes it unavailabe (and reenumerate with a new
handle).

Thankfully, the persist feature is generally able to work around stuff
like that. Unfortunately, it doesn't quite work in this particular case
because the controller will turn off the CurrentConnectStatus bit for an
instant while the device is reconnecting, which causes the kernel to
conclude that it permanently disappeared. This patch adds a tiny retry
mechanism to the core port resume code which will catch this case and
shouldn't have any notable impact on other controllers.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/core/hub.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aeb50bb..d1d0a66 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub, unsigned portstatus)
  */
 static int check_port_resume_type(struct usb_device *udev,
 		struct usb_hub *hub, int port1,
-		int status, unsigned portchange, unsigned portstatus)
+		int status, u16 portchange, u16 portstatus)
 {
 	struct usb_port *port_dev = hub->ports[port1 - 1];
+	int retries = 3;
 
+retry:
 	/* Is a warm reset needed to recover the connection? */
 	if (status == 0 && udev->reset_resume
 		&& hub_port_warm_reset_required(hub, port1, portstatus)) {
@@ -2909,8 +2911,15 @@ static int check_port_resume_type(struct usb_device *udev,
 	else if (status || port_is_suspended(hub, portstatus) ||
 			!port_is_power_on(hub, portstatus) ||
 			!(portstatus & USB_PORT_STAT_CONNECTION)) {
-		if (status >= 0)
+		if (status >= 0) {
+			if (retries--) {
+				usleep_range(200, 300);
+				status = hub_port_status(hub, port1,
+						&portstatus, &portchange);
+				goto retry;
+			}
 			status = -ENODEV;
+		}
 	}
 
 	/* Can't do a normal resume if the port isn't enabled,
-- 
2.1.2

Re: [PATCH v2] usb: Retry port status check on resume to work around RH bugs

[Alan Stern]

On Mon, 26 Jan 2015, Julius Werner wrote:

> The EHCI controller on the RK3288 SoC is violating basic parts of the
> USB spec and thereby unable to properly resume a suspended port. It does
> not start SOF generation within 3ms of finishing resume signaling, so
> the attached device will drop off the bus again. This is a particular
> problem with runtime PM, where accessing the device will trigger a
> resume that immediately makes it unavailabe (and reenumerate with a new

Typo: "unavailabe"

> handle).
> 
> Thankfully, the persist feature is generally able to work around stuff
> like that. Unfortunately, it doesn't quite work in this particular case
> because the controller will turn off the CurrentConnectStatus bit for an
> instant while the device is reconnecting, which causes the kernel to
> conclude that it permanently disappeared. This patch adds a tiny retry
> mechanism to the core port resume code which will catch this case and
> shouldn't have any notable impact on other controllers.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/usb/core/hub.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index aeb50bb..d1d0a66 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub, unsigned portstatus)
>   */
>  static int check_port_resume_type(struct usb_device *udev,
>  		struct usb_hub *hub, int port1,
> -		int status, unsigned portchange, unsigned portstatus)
> +		int status, u16 portchange, u16 portstatus)
>  {
>  	struct usb_port *port_dev = hub->ports[port1 - 1];
> +	int retries = 3;
>  
> +retry:

I prefer to indent statement labels by one space, so that diff doesn't 
see them as function declarations.

>  	/* Is a warm reset needed to recover the connection? */
>  	if (status == 0 && udev->reset_resume
>  		&& hub_port_warm_reset_required(hub, port1, portstatus)) {
> @@ -2909,8 +2911,15 @@ static int check_port_resume_type(struct usb_device *udev,
>  	else if (status || port_is_suspended(hub, portstatus) ||
>  			!port_is_power_on(hub, portstatus) ||
>  			!(portstatus & USB_PORT_STAT_CONNECTION)) {
> -		if (status >= 0)
> +		if (status >= 0) {

Thinking about this more carefully, it looks like the only thing you 
care about here is the !(portstatus & USB_PORT_STAT_CONNECTION) case.  
For everything else, you don't need to retry.

So why not separate out that one case?

	else if (status || port_is_suspended(hub, portstatus) ||
			!port_is_power_on(hub, portstatus)) {
		if (status >= 0)
			status = -ENODEV;
	} else if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
		retry stuff...
	}

> +			if (retries--) {
> +				usleep_range(200, 300);
> +				status = hub_port_status(hub, port1,
> +						&portstatus, &portchange);
> +				goto retry;
> +			}
>  			status = -ENODEV;
> +		}
>  	}
>  
>  	/* Can't do a normal resume if the port isn't enabled,

Alan Stern

[PATCH v3] usb: Retry port status check on resume to work around RH bugs

[Julius Werner]

The EHCI controller on the RK3288 SoC is violating basic parts of the
USB spec and thereby unable to properly resume a suspended port. It does
not start SOF generation within 3ms of finishing resume signaling, so
the attached device will drop of the bus again. This is a particular
problem with runtime PM, where accessing the device will trigger a
resume that immediately makes it unavailable (and reenumerate with a new
handle).

Thankfully, the persist feature is generally able to work around stuff
like that. Unfortunately, it doesn't quite work in this particular case
because the controller will turn off the CurrentConnectStatus bit for an
instant while the device is reconnecting, which causes the kernel to
conclude that it permanently disappeared. This patch adds a tiny retry
mechanism to the core port resume code which will catch this case and
shouldn't have any notable impact on other controllers.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/core/hub.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aeb50bb..26bdd96 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub, unsigned portstatus)
  */
 static int check_port_resume_type(struct usb_device *udev,
 		struct usb_hub *hub, int port1,
-		int status, unsigned portchange, unsigned portstatus)
+		int status, u16 portchange, u16 portstatus)
 {
 	struct usb_port *port_dev = hub->ports[port1 - 1];
+	int retries = 3;
 
+ retry:
 	/* Is a warm reset needed to recover the connection? */
 	if (status == 0 && udev->reset_resume
 		&& hub_port_warm_reset_required(hub, port1, portstatus)) {
@@ -2907,10 +2909,17 @@ static int check_port_resume_type(struct usb_device *udev,
 	}
 	/* Is the device still present? */
 	else if (status || port_is_suspended(hub, portstatus) ||
-			!port_is_power_on(hub, portstatus) ||
-			!(portstatus & USB_PORT_STAT_CONNECTION)) {
+			!port_is_power_on(hub, portstatus)) {
 		if (status >= 0)
 			status = -ENODEV;
+	} else if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
+		if (retries--) {
+			usleep_range(200, 300);
+			status = hub_port_status(hub, port1, &portstatus,
+							     &portchange);
+			goto retry;
+		}
+		status = -ENODEV;
 	}
 
 	/* Can't do a normal resume if the port isn't enabled,
-- 
2.1.2

Re: [PATCH v3] usb: Retry port status check on resume to work around RH bugs

[Alan Stern]

On Tue, 27 Jan 2015, Julius Werner wrote:

> The EHCI controller on the RK3288 SoC is violating basic parts of the
> USB spec and thereby unable to properly resume a suspended port. It does
> not start SOF generation within 3ms of finishing resume signaling, so
> the attached device will drop of the bus again. This is a particular
> problem with runtime PM, where accessing the device will trigger a
> resume that immediately makes it unavailable (and reenumerate with a new
> handle).
> 
> Thankfully, the persist feature is generally able to work around stuff
> like that. Unfortunately, it doesn't quite work in this particular case
> because the controller will turn off the CurrentConnectStatus bit for an
> instant while the device is reconnecting, which causes the kernel to
> conclude that it permanently disappeared. This patch adds a tiny retry
> mechanism to the core port resume code which will catch this case and
> shouldn't have any notable impact on other controllers.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/usb/core/hub.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index aeb50bb..26bdd96 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub, unsigned portstatus)
>   */
>  static int check_port_resume_type(struct usb_device *udev,
>  		struct usb_hub *hub, int port1,
> -		int status, unsigned portchange, unsigned portstatus)
> +		int status, u16 portchange, u16 portstatus)
>  {
>  	struct usb_port *port_dev = hub->ports[port1 - 1];
> +	int retries = 3;
>  
> + retry:
>  	/* Is a warm reset needed to recover the connection? */
>  	if (status == 0 && udev->reset_resume
>  		&& hub_port_warm_reset_required(hub, port1, portstatus)) {
> @@ -2907,10 +2909,17 @@ static int check_port_resume_type(struct usb_device *udev,
>  	}
>  	/* Is the device still present? */
>  	else if (status || port_is_suspended(hub, portstatus) ||
> -			!port_is_power_on(hub, portstatus) ||
> -			!(portstatus & USB_PORT_STAT_CONNECTION)) {
> +			!port_is_power_on(hub, portstatus)) {
>  		if (status >= 0)
>  			status = -ENODEV;
> +	} else if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
> +		if (retries--) {
> +			usleep_range(200, 300);
> +			status = hub_port_status(hub, port1, &portstatus,
> +							     &portchange);
> +			goto retry;
> +		}
> +		status = -ENODEV;
>  	}
>  
>  	/* Can't do a normal resume if the port isn't enabled,

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