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.
