On Thu, Aug 5, 2021 at 10:31 AM Jean Delvare <jdelvare@xxxxxxx> wrote:
>
> Hi Heiner,
>
> On Wed, 4 Aug 2021 21:02:39 +0200, Heiner Kallweit wrote:
> > On 04.08.2021 16:06, Rafael J. Wysocki wrote:
> > > On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
> > >> 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);
> >
> > 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
> >
> > 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.
> >
> > Not sure whether any of the old broken platforms is still relevant,
> > 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
> >
> > I work around this restriction with the following in an init script,
> > not sure how common distro's deal with this.
> >
> > # enable Runtime PM for all PCI devices
> > for i in /sys/bus/pci/devices/*/power/control; do
> > echo auto > $i
> > done
>
> FWIW, my distribution (openSUSE Leap 15.2) doesn't do anything with
> these attributes, basically leaving the decision to the drivers. As a
> result, your proposed patch leads to the following change for me:
>
> -/sys/bus/pci/devices/0000:00:1f.3/power/control:auto
> +/sys/bus/pci/devices/0000:00:1f.3/power/control:on
>
> I don't see that as an improvement.
>
> I also see that several other drivers I'm using (pcieport,
> snd_hda_intel, amdgpu) do enable runtime power management, so the
> i2c-i801 driver isn't an exception in this respect. Therefore I am not
> willing to accept this patch, sorry.
I tend to agree with this judgement. Drivers that already call
pm_runtime_allow() should be allowed to continue doing that or users
will be confused.
Calling pm_runtime_allow() from a PCI driver isn't nice with respect
to user space, because if the latter doesn't want the device to
runtime-suspend (whatever the reason), it needs to change the
PM-runtime control setting whenever the driver is bound to the device,
but that arguably is not a major problem.
It would still be useful to have a way to adjust the default behavior
for all PCI devices on a per-platform basis IMO.
