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

From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Evan Green <evgreen@chromium.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	Rajat Jain <rajatja@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Bjorn Helgaas <bhelgaas@google.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Youngjin Jang <yj84.jang@samsung.com>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-usb@vger.kernel.org
Subject: Re: [PATCH] USB: hcd-pci: Fully suspend across freeze/thaw cycle
Date: Thu, 14 Apr 2022 17:00:12 +0300	[thread overview]
Message-ID: <b1df80e4-af6a-e84f-f49d-c74500bdec05@linux.intel.com> (raw)
In-Reply-To: <YlWdfWRXYjkfHLIP@rowland.harvard.edu>

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.

> 
>>> (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.

-Mathias