Re: [PATCH] xhci: Disable connect, disconnect and over-current wakeup on system suspend

[Alan Stern]

On Fri, Aug 18, 2023 at 10:19:27PM +0800, Kai-Heng Feng wrote:
> On Fri, Aug 18, 2023 at 11:19 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Fri, Aug 18, 2023 at 08:00:39AM +0800, Kai-Heng Feng wrote:
> > > On Thu, Aug 17, 2023 at 10:07 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote:
> > > > > The system is designed to let display and touchpanel share the same
> > > > > power source, so when the display becomes off, the USB touchpanel also
> > > > > lost its power and disconnect itself from USB bus. That doesn't play
> > > > > well when most Desktop Environment lock and turnoff the display right
> > > > > before entering system suspend.
> > > >
> > > > I don't see why that should cause any trouble.  The display gets locked
> > > > and turned off, the touchpanel disconnects from the USB bus, and then
> > > > the system goes into suspend.  Why would there be a wakeup signal at
> > > > this point?
> > >
> > > The disconnecting can happens during the system suspend process, so
> > > the suspend process is aborted.
> >
> > Maybe these systems need to add a little delay when the display is
> > turned off, in order to give the touchpanel time to disconnect before
> > the system suspend begins.
> 
> Unfortunately the hardware can't be changed.

No, but the software can.  Change the kernel routine that controls the 
display's power, so that when it's turning the power off it delays a 
little before returning.


> > > > > So for system-wide suspend, also disable connect, disconnect and
> > > > > over-current wakeup to prevent spurious wakeup.
> > > >
> > > > Whether to disable these things is part of the userspace policy.  The
> > > > kernel should not make the decision; the user does by enabling or
> > > > disabling wakeups.
> > >
> > > The power/wakeup is already disabled.
> >
> > In that case the root hub should not generate a wakeup request in
> > response to the touchpanel disconnecting.
> 
> Here's the wakeup setting when the issue happens:
> controller - wakeup enabled
> root hub: wakeup disabled
> touchpanel: wakeup disabled

Your patch turns off the PORT_WAKE_BITS in xhci_disable_hub_port_wake(), 
right?  (Incidentally, the patch changes the code but doesn't change the 
immediately preceding comment, which will cause the comment to be 
wrong.)  But if the root hub was suspended with wakeup disabled then 
those bits should already be off.  So I don't see why your patch causes 
any change in behavior.


> > > The disconnecting event is from roothub and if roothub wakeup is
> > > disabled, other USB devices lose the ability to wake the system up
> > > from system suspend.
> >
> > That shouldn't happen either.  Disabling wakeup on the root hub should
> > not prevent the root hub from relaying wakeup requests it receives from
> > downstream devices.  It should merely prevent the root hub from
> > generating its own wakeup requests for connects, disconnects, and
> > over-current events.
> 
> Sorry, it was meant to be the xHCI controller. The didn't make the
> difference clear.
> 
> >
> > It sounds like the xhci root-hub code isn't doing the right thing, at
> > least, not on your systems.
> 
> I still don't fully understand why removing PORT_WAKE_BITS is not
> right in this case.

The decision about whether to turn those bits on or off should depend 
on the wakeup settings.  They should be on if wakeup is enabled and off 
if wakeup is disabled.  Your patch doesn't work that way; it turns the 
bits off even when wakeup is enabled.

Alan Stern