Re: [PATCH] PCI: Forbid RPM on ACPI systems before 5.0 only

[Heiner Kallweit <hkallweit1@gmail.com>]

On 18.01.2022 00:35, Bjorn Helgaas wrote:
> [+cc Kai-Heng, Lukas, Mika, since they were cc'd or commented on [0] below]
> 
> On Mon, Jan 17, 2022 at 11:51:54AM +0100, Heiner Kallweit wrote:
>> Currently PCI core forbids RPM and requires opt-in from userspace,
>> apart from few drivers calling pm_runtime_allow(). Reason is that some
>> early ACPI PM implementations conflict with RPM, see [0].
>> Note that as of today pm_runtime_forbid() is also called for non-ACPI
>> systems. Maybe it's time to allow RPM per default for non-ACPI systems
>> and recent enough ACPI versions. Let's allow RPM from ACPI 5.0 which
>> was published in 2011.
> 
> Let's reword this to use the positive sense, e.g., something like
> "enable runtime power management for non-ACPI and ACPI 5.0 and newer."
> 
> This feels like a potentially significant change that could cause
> breakage.  
> 
>   - How would a user recognize that we're doing something different?
>     Maybe we need a note in dmesg?
> 
>   - If a system broke because of this, what would it look like?  How
>     would a user notice a problem, and how would he or she connect the
>     problem to this change?
> 

Don't know what the exact symptoms of the original problem are.
I'd more see a certain risk that this change reveals bugs in RPM usage
of PCI device drivers. There's not a fixed list of potential symptoms.

One example: igb driver caused a hang on system shutdown when RPM was
enabled due to a RTNL deadlock in RPM resume path.

>   - Is there a kernel parameter that will get the previous behavior of
>     disabling runtime PM as a workaround until a quirk can be added?
>     If so, we should probably mention it here.  If not, should there
>     be?

For each device in sysfs: power/control: "auto" -> "on"

> 
>> [0] https://lkml.org/lkml/2020/11/17/1548
> 
> Please use an https://lore.kernel.org/r/... link instead.
> 
> Let's mention bb910a7040e9 ("PCI/PM Runtime: Make runtime PM of PCI
> devices inactive by default") as well to help connect the dots here.
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/pci/pci.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 428afd459..26e3a500c 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3101,7 +3101,12 @@ void pci_pm_init(struct pci_dev *dev)
>>  	u16 status;
>>  	u16 pmc;
>>  
>> -	pm_runtime_forbid(&dev->dev);
>> +#ifdef CONFIG_ACPI
>> +	/* Some early ACPI PM implementations conflict with RPM. */
>> +	if (acpi_gbl_FADT.header.revision > 0 &&
>> +	    acpi_gbl_FADT.header.revision < 5)
>> +		pm_runtime_forbid(&dev->dev);
>> +#endif
>>  	pm_runtime_set_active(&dev->dev);
>>  	pm_runtime_enable(&dev->dev);
>>  	device_enable_async_suspend(&dev->dev);
>> -- 
>> 2.34.1
>>