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

[Alan Stern]

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.

Alan Stern