From: Chris Chiu <[email protected]>
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
From: Chris Chiu <[email protected]>
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 <[email protected]>
---
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
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
On Thu, May 13, 2021 at 12:14:46PM +0800, [email protected] wrote:
> From: Chris Chiu <[email protected]>
>
> 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 <[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
>
On Thu, May 13, 2021 at 12:14:45PM +0800, [email protected] wrote:
> From: Chris Chiu <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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
>