| Message ID | 20250317093445.361821-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add more devm_ functions to fix PM imbalance in spi/atmel-quadspi.c | expand |
On Mon, Mar 17, 2025 at 10:35 AM Bence Csókás <csokas.bence@prolan.hu> wrote: > > Add `devm_pm_runtime_set_active()` and > `devm_pm_runtime_get_noresume()` for > simplifying common use cases in drivers. > > Signed-off-by: Bence Csókás <csokas.bence@prolan.hu> > --- > drivers/base/power/runtime.c | 36 ++++++++++++++++++++++++++++++++++++ > include/linux/pm_runtime.h | 4 ++++ > 2 files changed, 40 insertions(+) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 9589ccb0fda2..821a8b4961d4 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -1568,6 +1568,24 @@ void pm_runtime_enable(struct device *dev) > } > EXPORT_SYMBOL_GPL(pm_runtime_enable); > > +static void pm_runtime_set_suspended_action(void *data) > +{ > + pm_runtime_set_suspended(data); > +} > + > +/** > + * devm_pm_runtime_set_active - devres-enabled version of pm_runtime_set_active. > + * > + * @dev: Device to handle. > + */ > +int devm_pm_runtime_set_active(struct device *dev) > +{ > + pm_runtime_set_active(dev); > + > + return devm_add_action_or_reset(dev, pm_runtime_set_suspended_action, dev); > +} > +EXPORT_SYMBOL_GPL(devm_pm_runtime_set_active); I said I didn't like it and I'm still not liking it. The problem is that the primary role of pm_runtime_set_active() is to prepare the device for enabling runtime PM, so in the majority of cases it should be followed by pm_runtime_enable(). It is also not always necessary to call pm_runtime_set_suspended() after disabling runtime PM for a device, like when the device has been runtime-suspended before disabling runtime PM for it. This is not like releasing a resource that has been allocated and using devm for it in the above way is at least questionable. Now, there is a reason why calling pm_runtime_set_suspended() on a device after disabling runtime PM for it is a good idea at all. Namely, disabling runtime PM alone does not release the device's suppliers or its parent, so if you want to release them after disabling runtime PM for the device, you need to do something more. I'm thinking that this is a mistake in the design of the runtime PM core. If there were functions like pm_runtime_enable_in_state() (taking an additional state argument and acquiring all of the necessary references on the parent and suppliers of the target device) and pm_runtime_disable_and_forget() (that in addition to disabling runtime PM would drop the references acquired by the former), then it would make a perfect sense to provide a devm variant of pm_runtime_enable_in_state() with the cleanup action pointing to pm_runtime_disable_and_forget(). If this helps, I can do some work on providing pm_runtime_enable_in_state() and pm_runtime_disable_and_forget() or equivalent. > + > static void pm_runtime_disable_action(void *data) > { > pm_runtime_dont_use_autosuspend(data); > @@ -1590,6 +1608,24 @@ int devm_pm_runtime_enable(struct device *dev) > } > EXPORT_SYMBOL_GPL(devm_pm_runtime_enable); > > +static void pm_runtime_put_noidle_action(void *data) > +{ > + pm_runtime_put_noidle(data); > +} > + > +/** > + * devm_pm_runtime_get_noresume - devres-enabled version of pm_runtime_get_noresume. > + * > + * @dev: Device to handle. > + */ > +int devm_pm_runtime_get_noresume(struct device *dev) > +{ > + pm_runtime_get_noresume(dev); > + > + return devm_add_action_or_reset(dev, pm_runtime_put_noidle_action, dev); > +} > +EXPORT_SYMBOL_GPL(devm_pm_runtime_get_noresume); > + > /** > * pm_runtime_forbid - Block runtime PM of a device. > * @dev: Device to handle. > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 7fb5a459847e..364355da349a 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -96,7 +96,9 @@ extern void pm_runtime_new_link(struct device *dev); > extern void pm_runtime_drop_link(struct device_link *link); > extern void pm_runtime_release_supplier(struct device_link *link); > > +int devm_pm_runtime_set_active(struct device *dev); > extern int devm_pm_runtime_enable(struct device *dev); > +int devm_pm_runtime_get_noresume(struct device *dev); > > /** > * pm_suspend_ignore_children - Set runtime PM behavior regarding children. > @@ -294,7 +296,9 @@ static inline bool pm_runtime_blocked(struct device *dev) { return true; } > static inline void pm_runtime_allow(struct device *dev) {} > static inline void pm_runtime_forbid(struct device *dev) {} > > +static inline int devm_pm_runtime_set_active(struct device *dev) { return 0; } > static inline int devm_pm_runtime_enable(struct device *dev) { return 0; } > +static inline int devm_pm_runtime_get_noresume(struct device *dev) { return 0; } > > static inline void pm_suspend_ignore_children(struct device *dev, bool enable) {} > static inline void pm_runtime_get_noresume(struct device *dev) {} > -- > 2.48.1 > > >
Hi, On 2025. 03. 26. 18:38, Rafael J. Wysocki wrote: > I said I didn't like it and I'm still not liking it. You didn't really elaborate further, but now I'm glad I could understand your dislike. > The problem is that the primary role of pm_runtime_set_active() is to > prepare the device for enabling runtime PM, so in the majority of > cases it should be followed by pm_runtime_enable(). It is also not > always necessary to call pm_runtime_set_suspended() after disabling > runtime PM for a device, like when the device has been > runtime-suspended before disabling runtime PM for it. This is not > like releasing a resource that has been allocated and using devm for > it in the above way is at least questionable. > > Now, there is a reason why calling pm_runtime_set_suspended() on a > device after disabling runtime PM for it is a good idea at all. > Namely, disabling runtime PM alone does not release the device's > suppliers or its parent, so if you want to release them after > disabling runtime PM for the device, you need to do something more. > I'm thinking that this is a mistake in the design of the runtime PM > core. Well, this is the order in which the original driver worked before anyways. As a quick fix, would it work if we created a devm function that would pm_runtime_set_active(), immediately followed by pm_runtime_enable(), and on cleanup it would pm_runtime_set_suspended() followed by pm_runtime_disable_action() (i.e. pm_runtime_dont_use_autosuspend() and pm_runtime_disable())? > If there were functions like pm_runtime_enable_in_state() (taking an > additional state argument and acquiring all of the necessary > references on the parent and suppliers of the target device) and > pm_runtime_disable_and_forget() (that in addition to disabling runtime > PM would drop the references acquired by the former), then it would > make a perfect sense to provide a devm variant of > pm_runtime_enable_in_state() with the cleanup action pointing to > pm_runtime_disable_and_forget(). > > If this helps, I can do some work on providing > pm_runtime_enable_in_state() and pm_runtime_disable_and_forget() or > equivalent. I mean sure, if that's something you want to work on, but it sounds like it would entail much more work, plus it wouldn't be easy to backport it to 6.14.y stable later. Bence
On Thu, Mar 27, 2025 at 10:02 AM Csókás Bence <csokas.bence@prolan.hu> wrote: > > Hi, > > On 2025. 03. 26. 18:38, Rafael J. Wysocki wrote: > > I said I didn't like it and I'm still not liking it. > > You didn't really elaborate further, but now I'm glad I could understand > your dislike. > > > The problem is that the primary role of pm_runtime_set_active() is to > > prepare the device for enabling runtime PM, so in the majority of > > cases it should be followed by pm_runtime_enable(). It is also not > > always necessary to call pm_runtime_set_suspended() after disabling > > runtime PM for a device, like when the device has been > > runtime-suspended before disabling runtime PM for it. This is not > > like releasing a resource that has been allocated and using devm for > > it in the above way is at least questionable. > > > > Now, there is a reason why calling pm_runtime_set_suspended() on a > > device after disabling runtime PM for it is a good idea at all. > > Namely, disabling runtime PM alone does not release the device's > > suppliers or its parent, so if you want to release them after > > disabling runtime PM for the device, you need to do something more. > > I'm thinking that this is a mistake in the design of the runtime PM > > core. > > Well, this is the order in which the original driver worked before > anyways. As a quick fix, would it work if we created a devm function > that would pm_runtime_set_active(), immediately followed by > pm_runtime_enable(), and on cleanup it would pm_runtime_set_suspended() > followed by pm_runtime_disable_action() (i.e. > pm_runtime_dont_use_autosuspend() and pm_runtime_disable())? On cleanup you'd need to ensure that pm_runtime_disable() is followed by pm_runtime_set_suspended() (not the other way around). Also pm_runtime_dont_use_autosuspend() needs to be called when runtime PM is still enabled. With the above taken into account, it would work.
Hi, On 2025. 03. 27. 12:08, Rafael J. Wysocki wrote: >>> Now, there is a reason why calling pm_runtime_set_suspended() on a >>> device after disabling runtime PM for it is a good idea at all. >>> Namely, disabling runtime PM alone does not release the device's >>> suppliers or its parent, so if you want to release them after >>> disabling runtime PM for the device, you need to do something more. >>> I'm thinking that this is a mistake in the design of the runtime PM >>> core. >> >> Well, this is the order in which the original driver worked before >> anyways. As a quick fix, would it work if we created a devm function >> that would pm_runtime_set_active(), immediately followed by >> pm_runtime_enable(), and on cleanup it would pm_runtime_set_suspended() >> followed by pm_runtime_disable_action() (i.e. >> pm_runtime_dont_use_autosuspend() and pm_runtime_disable())? > > On cleanup you'd need to ensure that pm_runtime_disable() is followed > by pm_runtime_set_suspended() (not the other way around). Also > pm_runtime_dont_use_autosuspend() needs to be called when runtime PM > is still enabled. > > With the above taken into account, it would work. Ok, so which is the correct order then? 1. the way it is done now in [PATCH v5 2/2] (which is the same order the driver has been using before anyways): pm_runtime_use_autosuspend() /-- devm_pm_runtime_set_active() | /-- devm_pm_runtime_enable() | | /-- devm_pm_runtime_get_noresume() | | | | | \-> pm_runtime_put_noidle() | \-> pm_runtime_dont_use_autosuspend() && | pm_runtime_disable() \-> pm_runtime_set_suspended() or, 2. swapped set_suspended() and runtime_disable() pm_runtime_use_autosuspend() /-- devm_pm_runtime_set_active_enabled() [new fn] | == pm_runtime_set_active() && | pm_runtime_enable() | /-- devm_pm_runtime_get_noresume() | | | \-> pm_runtime_put_noidle() \--> pm_runtime_set_suspended() pm_runtime_dont_use_autosuspend() pm_runtime_disable() Bence
On Thu, Mar 27, 2025 at 2:24 PM Csókás Bence <csokas.bence@prolan.hu> wrote: > > Hi, > > On 2025. 03. 27. 12:08, Rafael J. Wysocki wrote: > >>> Now, there is a reason why calling pm_runtime_set_suspended() on a > >>> device after disabling runtime PM for it is a good idea at all. > >>> Namely, disabling runtime PM alone does not release the device's > >>> suppliers or its parent, so if you want to release them after > >>> disabling runtime PM for the device, you need to do something more. > >>> I'm thinking that this is a mistake in the design of the runtime PM > >>> core. > >> > >> Well, this is the order in which the original driver worked before > >> anyways. As a quick fix, would it work if we created a devm function > >> that would pm_runtime_set_active(), immediately followed by > >> pm_runtime_enable(), and on cleanup it would pm_runtime_set_suspended() > >> followed by pm_runtime_disable_action() (i.e. > >> pm_runtime_dont_use_autosuspend() and pm_runtime_disable())? > > > > On cleanup you'd need to ensure that pm_runtime_disable() is followed > > by pm_runtime_set_suspended() (not the other way around). Also > > pm_runtime_dont_use_autosuspend() needs to be called when runtime PM > > is still enabled. > > > > With the above taken into account, it would work. > > Ok, so which is the correct order then? > > 1. the way it is done now in [PATCH v5 2/2] (which is the same order the > driver has been using before anyways): > > pm_runtime_use_autosuspend() > /-- devm_pm_runtime_set_active() > | /-- devm_pm_runtime_enable() > | | /-- devm_pm_runtime_get_noresume() > | | | > | | \-> pm_runtime_put_noidle() > | \-> pm_runtime_dont_use_autosuspend() && > | pm_runtime_disable() > \-> pm_runtime_set_suspended() > > or, > 2. swapped set_suspended() and runtime_disable() > > pm_runtime_use_autosuspend() > /-- devm_pm_runtime_set_active_enabled() [new fn] > | == pm_runtime_set_active() && > | pm_runtime_enable() > | /-- devm_pm_runtime_get_noresume() > | | > | \-> pm_runtime_put_noidle() > \--> pm_runtime_set_suspended() > pm_runtime_dont_use_autosuspend() > pm_runtime_disable() /-- devm_pm_runtime_get_noresume() | /-- devm_{pm_runtime_set_active() + pm_runtime_enable() (in this order)} | | pm_runtime_use_autosuspend() | | | | Note that the device cannot be suspended here unless its runtime PM usage | | counter is dropped, in which it would need to be bumped up again later to | | retain the balance. | | | \-> pm_runtime_disable() + pm_runtime_set_suspended() (in this order) \-> pm_runtime_put_noidle() And pm_runtime_dont_use_autosuspend() is not really necessary after disabling runtime PM. Also, I think that the driver could be fixed without introducing the new devm_ stuff which would be way simpler, so why don't you do that and then think about devm_?
Hi, On 2025. 03. 27. 15:14, Rafael J. Wysocki wrote: > /-- devm_pm_runtime_get_noresume() > | /-- devm_{pm_runtime_set_active() + pm_runtime_enable() (in this order)} > | | pm_runtime_use_autosuspend() > | | > | | Note that the device cannot be suspended here unless its > runtime PM usage > | | counter is dropped, in which it would need to be bumped up > again later to > | | retain the balance. > | | > | \-> pm_runtime_disable() + pm_runtime_set_suspended() (in this order) > \-> pm_runtime_put_noidle() Ah, so basically what I've done originally, just calling `devm_pm_runtime_get_noresume()` _first_ instead of _last_, right? > And pm_runtime_dont_use_autosuspend() is not really necessary after > disabling runtime PM. It was done this way in devm_pm_runtime_enable() already, see commit b4060db9251f ("PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()"). I didn't change anything behaviourally there. > Also, I think that the driver could be fixed without introducing the > new devm_ stuff which would be way simpler, so why don't you do that > and then think about devm_? Sure, I could quick-fix this, go through all the possible error paths and whatnot and ref-count in my head, but it doesn't fix the underlying problem: in order to properly use PM, you have to do a bunch of calls in some set order, then undo them in reverse order on error and remove -- exactly the thing devm was designed for, and exactly the thing where it's easy for a human to forget one case by accident. Thus I prefer to use the *real* solution, devm. Bence
On Thu, Mar 27, 2025 at 5:11 PM Csókás Bence <csokas.bence@prolan.hu> wrote: > > Hi, > > On 2025. 03. 27. 15:14, Rafael J. Wysocki wrote: > > /-- devm_pm_runtime_get_noresume() > > | /-- devm_{pm_runtime_set_active() + pm_runtime_enable() (in this order)} > > | | pm_runtime_use_autosuspend() > > | | > > | | Note that the device cannot be suspended here unless its > > runtime PM usage > > | | counter is dropped, in which it would need to be bumped up > > again later to > > | | retain the balance. > > | | > > | \-> pm_runtime_disable() + pm_runtime_set_suspended() (in this order) > > \-> pm_runtime_put_noidle() > > Ah, so basically what I've done originally, just calling > `devm_pm_runtime_get_noresume()` _first_ instead of _last_, right? Right. If you want to use pm_runtime_get_noresume() to prevent the device from suspending and you do that after enabling runtime PM for it, the device may suspend between the "enable" and the "get_noresume". Doing the latter before the former prevents this race from occurring. > > And pm_runtime_dont_use_autosuspend() is not really necessary after > > disabling runtime PM. > > It was done this way in devm_pm_runtime_enable() already, see commit > b4060db9251f ("PM: runtime: Have devm_pm_runtime_enable() handle > pm_runtime_dont_use_autosuspend()"). I didn't change anything > behaviourally there. Yes, this is fine, although not really necessary. > > Also, I think that the driver could be fixed without introducing the > > new devm_ stuff which would be way simpler, so why don't you do that > > and then think about devm_? > > Sure, I could quick-fix this, go through all the possible error paths > and whatnot and ref-count in my head, but it doesn't fix the underlying > problem: in order to properly use PM, you have to do a bunch of calls in > some set order, then undo them in reverse order on error and remove -- > exactly the thing devm was designed for, and exactly the thing where > it's easy for a human to forget one case by accident. Thus I prefer to > use the *real* solution, devm. Except that you need to enforce the proper initial ordering or you may get undesirable results on the way out. Say you call devm_pm_runtime_set_active() after devm_pm_runtime_enable() by mistake. It doesn't do anything, but runtime PM may still work because the device gets resumed at one point and if it has never been in a low-power state in the first place, the "transition" may just be transparent. On the way out, the cleanup action for devm_pm_runtime_set_active() will run before the cleanup action for devm_pm_runtime_enable() AFAICS and so it won't do anything again, so the device's parent and suppliers (if any) may remain reference-counted. That's why I'm saying that it is better to combine pm_runtime_set_active() with pm_runtime_enable() (in the right order) in one devm_ call and define the cleanup action for it to run pm_runtime_disable() before pm_runtime_set_suspended(). Then, the mistake described above cannot be made.
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 9589ccb0fda2..821a8b4961d4 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1568,6 +1568,24 @@ void pm_runtime_enable(struct device *dev) } EXPORT_SYMBOL_GPL(pm_runtime_enable); +static void pm_runtime_set_suspended_action(void *data) +{ + pm_runtime_set_suspended(data); +} + +/** + * devm_pm_runtime_set_active - devres-enabled version of pm_runtime_set_active. + * + * @dev: Device to handle. + */ +int devm_pm_runtime_set_active(struct device *dev) +{ + pm_runtime_set_active(dev); + + return devm_add_action_or_reset(dev, pm_runtime_set_suspended_action, dev); +} +EXPORT_SYMBOL_GPL(devm_pm_runtime_set_active); + static void pm_runtime_disable_action(void *data) { pm_runtime_dont_use_autosuspend(data); @@ -1590,6 +1608,24 @@ int devm_pm_runtime_enable(struct device *dev) } EXPORT_SYMBOL_GPL(devm_pm_runtime_enable); +static void pm_runtime_put_noidle_action(void *data) +{ + pm_runtime_put_noidle(data); +} + +/** + * devm_pm_runtime_get_noresume - devres-enabled version of pm_runtime_get_noresume. + * + * @dev: Device to handle. + */ +int devm_pm_runtime_get_noresume(struct device *dev) +{ + pm_runtime_get_noresume(dev); + + return devm_add_action_or_reset(dev, pm_runtime_put_noidle_action, dev); +} +EXPORT_SYMBOL_GPL(devm_pm_runtime_get_noresume); + /** * pm_runtime_forbid - Block runtime PM of a device. * @dev: Device to handle. diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 7fb5a459847e..364355da349a 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -96,7 +96,9 @@ extern void pm_runtime_new_link(struct device *dev); extern void pm_runtime_drop_link(struct device_link *link); extern void pm_runtime_release_supplier(struct device_link *link); +int devm_pm_runtime_set_active(struct device *dev); extern int devm_pm_runtime_enable(struct device *dev); +int devm_pm_runtime_get_noresume(struct device *dev); /** * pm_suspend_ignore_children - Set runtime PM behavior regarding children. @@ -294,7 +296,9 @@ static inline bool pm_runtime_blocked(struct device *dev) { return true; } static inline void pm_runtime_allow(struct device *dev) {} static inline void pm_runtime_forbid(struct device *dev) {} +static inline int devm_pm_runtime_set_active(struct device *dev) { return 0; } static inline int devm_pm_runtime_enable(struct device *dev) { return 0; } +static inline int devm_pm_runtime_get_noresume(struct device *dev) { return 0; } static inline void pm_suspend_ignore_children(struct device *dev, bool enable) {} static inline void pm_runtime_get_noresume(struct device *dev) {}
Add `devm_pm_runtime_set_active()` and `devm_pm_runtime_get_noresume()` for simplifying common use cases in drivers. Signed-off-by: Bence Csókás <csokas.bence@prolan.hu> --- drivers/base/power/runtime.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/pm_runtime.h | 4 ++++ 2 files changed, 40 insertions(+)