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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAAd53p67c0qQijUreu0AShsKucgPY03OQP+RGw=P7-vCV3Y6eg@mail.gmail.com>
Date: Thu, 12 Sep 2024 14:28:15 +0800
From: Kai-Heng Feng <kai.heng.feng@...onical.com>
To: Alan Stern <stern@...land.harvard.edu>
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, kaihengfeng@...il.com
Subject: Re: [PATCH v3] platform/x86/hp: Avoid spurious wakeup on HP ProOne 440

On Tue, Sep 10, 2024 at 9:13 PM Alan Stern <stern@...land.harvard.edu> 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@...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.

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