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
