[PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Douglas Anderson]

The PM Runtime docs say:
  Drivers in ->remove() callback should undo the runtime PM changes done
  in ->probe(). Usually this means calling pm_runtime_disable(),
  pm_runtime_dont_use_autosuspend() etc.

From grepping code, it's clear that many people aren't aware of the
need to call pm_runtime_dont_use_autosuspend().

When brainstorming solutions, one idea that came up was to leverage
the new-ish devm_pm_runtime_enable() function. The idea here is that:
* When the devm action is called we know that the driver is being
  removed. It's the perfect time to undo the use_autosuspend.
* The code of pm_runtime_dont_use_autosuspend() already handles the
  case of being called when autosuspend wasn't enabled.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/base/power/runtime.c | 5 +++++
 include/linux/pm_runtime.h   | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 2f3cce17219b..d4059e6ffeae 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1476,11 +1476,16 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
 
 static void pm_runtime_disable_action(void *data)
 {
+	pm_runtime_dont_use_autosuspend(data);
 	pm_runtime_disable(data);
 }
 
 /**
  * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.
+ *
+ * NOTE: this will also handle calling pm_runtime_dont_use_autosuspend() for
+ * you at driver exit time if needed.
+ *
  * @dev: Device to handle.
  */
 int devm_pm_runtime_enable(struct device *dev)
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 9f09601c465a..2bff6a10095d 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -567,6 +567,10 @@ static inline void pm_runtime_disable(struct device *dev)
  * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
  * requested (or "autosuspend" will be handled as direct runtime-suspend for
  * it).
+ *
+ * NOTE: It's important to undo this with pm_runtime_dont_use_autosuspend()
+ * at driver exit time unless your driver initially enabled pm_runtime
+ * with devm_pm_runtime_enable() (which handles it for you).
  */
 static inline void pm_runtime_use_autosuspend(struct device *dev)
 {
-- 
2.35.1.473.g83b2b277ed-goog

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Ulf Hansson]

On Wed, 23 Feb 2022 at 17:35, Douglas Anderson <dianders@chromium.org> wrote:
>
> The PM Runtime docs say:
>   Drivers in ->remove() callback should undo the runtime PM changes done
>   in ->probe(). Usually this means calling pm_runtime_disable(),
>   pm_runtime_dont_use_autosuspend() etc.
>
> From grepping code, it's clear that many people aren't aware of the
> need to call pm_runtime_dont_use_autosuspend().

Well, I admit it's good practice that they should take care of this.

However, it doesn't really matter to keep the autosuspend turned on
when runtime PM becomes disabled, I think. When the driver gets probed
again, it will most likely call pm_runtime_use_autosuspend() again,
which should work fine, right?

>
> When brainstorming solutions, one idea that came up was to leverage
> the new-ish devm_pm_runtime_enable() function. The idea here is that:
> * When the devm action is called we know that the driver is being
>   removed. It's the perfect time to undo the use_autosuspend.
> * The code of pm_runtime_dont_use_autosuspend() already handles the
>   case of being called when autosuspend wasn't enabled.

Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
currently makes it look too simple to turn off things at ->remove()
for runtime PM. While in fact it's more complicated.

A bigger problem, for example, is that a driver calls
pm_runtime_put_sync() during ->remove(), relying on that it actually
ends up calling its ->runtime_suspend() callback to turn off various
specific resources for the device. And in fact there are no guarantees
that will happen - and when it doesn't, the next time the driver's
->probe() runs, things are likely to be really screwed up.

To cover this case, one could use the below code in the ->remove() callback:

...
pm_runtime_get_sync();

"turn off resources for the devices - like calling
clk_disable_unprepare(), for example"

pm_runtime_disable();
pm_runtime_put_noidle();
...

In this example, it would be too late to call pm_runtime_disable()
through the pm_runtime_disable_action().

Kind regards
Uffe

>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/base/power/runtime.c | 5 +++++
>  include/linux/pm_runtime.h   | 4 ++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 2f3cce17219b..d4059e6ffeae 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1476,11 +1476,16 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
>
>  static void pm_runtime_disable_action(void *data)
>  {
> +       pm_runtime_dont_use_autosuspend(data);
>         pm_runtime_disable(data);
>  }
>
>  /**
>   * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.
> + *
> + * NOTE: this will also handle calling pm_runtime_dont_use_autosuspend() for
> + * you at driver exit time if needed.
> + *
>   * @dev: Device to handle.
>   */
>  int devm_pm_runtime_enable(struct device *dev)
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 9f09601c465a..2bff6a10095d 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -567,6 +567,10 @@ static inline void pm_runtime_disable(struct device *dev)
>   * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
>   * requested (or "autosuspend" will be handled as direct runtime-suspend for
>   * it).
> + *
> + * NOTE: It's important to undo this with pm_runtime_dont_use_autosuspend()
> + * at driver exit time unless your driver initially enabled pm_runtime
> + * with devm_pm_runtime_enable() (which handles it for you).
>   */
>  static inline void pm_runtime_use_autosuspend(struct device *dev)
>  {
> --
> 2.35.1.473.g83b2b277ed-goog
>

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Laurent Pinchart]

Hi Ulf,

On Tue, Mar 01, 2022 at 11:26:46AM +0100, Ulf Hansson wrote:
> On Wed, 23 Feb 2022 at 17:35, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > The PM Runtime docs say:
> >   Drivers in ->remove() callback should undo the runtime PM changes done
> >   in ->probe(). Usually this means calling pm_runtime_disable(),
> >   pm_runtime_dont_use_autosuspend() etc.
> >
> > From grepping code, it's clear that many people aren't aware of the
> > need to call pm_runtime_dont_use_autosuspend().
> 
> Well, I admit it's good practice that they should take care of this.
> 
> However, it doesn't really matter to keep the autosuspend turned on
> when runtime PM becomes disabled, I think. When the driver gets probed
> again, it will most likely call pm_runtime_use_autosuspend() again,
> which should work fine, right?

For the probe path I agree, but are there valid use cases where, at
runtime, a driver would disable runtime PM and re-enable it a bit later
? If so, we need to ensure this won't disable auto-suspend.

> > When brainstorming solutions, one idea that came up was to leverage
> > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > * When the devm action is called we know that the driver is being
> >   removed. It's the perfect time to undo the use_autosuspend.
> > * The code of pm_runtime_dont_use_autosuspend() already handles the
> >   case of being called when autosuspend wasn't enabled.
> 
> Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> currently makes it look too simple to turn off things at ->remove()
> for runtime PM. While in fact it's more complicated.
> 
> A bigger problem, for example, is that a driver calls
> pm_runtime_put_sync() during ->remove(), relying on that it actually
> ends up calling its ->runtime_suspend() callback to turn off various
> specific resources for the device. And in fact there are no guarantees
> that will happen - and when it doesn't, the next time the driver's
> ->probe() runs, things are likely to be really screwed up.
> 
> To cover this case, one could use the below code in the ->remove() callback:
> 
> ...
> pm_runtime_get_sync();
> 
> "turn off resources for the devices - like calling
> clk_disable_unprepare(), for example"
> 
> pm_runtime_disable();
> pm_runtime_put_noidle();
> ...
> 
> In this example, it would be too late to call pm_runtime_disable()
> through the pm_runtime_disable_action().

My experience with runtime PM is that it's hard to use, at least if you
want to get it right :-) That's especially the case if a driver wants to
support both CONFIG_PM and !CONFIG_PM. Here's an example at probe time:

	/*
	 * We need the driver to work in the event that CONFIG_PM is disabled in
	 * the kernel, so power up and verify the chip now. In the event that
	 * CONFIG_PM is disabled this will leave the chip on, so that streaming
	 * will work.
	 */
	ret = ov5693_sensor_powerup(ov5693);
	if (ret)
		goto err_media_entity_cleanup;

	ret = ov5693_detect(ov5693);
	if (ret)
		goto err_powerdown;

	pm_runtime_set_active(&client->dev);
	pm_runtime_get_noresume(&client->dev);
	pm_runtime_enable(&client->dev);

	ret = v4l2_async_register_subdev_sensor(&ov5693->sd);
	if (ret) {
		dev_err(&client->dev, "failed to register V4L2 subdev: %d",
			ret);
		goto err_pm_runtime;
	}

	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
	pm_runtime_use_autosuspend(&client->dev);
	pm_runtime_put_autosuspend(&client->dev);

And the corresponding code at remove time:

	/*
	 * Disable runtime PM. In case CONFIG_PM is disabled in the kernel,
	 * make sure to turn power off manually.
	 */
	pm_runtime_disable(&client->dev);
	if (!pm_runtime_status_suspended(&client->dev))
		ov5693_sensor_powerdown(ov5693);
	pm_runtime_set_suspended(&client->dev);

And of course there's no documentation that explains all this, so there
are endless variations of patterns originating from cargo-cult
programming.

I don't know what the right solution is, but we need to move towards an
easier to use API if we want drivers to get it right. Any step in that
direction would be welcome.

> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  drivers/base/power/runtime.c | 5 +++++
> >  include/linux/pm_runtime.h   | 4 ++++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 2f3cce17219b..d4059e6ffeae 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1476,11 +1476,16 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> >
> >  static void pm_runtime_disable_action(void *data)
> >  {
> > +       pm_runtime_dont_use_autosuspend(data);
> >         pm_runtime_disable(data);
> >  }
> >
> >  /**
> >   * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.
> > + *
> > + * NOTE: this will also handle calling pm_runtime_dont_use_autosuspend() for
> > + * you at driver exit time if needed.
> > + *
> >   * @dev: Device to handle.
> >   */
> >  int devm_pm_runtime_enable(struct device *dev)
> > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > index 9f09601c465a..2bff6a10095d 100644
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -567,6 +567,10 @@ static inline void pm_runtime_disable(struct device *dev)
> >   * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
> >   * requested (or "autosuspend" will be handled as direct runtime-suspend for
> >   * it).
> > + *
> > + * NOTE: It's important to undo this with pm_runtime_dont_use_autosuspend()
> > + * at driver exit time unless your driver initially enabled pm_runtime
> > + * with devm_pm_runtime_enable() (which handles it for you).
> >   */
> >  static inline void pm_runtime_use_autosuspend(struct device *dev)
> >  {

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Ulf Hansson]

On Tue, 1 Mar 2022 at 11:49, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ulf,
>
> On Tue, Mar 01, 2022 at 11:26:46AM +0100, Ulf Hansson wrote:
> > On Wed, 23 Feb 2022 at 17:35, Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > The PM Runtime docs say:
> > >   Drivers in ->remove() callback should undo the runtime PM changes done
> > >   in ->probe(). Usually this means calling pm_runtime_disable(),
> > >   pm_runtime_dont_use_autosuspend() etc.
> > >
> > > From grepping code, it's clear that many people aren't aware of the
> > > need to call pm_runtime_dont_use_autosuspend().
> >
> > Well, I admit it's good practice that they should take care of this.
> >
> > However, it doesn't really matter to keep the autosuspend turned on
> > when runtime PM becomes disabled, I think. When the driver gets probed
> > again, it will most likely call pm_runtime_use_autosuspend() again,
> > which should work fine, right?
>
> For the probe path I agree, but are there valid use cases where, at
> runtime, a driver would disable runtime PM and re-enable it a bit later
> ? If so, we need to ensure this won't disable auto-suspend.

I am not sure I fully understand whether there is a problem.

Can you perhaps write the sequence of the runtime PM calls that may
cause an issue?

>
> > > When brainstorming solutions, one idea that came up was to leverage
> > > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > > * When the devm action is called we know that the driver is being
> > >   removed. It's the perfect time to undo the use_autosuspend.
> > > * The code of pm_runtime_dont_use_autosuspend() already handles the
> > >   case of being called when autosuspend wasn't enabled.
> >
> > Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> > currently makes it look too simple to turn off things at ->remove()
> > for runtime PM. While in fact it's more complicated.
> >
> > A bigger problem, for example, is that a driver calls
> > pm_runtime_put_sync() during ->remove(), relying on that it actually
> > ends up calling its ->runtime_suspend() callback to turn off various
> > specific resources for the device. And in fact there are no guarantees
> > that will happen - and when it doesn't, the next time the driver's
> > ->probe() runs, things are likely to be really screwed up.
> >
> > To cover this case, one could use the below code in the ->remove() callback:
> >
> > ...
> > pm_runtime_get_sync();
> >
> > "turn off resources for the devices - like calling
> > clk_disable_unprepare(), for example"
> >
> > pm_runtime_disable();
> > pm_runtime_put_noidle();
> > ...
> >
> > In this example, it would be too late to call pm_runtime_disable()
> > through the pm_runtime_disable_action().
>
> My experience with runtime PM is that it's hard to use, at least if you
> want to get it right :-) That's especially the case if a driver wants to
> support both CONFIG_PM and !CONFIG_PM. Here's an example at probe time:
>
>         /*
>          * We need the driver to work in the event that CONFIG_PM is disabled in
>          * the kernel, so power up and verify the chip now. In the event that
>          * CONFIG_PM is disabled this will leave the chip on, so that streaming
>          * will work.
>          */
>         ret = ov5693_sensor_powerup(ov5693);
>         if (ret)
>                 goto err_media_entity_cleanup;
>
>         ret = ov5693_detect(ov5693);
>         if (ret)
>                 goto err_powerdown;
>
>         pm_runtime_set_active(&client->dev);
>         pm_runtime_get_noresume(&client->dev);
>         pm_runtime_enable(&client->dev);
>
>         ret = v4l2_async_register_subdev_sensor(&ov5693->sd);
>         if (ret) {
>                 dev_err(&client->dev, "failed to register V4L2 subdev: %d",
>                         ret);
>                 goto err_pm_runtime;
>         }
>
>         pm_runtime_set_autosuspend_delay(&client->dev, 1000);
>         pm_runtime_use_autosuspend(&client->dev);
>         pm_runtime_put_autosuspend(&client->dev);
>
> And the corresponding code at remove time:
>
>         /*
>          * Disable runtime PM. In case CONFIG_PM is disabled in the kernel,
>          * make sure to turn power off manually.
>          */
>         pm_runtime_disable(&client->dev);
>         if (!pm_runtime_status_suspended(&client->dev))
>                 ov5693_sensor_powerdown(ov5693);
>         pm_runtime_set_suspended(&client->dev);
>
> And of course there's no documentation that explains all this, so there
> are endless variations of patterns originating from cargo-cult
> programming.
>
> I don't know what the right solution is, but we need to move towards an
> easier to use API if we want drivers to get it right. Any step in that
> direction would be welcome.

Yep, I fully agree with you, while it's not an easy task. At least the
example above looks fine to me. :-)

Recently I noticed that some drivers are calling
pm_runtime_force_suspend() at ->remove(). This works fine in quite
many cases, but it wouldn't solve the case when CONFIG_PM is unset.

Perhaps we should explore adding a new API, along the lines of
pm_runtime_force_suspend(), but make it specific for the ->remove()
path, and in some way make it work for when CONFIG_PM is unset too.

Kind regards
Uffe

>
> > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > >  drivers/base/power/runtime.c | 5 +++++
> > >  include/linux/pm_runtime.h   | 4 ++++
> > >  2 files changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index 2f3cce17219b..d4059e6ffeae 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -1476,11 +1476,16 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > >
> > >  static void pm_runtime_disable_action(void *data)
> > >  {
> > > +       pm_runtime_dont_use_autosuspend(data);
> > >         pm_runtime_disable(data);
> > >  }
> > >
> > >  /**
> > >   * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.
> > > + *
> > > + * NOTE: this will also handle calling pm_runtime_dont_use_autosuspend() for
> > > + * you at driver exit time if needed.
> > > + *
> > >   * @dev: Device to handle.
> > >   */
> > >  int devm_pm_runtime_enable(struct device *dev)
> > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > > index 9f09601c465a..2bff6a10095d 100644
> > > --- a/include/linux/pm_runtime.h
> > > +++ b/include/linux/pm_runtime.h
> > > @@ -567,6 +567,10 @@ static inline void pm_runtime_disable(struct device *dev)
> > >   * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
> > >   * requested (or "autosuspend" will be handled as direct runtime-suspend for
> > >   * it).
> > > + *
> > > + * NOTE: It's important to undo this with pm_runtime_dont_use_autosuspend()
> > > + * at driver exit time unless your driver initially enabled pm_runtime
> > > + * with devm_pm_runtime_enable() (which handles it for you).
> > >   */
> > >  static inline void pm_runtime_use_autosuspend(struct device *dev)
> > >  {
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Laurent Pinchart]

Hi Ulf,

On Tue, Mar 01, 2022 at 12:18:02PM +0100, Ulf Hansson wrote:
> On Tue, 1 Mar 2022 at 11:49, Laurent Pinchart wrote:
> > On Tue, Mar 01, 2022 at 11:26:46AM +0100, Ulf Hansson wrote:
> > > On Wed, 23 Feb 2022 at 17:35, Douglas Anderson wrote:
> > > >
> > > > The PM Runtime docs say:
> > > >   Drivers in ->remove() callback should undo the runtime PM changes done
> > > >   in ->probe(). Usually this means calling pm_runtime_disable(),
> > > >   pm_runtime_dont_use_autosuspend() etc.
> > > >
> > > > From grepping code, it's clear that many people aren't aware of the
> > > > need to call pm_runtime_dont_use_autosuspend().
> > >
> > > Well, I admit it's good practice that they should take care of this.
> > >
> > > However, it doesn't really matter to keep the autosuspend turned on
> > > when runtime PM becomes disabled, I think. When the driver gets probed
> > > again, it will most likely call pm_runtime_use_autosuspend() again,
> > > which should work fine, right?
> >
> > For the probe path I agree, but are there valid use cases where, at
> > runtime, a driver would disable runtime PM and re-enable it a bit later
> > ? If so, we need to ensure this won't disable auto-suspend.
> 
> I am not sure I fully understand whether there is a problem.
> 
> Can you perhaps write the sequence of the runtime PM calls that may
> cause an issue?

Simply

	pm_runtime_disable();
	/* Do something that requires runtime PM to be disabled */
	pm_runtime_enable();

at runtime (not at probe or remove time). If probe() has enabled
auto-suspend, we don't want the above sequence to disable it. What I'm
not sure is if there are any valid use cases for the above sequence.

> > > > When brainstorming solutions, one idea that came up was to leverage
> > > > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > > > * When the devm action is called we know that the driver is being
> > > >   removed. It's the perfect time to undo the use_autosuspend.
> > > > * The code of pm_runtime_dont_use_autosuspend() already handles the
> > > >   case of being called when autosuspend wasn't enabled.
> > >
> > > Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> > > currently makes it look too simple to turn off things at ->remove()
> > > for runtime PM. While in fact it's more complicated.
> > >
> > > A bigger problem, for example, is that a driver calls
> > > pm_runtime_put_sync() during ->remove(), relying on that it actually
> > > ends up calling its ->runtime_suspend() callback to turn off various
> > > specific resources for the device. And in fact there are no guarantees
> > > that will happen - and when it doesn't, the next time the driver's
> > > ->probe() runs, things are likely to be really screwed up.
> > >
> > > To cover this case, one could use the below code in the ->remove() callback:
> > >
> > > ...
> > > pm_runtime_get_sync();
> > >
> > > "turn off resources for the devices - like calling
> > > clk_disable_unprepare(), for example"
> > >
> > > pm_runtime_disable();
> > > pm_runtime_put_noidle();
> > > ...
> > >
> > > In this example, it would be too late to call pm_runtime_disable()
> > > through the pm_runtime_disable_action().
> >
> > My experience with runtime PM is that it's hard to use, at least if you
> > want to get it right :-) That's especially the case if a driver wants to
> > support both CONFIG_PM and !CONFIG_PM. Here's an example at probe time:
> >
> >         /*
> >          * We need the driver to work in the event that CONFIG_PM is disabled in
> >          * the kernel, so power up and verify the chip now. In the event that
> >          * CONFIG_PM is disabled this will leave the chip on, so that streaming
> >          * will work.
> >          */
> >         ret = ov5693_sensor_powerup(ov5693);
> >         if (ret)
> >                 goto err_media_entity_cleanup;
> >
> >         ret = ov5693_detect(ov5693);
> >         if (ret)
> >                 goto err_powerdown;
> >
> >         pm_runtime_set_active(&client->dev);
> >         pm_runtime_get_noresume(&client->dev);
> >         pm_runtime_enable(&client->dev);
> >
> >         ret = v4l2_async_register_subdev_sensor(&ov5693->sd);
> >         if (ret) {
> >                 dev_err(&client->dev, "failed to register V4L2 subdev: %d",
> >                         ret);
> >                 goto err_pm_runtime;
> >         }
> >
> >         pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> >         pm_runtime_use_autosuspend(&client->dev);
> >         pm_runtime_put_autosuspend(&client->dev);
> >
> > And the corresponding code at remove time:
> >
> >         /*
> >          * Disable runtime PM. In case CONFIG_PM is disabled in the kernel,
> >          * make sure to turn power off manually.
> >          */
> >         pm_runtime_disable(&client->dev);
> >         if (!pm_runtime_status_suspended(&client->dev))
> >                 ov5693_sensor_powerdown(ov5693);
> >         pm_runtime_set_suspended(&client->dev);
> >
> > And of course there's no documentation that explains all this, so there
> > are endless variations of patterns originating from cargo-cult
> > programming.
> >
> > I don't know what the right solution is, but we need to move towards an
> > easier to use API if we want drivers to get it right. Any step in that
> > direction would be welcome.
> 
> Yep, I fully agree with you, while it's not an easy task. At least the
> example above looks fine to me. :-)

It took me several days to figure out how to get it right. Most
developers don't bother, so we end up with drivers broken in different
ways :-S

> Recently I noticed that some drivers are calling
> pm_runtime_force_suspend() at ->remove(). This works fine in quite
> many cases, but it wouldn't solve the case when CONFIG_PM is unset.
> 
> Perhaps we should explore adding a new API, along the lines of
> pm_runtime_force_suspend(), but make it specific for the ->remove()
> path, and in some way make it work for when CONFIG_PM is unset too.

I'm all for an improved API for drivers that would make the above
simpler. And documentation too, Documentation/power/runtime_pm.rst is
more of a documentation of the runtime PM core than the driver API.
There are some useful tips for drivers, but they're lost in a sea of
difficult to understand and/or irrelevant information (and there's also
a tiny bit of information in Documentation/driver-api/pm/devices.rst).
We're missing a document targetted at driver authors.

> > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > >
> > > >  drivers/base/power/runtime.c | 5 +++++
> > > >  include/linux/pm_runtime.h   | 4 ++++
> > > >  2 files changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > index 2f3cce17219b..d4059e6ffeae 100644
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -1476,11 +1476,16 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > >
> > > >  static void pm_runtime_disable_action(void *data)
> > > >  {
> > > > +       pm_runtime_dont_use_autosuspend(data);
> > > >         pm_runtime_disable(data);
> > > >  }
> > > >
> > > >  /**
> > > >   * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.
> > > > + *
> > > > + * NOTE: this will also handle calling pm_runtime_dont_use_autosuspend() for
> > > > + * you at driver exit time if needed.
> > > > + *
> > > >   * @dev: Device to handle.
> > > >   */
> > > >  int devm_pm_runtime_enable(struct device *dev)
> > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > > > index 9f09601c465a..2bff6a10095d 100644
> > > > --- a/include/linux/pm_runtime.h
> > > > +++ b/include/linux/pm_runtime.h
> > > > @@ -567,6 +567,10 @@ static inline void pm_runtime_disable(struct device *dev)
> > > >   * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
> > > >   * requested (or "autosuspend" will be handled as direct runtime-suspend for
> > > >   * it).
> > > > + *
> > > > + * NOTE: It's important to undo this with pm_runtime_dont_use_autosuspend()
> > > > + * at driver exit time unless your driver initially enabled pm_runtime
> > > > + * with devm_pm_runtime_enable() (which handles it for you).
> > > >   */
> > > >  static inline void pm_runtime_use_autosuspend(struct device *dev)
> > > >  {

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Ulf Hansson]

Hi Laurent, Doug,

On Tue, 1 Mar 2022 at 17:29, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ulf,
>
> On Tue, Mar 01, 2022 at 12:18:02PM +0100, Ulf Hansson wrote:
> > On Tue, 1 Mar 2022 at 11:49, Laurent Pinchart wrote:
> > > On Tue, Mar 01, 2022 at 11:26:46AM +0100, Ulf Hansson wrote:
> > > > On Wed, 23 Feb 2022 at 17:35, Douglas Anderson wrote:
> > > > >
> > > > > The PM Runtime docs say:
> > > > >   Drivers in ->remove() callback should undo the runtime PM changes done
> > > > >   in ->probe(). Usually this means calling pm_runtime_disable(),
> > > > >   pm_runtime_dont_use_autosuspend() etc.
> > > > >
> > > > > From grepping code, it's clear that many people aren't aware of the
> > > > > need to call pm_runtime_dont_use_autosuspend().
> > > >
> > > > Well, I admit it's good practice that they should take care of this.
> > > >
> > > > However, it doesn't really matter to keep the autosuspend turned on
> > > > when runtime PM becomes disabled, I think. When the driver gets probed
> > > > again, it will most likely call pm_runtime_use_autosuspend() again,
> > > > which should work fine, right?
> > >
> > > For the probe path I agree, but are there valid use cases where, at
> > > runtime, a driver would disable runtime PM and re-enable it a bit later
> > > ? If so, we need to ensure this won't disable auto-suspend.
> >
> > I am not sure I fully understand whether there is a problem.
> >
> > Can you perhaps write the sequence of the runtime PM calls that may
> > cause an issue?
>
> Simply
>
>         pm_runtime_disable();
>         /* Do something that requires runtime PM to be disabled */
>         pm_runtime_enable();
>
> at runtime (not at probe or remove time). If probe() has enabled
> auto-suspend, we don't want the above sequence to disable it. What I'm
> not sure is if there are any valid use cases for the above sequence.

The above sequence certainly exists already, for example during system
suspend/resume.

So what happens is that the runtime PM auto-suspend feature gets
temporarily disabled between pm_runtime_disable() and
pm_runtime_enable(). That seems correct, right?

>
> > > > > When brainstorming solutions, one idea that came up was to leverage
> > > > > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > > > > * When the devm action is called we know that the driver is being
> > > > >   removed. It's the perfect time to undo the use_autosuspend.
> > > > > * The code of pm_runtime_dont_use_autosuspend() already handles the
> > > > >   case of being called when autosuspend wasn't enabled.
> > > >
> > > > Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> > > > currently makes it look too simple to turn off things at ->remove()
> > > > for runtime PM. While in fact it's more complicated.
> > > >
> > > > A bigger problem, for example, is that a driver calls
> > > > pm_runtime_put_sync() during ->remove(), relying on that it actually
> > > > ends up calling its ->runtime_suspend() callback to turn off various
> > > > specific resources for the device. And in fact there are no guarantees
> > > > that will happen - and when it doesn't, the next time the driver's
> > > > ->probe() runs, things are likely to be really screwed up.
> > > >
> > > > To cover this case, one could use the below code in the ->remove() callback:
> > > >
> > > > ...
> > > > pm_runtime_get_sync();
> > > >
> > > > "turn off resources for the devices - like calling
> > > > clk_disable_unprepare(), for example"
> > > >
> > > > pm_runtime_disable();
> > > > pm_runtime_put_noidle();
> > > > ...
> > > >
> > > > In this example, it would be too late to call pm_runtime_disable()
> > > > through the pm_runtime_disable_action().
> > >
> > > My experience with runtime PM is that it's hard to use, at least if you
> > > want to get it right :-) That's especially the case if a driver wants to
> > > support both CONFIG_PM and !CONFIG_PM. Here's an example at probe time:
> > >
> > >         /*
> > >          * We need the driver to work in the event that CONFIG_PM is disabled in
> > >          * the kernel, so power up and verify the chip now. In the event that
> > >          * CONFIG_PM is disabled this will leave the chip on, so that streaming
> > >          * will work.
> > >          */
> > >         ret = ov5693_sensor_powerup(ov5693);
> > >         if (ret)
> > >                 goto err_media_entity_cleanup;
> > >
> > >         ret = ov5693_detect(ov5693);
> > >         if (ret)
> > >                 goto err_powerdown;
> > >
> > >         pm_runtime_set_active(&client->dev);
> > >         pm_runtime_get_noresume(&client->dev);
> > >         pm_runtime_enable(&client->dev);
> > >
> > >         ret = v4l2_async_register_subdev_sensor(&ov5693->sd);
> > >         if (ret) {
> > >                 dev_err(&client->dev, "failed to register V4L2 subdev: %d",
> > >                         ret);
> > >                 goto err_pm_runtime;
> > >         }
> > >
> > >         pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> > >         pm_runtime_use_autosuspend(&client->dev);
> > >         pm_runtime_put_autosuspend(&client->dev);
> > >
> > > And the corresponding code at remove time:
> > >
> > >         /*
> > >          * Disable runtime PM. In case CONFIG_PM is disabled in the kernel,
> > >          * make sure to turn power off manually.
> > >          */
> > >         pm_runtime_disable(&client->dev);
> > >         if (!pm_runtime_status_suspended(&client->dev))
> > >                 ov5693_sensor_powerdown(ov5693);
> > >         pm_runtime_set_suspended(&client->dev);
> > >
> > > And of course there's no documentation that explains all this, so there
> > > are endless variations of patterns originating from cargo-cult
> > > programming.
> > >
> > > I don't know what the right solution is, but we need to move towards an
> > > easier to use API if we want drivers to get it right. Any step in that
> > > direction would be welcome.
> >
> > Yep, I fully agree with you, while it's not an easy task. At least the
> > example above looks fine to me. :-)
>
> It took me several days to figure out how to get it right. Most
> developers don't bother, so we end up with drivers broken in different
> ways :-S

Yes, it's definitely non-trivial.

Power management in general relies on cross-interaction of several
different frameworks, so one really needs a decent overview too,
before adding PM support in a driver.

>
> > Recently I noticed that some drivers are calling
> > pm_runtime_force_suspend() at ->remove(). This works fine in quite
> > many cases, but it wouldn't solve the case when CONFIG_PM is unset.
> >
> > Perhaps we should explore adding a new API, along the lines of
> > pm_runtime_force_suspend(), but make it specific for the ->remove()
> > path, and in some way make it work for when CONFIG_PM is unset too.
>
> I'm all for an improved API for drivers that would make the above
> simpler. And documentation too, Documentation/power/runtime_pm.rst is
> more of a documentation of the runtime PM core than the driver API.
> There are some useful tips for drivers, but they're lost in a sea of
> difficult to understand and/or irrelevant information (and there's also
> a tiny bit of information in Documentation/driver-api/pm/devices.rst).
> We're missing a document targetted at driver authors.

Yes, I agree - the docs can certainly be improved! I will add it to my
TODO list and try to put some time on it, not too far ahead I hope. I
was actually planning for a blog-post/LWN article, maybe I should
spend some time on this instead - or both. :-)

When it comes to the improved API for the ->remove() case, we need to
explore this a bit more. I will think about it.

About $subject patch, if you or Doug insist that you want to move
forward on it, I will not object - even if I think we need something
entirely different, in the long run.

[...]

Kind regards
Uffe

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Doug Anderson]

Hi,

On Thu, Mar 3, 2022 at 3:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Hi Laurent, Doug,
>
> On Tue, 1 Mar 2022 at 17:29, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Ulf,
> >
> > On Tue, Mar 01, 2022 at 12:18:02PM +0100, Ulf Hansson wrote:
> > > On Tue, 1 Mar 2022 at 11:49, Laurent Pinchart wrote:
> > > > On Tue, Mar 01, 2022 at 11:26:46AM +0100, Ulf Hansson wrote:
> > > > > On Wed, 23 Feb 2022 at 17:35, Douglas Anderson wrote:
> > > > > >
> > > > > > The PM Runtime docs say:
> > > > > >   Drivers in ->remove() callback should undo the runtime PM changes done
> > > > > >   in ->probe(). Usually this means calling pm_runtime_disable(),
> > > > > >   pm_runtime_dont_use_autosuspend() etc.
> > > > > >
> > > > > > From grepping code, it's clear that many people aren't aware of the
> > > > > > need to call pm_runtime_dont_use_autosuspend().
> > > > >
> > > > > Well, I admit it's good practice that they should take care of this.
> > > > >
> > > > > However, it doesn't really matter to keep the autosuspend turned on
> > > > > when runtime PM becomes disabled, I think. When the driver gets probed
> > > > > again, it will most likely call pm_runtime_use_autosuspend() again,
> > > > > which should work fine, right?
> > > >
> > > > For the probe path I agree, but are there valid use cases where, at
> > > > runtime, a driver would disable runtime PM and re-enable it a bit later
> > > > ? If so, we need to ensure this won't disable auto-suspend.
> > >
> > > I am not sure I fully understand whether there is a problem.
> > >
> > > Can you perhaps write the sequence of the runtime PM calls that may
> > > cause an issue?
> >
> > Simply
> >
> >         pm_runtime_disable();
> >         /* Do something that requires runtime PM to be disabled */
> >         pm_runtime_enable();
> >
> > at runtime (not at probe or remove time). If probe() has enabled
> > auto-suspend, we don't want the above sequence to disable it. What I'm
> > not sure is if there are any valid use cases for the above sequence.
>
> The above sequence certainly exists already, for example during system
> suspend/resume.
>
> So what happens is that the runtime PM auto-suspend feature gets
> temporarily disabled between pm_runtime_disable() and
> pm_runtime_enable(). That seems correct, right?
>
> >
> > > > > > When brainstorming solutions, one idea that came up was to leverage
> > > > > > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > > > > > * When the devm action is called we know that the driver is being
> > > > > >   removed. It's the perfect time to undo the use_autosuspend.
> > > > > > * The code of pm_runtime_dont_use_autosuspend() already handles the
> > > > > >   case of being called when autosuspend wasn't enabled.
> > > > >
> > > > > Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> > > > > currently makes it look too simple to turn off things at ->remove()
> > > > > for runtime PM. While in fact it's more complicated.
> > > > >
> > > > > A bigger problem, for example, is that a driver calls
> > > > > pm_runtime_put_sync() during ->remove(), relying on that it actually
> > > > > ends up calling its ->runtime_suspend() callback to turn off various
> > > > > specific resources for the device. And in fact there are no guarantees
> > > > > that will happen - and when it doesn't, the next time the driver's
> > > > > ->probe() runs, things are likely to be really screwed up.
> > > > >
> > > > > To cover this case, one could use the below code in the ->remove() callback:
> > > > >
> > > > > ...
> > > > > pm_runtime_get_sync();
> > > > >
> > > > > "turn off resources for the devices - like calling
> > > > > clk_disable_unprepare(), for example"
> > > > >
> > > > > pm_runtime_disable();
> > > > > pm_runtime_put_noidle();
> > > > > ...
> > > > >
> > > > > In this example, it would be too late to call pm_runtime_disable()
> > > > > through the pm_runtime_disable_action().
> > > >
> > > > My experience with runtime PM is that it's hard to use, at least if you
> > > > want to get it right :-) That's especially the case if a driver wants to
> > > > support both CONFIG_PM and !CONFIG_PM. Here's an example at probe time:
> > > >
> > > >         /*
> > > >          * We need the driver to work in the event that CONFIG_PM is disabled in
> > > >          * the kernel, so power up and verify the chip now. In the event that
> > > >          * CONFIG_PM is disabled this will leave the chip on, so that streaming
> > > >          * will work.
> > > >          */
> > > >         ret = ov5693_sensor_powerup(ov5693);
> > > >         if (ret)
> > > >                 goto err_media_entity_cleanup;
> > > >
> > > >         ret = ov5693_detect(ov5693);
> > > >         if (ret)
> > > >                 goto err_powerdown;
> > > >
> > > >         pm_runtime_set_active(&client->dev);
> > > >         pm_runtime_get_noresume(&client->dev);
> > > >         pm_runtime_enable(&client->dev);
> > > >
> > > >         ret = v4l2_async_register_subdev_sensor(&ov5693->sd);
> > > >         if (ret) {
> > > >                 dev_err(&client->dev, "failed to register V4L2 subdev: %d",
> > > >                         ret);
> > > >                 goto err_pm_runtime;
> > > >         }
> > > >
> > > >         pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> > > >         pm_runtime_use_autosuspend(&client->dev);
> > > >         pm_runtime_put_autosuspend(&client->dev);
> > > >
> > > > And the corresponding code at remove time:
> > > >
> > > >         /*
> > > >          * Disable runtime PM. In case CONFIG_PM is disabled in the kernel,
> > > >          * make sure to turn power off manually.
> > > >          */
> > > >         pm_runtime_disable(&client->dev);
> > > >         if (!pm_runtime_status_suspended(&client->dev))
> > > >                 ov5693_sensor_powerdown(ov5693);
> > > >         pm_runtime_set_suspended(&client->dev);
> > > >
> > > > And of course there's no documentation that explains all this, so there
> > > > are endless variations of patterns originating from cargo-cult
> > > > programming.
> > > >
> > > > I don't know what the right solution is, but we need to move towards an
> > > > easier to use API if we want drivers to get it right. Any step in that
> > > > direction would be welcome.
> > >
> > > Yep, I fully agree with you, while it's not an easy task. At least the
> > > example above looks fine to me. :-)
> >
> > It took me several days to figure out how to get it right. Most
> > developers don't bother, so we end up with drivers broken in different
> > ways :-S
>
> Yes, it's definitely non-trivial.
>
> Power management in general relies on cross-interaction of several
> different frameworks, so one really needs a decent overview too,
> before adding PM support in a driver.
>
> >
> > > Recently I noticed that some drivers are calling
> > > pm_runtime_force_suspend() at ->remove(). This works fine in quite
> > > many cases, but it wouldn't solve the case when CONFIG_PM is unset.
> > >
> > > Perhaps we should explore adding a new API, along the lines of
> > > pm_runtime_force_suspend(), but make it specific for the ->remove()
> > > path, and in some way make it work for when CONFIG_PM is unset too.
> >
> > I'm all for an improved API for drivers that would make the above
> > simpler. And documentation too, Documentation/power/runtime_pm.rst is
> > more of a documentation of the runtime PM core than the driver API.
> > There are some useful tips for drivers, but they're lost in a sea of
> > difficult to understand and/or irrelevant information (and there's also
> > a tiny bit of information in Documentation/driver-api/pm/devices.rst).
> > We're missing a document targetted at driver authors.
>
> Yes, I agree - the docs can certainly be improved! I will add it to my
> TODO list and try to put some time on it, not too far ahead I hope. I
> was actually planning for a blog-post/LWN article, maybe I should
> spend some time on this instead - or both. :-)
>
> When it comes to the improved API for the ->remove() case, we need to
> explore this a bit more. I will think about it.
>
> About $subject patch, if you or Doug insist that you want to move
> forward on it, I will not object - even if I think we need something
> entirely different, in the long run.

Hrm, I guess the question is how far away are we from the long run.
;-) If this doesn't impede the long term solution and the long term
solution is still a ways out, it feels worth landing this or something
similar to it just to help make the existing code a little more
robust, even if it doesn't solve every problem. That being said, I
don't have any code that depends on this patch and I myself will
likely not mess this up again in code that I write. ;-)  Just for
context, the patch was originally suggested by Laurent [1] in response
to me fixing some code that I wrote.

[1] https://lore.kernel.org/r/YhZY+FLTlv7V5BIB@pendragon.ideasonboard.com

-Doug

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Laurent Pinchart]

Hi Ulf,

On Fri, Mar 04, 2022 at 12:10:48AM +0100, Ulf Hansson wrote:
> On Tue, 1 Mar 2022 at 17:29, Laurent Pinchart wrote:
> > On Tue, Mar 01, 2022 at 12:18:02PM +0100, Ulf Hansson wrote:
> > > On Tue, 1 Mar 2022 at 11:49, Laurent Pinchart wrote:
> > > > On Tue, Mar 01, 2022 at 11:26:46AM +0100, Ulf Hansson wrote:
> > > > > On Wed, 23 Feb 2022 at 17:35, Douglas Anderson wrote:
> > > > > >
> > > > > > The PM Runtime docs say:
> > > > > >   Drivers in ->remove() callback should undo the runtime PM changes done
> > > > > >   in ->probe(). Usually this means calling pm_runtime_disable(),
> > > > > >   pm_runtime_dont_use_autosuspend() etc.
> > > > > >
> > > > > > From grepping code, it's clear that many people aren't aware of the
> > > > > > need to call pm_runtime_dont_use_autosuspend().
> > > > >
> > > > > Well, I admit it's good practice that they should take care of this.
> > > > >
> > > > > However, it doesn't really matter to keep the autosuspend turned on
> > > > > when runtime PM becomes disabled, I think. When the driver gets probed
> > > > > again, it will most likely call pm_runtime_use_autosuspend() again,
> > > > > which should work fine, right?
> > > >
> > > > For the probe path I agree, but are there valid use cases where, at
> > > > runtime, a driver would disable runtime PM and re-enable it a bit later
> > > > ? If so, we need to ensure this won't disable auto-suspend.
> > >
> > > I am not sure I fully understand whether there is a problem.
> > >
> > > Can you perhaps write the sequence of the runtime PM calls that may
> > > cause an issue?
> >
> > Simply
> >
> >         pm_runtime_disable();
> >         /* Do something that requires runtime PM to be disabled */
> >         pm_runtime_enable();
> >
> > at runtime (not at probe or remove time). If probe() has enabled
> > auto-suspend, we don't want the above sequence to disable it. What I'm
> > not sure is if there are any valid use cases for the above sequence.
> 
> The above sequence certainly exists already, for example during system
> suspend/resume.
> 
> So what happens is that the runtime PM auto-suspend feature gets
> temporarily disabled between pm_runtime_disable() and
> pm_runtime_enable(). That seems correct, right?

Sorry, I haven't expressed myself clearly. What I meant is that we can't
turn off auto-suspend completely in pm_runtime_disable() (by calling
pm_runtime_dont_use_autosuspend()), otherwise the above sequence will
leave auto-suspend disabled after pm_runtime_enable(). That's why Doug
proposed a solution for the devm_ version only.

> > > > > > When brainstorming solutions, one idea that came up was to leverage
> > > > > > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > > > > > * When the devm action is called we know that the driver is being
> > > > > >   removed. It's the perfect time to undo the use_autosuspend.
> > > > > > * The code of pm_runtime_dont_use_autosuspend() already handles the
> > > > > >   case of being called when autosuspend wasn't enabled.
> > > > >
> > > > > Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> > > > > currently makes it look too simple to turn off things at ->remove()
> > > > > for runtime PM. While in fact it's more complicated.
> > > > >
> > > > > A bigger problem, for example, is that a driver calls
> > > > > pm_runtime_put_sync() during ->remove(), relying on that it actually
> > > > > ends up calling its ->runtime_suspend() callback to turn off various
> > > > > specific resources for the device. And in fact there are no guarantees
> > > > > that will happen - and when it doesn't, the next time the driver's
> > > > > ->probe() runs, things are likely to be really screwed up.
> > > > >
> > > > > To cover this case, one could use the below code in the ->remove() callback:
> > > > >
> > > > > ...
> > > > > pm_runtime_get_sync();
> > > > >
> > > > > "turn off resources for the devices - like calling
> > > > > clk_disable_unprepare(), for example"
> > > > >
> > > > > pm_runtime_disable();
> > > > > pm_runtime_put_noidle();
> > > > > ...
> > > > >
> > > > > In this example, it would be too late to call pm_runtime_disable()
> > > > > through the pm_runtime_disable_action().
> > > >
> > > > My experience with runtime PM is that it's hard to use, at least if you
> > > > want to get it right :-) That's especially the case if a driver wants to
> > > > support both CONFIG_PM and !CONFIG_PM. Here's an example at probe time:
> > > >
> > > >         /*
> > > >          * We need the driver to work in the event that CONFIG_PM is disabled in
> > > >          * the kernel, so power up and verify the chip now. In the event that
> > > >          * CONFIG_PM is disabled this will leave the chip on, so that streaming
> > > >          * will work.
> > > >          */
> > > >         ret = ov5693_sensor_powerup(ov5693);
> > > >         if (ret)
> > > >                 goto err_media_entity_cleanup;
> > > >
> > > >         ret = ov5693_detect(ov5693);
> > > >         if (ret)
> > > >                 goto err_powerdown;
> > > >
> > > >         pm_runtime_set_active(&client->dev);
> > > >         pm_runtime_get_noresume(&client->dev);
> > > >         pm_runtime_enable(&client->dev);
> > > >
> > > >         ret = v4l2_async_register_subdev_sensor(&ov5693->sd);
> > > >         if (ret) {
> > > >                 dev_err(&client->dev, "failed to register V4L2 subdev: %d",
> > > >                         ret);
> > > >                 goto err_pm_runtime;
> > > >         }
> > > >
> > > >         pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> > > >         pm_runtime_use_autosuspend(&client->dev);
> > > >         pm_runtime_put_autosuspend(&client->dev);
> > > >
> > > > And the corresponding code at remove time:
> > > >
> > > >         /*
> > > >          * Disable runtime PM. In case CONFIG_PM is disabled in the kernel,
> > > >          * make sure to turn power off manually.
> > > >          */
> > > >         pm_runtime_disable(&client->dev);
> > > >         if (!pm_runtime_status_suspended(&client->dev))
> > > >                 ov5693_sensor_powerdown(ov5693);
> > > >         pm_runtime_set_suspended(&client->dev);
> > > >
> > > > And of course there's no documentation that explains all this, so there
> > > > are endless variations of patterns originating from cargo-cult
> > > > programming.
> > > >
> > > > I don't know what the right solution is, but we need to move towards an
> > > > easier to use API if we want drivers to get it right. Any step in that
> > > > direction would be welcome.
> > >
> > > Yep, I fully agree with you, while it's not an easy task. At least the
> > > example above looks fine to me. :-)
> >
> > It took me several days to figure out how to get it right. Most
> > developers don't bother, so we end up with drivers broken in different
> > ways :-S
> 
> Yes, it's definitely non-trivial.
> 
> Power management in general relies on cross-interaction of several
> different frameworks, so one really needs a decent overview too,
> before adding PM support in a driver.

Given that most (if not all) drivers should have PM support, we need to
make that simpler, at least for the common cases. Otherwise we'll always
have no PM support at all, or most of the time, broken PM support.

> > > Recently I noticed that some drivers are calling
> > > pm_runtime_force_suspend() at ->remove(). This works fine in quite
> > > many cases, but it wouldn't solve the case when CONFIG_PM is unset.
> > >
> > > Perhaps we should explore adding a new API, along the lines of
> > > pm_runtime_force_suspend(), but make it specific for the ->remove()
> > > path, and in some way make it work for when CONFIG_PM is unset too.
> >
> > I'm all for an improved API for drivers that would make the above
> > simpler. And documentation too, Documentation/power/runtime_pm.rst is
> > more of a documentation of the runtime PM core than the driver API.
> > There are some useful tips for drivers, but they're lost in a sea of
> > difficult to understand and/or irrelevant information (and there's also
> > a tiny bit of information in Documentation/driver-api/pm/devices.rst).
> > We're missing a document targetted at driver authors.
> 
> Yes, I agree - the docs can certainly be improved! I will add it to my
> TODO list and try to put some time on it, not too far ahead I hope. I
> was actually planning for a blog-post/LWN article, maybe I should
> spend some time on this instead - or both. :-)

Thank you, much appreciated.

> When it comes to the improved API for the ->remove() case, we need to
> explore this a bit more. I will think about it.
> 
> About $subject patch, if you or Doug insist that you want to move
> forward on it, I will not object - even if I think we need something
> entirely different, in the long run.

I quite like Doug's proposal, even if it ends up being temporary only,
as it should do the job for now, and will flag drivers that need to be
updated to any better future API by providing a simple grep pattern.

> [...]
> 
> Kind regards
> Uffe

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Doug Anderson]

Hi,

On Tue, Mar 1, 2022 at 2:27 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 23 Feb 2022 at 17:35, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > The PM Runtime docs say:
> >   Drivers in ->remove() callback should undo the runtime PM changes done
> >   in ->probe(). Usually this means calling pm_runtime_disable(),
> >   pm_runtime_dont_use_autosuspend() etc.
> >
> > From grepping code, it's clear that many people aren't aware of the
> > need to call pm_runtime_dont_use_autosuspend().
>
> Well, I admit it's good practice that they should take care of this.
>
> However, it doesn't really matter to keep the autosuspend turned on
> when runtime PM becomes disabled, I think. When the driver gets probed
> again, it will most likely call pm_runtime_use_autosuspend() again,
> which should work fine, right?
>
> >
> > When brainstorming solutions, one idea that came up was to leverage
> > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > * When the devm action is called we know that the driver is being
> >   removed. It's the perfect time to undo the use_autosuspend.
> > * The code of pm_runtime_dont_use_autosuspend() already handles the
> >   case of being called when autosuspend wasn't enabled.
>
> Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> currently makes it look too simple to turn off things at ->remove()
> for runtime PM. While in fact it's more complicated.
>
> A bigger problem, for example, is that a driver calls
> pm_runtime_put_sync() during ->remove(), relying on that it actually
> ends up calling its ->runtime_suspend() callback to turn off various
> specific resources for the device. And in fact there are no guarantees
> that will happen - and when it doesn't, the next time the driver's
> ->probe() runs, things are likely to be really screwed up.
>
> To cover this case, one could use the below code in the ->remove() callback:
>
> ...
> pm_runtime_get_sync();
>
> "turn off resources for the devices - like calling
> clk_disable_unprepare(), for example"
>
> pm_runtime_disable();
> pm_runtime_put_noidle();
> ...
>
> In this example, it would be too late to call pm_runtime_disable()
> through the pm_runtime_disable_action().

In the example you're listing above, though, you can't use
devm_pm_runtime_enable() anyway, right? If you have a reason not to
use the devm approach then that's fine and this patch won't affect
you. However, if you're using devm_pm_runtime_enable() already then
there should be no downside, right?

Though I'm by no means a Runtime PM expert, I think the most common
use case might be when a driver is kept simple by _rely_ing on
CONFIG_PM. Certainly folks have debated about this in the past, but at
least for a set of drivers where nobody has a need for running them
without CONFIG_PM they can be kept simpler by relying on it. When you
do this, you don't need quite as many machinations with regards to
playing with power management when runtime PM is off. You can
basically just enable runtime power management and assume that the
runtime resume / runtime suspend will do the work of powering you,
right?


-Doug

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Ulf Hansson]

On Wed, 23 Feb 2022 at 17:35, Douglas Anderson <dianders@chromium.org> wrote:
>
> The PM Runtime docs say:
>   Drivers in ->remove() callback should undo the runtime PM changes done
>   in ->probe(). Usually this means calling pm_runtime_disable(),
>   pm_runtime_dont_use_autosuspend() etc.
>
> From grepping code, it's clear that many people aren't aware of the
> need to call pm_runtime_dont_use_autosuspend().
>
> When brainstorming solutions, one idea that came up was to leverage
> the new-ish devm_pm_runtime_enable() function. The idea here is that:
> * When the devm action is called we know that the driver is being
>   removed. It's the perfect time to undo the use_autosuspend.
> * The code of pm_runtime_dont_use_autosuspend() already handles the
>   case of being called when autosuspend wasn't enabled.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Okay, this provides an improvement from the short term perspective.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

When I get some time, I will try to explore the ->remove() and
->probe() error-path case a bit more, to try to suggest a new
interface that might be able to replace the devm_pm_runtime_enable().
Let's see...

Kind regards
Uffe

> ---
>
>  drivers/base/power/runtime.c | 5 +++++
>  include/linux/pm_runtime.h   | 4 ++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 2f3cce17219b..d4059e6ffeae 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1476,11 +1476,16 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
>
>  static void pm_runtime_disable_action(void *data)
>  {
> +       pm_runtime_dont_use_autosuspend(data);
>         pm_runtime_disable(data);
>  }
>
>  /**
>   * devm_pm_runtime_enable - devres-enabled version of pm_runtime_enable.
> + *
> + * NOTE: this will also handle calling pm_runtime_dont_use_autosuspend() for
> + * you at driver exit time if needed.
> + *
>   * @dev: Device to handle.
>   */
>  int devm_pm_runtime_enable(struct device *dev)
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 9f09601c465a..2bff6a10095d 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -567,6 +567,10 @@ static inline void pm_runtime_disable(struct device *dev)
>   * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
>   * requested (or "autosuspend" will be handled as direct runtime-suspend for
>   * it).
> + *
> + * NOTE: It's important to undo this with pm_runtime_dont_use_autosuspend()
> + * at driver exit time unless your driver initially enabled pm_runtime
> + * with devm_pm_runtime_enable() (which handles it for you).
>   */
>  static inline void pm_runtime_use_autosuspend(struct device *dev)
>  {
> --
> 2.35.1.473.g83b2b277ed-goog
>

Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

[Rafael J. Wysocki]

On Fri, Mar 4, 2022 at 11:04 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 23 Feb 2022 at 17:35, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > The PM Runtime docs say:
> >   Drivers in ->remove() callback should undo the runtime PM changes done
> >   in ->probe(). Usually this means calling pm_runtime_disable(),
> >   pm_runtime_dont_use_autosuspend() etc.
> >
> > From grepping code, it's clear that many people aren't aware of the
> > need to call pm_runtime_dont_use_autosuspend().
> >
> > When brainstorming solutions, one idea that came up was to leverage
> > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > * When the devm action is called we know that the driver is being
> >   removed. It's the perfect time to undo the use_autosuspend.
> > * The code of pm_runtime_dont_use_autosuspend() already handles the
> >   case of being called when autosuspend wasn't enabled.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Okay, this provides an improvement from the short term perspective.

I agree.

> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

And so I've queued up the patch (for 5.18).

Thanks!