Re: [PATCH v2] PCI: pciehp: Don't enable PME on runtime suspend

[Bjorn Helgaas]

On Mon, Feb 06, 2017 at 06:54:37AM +0100, Lukas Wunner wrote:
> Since commit 68db9bc81436 ("PCI: pciehp: Add runtime PM support for PCIe
> hotplug ports") we runtime suspend a hotplug port to D3hot when all its
> children are runtime suspended or none are present.
> 
> When runtime suspending the port the PCI core automatically enables PME:
>     pci_pm_runtime_suspend()
>         pci_finish_runtime_suspend()
>             __pci_enable_wake()
> 
> According to the PCI Express Base Specification, section 6.7.3.4:
>    "Note that PME and Hot-Plug Event interrupts (when both are
>     implemented) always share the same MSI or MSI-X vector [...]
>     If wake generation is required by the associated form factor
>     specification, a hot-plug capable Downstream Port must support
>     generation of a wakeup event (using the PME mechanism) on hotplug
>     events that occur when the system is in a sleep state or the Port
>     is in device state D1, D2, or D3Hot."
> 
> Thus, if the port is runtime suspended even though it is still occupied,
> it may immediately be woken by a PME interrupt.  

The spec goes on to say that a wakeup event should be generated when
all three of these conditions occur:

  - status register for an enabled [hotplug] event transitions from
    not set to set

  - Port is in D1, D2, or D3hot,

  - PME_En is set

I think you're saying that if we put a hotplug-capable port that
controls an occupied slot into D3hot, the port may immediately
generate a wakeup PME.

What is the hotplug event that causes generation of this wakeup event?

> One scenario where this
> happens is if all children of the hotplug port have runtime suspended.
> Another scenario is power control via sysfs:  If a user manually turns
> the hotplug port off (e.g. to safely remove the card), PME will signal
> an interrupt for the still-occupied slot, which is interpreted by pciehp
> as re-insertion of a card.  As a result, power control via sysfs is no
> longer possible.  This was observed and reported by Yinghai Lu.
> 
> PME is in fact unnecessary on hotplug ports:  Hotplug can be signaled
> even in D3hot,

How are hotplug events signaled in D3hot without using PME?  I'm
looking at the PCI PM spec, r1.2, table 5-4 (p 49 in my copy), which
says a function in D3hot can only generate PME.

> and commit 68db9bc81436 ensures that all parents of the
> hotplug port are kept awake so that interrupts can be delivered.
> PME would allow us to runtime suspend the parent ports as well, but we
> do not make use of it because we cannot be sure if PME actually works.
> Thunderbolt controllers for instance advertise PME capability, but at
> least on Macs the PME pin is not connected.
> 
> Since we do not rely on PME for hotplug ports, we may as well not enable
> it, thereby avoiding its negative side effects.  However the present
> commit deliberately only avoids enabling PME on runtime suspend, the
> ability to enable it for system sleep is retained.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193951
> Fixes: 68db9bc81436 ("PCI: pciehp: Add runtime PM support for PCIe
>     hotplug ports")
> Reported-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> ---
> 
> v1-> v2:
>  Move check for is_hotplug_bridge from pci_finish_runtime_suspend()
>  down into pci_dev_run_wake(), this seems cleaner, less clumsy.
> 
>  drivers/pci/pci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a881c0d..9c22e62 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2096,6 +2096,14 @@ bool pci_dev_run_wake(struct pci_dev *dev)
>  {
>  	struct pci_bus *bus = dev->bus;
>  
> +	/*
> +	 * Don't enable PME at runtime on hotplug ports (even if supported)
> +	 * since PME sends unwanted interrupts if the slot is occupied while
> +	 * suspended to D3hot (PCIe Base Specification, section 6.7.3.4).
> +	 */
> +	if (dev->is_hotplug_bridge)
> +		return false;
> +
>  	if (device_run_wake(&dev->dev))
>  		return true;
>  
> -- 
> 2.11.0
>