Messages in this thread | | | From | Ulf Hansson <> | Date | Tue, 1 Mar 2022 12:18:02 +0100 | Subject | Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend() |
| |
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
| |