2021-05-07 12:27:23

by Chris Chiu

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

From: Chris Chiu <[email protected]>

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

Chris Chiu (2):
USB: reset-resume the device when PORT_SUSPEND is set but timeout
Revert "USB: Add reset-resume quirk for WD19's Realtek Hub"

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

--
2.20.1


2021-05-07 12:28:01

by Chris Chiu

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

From: Chris Chiu <[email protected]>

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 <[email protected]>
---
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

2021-05-07 12:29:31

by Chris Chiu

[permalink] [raw]
Subject: [PATCH 1/2] USB: reset-resume the device when PORT_SUSPEND is set but timeout

From: Chris Chiu <[email protected]>

On the Realtek hub of Dell Dock WD19, 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
devices connecting via the port are lost after resume.

This commit force reset-resume the device connected to the timeout
but suspended port so that the hub will have chance to clear the
PORT_SUSPEND feature during resume.

Signed-off-by: Chris Chiu <[email protected]>
---
drivers/usb/core/hub.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b2bc4b7c4289..18603949a8de 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;
+
+ status = hub_port_status(hub, port1, &portstatus,
+ &portchange);
+
+ dev_dbg(&port_dev->dev,
+ "suspend timeout, status %04x\n", portstatus);
+
+ if (status == 0 && port_is_suspended(hub, portstatus)) {
+ udev->reset_resume = 1;
+ goto err_wakeup;
+ }
+ }
+
dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);

/* Try to enable USB3 LTM again */
--
2.20.1

2021-05-07 17:33:33

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH 1/2] USB: reset-resume the device when PORT_SUSPEND is set but timeout

On Fri, May 07, 2021 at 05:33:28PM +0800, [email protected] wrote:
> From: Chris Chiu <[email protected]>
>
> On the Realtek hub of Dell Dock WD19, 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
> devices connecting via the port are lost after resume.
>
> This commit force reset-resume the device connected to the timeout
> but suspended port so that the hub will have chance to clear the
> PORT_SUSPEND feature during resume.
>
> Signed-off-by: Chris Chiu <[email protected]>
> ---
> drivers/usb/core/hub.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index b2bc4b7c4289..18603949a8de 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;
> +
> + status = hub_port_status(hub, port1, &portstatus,
> + &portchange);

Don't reuse status like this. There will be a problem if this line sets
status to 0 but the port isn't actually suspended. Use a different
variable instead.

Alan Stern

> +
> + dev_dbg(&port_dev->dev,
> + "suspend timeout, status %04x\n", portstatus);
> +
> + if (status == 0 && port_is_suspended(hub, portstatus)) {
> + udev->reset_resume = 1;
> + goto err_wakeup;
> + }
> + }
> +
> dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);
>
> /* Try to enable USB3 LTM again */
> --
> 2.20.1
>