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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 04.08.2021 16:06, Rafael J. Wysocki wrote:
> On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
> <jarkko.nikula@linux.intel.com> 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@gmail.com>
>>>>> ---
>>>>>   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);

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