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

[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@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, Apr 12, 2021 at 11:00:06PM +0800, chris.chiu@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> > > ---
> >
> >
> > > 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