Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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
References: <20220407115918.1.I8226c7fdae88329ef70957b96a39b346c69a914e@changeid>
 <YlBGvFFSp/R2CBmh@rowland.harvard.edu>
 <CAE=gft7Zi9tpJ74Tf2iqPRbwJkmSLiKJt-WhwD+h-DxQh75D6g@mail.gmail.com>
 <YlDoSY19HYNJGI50@rowland.harvard.edu>
 <022a50ac-7866-2140-1b40-776255f3a036@linux.intel.com>
 <YlRATrMxRWt9gVqt@rowland.harvard.edu>
 <4353a956-9855-9c14-7dbf-bf16580abe32@linux.intel.com>
 <YlWdfWRXYjkfHLIP@rowland.harvard.edu>
From:   Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: Re: [PATCH] USB: hcd-pci: Fully suspend across freeze/thaw cycle
Message-ID: <b1df80e4-af6a-e84f-f49d-c74500bdec05@linux.intel.com>
Date:   Thu, 14 Apr 2022 17:00:12 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <YlWdfWRXYjkfHLIP@rowland.harvard.edu>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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