| Message ID | 20210412150006.53909-1-chris.chiu@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub | expand |
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
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
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
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
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
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
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 */