On Wed, Aug 4, 2021 at 9:02 PM Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>
> On 04.08.2021 16:06, Rafael J. Wysocki wrote:
> > On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
> > <jarkko.nikula@xxxxxxxxxxxxxxx> wrote:
> >>
> >> Hi
> >>
> >> On 8/2/21 7:31 PM, Heiner Kallweit wrote:
> >>> On 02.08.2021 14:53, Jean Delvare wrote:
> >>>> Hi Heiner,
> >>>>
> >>>> On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
> >>>>> Drivers should not call pm_runtime_allow(), see
> >>>>> Documentation/power/pci.rst. Therefore remove the call and leave this
> >>>>> to user space. Also remove the not needed call to pm_runtime_forbid().
> >>>>>
> >>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> >>>>> ---
> >>>>> drivers/i2c/busses/i2c-i801.c | 2 --
> >>>>> 1 file changed, 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >>>>> index 92ec291c0..362e74761 100644
> >>>>> --- a/drivers/i2c/busses/i2c-i801.c
> >>>>> +++ b/drivers/i2c/busses/i2c-i801.c
> >>>>> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>>>> pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
> >>>>> pm_runtime_use_autosuspend(&dev->dev);
> >>>>> pm_runtime_put_autosuspend(&dev->dev);
> >>>>> - pm_runtime_allow(&dev->dev);
> >>>>>
> >>>>> return 0;
> >>>>> }
> >>>>> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
> >>>>> {
> >>>>> struct i801_priv *priv = pci_get_drvdata(dev);
> >>>>>
> >>>>> - pm_runtime_forbid(&dev->dev);
> >>>>> pm_runtime_get_noresume(&dev->dev);
> >>>>>
> >>>>> i801_disable_host_notify(priv);
> >>>>
> >>>> These calls were added by Jarkko (Cc'd) and I'm not familiar with power
> >>>> management so I'll need an explicit ack from him before I can accept
> >>>> this patch.
> >>>>
> >>> The calls were part of the initial submission for rpm support and supposedly
> >>> just copied from another driver. But fine with me to wait for his feedback.
> >>>
> >> Yes, I'm quite sure I've copied it from another driver :-)
> >>
> >> This patch will cause the device here won't go automatically to D3
> >> before some user space script allows it. E.g
> >>
> >> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
> >>
> >> I think this is kind of PM regression with this patch. It's not clear to
> >> me from the Documentation/power/pci.rst why driver should not call the
> >> pm_runtime_allow() and what would be allowed kernel alternative for it.
> >
> > Please see the comment in local_pci_probe().
> >
> > Because the PCI bus type is involved in power management, the driver
> > needs to cooperate.
> >
> >> Rafael: what would be the correct way here to allow runtime PM from the
> >> driver or does it really require some user space script for it?
> >
> > No, it doesn't.
> >
>
> PCI core code includes the following because of historic issues
> with broken ACPI support on some platforms:
>
> void pci_pm_init(struct pci_dev *dev)
> {
> int pm;
> u16 status;
> u16 pmc;
>
> pm_runtime_forbid(&dev->dev);
> pm_runtime_set_active(&dev->dev);
> pm_runtime_enable(&dev->dev);
Well, thanks for reminding me about that!
> That's why RPM has to be enabled by userspace for PCI devices:
> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
Not really enabled, but rather "unlocked".
> Or drivers (that know that they can't be used on one of the broken
> platforms) call pm_runtime_allow(), what however is explicitly
> discouraged.
The problem here is that whether or not PM-runtime works in the given
configuration is not a property of a driver or an individual device,
but it depends on the platform.
Also if the driver is unbound from the device, the modified setting is
left behind it which isn't particularly nice.
> Not sure whether any of the old broken platforms is still relevant,
That's a good question, but it boils down to whether or not any of
them are still in use, which is hard to measure.
> therefore I started a discussion about it, which however ended
> w/o tangible result. See here:
> https://www.spinics.net/lists/linux-pci/msg103281.html
So I'm thinking that there could be a global flag accessible via a
kernel command line option, say pci_pm_runtime=allow/deny that would
allow the default behavior to be adjusted. Now, the default value of
that flag could depend on some heuristics, like the BIOS date or
whether or not the system has ACPI etc.
> I work around this restriction with the following in an init script,
> not sure how common distro's deal with this.
Some of them use powertop to do an equivalent of the loop below IIRC.
> # enable Runtime PM for all PCI devices
> for i in /sys/bus/pci/devices/*/power/control; do
> echo auto > $i
> done
