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

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

[Bjorn Helgaas]

[+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.
> 
> 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.

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

[Rafael J. Wysocki]

On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas <helgaas@kernel.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.

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

[Heiner Kallweit]

On 17.11.2020 17:57, Rafael J. Wysocki wrote:
> On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas <helgaas@kernel.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.
> 

To follow up on this discussion:
We could call pm_runtime_forbid() conditionally, e.g. with the following
condition. This would enable runtime pm per default for all non-ACPI
systems, and it uses the BIOS date as an indicator for a hopefully
not that broken ACPI implementation. However I could understand the
argument that this looks a little hacky ..

if (IS_ENABLED(CONFIG_ACPI) && dmi_get_bios_year() <= 2016)



>>> 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.
> 

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

[Kai-Heng Feng]

On Sat, Dec 26, 2020 at 11:26 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 17.11.2020 17:57, Rafael J. Wysocki wrote:
> > On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas <helgaas@kernel.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.
> >
>
> To follow up on this discussion:
> We could call pm_runtime_forbid() conditionally, e.g. with the following
> condition. This would enable runtime pm per default for all non-ACPI
> systems, and it uses the BIOS date as an indicator for a hopefully
> not that broken ACPI implementation. However I could understand the
> argument that this looks a little hacky ..
>
> if (IS_ENABLED(CONFIG_ACPI) && dmi_get_bios_year() <= 2016)

dmi_get_bios_year() may not be a good indicator. Last time I used it
caused regression, because the value changed after BIOS update.
For example, my MacBook Pro is manufactured in 2011, but
dmi_get_bios_year() returns 2018 with latest BIOS.

Kai-Heng

>
>
>
> >>> 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.
> >
>

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

[Heiner Kallweit]

On 29.12.2020 12:56, Kai-Heng Feng wrote:
> On Sat, Dec 26, 2020 at 11:26 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 17.11.2020 17:57, Rafael J. Wysocki wrote:
>>> On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas <helgaas@kernel.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.
>>>
>>
>> To follow up on this discussion:
>> We could call pm_runtime_forbid() conditionally, e.g. with the following
>> condition. This would enable runtime pm per default for all non-ACPI
>> systems, and it uses the BIOS date as an indicator for a hopefully
>> not that broken ACPI implementation. However I could understand the
>> argument that this looks a little hacky ..
>>
>> if (IS_ENABLED(CONFIG_ACPI) && dmi_get_bios_year() <= 2016)
> 
> dmi_get_bios_year() may not be a good indicator. Last time I used it
> caused regression, because the value changed after BIOS update.
> For example, my MacBook Pro is manufactured in 2011, but
> dmi_get_bios_year() returns 2018 with latest BIOS.
> 
Right, it's a weak indicator, and I'm aware of it. My thought is:
A massively broken ACPI implementation would have been fixed over
time if there are years between manufacturing date and last BIOS
update. Of course this doesn't have to be true ..

I just had a look at the ACPI spec and maybe we could use the ACPI
version information (major.minor) in the FADT table. E.g. we could
enable runtime pm from version 6.0. That should be reasonable new.
I'm open for any other proposals ..
 
> Kai-Heng
> 
>>
>>
>>
>>>>> 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.
>>>
>>

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

[Heiner Kallweit]

On 17.11.2020 17:57, Rafael J. Wysocki wrote:
> On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas <helgaas@kernel.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

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

[Lukas Wunner]

On Wed, Dec 30, 2020 at 11:56:04PM +0100, Heiner Kallweit wrote:
> --- 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);
> +

Generic PCI code usually does not call ACPI-specific functions directly,
but rather through a pci_platform_pm_ops callback.

FWIW, if platform_pci_power_manageable() returns true, it can probably
be assumed that allowing runtime PM by default is okay.  So as a first
step, you may want to call that instead of adding a new callback.

Thanks,

Lukas

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

[Heiner Kallweit]

On 31.12.2020 05:07, Lukas Wunner wrote:
> On Wed, Dec 30, 2020 at 11:56:04PM +0100, Heiner Kallweit wrote:
>> --- 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);
>> +
> 
> Generic PCI code usually does not call ACPI-specific functions directly,
> but rather through a pci_platform_pm_ops callback.
> 
> FWIW, if platform_pci_power_manageable() returns true, it can probably
> be assumed that allowing runtime PM by default is okay.  So as a first
> step, you may want to call that instead of adding a new callback.
> 
I don't think that's sufficient. Most likely all the broken old systems
return true for platform_pci_power_manageable(). So yes, most likely
we'd need a new callback if we want to have the platform ops abstraction.
But it could be an optional callback, something like: forbid_runtime_pm
The question is just: is it worth it?

By the way: pci_set_platform_pm() returns an error if a callback isn't
set, but no existing caller bothers to check the return code.

> Thanks,
> 
> Lukas
> 
Heiner

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

[Heiner Kallweit]

On 31.12.2020 10:38, Heiner Kallweit wrote:
> On 31.12.2020 05:07, Lukas Wunner wrote:
>> On Wed, Dec 30, 2020 at 11:56:04PM +0100, Heiner Kallweit wrote:
>>> --- 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);
>>> +
>>
>> Generic PCI code usually does not call ACPI-specific functions directly,
>> but rather through a pci_platform_pm_ops callback.
>>
>> FWIW, if platform_pci_power_manageable() returns true, it can probably
>> be assumed that allowing runtime PM by default is okay.  So as a first
>> step, you may want to call that instead of adding a new callback.
>>
> I don't think that's sufficient. Most likely all the broken old systems
> return true for platform_pci_power_manageable(). So yes, most likely
> we'd need a new callback if we want to have the platform ops abstraction.
> But it could be an optional callback, something like: forbid_runtime_pm
> The question is just: is it worth it?
> 
> By the way: pci_set_platform_pm() returns an error if a callback isn't
> set, but no existing caller bothers to check the return code.
> 
>> Thanks,
>>
>> Lukas
>>
> Heiner
> 

This would be the version with the abstraction.

---
 drivers/pci/pci-acpi.c | 6 ++++++
 drivers/pci/pci.c      | 9 ++++++++-
 drivers/pci/pci.h      | 5 ++++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 53502a751..b57a79af7 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1108,6 +1108,11 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)
 	return !!adev->power.flags.dsw_present;
 }
 
+static bool acpi_pci_forbid_runtime_pm(void)
+{
+	return acpi_gbl_FADT.header.revision < 6;
+}
+
 static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
 	.bridge_d3 = acpi_pci_bridge_d3,
 	.is_manageable = acpi_pci_power_manageable,
@@ -1117,6 +1122,7 @@ static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
 	.choose_state = acpi_pci_choose_state,
 	.set_wakeup = acpi_pci_wakeup,
 	.need_resume = acpi_pci_need_resume,
+	.forbid_runtime_pm = acpi_pci_forbid_runtime_pm,
 };
 
 void acpi_pci_add_bus(struct pci_bus *bus)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc25d..3af832b7f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -982,6 +982,12 @@ static inline bool platform_pci_need_resume(struct pci_dev *dev)
 	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
 }
 
+static inline bool platform_pci_forbid_runtime_pm(void)
+{
+	return pci_platform_pm && pci_platform_pm->forbid_runtime_pm ?
+	       pci_platform_pm->forbid_runtime_pm() : false;
+}
+
 static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
 {
 	if (pci_platform_pm && pci_platform_pm->bridge_d3)
@@ -3024,7 +3030,8 @@ void pci_pm_init(struct pci_dev *dev)
 	u16 status;
 	u16 pmc;
 
-	pm_runtime_forbid(&dev->dev);
+	if (platform_pci_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..d2d406ee4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -71,8 +71,10 @@ int pci_bus_error_reset(struct pci_dev *dev);
  *		 suspended) needs to be resumed to be configured for system
  *		 wakeup.
  *
+ * @forbid_runtime_pm: returns true in case of known issues breaking runtime pm
+ *
  * If given platform is generally capable of power managing PCI devices, all of
- * these callbacks are mandatory.
+ * these callbacks are mandatory (except forbid_runtime_pm).
  */
 struct pci_platform_pm_ops {
 	bool (*bridge_d3)(struct pci_dev *dev);
@@ -83,6 +85,7 @@ struct pci_platform_pm_ops {
 	pci_power_t (*choose_state)(struct pci_dev *dev);
 	int (*set_wakeup)(struct pci_dev *dev, bool enable);
 	bool (*need_resume)(struct pci_dev *dev);
+	bool (*forbid_runtime_pm)(void);
 };
 
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
-- 
2.29.2

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

[Lukas Wunner]

On Thu, Dec 31, 2020 at 10:38:12AM +0100, Heiner Kallweit wrote:
> On 31.12.2020 05:07, Lukas Wunner wrote:
> > FWIW, if platform_pci_power_manageable() returns true, it can probably
> > be assumed that allowing runtime PM by default is okay.  So as a first
> > step, you may want to call that instead of adding a new callback.
> 
> I don't think that's sufficient. Most likely all the broken old systems
> return true for platform_pci_power_manageable().

platform_pci_power_manageable() is not a global flag, but rather
a per-device flag whether the platform is capable of power-managing
that device.  E.g. for the ACPI platform, it indicates that objects
such as _PS0 or _PS3 are present in the device's namespace.

My point is that if the platform can power-manage a device,
then it ought to be safe to enable runtime PM by default for it.

If you insist on a "big hammer" approach by turning on runtime PM
by default for everything, you risk regressions.  You can avoid
that by going for a smart approach which enables runtime PM in
cases when it's safe.

Thanks,

Lukas

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

[Heiner Kallweit]

On 04.01.2021 18:39, Lukas Wunner wrote:
> On Thu, Dec 31, 2020 at 10:38:12AM +0100, Heiner Kallweit wrote:
>> On 31.12.2020 05:07, Lukas Wunner wrote:
>>> FWIW, if platform_pci_power_manageable() returns true, it can probably
>>> be assumed that allowing runtime PM by default is okay.  So as a first
>>> step, you may want to call that instead of adding a new callback.
>>
>> I don't think that's sufficient. Most likely all the broken old systems
>> return true for platform_pci_power_manageable().
> 
> platform_pci_power_manageable() is not a global flag, but rather
> a per-device flag whether the platform is capable of power-managing
> that device.  E.g. for the ACPI platform, it indicates that objects
> such as _PS0 or _PS3 are present in the device's namespace.
> 
> My point is that if the platform can power-manage a device,
> then it ought to be safe to enable runtime PM by default for it.
> 
Not sure about that. Just that the BIOS claims it can power-manage
the device, doesn't rule out that it's broken and fails to do so.

> If you insist on a "big hammer" approach by turning on runtime PM
> by default for everything, you risk regressions.  You can avoid
> that by going for a smart approach which enables runtime PM in
> cases when it's safe.
> 
> Thanks,
> 
> Lukas
> 
Heiner

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

[Lukas Wunner]

> On Tue, Nov 17, 2020 at 04:56:09PM +0100, Heiner Kallweit wrote:
> > 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.

Removing the recommendation in pci.txt and allowing runtime PM in more
drivers by default seems sensible to me.  Such an incremental approach
is less risky with regards to regressions than a big hammer.  Heiner,
care to submit patches to that effect?

Thanks,

Lukas