On Tue, Jan 18, 2022 at 6:42 PM Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote: > > On 18.01.2022 18:11, Rafael J. Wysocki wrote: > > On Tue, Jan 18, 2022 at 5:57 PM Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote: > >> > >> On 18.01.2022 17:28, Rafael J. Wysocki wrote: > >>> On Mon, Jan 17, 2022 at 11:52 AM Heiner Kallweit <hkallweit1@xxxxxxxxx> 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@xxxxxxxxx> > >>>> --- > >>>> 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. > > > When ACPI 5 was published the workaround to disable RPM in general > was in place already. I'd assume that the majority of users does not > opt in for RPM, therefore it may be hard to find out whether any > system with ACPI 5 or ACPI 6 suffers from the same problem as the > affected old systems. Which kind of demonstrates the problem with the proposed approach which is based on speculation. > >>> 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. > > >From "Documentation/power/pci.rst": > If a PCI driver implements the runtime PM callbacks and intends to use the > runtime PM framework provided by the PM core and the PCI subsystem, it needs > to decrement the device's runtime PM usage counter in its probe callback > function. If it doesn't do that, the counter will always be different from > zero for the device and it will never be runtime-suspended. I'm not sure how this is related to what I said above. > Having said that I don't see how there can be a RPM-related problem w/o > the driver calling one of the RPM put functions. Maybe some of the problems > in the past were caused by PCI core bugs that have long been fixed. > > To reduce the risk we could enable RPM for a certain subset of PCI devices > only in a first step, e.g. for PCIe devices. I'm still not sure if runtime-suspending them when they have no drivers is a good idea. It might be better to somehow do the pm_runtime_allow() automatically in local_pci_probe() if the usage counter is 1 and power.runtime_auto is false after running the driver's ->probe() callback.
