linux-kernel - Re: Time to re-enable Runtime PM per default for PCI devcies?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9ca0fb46-9e65-31e2-103f-1c98ce8362c7@gmail.com>
Date:   Wed, 30 Dec 2020 23:56:04 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Bjorn Helgaas <helgaas@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Kai Heng Feng <kai.heng.feng@...onical.com>,
        Lukas Wunner <lukas@...ner.de>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: Time to re-enable Runtime PM per default for PCI devcies?

On 17.11.2020 17:57, Rafael J. Wysocki wrote:
> On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>>
>> [+to Rafael, author of the commit you mentioned,
>> +cc Mika, Kai Heng, Lukas, linux-pm, linux-kernel]
>>
>> On Tue, Nov 17, 2020 at 04:56:09PM +0100, Heiner Kallweit wrote:
>>> More than 10 yrs ago Runtime PM was disabled per default by bb910a7040
>>> ("PCI/PM Runtime: Make runtime PM of PCI devices inactive by default").
>>>
>>> Reason given: "avoid breakage on systems where ACPI-based wake-up is
>>> known to fail for some devices"
>>> Unfortunately the commit message doesn't mention any affected  devices
>>> or systems.
> 
> Even if it did that, it wouldn't have been a full list almost for sure.
> 
> We had received multiple problem reports related to that, most likely
> because the ACPI PM in BIOSes at that time was tailored for
> system-wide PM transitions only.
> 
>>> With Runtime PM disabled e.g. the PHY on network devices may remain
>>> powered up even with no cable plugged in, affecting battery lifetime
>>> on mobile devices. Currently we have to rely on the respective distro
>>> or user to enable Runtime PM via sysfs (echo auto > power/control).
>>> Some devices work around this restriction by calling pm_runtime_allow
>>> in their probe routine, even though that's not recommended by
>>> https://www.kernel.org/doc/Documentation/power/pci.txt
>>>
>>> Disabling Runtime PM per default seems to be a big hammer, a quirk
>>> for affected devices / systems may had been better. And we still
>>> have the option to disable Runtime PM for selected devices via sysfs.
>>>
>>> So, to cut a long story short: Wouldn't it be time to remove this
>>> restriction?
>>
>> I don't know the history of this, but maybe Rafael or the others can
>> shed some light on it.
> 
> The systems that had those problems 10 years ago would still have
> them, but I expect there to be more systems where runtime PM can be
> enabled by default for PCI devices without issues.
> 

As a proposal, maybe we can use the ACPI revision as an indicator for
whether ACPI implementation is new enough to not be affected by the old
problems. With the following simple patch runtime pm won't be disabled
per default for ACPI versions >= 6.0. AFAIK ACPI 6.0 was published in 2015.

On a side note:
It seems the sole motivation to disable runtime pm per default is ACPI
problems. So why do we disable runtime pm also on non-ACPI systems?
With the proposed patch runtime pm is enabled per default if
CONFIG_ACPI isn't defined.

---
 drivers/pci/pci-acpi.c | 5 +++++
 drivers/pci/pci.c      | 4 +++-
 drivers/pci/pci.h      | 5 +++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 53502a751..265f5d2c4 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -27,6 +27,11 @@ const guid_t pci_acpi_dsm_guid =
 	GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
 		  0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
 
+bool pci_acpi_forbid_runtime_pm(void)
+{
+	return acpi_gbl_FADT.header.revision < 6;
+}
+
 #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
 static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
 {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc25d..83b5a7e63 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3024,7 +3024,9 @@ void pci_pm_init(struct pci_dev *dev)
 	u16 status;
 	u16 pmc;
 
-	pm_runtime_forbid(&dev->dev);
+	if (pci_acpi_forbid_runtime_pm())
+		pm_runtime_forbid(&dev->dev);
+
 	pm_runtime_set_active(&dev->dev);
 	pm_runtime_enable(&dev->dev);
 	device_enable_async_suspend(&dev->dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5c5936509..c1d521fb2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -701,11 +701,16 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL
 
 #ifdef CONFIG_ACPI
 int pci_acpi_program_hp_params(struct pci_dev *dev);
+bool pci_acpi_forbid_runtime_pm(void);
 #else
 static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
 {
 	return -ENODEV;
 }
+static inline bool pci_acpi_forbid_runtime_pm(void)
+{
+	return false;
+}
 #endif
 
 #ifdef CONFIG_PCIEASPM
-- 
2.29.2