Re: [PATCH v3] platform/x86/hp: Avoid spurious wakeup on HP ProOne 440

[Kai-Heng Feng]

On Tue, Sep 10, 2024 at 9:13 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Sep 10, 2024 at 11:33:02AM +0800, Kai-Heng Feng wrote:
> > On Mon, Sep 9, 2024 at 10:39 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 09, 2024 at 11:05:05AM +0800, Kai-Heng Feng wrote:
> > > > On Fri, Sep 6, 2024 at 10:22 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, Sep 06, 2024 at 01:30:47PM +0800, Kai-Heng Feng wrote:
> > > > > > The HP ProOne 440 has a power saving design that when the display is
> > > > > > off, it also cuts the USB touchscreen device's power off.
> > > > > >
> > > > > > This can cause system early wakeup because cutting the power off the
> > > > > > touchscreen device creates a disconnect event and prevent the system
> > > > > > from suspending:
> > > > >
> > > > > Is the touchscreen device connected directly to the root hub?  If it is
> > > > > then it looks like there's a separate bug here, which needs to be fixed.
> > > > >
> > > > > > [  445.814574] hub 2-0:1.0: hub_suspend
> > > > > > [  445.814652] usb usb2: bus suspend, wakeup 0
> > > > >
> > > > > Since the wakeup flag is set to 0, the root hub should not generate a
> > > > > wakeup request when a port-status-change event happens.
> > > >
> > > > The disconnect event itself should not generate a wake request, but
> > > > the interrupt itself still needs to be handled.
> > > >
> > > > >
> > > > > > [  445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0
> > > > > > [  445.824639] xhci_hcd 0000:00:14.0: resume root hub
> > > > >
> > > > > But it did.  This appears to be a bug in one of the xhci-hcd suspend
> > > > > routines.
> > >
> > > I failed to notice before that the suspend message message above is for
> > > bus 2 whereas the port change event here is on bus 1.  Nevertheless, I
> > > assume that bus 1 was suspended with wakeup = 0, so the idea is the
> > > same.
> >
> > Yes the bus 1 was already suspended.
> >
> > >
> > > > So should the xhci-hcd delay all interrupt handling after system resume?
> > >
> > > It depends on how the hardware works; I don't know the details.  The
> > > best approach would be: when suspending the root hub with wakeup = 0,
> > > the driver will tell the hardware not to generate interrupt requests for
> > > port-change events (and then re-enable those interrupt requests when the
> > > root hub is resumed, later on).
> >
> > So the XHCI_CMD_EIE needs to be cleared in prepare callback to ensure
> > there's no interrupt in suspend callback.
>
> Not in the prepare callback.  Clear it during the suspend callback.
>
> But now reading this and the earlier section, I realize what the problem
> is.  There's only one bit in the command register to control IRQ
> generation, so you can't turn off interrupt requests for bus 1 (the
> legacy USB-2 bus) without also turning them off for bus 2 (the USB-3
> bus).
>
> > Maybe this can be done, but this seems to greatly alter the xHCI suspend flow.
> Yes, this approach isn't feasible.
>
> > > If that's not possible, another possibility is that the driver could
> > > handle the interrupt and clear the hardware's port-change status bit but
> > > then not ask for the root hub to be resumed.  However, this would
> > > probably be more difficult to get right.
> >
> > IIUC the portsc status bit gets cleared after roothub is resumed. So
> > this also brings not insignificant change.
> > Not sure if its the best approach.
>
> It should be possible for this to work.  Just make the interrupt
> handler skip calling usb_hcd_resume_root_hub() if wakeup is not enabled
> for the root hub getting the port-status change.  When the root hub
> resumes as part of the system resume later on, the hub driver will check
> and see that a connect change occurred.

This can work. But should the change be made in
usb_hcd_resume_root_hub() or by the caller?
The issue can potentially happen to all USB controllers, not just xHCI.

Kai-Heng

>
> Alan Stern