LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	Rafael Wysocki <rafael.j.wysocki@intel.com>,
	linux-pci@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	linux-usb@vger.kernel.org
Subject: Re: [PATCH] PCI / PM: Don't runtime suspend when device only supports wakeup from D0
Date: Thu, 23 May 2019 12:39:23 +0800	[thread overview]
Message-ID: <010C1D41-C66D-45C0-8AFF-6F746306CE29@canonical.com> (raw)
In-Reply-To: <20190522205231.GD79339@google.com>

at 04:52, Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Wed, May 22, 2019 at 02:39:56PM -0400, Alan Stern wrote:
>> On Wed, 22 May 2019, Bjorn Helgaas wrote:
>>> On Wed, May 22, 2019 at 11:46:25PM +0800, Kai Heng Feng wrote:
>>>>> On May 22, 2019, at 9:48 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Wed, May 22, 2019 at 11:42:14AM +0800, Kai Heng Feng wrote:
>>>>>> at 6:23 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>> On Wed, May 22, 2019 at 12:31:04AM +0800, Kai-Heng Feng wrote:
>>>>>>>> There's an xHC device that doesn't wake when a USB device gets  
>>>>>>>> plugged
>>>>>>>> to its USB port. The driver's own runtime suspend callback was  
>>>>>>>> called,
>>>>>>>> PME signaling was enabled, but it stays at PCI D0.
>>>
>>>>> ...
>>>>> And I guess this patch basically means we wouldn't call the driver's
>>>>> suspend callback if we're merely going to stay at D0, so the driver
>>>>> would have no idea anything happened.  That might match
>>>>> Documentation/power/pci.txt better, because it suggests that the
>>>>> suspend callback is related to putting a device in a low-power state,
>>>>> and D0 is not a low-power state.
>>>>
>>>> Yes, the patch is to let the device stay at D0 and don’t run driver’s  
>>>> own
>>>> runtime suspend routine.
>>>>
>>>> I guess I’ll just proceed to send a V2 with updated commit message?
>>>
>>> Now that I understand what "runtime suspended to D0" means, help me
>>> understand what's actually wrong.
>>
>> Kai's point is that the xhci-hcd driver thinks the device is now in
>> runtime suspend, because the runtime_suspend method has been executed.
>> But in fact the device is still in D0, and as a result, PME signalling
>> may not work correctly.
>
> The device claims to be able to signal PME from D0 (this is from the lspci
> in https://bugzilla.kernel.org/show_bug.cgi?id=203673):
>
>   00:10.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB XHCI Controller (rev 20) (prog-if 30 [XHCI])
>     Capabilities: [50] Power Management version 3
>       Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
>
> From the xHCI spec r1.0, sec 4.15.2.3, it looks like a connect
> detected while in D0 should assert PME# if enabled (and WCE is set).

I think section 4.15.2.3 is about S3 wake up, no S0 we are discussing here.

>
>> On the other hand, it wasn't clear from the patch description whether
>> this actually causes a problem on real systems.  The description only
>> said that the problem was theoretical.
>
> Kai did say nothing happens when hot-adding a USB device, so I think
> there really is a problem.  This should be an obvious problem that
> lots of people would trip over, so I expect there should be reports in
> launchpad, etc.  I'd really like to have those bread crumbs.  Kai, can
> you add a complete dmesg log to the bugzilla?  Hints from the log,
> like the platform name, can help find related reports.

It’s a platform in development so the name can’t be disclosed.

>
>>> The PCI core apparently *does* enable PME when we "suspend to D0".
>>> But somehow calling the xHCI runtime suspend callback makes the
>>> driver unable to notice when the PME is signaled?
>>
>> According to Kai, PME signalling doesn't work in D0 -- or at least,
>> it is _documented_ not to work in D0 -- even though it is enabled
>> and the device claims to support it.
>
> I didn't understand this part.  From a PCI perspective, PME signaling
> while in D0 is an optional feature and should work if the device
> advertises support for it.  If it doesn't work on this device, we
> should have a quirk to indicate that.

The only document I can find is the "Device Working State D0” from Microsoft.
It says:
"As a best practice, the driver should configure the device to generate  
interrupts only when the device is in D0, and to generate wake signals only  
when the device is in a low-power Dx state.”

Wake-up capability
Not applicable.

Unfortunately PCI spec isn’t publicly available so I can only refer to  
Microsoft document.

>
> But I thought Kai said the device *can* signal PME from D0, but for
> some reason we don't handle it correctly if we have called the xHCI
> suspend callback.

Sorry, what I meant is PME signaling is enabled, i.e.
"Status: D0 NoSoftRst+ PME-Enable+ DSel=0 DScale=0 PME-“

But no signal was actually regenerated when USB device gets plugged to the  
port.
So there’s no wake up event to let PCI know it should runtime resume the  
device.

>
> That's the part I don't understand.  Is this an xHCI driver issue?
> Should the suspend callback do something different if we're staying in
> D0?  I'm not sure the callback even knows what Dx state we're going
> to.

As there’s no PME signal to wakeup event to signal PCI to runtime resume, I  
don’t think it’s an xHCI bug.

Kai-Heng


  reply	other threads:[~2019-05-23  4:39 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21 16:31 Kai-Heng Feng
2019-05-21 22:23 ` Bjorn Helgaas
2019-05-22  3:42   ` Kai Heng Feng
2019-05-22 13:48     ` Bjorn Helgaas
2019-05-22 15:46       ` Kai Heng Feng
2019-05-22 18:11         ` Bjorn Helgaas
2019-05-22 18:39           ` Alan Stern
2019-05-22 18:53             ` Lukas Wunner
2019-05-22 19:05               ` Kai Heng Feng
2019-05-22 20:52             ` Bjorn Helgaas
2019-05-23  4:39               ` Kai-Heng Feng [this message]
2019-05-27 16:57                 ` Bjorn Helgaas
2019-06-05 11:57                   ` Bjorn Helgaas
2019-07-05  7:02                     ` Kai-Heng Feng
2019-07-05  9:39                       ` Rafael J. Wysocki
2019-07-05 13:51                         ` Kai-Heng Feng
2019-07-09 13:45                       ` Bjorn Helgaas
2019-09-02 13:47                         ` Kai-Heng Feng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=010C1D41-C66D-45C0-8AFF-6F746306CE29@canonical.com \
    --to=kai.heng.feng@canonical.com \
    --cc=helgaas@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=stern@rowland.harvard.edu \
    --subject='Re: [PATCH] PCI / PM: Don'\''t runtime suspend when device only supports wakeup from D0' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).