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

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

On Fri, Apr 16, 2021 at 09:24:30AM +0800, Chris Chiu wrote:
> On Fri, Apr 16, 2021 at 2:46 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, Apr 16, 2021 at 12:13:43AM +0800, Chris Chiu wrote:
> > > 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.
> >
> > I thought you said earlier that the port got into trouble while it was
> > suspending, not while it was resuming.  You wrote:
> >
> > > [ 2789.679812] usb 3-4-port3: can't suspend, status -110
> >
> > So if the problem occurs during suspend, how can it be possible to avoid
> > the problem by taking some particular action later while resuming?
> >
> 
> The ETIMEDOUT is still there, but the port can resume w/o problems and I
> don't really know what the hub did. I can only reset_resume the hub to bring it
> back if I'm not waking up from the connected keyboard.

What if two devices are plugged into the hub, only one of them is 
runtime suspended, and you need to runtime resume that device?  Then you 
can't do a reset-resume of the hub, because the hub isn't suspended.

> > > But the usbcore kernel log shows me wPortStatus: 0503 wPortChane: 0004
> > > of that port while resuming. In normal cases, they are 0507:0000.
> >
> > The 0004 bit of wPortChange means that the suspend status has changed;
> > the port is no longer suspended because the device attached to that port
> > (your keyboard) issued a wakeup request.
> >
> > >  I don't know how to SetPortFeature() with setting the status change bit only.
> >
> > You can't.  Only the hub itself can set the wPortChange bits.
> >
> > > Or maybe it's just some kind of timing issue of the
> > > idle/suspend/resume signaling.
> >
> > Not timing.  It's because you woke the system up from the attached
> > keyboard.
> >
> > Alan Stern
> 
> Got it. I'm just confused by the USB 2.0 spec 11.24.2.7.2 that
> "Hubs may allow setting of the status change bits with a SetPortFeature()
>  request for diagnostic purposes."

Yeah, I don't think very many hubs actually do that.

> So for this case, I think USB_QUIRK_RESET_RESUME would be a
> better option to at least bring back the port. Any suggestion about what
> kind of test I can do on this hub? Thanks

I'm not sure what you're proposing.

If (as mentioned above) the hub doesn't handle the 
Set-Port-Feature(suspend) request properly, then we should avoid issuing 
that request.  Which means runtime suspend attempts should not be 
allowed, as in your most recent patch.

Alan Stern