All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
@ 2021-04-12 15:00 chris.chiu
  2021-04-12 15:12 ` Alan Stern
  0 siblings, 1 reply; 7+ messages in thread
From: chris.chiu @ 2021-04-12 15:00 UTC (permalink / raw)
  To: gregkh, stern, m.v.b, hadess; +Cc: linux-usb, linux-kernel, Chris Chiu

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

Realtek Hub (0bda:5413) in Dell Dock WD19 sometimes fails to work
after the system resumes from suspend with remote wakeup enabled
device connected:
[ 1947.640907] hub 5-2.3:1.0: hub_ext_port_status failed (err = -71)
[ 1947.641208] usb 5-2.3-port5: cannot disable (err = -71)
[ 1947.641401] hub 5-2.3:1.0: hub_ext_port_status failed (err = -71)
[ 1947.641450] usb 5-2.3-port4: cannot reset (err = -71)

Information of this hub:
T:  Bus=01 Lev=02 Prnt=02 Port=02 Cnt=01 Dev#=  9 Spd=480  MxCh= 6
D:  Ver= 2.10 Cls=09(hub  ) Sub=00 Prot=02 MxPS=64 #Cfgs=  1
P:  Vendor=0bda ProdID=5413 Rev= 1.21
S:  Manufacturer=Dell Inc.
S:  Product=Dell dock
C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=  0mA
I:  If#= 0 Alt= 0 #EPs= 1 Cls=09(hub  ) Sub=00 Prot=01 Driver=hub
E:  Ad=81(I) Atr=03(Int.) MxPS=   1 Ivl=256ms
I:* If#= 0 Alt= 1 #EPs= 1 Cls=09(hub  ) Sub=00 Prot=02 Driver=hub
E:  Ad=81(I) Atr=03(Int.) MxPS=   1 Ivl=256ms

The failure results from the ETIMEDOUT by chance when turning on
the suspend feature of the hub. The usb_resume_device will not be
invoked since the device state is not set to suspended, then the
hub fails to activate subsequently.

The USB_PORT_FEAT_SUSPEND is not really necessary due to the
"global suspend" in USB 2.0 spec. It's only for many hub devices
which don't relay wakeup requests from the devices connected to
downstream ports. For this realtek hub, there's no problem waking
up the system from connected keyboard.

This commit bypasses the USB_PORT_FEAT_SUSPEND for the quirky hub.

Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 3 +++
 drivers/usb/core/hub.c                          | 7 +++++--
 drivers/usb/core/quirks.c                       | 5 +++++
 include/linux/usb/quirks.h                      | 3 +++
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 04545725f187..c14214469ad6 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5682,6 +5682,9 @@
 					pause after every control message);
 				o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra
 					delay after resetting its port);
+				p = USB_QUIRK_NO_SET_FEAT_SUSPEND (Device can't
+					handle set port-feature-suspend request
+					correctly);
 			Example: quirks=0781:5580:bk,0a5c:5834:gij
 
 	usbhid.mousepoll=
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 7f71218cc1e5..8478d49bba77 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 	 * descendants is enabled for remote wakeup.
 	 */
 	else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
-		status = set_port_feature(hub->hdev, port1,
-				USB_PORT_FEAT_SUSPEND);
+		if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND)
+			status = 0;
+		else
+			status = set_port_feature(hub->hdev, port1,
+					USB_PORT_FEAT_SUSPEND);
 	else {
 		really_suspend = false;
 		status = 0;
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 76ac5d6555ae..53689de53900 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -138,6 +138,9 @@ static int quirks_param_set(const char *value, const struct kernel_param *kp)
 			case 'o':
 				flags |= USB_QUIRK_HUB_SLOW_RESET;
 				break;
+			case 'p':
+				flags |= USB_QUIRK_NO_SET_FEAT_SUSPEND;
+				break;
 			/* Ignore unrecognized flag characters */
 			}
 		}
@@ -406,6 +409,8 @@ 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, 0x5413), .driver_info =
+			USB_QUIRK_NO_SET_FEAT_SUSPEND },
 
 	/* Generic RTL8153 based ethernet adapters */
 	{ USB_DEVICE(0x0bda, 0x8153), .driver_info = USB_QUIRK_NO_LPM },
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index 5e4c497f54d6..3b8930b99554 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,7 @@
 /* device has endpoints that should be ignored */
 #define USB_QUIRK_ENDPOINT_IGNORE		BIT(15)
 
+/* Device can't handle set port-feature-suspend request correctly */
+#define USB_QUIRK_NO_SET_FEAT_SUSPEND		BIT(16)
+
 #endif /* __LINUX_USB_QUIRKS_H */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
  2021-04-12 15:00 [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub chris.chiu
@ 2021-04-12 15:12 ` Alan Stern
  2021-04-13  7:52   ` Chris Chiu
  0 siblings, 1 reply; 7+ messages in thread
From: Alan Stern @ 2021-04-12 15:12 UTC (permalink / raw)
  To: chris.chiu; +Cc: gregkh, m.v.b, hadess, linux-usb, linux-kernel

On Mon, Apr 12, 2021 at 11:00:06PM +0800, chris.chiu@canonical.com wrote:
> From: Chris Chiu <chris.chiu@canonical.com>
> 
> Realtek Hub (0bda:5413) in Dell Dock WD19 sometimes fails to work
> after the system resumes from suspend with remote wakeup enabled
> device connected:
> [ 1947.640907] hub 5-2.3:1.0: hub_ext_port_status failed (err = -71)
> [ 1947.641208] usb 5-2.3-port5: cannot disable (err = -71)
> [ 1947.641401] hub 5-2.3:1.0: hub_ext_port_status failed (err = -71)
> [ 1947.641450] usb 5-2.3-port4: cannot reset (err = -71)
> 
> Information of this hub:
> T:  Bus=01 Lev=02 Prnt=02 Port=02 Cnt=01 Dev#=  9 Spd=480  MxCh= 6
> D:  Ver= 2.10 Cls=09(hub  ) Sub=00 Prot=02 MxPS=64 #Cfgs=  1
> P:  Vendor=0bda ProdID=5413 Rev= 1.21
> S:  Manufacturer=Dell Inc.
> S:  Product=Dell dock
> C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=  0mA
> I:  If#= 0 Alt= 0 #EPs= 1 Cls=09(hub  ) Sub=00 Prot=01 Driver=hub
> E:  Ad=81(I) Atr=03(Int.) MxPS=   1 Ivl=256ms
> I:* If#= 0 Alt= 1 #EPs= 1 Cls=09(hub  ) Sub=00 Prot=02 Driver=hub
> E:  Ad=81(I) Atr=03(Int.) MxPS=   1 Ivl=256ms
> 
> The failure results from the ETIMEDOUT by chance when turning on
> the suspend feature of the hub. The usb_resume_device will not be
> invoked since the device state is not set to suspended, then the
> hub fails to activate subsequently.
> 
> The USB_PORT_FEAT_SUSPEND is not really necessary due to the
> "global suspend" in USB 2.0 spec. It's only for many hub devices
> which don't relay wakeup requests from the devices connected to
> downstream ports. For this realtek hub, there's no problem waking
> up the system from connected keyboard.

What about runtime suspend?  That _does_ require USB_PORT_FEAT_SUSPEND.

> This commit bypasses the USB_PORT_FEAT_SUSPEND for the quirky hub.
> 
> Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
> ---


> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 7f71218cc1e5..8478d49bba77 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  	 * descendants is enabled for remote wakeup.
>  	 */
>  	else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
> -		status = set_port_feature(hub->hdev, port1,
> -				USB_PORT_FEAT_SUSPEND);
> +		if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND)

You should test hub->hdev->quirks, here, not udev->quirks.  The quirk 
belongs to the Realtek hub, not to the device that's plugged into the 
hub.

Alan Stern

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
  2021-04-12 15:12 ` Alan Stern
@ 2021-04-13  7:52   ` Chris Chiu
  2021-04-13 14:44     ` Alan Stern
  0 siblings, 1 reply; 7+ messages in thread
From: Chris Chiu @ 2021-04-13  7:52 UTC (permalink / raw)
  To: Alan Stern; +Cc: gregkh, m.v.b, hadess, linux-usb, Linux Kernel

On Mon, Apr 12, 2021 at 11:12 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Apr 12, 2021 at 11:00:06PM +0800, chris.chiu@canonical.com wrote:
> > From: Chris Chiu <chris.chiu@canonical.com>
> >
> > Realtek Hub (0bda:5413) in Dell Dock WD19 sometimes fails to work
> > after the system resumes from suspend with remote wakeup enabled
> > device connected:
> > [ 1947.640907] hub 5-2.3:1.0: hub_ext_port_status failed (err = -71)
> > [ 1947.641208] usb 5-2.3-port5: cannot disable (err = -71)
> > [ 1947.641401] hub 5-2.3:1.0: hub_ext_port_status failed (err = -71)
> > [ 1947.641450] usb 5-2.3-port4: cannot reset (err = -71)
> >
> > Information of this hub:
> > T:  Bus=01 Lev=02 Prnt=02 Port=02 Cnt=01 Dev#=  9 Spd=480  MxCh= 6
> > D:  Ver= 2.10 Cls=09(hub  ) Sub=00 Prot=02 MxPS=64 #Cfgs=  1
> > P:  Vendor=0bda ProdID=5413 Rev= 1.21
> > S:  Manufacturer=Dell Inc.
> > S:  Product=Dell dock
> > C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=  0mA
> > I:  If#= 0 Alt= 0 #EPs= 1 Cls=09(hub  ) Sub=00 Prot=01 Driver=hub
> > E:  Ad=81(I) Atr=03(Int.) MxPS=   1 Ivl=256ms
> > I:* If#= 0 Alt= 1 #EPs= 1 Cls=09(hub  ) Sub=00 Prot=02 Driver=hub
> > E:  Ad=81(I) Atr=03(Int.) MxPS=   1 Ivl=256ms
> >
> > The failure results from the ETIMEDOUT by chance when turning on
> > the suspend feature of the hub. The usb_resume_device will not be
> > invoked since the device state is not set to suspended, then the
> > hub fails to activate subsequently.
> >
> > The USB_PORT_FEAT_SUSPEND is not really necessary due to the
> > "global suspend" in USB 2.0 spec. It's only for many hub devices
> > which don't relay wakeup requests from the devices connected to
> > downstream ports. For this realtek hub, there's no problem waking
> > up the system from connected keyboard.
>
> What about runtime suspend?  That _does_ require USB_PORT_FEAT_SUSPEND.

It's hard to reproduce the same thing with runtime PM. I also don't
know the aggressive
way to trigger runtime suspend. So I'm assuming the same thing will happen in
runtime PM case because they both go the same usb_port_resume path. Could
you please suggest a better way to verify this for runtime PM?

>
> > This commit bypasses the USB_PORT_FEAT_SUSPEND for the quirky hub.
> >
> > Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
> > ---
>
>
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 7f71218cc1e5..8478d49bba77 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> >        * descendants is enabled for remote wakeup.
> >        */
> >       else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
> > -             status = set_port_feature(hub->hdev, port1,
> > -                             USB_PORT_FEAT_SUSPEND);
> > +             if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND)
>
> You should test hub->hdev->quirks, here, not udev->quirks.  The quirk
> belongs to the Realtek hub, not to the device that's plugged into the
> hub.
>

Thanks for pointing that out. I'll verify again and propose a V2 after
it's done.

> Alan Stern

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
  2021-04-13  7:52   ` Chris Chiu
@ 2021-04-13 14:44     ` Alan Stern
  2021-04-14  5:07       ` Chris Chiu
  0 siblings, 1 reply; 7+ messages in thread
From: Alan Stern @ 2021-04-13 14:44 UTC (permalink / raw)
  To: Chris Chiu; +Cc: gregkh, m.v.b, hadess, linux-usb, Linux Kernel

On Tue, Apr 13, 2021 at 03:52:14PM +0800, Chris Chiu wrote:
> On Mon, Apr 12, 2021 at 11:12 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Mon, Apr 12, 2021 at 11:00:06PM +0800, chris.chiu@canonical.com wrote:
> > > The USB_PORT_FEAT_SUSPEND is not really necessary due to the
> > > "global suspend" in USB 2.0 spec. It's only for many hub devices
> > > which don't relay wakeup requests from the devices connected to
> > > downstream ports. For this realtek hub, there's no problem waking
> > > up the system from connected keyboard.
> >
> > What about runtime suspend?  That _does_ require USB_PORT_FEAT_SUSPEND.
> 
> It's hard to reproduce the same thing with runtime PM. I also don't
> know the aggressive
> way to trigger runtime suspend. So I'm assuming the same thing will happen in
> runtime PM case because they both go the same usb_port_resume path. Could
> you please suggest a better way to verify this for runtime PM?

To put a USB device into runtime suspend, do this:

	echo 0 >/sys/bus/usb/devices/.../bConfigurationValue
	echo auto >/sys/bus/usb/devices/.../power/control

where ... is the pathname for the device you want to suspend.  (Note 
that this will unbind the device from its driver, so make sure there's 
no possibility of data loss before you do it.)

To resume the device, write "on" to the power/control file.  You can 
verify the runtime-PM status by reading the files in the power/ 
subdirectory.

> > > This commit bypasses the USB_PORT_FEAT_SUSPEND for the quirky hub.
> > >
> > > Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
> > > ---
> >
> >
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 7f71218cc1e5..8478d49bba77 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> > >        * descendants is enabled for remote wakeup.
> > >        */
> > >       else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
> > > -             status = set_port_feature(hub->hdev, port1,
> > > -                             USB_PORT_FEAT_SUSPEND);
> > > +             if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND)
> >
> > You should test hub->hdev->quirks, here, not udev->quirks.  The quirk
> > belongs to the Realtek hub, not to the device that's plugged into the
> > hub.
> >
> 
> Thanks for pointing that out. I'll verify again and propose a V2 after
> it's done.

Another thing to consider: You shouldn't return 0 from usb_port_suspend 
if the port wasn't actually suspended.  We don't want to kernel to have 
a false idea of the hardware's current state.

Alan Stern

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
  2021-04-13 14:44     ` Alan Stern
@ 2021-04-14  5:07       ` Chris Chiu
  2021-04-14 14:32         ` Alan Stern
  0 siblings, 1 reply; 7+ messages in thread
From: Chris Chiu @ 2021-04-14  5:07 UTC (permalink / raw)
  To: Alan Stern; +Cc: gregkh, m.v.b, hadess, linux-usb, Linux Kernel

On Tue, Apr 13, 2021 at 10:44 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Apr 13, 2021 at 03:52:14PM +0800, Chris Chiu wrote:
> > On Mon, Apr 12, 2021 at 11:12 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Mon, Apr 12, 2021 at 11:00:06PM +0800, chris.chiu@canonical.com wrote:
> > > > The USB_PORT_FEAT_SUSPEND is not really necessary due to the
> > > > "global suspend" in USB 2.0 spec. It's only for many hub devices
> > > > which don't relay wakeup requests from the devices connected to
> > > > downstream ports. For this realtek hub, there's no problem waking
> > > > up the system from connected keyboard.
> > >
> > > What about runtime suspend?  That _does_ require USB_PORT_FEAT_SUSPEND.
> >
> > It's hard to reproduce the same thing with runtime PM. I also don't
> > know the aggressive
> > way to trigger runtime suspend. So I'm assuming the same thing will happen in
> > runtime PM case because they both go the same usb_port_resume path. Could
> > you please suggest a better way to verify this for runtime PM?
>
> To put a USB device into runtime suspend, do this:
>
>         echo 0 >/sys/bus/usb/devices/.../bConfigurationValue
>         echo auto >/sys/bus/usb/devices/.../power/control
>
> where ... is the pathname for the device you want to suspend.  (Note
> that this will unbind the device from its driver, so make sure there's
> no possibility of data loss before you do it.)
>
> To resume the device, write "on" to the power/control file.  You can
> verify the runtime-PM status by reading the files in the power/
> subdirectory.
>
Thanks for the instructions. I can hit the same timeout problem with
runtime PM. The
fail rate seems the same as normal PM. (around 1/4 ~ 1/7)
root@:/sys/bus/usb/devices/3-4.3# echo auto > power/control
root@:/sys/bus/usb/devices/3-4.3# echo on > power/control
root@:/sys/bus/usb/devices/3-4.3# dmesg -c
[ 2789.679807] usb 3-4: kworker/7:0 timed out on ep0out len=0/0
[ 2789.679812] usb 3-4-port3: can't suspend, status -110
[ 2789.680078] usb 3-4.3: Failed to suspend device, error -110


> > > > This commit bypasses the USB_PORT_FEAT_SUSPEND for the quirky hub.
> > > >
> > > > Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
> > > > ---
> > >
> > >
> > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > > index 7f71218cc1e5..8478d49bba77 100644
> > > > --- a/drivers/usb/core/hub.c
> > > > +++ b/drivers/usb/core/hub.c
> > > > @@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> > > >        * descendants is enabled for remote wakeup.
> > > >        */
> > > >       else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
> > > > -             status = set_port_feature(hub->hdev, port1,
> > > > -                             USB_PORT_FEAT_SUSPEND);
> > > > +             if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND)
> > >
> > > You should test hub->hdev->quirks, here, not udev->quirks.  The quirk
> > > belongs to the Realtek hub, not to the device that's plugged into the
> > > hub.
> > >
> >
> > Thanks for pointing that out. I'll verify again and propose a V2 after
> > it's done.
>
> Another thing to consider: You shouldn't return 0 from usb_port_suspend
> if the port wasn't actually suspended.  We don't want to kernel to have
> a false idea of the hardware's current state.
>
So we still need the "really_suspend=false". What if I replace it with
the following?
It's a little verbose but expressive enough. Any suggestions?

+       else if (!(hub->hdev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND) &&
+               (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0))
+               status = set_port_feature(hub->hdev, port1,
+                               USB_PORT_FEAT_SUSPEND);
        else {
                really_suspend = false;
                status = 0;

Chris

> Alan Stern

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
  2021-04-14  5:07       ` Chris Chiu
@ 2021-04-14 14:32         ` Alan Stern
  2021-04-14 17:05           ` Chris Chiu
  0 siblings, 1 reply; 7+ messages in thread
From: Alan Stern @ 2021-04-14 14:32 UTC (permalink / raw)
  To: Chris Chiu; +Cc: gregkh, m.v.b, hadess, linux-usb, Linux Kernel

On Wed, Apr 14, 2021 at 01:07:43PM +0800, Chris Chiu wrote:
> Thanks for the instructions. I can hit the same timeout problem with
> runtime PM. The
> fail rate seems the same as normal PM. (around 1/4 ~ 1/7)
> root@:/sys/bus/usb/devices/3-4.3# echo auto > power/control
> root@:/sys/bus/usb/devices/3-4.3# echo on > power/control
> root@:/sys/bus/usb/devices/3-4.3# dmesg -c
> [ 2789.679807] usb 3-4: kworker/7:0 timed out on ep0out len=0/0
> [ 2789.679812] usb 3-4-port3: can't suspend, status -110
> [ 2789.680078] usb 3-4.3: Failed to suspend device, error -110

Since these are random failures, occurring at a low rate, maybe it would 
help simply to retry the transfer that timed out.  Have you tested this?

> > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > > > index 7f71218cc1e5..8478d49bba77 100644
> > > > > --- a/drivers/usb/core/hub.c
> > > > > +++ b/drivers/usb/core/hub.c
> > > > > @@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> > > > >        * descendants is enabled for remote wakeup.
> > > > >        */
> > > > >       else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
> > > > > -             status = set_port_feature(hub->hdev, port1,
> > > > > -                             USB_PORT_FEAT_SUSPEND);
> > > > > +             if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND)
> > > >
> > > > You should test hub->hdev->quirks, here, not udev->quirks.  The quirk
> > > > belongs to the Realtek hub, not to the device that's plugged into the
> > > > hub.
> > > >
> > >
> > > Thanks for pointing that out. I'll verify again and propose a V2 after
> > > it's done.
> >
> > Another thing to consider: You shouldn't return 0 from usb_port_suspend
> > if the port wasn't actually suspended.  We don't want to kernel to have
> > a false idea of the hardware's current state.
> >
> So we still need the "really_suspend=false". What if I replace it with
> the following?
> It's a little verbose but expressive enough. Any suggestions?
> 
> +       else if (!(hub->hdev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND) &&
> +               (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0))
> +               status = set_port_feature(hub->hdev, port1,
> +                               USB_PORT_FEAT_SUSPEND);
>         else {
>                 really_suspend = false;
>                 status = 0;

You should do something more like this:

-	else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
-		status = set_port_feature(hub->hdev, port1,
-				USB_PORT_FEAT_SUSPEND);
-	else {
+	else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0) {
+		if (hub->hdev->quirks & USB_QUIRK_NO_SUSPEND)
+			status = -EIO;
+		else
+			status = set_port_feature(hub->hdev, port1,
+					USB_PORT_FEAT_SUSPEND);
+	} else {
		really_suspend = false;
		status = 0;
	}

But I would prefer to find a way to make port suspend actually work, 
instead of giving up on it.

Alan Stern

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
  2021-04-14 14:32         ` Alan Stern
@ 2021-04-14 17:05           ` Chris Chiu
  0 siblings, 0 replies; 7+ messages in thread
From: Chris Chiu @ 2021-04-14 17:05 UTC (permalink / raw)
  To: Alan Stern; +Cc: gregkh, m.v.b, hadess, linux-usb, Linux Kernel

On Wed, Apr 14, 2021 at 10:32 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Apr 14, 2021 at 01:07:43PM +0800, Chris Chiu wrote:
> > Thanks for the instructions. I can hit the same timeout problem with
> > runtime PM. The
> > fail rate seems the same as normal PM. (around 1/4 ~ 1/7)
> > root@:/sys/bus/usb/devices/3-4.3# echo auto > power/control
> > root@:/sys/bus/usb/devices/3-4.3# echo on > power/control
> > root@:/sys/bus/usb/devices/3-4.3# dmesg -c
> > [ 2789.679807] usb 3-4: kworker/7:0 timed out on ep0out len=0/0
> > [ 2789.679812] usb 3-4-port3: can't suspend, status -110
> > [ 2789.680078] usb 3-4.3: Failed to suspend device, error -110
>
> Since these are random failures, occurring at a low rate, maybe it would
> help simply to retry the transfer that timed out.  Have you tested this?
>

The problem is the port seems to be dead (at least unresponsive) after
USB_PORT_FEAT_SUSPEND. If I turned on the xhci_hcd debug message,
I'll see lots of retries of the control URB as follows which never get acked
in failure cases.
[  126.616105] xhci_hcd 0000:04:00.3: ep 0x81 - asked for 2 bytes, 1
bytes untransferred

I tried to increase the timeout from 1 second to 2 second and also tried
set USB_PORT_FEAT_SUSPEND again after timeout, but still get timeout.
That also explains why hub_ext_port_status returns -71 (EPROTO from xhci)
in hub_activate() during resuming since the port is in an unknown state.

> > > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > > > > index 7f71218cc1e5..8478d49bba77 100644
> > > > > > --- a/drivers/usb/core/hub.c
> > > > > > +++ b/drivers/usb/core/hub.c
> > > > > > @@ -3329,8 +3329,11 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> > > > > >        * descendants is enabled for remote wakeup.
> > > > > >        */
> > > > > >       else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
> > > > > > -             status = set_port_feature(hub->hdev, port1,
> > > > > > -                             USB_PORT_FEAT_SUSPEND);
> > > > > > +             if (udev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND)
> > > > >
> > > > > You should test hub->hdev->quirks, here, not udev->quirks.  The quirk
> > > > > belongs to the Realtek hub, not to the device that's plugged into the
> > > > > hub.
> > > > >
> > > >
> > > > Thanks for pointing that out. I'll verify again and propose a V2 after
> > > > it's done.
> > >
> > > Another thing to consider: You shouldn't return 0 from usb_port_suspend
> > > if the port wasn't actually suspended.  We don't want to kernel to have
> > > a false idea of the hardware's current state.
> > >
> > So we still need the "really_suspend=false". What if I replace it with
> > the following?
> > It's a little verbose but expressive enough. Any suggestions?
> >
> > +       else if (!(hub->hdev->quirks & USB_QUIRK_NO_SET_FEAT_SUSPEND) &&
> > +               (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0))
> > +               status = set_port_feature(hub->hdev, port1,
> > +                               USB_PORT_FEAT_SUSPEND);
> >         else {
> >                 really_suspend = false;
> >                 status = 0;
>
> You should do something more like this:
>
> -       else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
> -               status = set_port_feature(hub->hdev, port1,
> -                               USB_PORT_FEAT_SUSPEND);
> -       else {
> +       else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0) {
> +               if (hub->hdev->quirks & USB_QUIRK_NO_SUSPEND)
> +                       status = -EIO;
> +               else
> +                       status = set_port_feature(hub->hdev, port1,
> +                                       USB_PORT_FEAT_SUSPEND);
> +       } else {
>                 really_suspend = false;
>                 status = 0;
>         }
>
> But I would prefer to find a way to make port suspend actually work,
> instead of giving up on it.
>
> Alan Stern

I tried to do that and also dig into the xhci code to check why the TD
(Transfer Descriptor) of the corresponding control msg URB was not
completed. Unfortunately, I didn't find a reasonable answer. I'll verify
the status -EIO and propose a v3 if everything's fine. Please let me
know if there's anything worth trying. Thanks.

Chris

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-04-14 17:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 15:00 [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub chris.chiu
2021-04-12 15:12 ` Alan Stern
2021-04-13  7:52   ` Chris Chiu
2021-04-13 14:44     ` Alan Stern
2021-04-14  5:07       ` Chris Chiu
2021-04-14 14:32         ` Alan Stern
2021-04-14 17:05           ` Chris Chiu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.