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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe0d3259-c60b-4ef8-aa42-edb5ca2e2d90@rowland.harvard.edu>
Date: Tue, 10 Sep 2024 09:13:45 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc: Mathias Nyman <mathias.nyman@...ux.intel.com>, hdegoede@...hat.com,
	ilpo.jarvinen@...ux.intel.com, gregkh@...uxfoundation.org,
	jorge.lopez2@...com, acelan.kao@...onical.com,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org
Subject: Re: [PATCH v3] platform/x86/hp: Avoid spurious wakeup on HP ProOne
 440

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@...land.harvard.edu> 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@...land.harvard.edu> 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