Re: [PATCH] PCI / PM: Don't runtime suspend when device only supports wakeup from D0

[Kai-Heng Feng]

at 04:52, Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > > > On Wed, May 22, 2019 at 11:42:14AM +0800, Kai Heng Feng wrote:
> > > > > > at 6:23 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> 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