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

From: Chris Chiu <chris.chiu@canonical.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: gregkh@linuxfoundation.org, m.v.b@runbox.com, hadess@hadess.net,
	linux-usb@vger.kernel.org,
	Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] USB: Don't set USB_PORT_FEAT_SUSPEND on WD19's Realtek Hub
Date: Tue, 13 Apr 2021 15:52:14 +0800	[thread overview]
Message-ID: <CABTNMG1fvbOMrP+FmH0X5Yh04gf6vvhqhXfRrmpJ=f-fPBx4xw@mail.gmail.com> (raw)
In-Reply-To: <20210412151205.GB1420451@rowland.harvard.edu>

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