Hi Heiner,

On Wed, 4 Aug 2021 21:02:39 +0200, Heiner Kallweit wrote:

On 04.08.2021 16:06, Rafael J. Wysocki wrote:

quoted

On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula

quoted

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.
  

quoted

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