Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub

[Alan Stern <stern@rowland.harvard.edu>]

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