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

[Chris Chiu]

On Fri, Apr 16, 2021 at 11:39 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxxxxxx> 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

Sorry that I didn't make myself clear. I found that if I applied RESET_RESUME
quirk on the problematic hub, the Set-Port-Feature(suspend) timeout error
disappeared. SInce the timeout is not happening for each suspend by default,
I suspect maybe reset-resume take everything back to clean state for the hub
and the Set-Port-Feature(suspend) can be taken care of w/o problems.

I didn't like RESET_RESUME because runtime PM would not work on the quirked
device. But if the Set-Port-Feature(suspend) can't be handled and
skipped, I can't
expect the runtime PM to work for all devices connected to the hub either.
Is that right? If what I proposed in the patch can not get better
result than existing
quirk, I think using the RESET_RESUME would be a better option. Any suggestions?

Chris