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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jan 18, 2022 at 09:06:12AM +0100, Heiner Kallweit wrote:
> 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"

Thanks.  In case it wasn't clear, this is information that I would
like to have in the commit log so that if anybody *does* see a
problem, there's a hint about how to debug it and work around it.

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