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