| Message ID | 20220223083441.1.I925ce9fa12992a58caed6b297e0171d214866fe7@changeid (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend() | expand |
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 >
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) > > {
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
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
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) > > > > {
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
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
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
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 >
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!
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) {
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(+)