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

["Rafael J. Wysocki" <rafael@kernel.org>]

On Tue, Jan 18, 2022 at 5:57 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 18.01.2022 17:28, Rafael J. Wysocki wrote:
> > On Mon, Jan 17, 2022 at 11:52 AM Heiner Kallweit <hkallweit1@gmail.com> 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.
> >>
> >> [0] https://lkml.org/lkml/2020/11/17/1548
> >>
> >> 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
> >
> > Well, there are two things here.
> >
> > First, there were systems in which ACPI PM was not ready for changing
> > power states in the S0 system state (ie. run-time) and it was assuming
> > that power states would only be changed during transitions to sleep
> > states (S1 - S4) and to S5.  This can be covered by the ACPI revicion
> > check, but I'm not sure if ACPI 5.0 is the right one.  Why ACPI 5 and
> > not ACPI 6, for instance?
> >
> Just based on the assumption that ACPI 5.0 should be recent enough.
> We can also go with ACPI 6.

I know that we can, the question is whether or not we should.

IOW, there needs to be at least some technical grounds on which to
assume that a given ACPI release is safe enough.

> > Second, there were PCI devices without ACPI PM where the PCI standard
> > PM didn't work correctly.  This is not related to ACPI at all and I'm
> > not sure why the ACPI revision check would be sufficient to cover
> > these cases.
> >
> Didn't know that there were such cases. Can you provide any examples or
> links to reports about such misbehaving devices?

Admittedly, I don't have a list of them, so I would need to look them
up and not just in the mailing lists.

> >>         pm_runtime_set_active(&dev->dev);
> >>         pm_runtime_enable(&dev->dev);
> >>         device_enable_async_suspend(&dev->dev);
> >> --

Also note that this change will allow PM-runtime to be used on PCI
devices without drivers by default and that may not be entirely safe
either.  It didn't work really well in the past IIRC, so I'm wondering
what's the reason to believe that it will work just fine this time.