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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 06.08.2021 15:52, Rafael J. Wysocki wrote:
> On Wed, Aug 4, 2021 at 9:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> 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);
> 
> 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.
> 
Right, such proposals have been made. See e.g. here:
https://www.spinics.net/lists/linux-pci/msg103313.html

1. use dmi_get_bios_year() as indicator
2. Use ACPI version (major.minor) as indicator

Now it just takes a brave person who says:
There's no perfect indicator, let's go with the following and see
whether anybody complains.

>> 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