Re: [PATCH v5 4/4] PCI: Add runtime PM support for PCIe ports

[Lukas Wunner <lukas@wunner.de>]

On Fri, Jun 17, 2016 at 03:48:24PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 29, 2016 at 11:51:59AM +0300, Mika Westerberg wrote:
[snip]
> > +
> > +	/*
> > +	 * Prevent runtime PM if the port is advertising support for PCIe
> > +	 * hotplug. Otherwise the BIOS hotplug SMI code might not be able
> > +	 * to enumerate devices behind this port properly (the port is
> > +	 * powered down preventing all config space accesses to the
> > +	 * subordinate devices). We can't be sure for native PCIe hotplug
> > +	 * either so prevent that as well.
> 
> Can we wordsmith this comment a bit?  I'm not sure what "BIOS hotplug
> SMI code" even is or why it is relevant.  And it seems x86-centric so
> may not apply to other arches.

The comment pertains to:
https://bugzilla.kernel.org/show_bug.cgi?id=53811

It was discussed on April 12/13:
https://patchwork.kernel.org/patch/8782801/

Executive summary:
On non-Macs, Thunderbolt is driven by the firmware in System Management
Mode, on hotplug the CPU receives a Sytem Management Interrupt (SMI) and
jumps to SMM code. This doesn't work reliably if the hotplug port has
been transitioned to D3hot, the SMM code tries to access config space
of hotplugged devices (not possible if port is in D3hot) and is apparently
too dumb to check if the port is in D3hot and wake it up.

On Macs, Thunderbolt is not driven by the firmware except immediately on
boot: It is initially set up by an EFI driver, but once ExitBootServices
has been called, the OS is responsible. Therefore on Macs the hotplug
ports may be suspended to D3hot just fine. This is done by patch
[08/13] of my series "Runtime PM for Thunderbolt on Macs".

> 
> > +	 */
> > +	if (!dev->is_hotplug_bridge) {
> 
> Your changelog mentions ACPI hotplug (I assume this means acpiphp).
> Would it be sufficient to prevent runtime PM when we're using acpiphp?
> That would be more selective and would avoid penalizing non-ACPI
> platforms and newer systems that use native pciehp.
> 
> PCIe r3.0, sec 6.7.3.4 says a port should generate a wakeup event
> using PME on hotplug events that occur when the port is in D1, D2, or
> D3hot (it doesn't mention D3cold).  Is that relevant here?  Is there a
> way to control which D-states we use based on which ones can assert
> PME?

This was discussed between April 18 and April 21, please refer to
the above-linked patchwork entry.

I had done some tests and the result was:
"A hotplug port may go to D3hot and will still generate interrupts on
hotplug, but all its parent ports are *not* allowed to go to D3hot
because otherwise the hotplug interrupts will no longer come through.
The algorithm in pci_bridge_d3_update() and pci_dev_check_d3cold()
needs to be amended to take that into account. Hm, it's nontrivial
I guess, allowing bridge_d3 = true for the lowest hotplug bridge in a
hierarchy but not for anything above?"

Mika's reply was:
"If we need to do things like that, it will get pretty complex and we
still cannot be sure whether hotplug works. I think it is safer to go
back to what I already had and disable runtime PM from such ports."

Best regards,

Lukas