Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Jean Delvare <jdelvare@suse.de>]

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.

-- 
Jean Delvare
SUSE L3 Support