[PATCH v3 0/2] USB: propose a generic fix for PORT_SUSPEND set feature timeout

[PATCH v3 0/2] USB: propose a generic fix for PORT_SUSPEND set feature timeout

[chris.chiu]

From: Chris Chiu <chris.chiu@canonical.com>

For the Realtek Hub which fails to resume the port which has wakeup
enable descendants, trying to propose a more generic and better fix
to have the runtime suspend/resume work instead of a reset-resume quirk.

Chris Chiu (2):
  USB: Verify the port status when timeout happens during port suspend
  Revert "USB: Add reset-resume quirk for WD19's Realtek Hub"

 drivers/usb/core/hub.c    | 16 ++++++++++++++++
 drivers/usb/core/quirks.c |  1 -
 2 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH v3 1/2] USB: Verify the port status when timeout happens during port suspend

[chris.chiu]

From: Chris Chiu <chris.chiu@canonical.com>

On the Realtek high-speed Hub(0bda:5487), the port which has wakeup
enabled_descendants will sometimes timeout when setting PORT_SUSPEND
feature. After checking the PORT_SUSPEND bit in wPortStatus, it is
already set. However, the hub will fail to activate because the
PORT_SUSPEND feature of that port is not cleared during resume. All
connected devices are lost after resume.

Check the port status to verify whether it's really suspended when
timeout happpens. If yes, mark it as suspended so the device can
be resumed correctly.

Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
---

Changelog:
  v3:
    - create a new goto target for the timeout case instead of
      reset_resume
    - Revise the commit title/message because reset_resume is not
      required.
  v2:
    - create a new variable to keep the result of hub_port_status
      when suspend timeout.

 drivers/usb/core/hub.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b2bc4b7c4289..c5d64175eaa9 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3385,6 +3385,21 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		status = 0;
 	}
 	if (status) {
+		if (status == -ETIMEDOUT) {
+			u16 portstatus, portchange;
+
+			int ret = hub_port_status(hub, port1, &portstatus,
+					&portchange);
+
+			dev_dbg(&port_dev->dev,
+				"suspend timeout, status %04x\n", portstatus);
+
+			if (ret == 0 && port_is_suspended(hub, portstatus)) {
+				status = 0;
+				goto suspend_done;
+			}
+		}
+
 		dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);
 
 		/* Try to enable USB3 LTM again */
@@ -3401,6 +3416,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		if (!PMSG_IS_AUTO(msg))
 			status = 0;
 	} else {
+ suspend_done:
 		dev_dbg(&udev->dev, "usb %ssuspend, wakeup %d\n",
 				(PMSG_IS_AUTO(msg) ? "auto-" : ""),
 				udev->do_remote_wakeup);
-- 
2.20.1

[PATCH v3 2/2] Revert "USB: Add reset-resume quirk for WD19's Realtek Hub"

[chris.chiu]

From: Chris Chiu <chris.chiu@canonical.com>

This reverts commit ca91fd8c7643d93bfc18a6fec1a0d3972a46a18a. The
problematic hub should be taken care for each setting PORT_SUSPEND
feature timeout instead of reset-resume all the time.

Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
---
 drivers/usb/core/quirks.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 21e7522655ac..6114cf83bb44 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -406,7 +406,6 @@ static const struct usb_device_id usb_quirk_list[] = {
 
 	/* Realtek hub in Dell WD19 (Type-C) */
 	{ USB_DEVICE(0x0bda, 0x0487), .driver_info = USB_QUIRK_NO_LPM },
-	{ USB_DEVICE(0x0bda, 0x5487), .driver_info = USB_QUIRK_RESET_RESUME },
 
 	/* Generic RTL8153 based ethernet adapters */
 	{ USB_DEVICE(0x0bda, 0x8153), .driver_info = USB_QUIRK_NO_LPM },
-- 
2.20.1

Re: [PATCH v3 1/2] USB: Verify the port status when timeout happens during port suspend

[Alan Stern]

On Thu, May 13, 2021 at 12:14:45PM +0800, chris.chiu@canonical.com wrote:
> From: Chris Chiu <chris.chiu@canonical.com>
> 
> On the Realtek high-speed Hub(0bda:5487), the port which has wakeup
> enabled_descendants will sometimes timeout when setting PORT_SUSPEND
> feature. After checking the PORT_SUSPEND bit in wPortStatus, it is
> already set. However, the hub will fail to activate because the
> PORT_SUSPEND feature of that port is not cleared during resume. All
> connected devices are lost after resume.

The last two sentences of this paragraph are now inaccurate.  Please fix 
them up to match the current patch version.

> Check the port status to verify whether it's really suspended when
> timeout happpens. If yes, mark it as suspended so the device can
> be resumed correctly.
> 
> Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
> ---
> 
> Changelog:
>   v3:
>     - create a new goto target for the timeout case instead of
>       reset_resume
>     - Revise the commit title/message because reset_resume is not
>       required.
>   v2:
>     - create a new variable to keep the result of hub_port_status
>       when suspend timeout.
> 
>  drivers/usb/core/hub.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index b2bc4b7c4289..c5d64175eaa9 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3385,6 +3385,21 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  		status = 0;
>  	}
>  	if (status) {
> +		if (status == -ETIMEDOUT) {
> +			u16 portstatus, portchange;
> +

Extra blank line.

> +			int ret = hub_port_status(hub, port1, &portstatus,
> +					&portchange);
> +
> +			dev_dbg(&port_dev->dev,
> +				"suspend timeout, status %04x\n", portstatus);

The portstatus value shouldn't be printed if ret < 0, because it won't 
be initialized.  If you want, initialize portstatus to 0 in the 
declaration.

Also, there should be a comment here explaining why this code is needed.  
It should say pretty the same thing as the patch description, but more 
briefly (two sentences should be sufficient).

Alan Stern

> +
> +			if (ret == 0 && port_is_suspended(hub, portstatus)) {
> +				status = 0;
> +				goto suspend_done;
> +			}
> +		}
> +
>  		dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);
>  
>  		/* Try to enable USB3 LTM again */
> @@ -3401,6 +3416,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  		if (!PMSG_IS_AUTO(msg))
>  			status = 0;
>  	} else {
> + suspend_done:
>  		dev_dbg(&udev->dev, "usb %ssuspend, wakeup %d\n",
>  				(PMSG_IS_AUTO(msg) ? "auto-" : ""),
>  				udev->do_remote_wakeup);
> -- 
> 2.20.1
> 

Re: [PATCH v3 2/2] Revert "USB: Add reset-resume quirk for WD19's Realtek Hub"

[Alan Stern]

On Thu, May 13, 2021 at 12:14:46PM +0800, chris.chiu@canonical.com wrote:
> From: Chris Chiu <chris.chiu@canonical.com>
> 
> This reverts commit ca91fd8c7643d93bfc18a6fec1a0d3972a46a18a. The

Use the proper format for referring to commits: just the first 12 hex 
digits of the commit ID, followed by the commit's name in parentheses 
and quotation marks.  See other patch submissions in the mailing list 
archives for examples.

> problematic hub should be taken care for each setting PORT_SUSPEND
> feature timeout instead of reset-resume all the time.

The last sentence is not grammatical.  It should say something more 
like:

	The previous patch in this series now handles problematic hubs
	by checking the port status when a PORT_SUSPEND timeout occurs,
	so we don't need to use reset-resume all the time.

Alan Stern

> 
> Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
> ---
>  drivers/usb/core/quirks.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 21e7522655ac..6114cf83bb44 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -406,7 +406,6 @@ static const struct usb_device_id usb_quirk_list[] = {
>  
>  	/* Realtek hub in Dell WD19 (Type-C) */
>  	{ USB_DEVICE(0x0bda, 0x0487), .driver_info = USB_QUIRK_NO_LPM },
> -	{ USB_DEVICE(0x0bda, 0x5487), .driver_info = USB_QUIRK_RESET_RESUME },
>  
>  	/* Generic RTL8153 based ethernet adapters */
>  	{ USB_DEVICE(0x0bda, 0x8153), .driver_info = USB_QUIRK_NO_LPM },
> -- 
> 2.20.1
>