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

[Kai-Heng Feng]

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.
Maybe this can be done, but this seems to greatly alter the xHCI suspend flow.

>
> 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.

>
> Alan Stern