| Message ID | c0eeddf6-f630-d18b-cdae-0d74ed095f9c@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] i2c: i801: Don't call pm_runtime_allow | expand |
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. -- Jean Delvare SUSE L3 Support
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. Heiner
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. 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? Jarkko
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.
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
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
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. > 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
On Thu, Aug 5, 2021 at 10:31 AM Jean Delvare <jdelvare@suse.de> wrote: > > 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. I tend to agree with this judgement. Drivers that already call pm_runtime_allow() should be allowed to continue doing that or users will be confused. Calling pm_runtime_allow() from a PCI driver isn't nice with respect to user space, because if the latter doesn't want the device to runtime-suspend (whatever the reason), it needs to change the PM-runtime control setting whenever the driver is bound to the device, but that arguably is not a major problem. It would still be useful to have a way to adjust the default behavior for all PCI devices on a per-platform basis IMO.
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
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);
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(-)