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

[Chris Chiu]

On Thu, Apr 15, 2021 at 8:32 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Apr 15, 2021 at 07:48:56PM +0800, chris.chiu@xxxxxxxxxxxxx wrote:
> > From: Chris Chiu <chris.chiu@xxxxxxxxxxxxx>
> >
> > Realtek Hub (0bda:5487) 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)
>
> How does other operating systems handle this?  The hardware seems like
> it does not follow the USB spec, how did it get "certified"?
>
I agree. All usb hubs that claim to support Selective Scan should handle
SetPortFeature (PORT_SUSPEND) w/o problems. As I know, the Windows
usb device driver should submit an Idle Request before it's really going to
suspend, and the bus driver will "selectively" suspend the port.

One thing worth mentioning here, I never hit the hub_ext_port_status -71
problem if I resume by waking up from the keyboard connected to the hub.
But the usbcore kernel log shows me wPortStatus: 0503 wPortChane: 0004
of that port while resuming. In normal cases, they are 0507:0000.
 I don't know how to SetPortFeature() with setting the status change bit only.
Or maybe it's just some kind of timing issue of the
idle/suspend/resume signaling.

Chris

> > Information of this hub:
> > T:  Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 10 Spd=480  MxCh= 5
> > D:  Ver= 2.10 Cls=09(hub  ) Sub=00 Prot=02 MxPS=64 #Cfgs=  1
> > P:  Vendor=0bda ProdID=5487 Rev= 1.47
> > S:  Manufacturer=Dell Inc.
> > S:  Product=Dell dock
> > C:* #Ifs= 1 Cfg#= 1 Atr=e0 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 for the target port of the hub. The port
> > will be unresponsive and placed in unknown state. The hub_activate
> > invoked during resume will fail to get the port stautus with -EPROTO.
> > Then all devices connected to the hub will never be found and probed.
> >
> > 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.
>
> Can you make this only be allowed for hubs?  But why doesn't the nomal
> "this can not suspend properly" bit work for this?  We have that for
> other USB devices already.
>
> thanks,
>
> greg k-h