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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b1df80e4-af6a-e84f-f49d-c74500bdec05@linux.intel.com>
Date:   Thu, 14 Apr 2022 17:00:12 +0300
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Evan Green <evgreen@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Rajat Jain <rajatja@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Youngjin Jang <yj84.jang@...sung.com>,
        LKML <linux-kernel@...r.kernel.org>, linux-usb@...r.kernel.org
Subject: Re: [PATCH] USB: hcd-pci: Fully suspend across freeze/thaw cycle

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