Re: [PATCH] USB: hcd-pci: Fully suspend across freeze/thaw cycle

[Alan Stern]

On Thu, Apr 14, 2022 at 05:00:12PM +0300, Mathias Nyman wrote:
> On 12.4.2022 18.40, Alan Stern wrote:
> > On Tue, Apr 12, 2022 at 05:56:42PM +0300, Mathias Nyman wrote:
> >> On 11.4.2022 17.50, Alan Stern wrote:
> >>> For example, what would happen if the user unplugs a device right in the 
> >>> middle of the freeze transition, after the root hub has been frozen but 
> >>> before the controller is frozen?  We don't want such an unplug event to 
> >>> prevent the system from going into hibernation -- especially if the root 
> >>> hub was not enabled for wakeup.
> >>
> >> We should be able to let system go to hibernate even if we get a disconnect
> >> interrupt between roothub and host controller freeze.
> >> Host is not yet suspended so no PME# wake is generated, only an interrupt.
> >>
> >> From Linux PM point of view it should be ok as well as the actual xhci
> >> device that is generating the interrupt is hasnt completer freeze() 
> >>
> >> The xhci interrupt handler just needs to make sure that the disconnect
> >> isn't propagated if roothub is suspended and wake on disconnect
> >> is not set. And definitely make sure xhci doesn't start roothub polling. 
> >>
> >> When freeze() is called for the host we should prevent the host from 
> >> generating interrupts.
> > 
> > I guess that means adding a new callback.  Or we could just suspend the 
> > controller, like Evan proposed originally
> 
> Suspending the host in freeze should work.
> It will do an extra xhci controller state save stage, but that should be harmless.
> 
> But is there really a need for the suggested noirq part?
> 
> +	.freeze_noirq	= hcd_pci_suspend_noirq, 
> 
> That will try to set the host to PCI D3 state.
> It seems a bit unnecessary for freeze.

Agreed.

> >>> (If the root hub _is_ enabled for wakeup then it's questionable.  
> >>> Unplugging a device would be a wakeup event, so you could easily argue 
> >>> that it _should_ prevent the system from going into hibernation.  After 
> >>> all, if the unplug happened a few milliseconds later, after the system 
> >>> had fully gone into hibernation, then it would cause the system to wake 
> >>> up.)
> >>>
> >>>> Would it make sense prevent xHCI interrupt generation in the host
> >>>> freeze() stage, clearing the xHCI EINT bit in addition to calling 
> >>>> check_roothub_suspend()?
> >>>> Then enable it back in thaw()
> >>>
> >>> That won't fully eliminate the problem mentioned in the preceding 
> >>> paragraphs, although I guess it would help somewhat.
> >>
> >> Would the following steps solve this?
> >>
> >> 1. Disable device initiated resume for connected usb devices in freeze()
> >>
> >> 2. Don't propagate connect or OC changes if roothub is suspended and port wake
> >>    flags are disabled. I.E don't kick roothub polling in xhci interrupt
> >>    handler here.
> > 
> > I guess you can't just halt the entire host controller when only one of 
> > the root hubs is suspended with wakeup disabled.  That does complicate 
> > things.  But you could halt it as soon as both of the root hubs are 
> > frozen.  Wouldn't that prevent interrupt generation?
> 
> True, but probably easier to just suspend host in freeze() as you stated above.

Okay.

Evan, this discussion suggests that you rewrite your patch as a series 
of three:

     1. Change choose_wakeup() so that for PM_EVENT_FREEZE, wakeup is
	always disabled.

     2. Change the xhci-hcd interrupt handler so that port-status 
	changes are ignored if the port's root hub is suspended with 
	wakeup disabled.

     3. As in the original patch, make the .freeze and .thaw callbacks 
	in hcd-pci.c call the appropriate suspend and resume routines, 
	but don't do anything for .freeze_noirq and .thaw_noirq.

How does that sound?

Alan Stern