From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751933AbdBFMB0 (ORCPT ); Mon, 6 Feb 2017 07:01:26 -0500 Received: from cloudserver094114.home.net.pl ([79.96.170.134]:55575 "EHLO cloudserver094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751818AbdBFMBX (ORCPT ); Mon, 6 Feb 2017 07:01:23 -0500 From: "Rafael J. Wysocki" To: Lukas Wunner Cc: Bjorn Helgaas , Yinghai Lu , "Rafael J. Wysocki" , Mika Westerberg , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] PCI: pciehp: Don't enable PME on runtime suspend Date: Mon, 06 Feb 2017 12:56:36 +0100 Message-ID: <1585539.p8kZNrfTxC@aspire.rjw.lan> User-Agent: KMail/4.14.10 (Linux/4.10.0-rc3+; KDE/4.14.9; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, February 06, 2017 06:54:37 AM 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. 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, 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 > Cc: Rafael J. Wysocki > Cc: Mika Westerberg > Cc: Bjorn Helgaas > Signed-off-by: Lukas Wunner > --- > > v1-> v2: > Move check for is_hotplug_bridge from pci_finish_runtime_suspend() > down into pci_dev_run_wake(), this seems cleaner, less clumsy. ACK > > 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; Thanks, Rafael