[PATCH v5 0/2] Add more devm_ functions to fix PM imbalance in spi/atmel-quadspi.c

[PATCH v5 0/2] Add more devm_ functions to fix PM imbalance in spi/atmel-quadspi.c

[Bence Csókás]

The probe() function of the atmel-quadspi driver got quite convoluted,
especially since the addition of SAMA7G5 support, that was forward-ported
from an older vendor kernel. During the port, a bug was introduced, where
the PM get() and put() calls were imbalanced. To alleivate this - and
similar problems in the future - an effort was made to migrate as many
functions as possible, to their devm_ managed counterparts. The few
functions, which did not yet have a devm_ variant, are added in patch 1 of
this series. Patch 2 then uses these APIs to fix the probe() function.

Change in v4:
* the DMA cleanup was split out and will be submitted separately for 6.15

Change in v5:
* rebased to linux-pm/linux-next, will now target 6.15

Links to previous versions:
pre-series:
https://lore.kernel.org/linux-kernel/20250114222851.1023194-1-csokas.bence@prolan.hu/
v1:
https://lore.kernel.org/linux-kernel/20250115160244.1102881-1-csokas.bence@prolan.hu/
v2:
https://lore.kernel.org/linux-kernel/20250124085221.766303-8-csokas.bence@prolan.hu/
v3:
https://lore.kernel.org/linux-kernel/20250207124802.165408-1-csokas.bence@prolan.hu/
v4:
https://lore.kernel.org/linux-kernel/20250210111008.248929-1-csokas.bence@prolan.hu/

Bence Csókás (2):
  pm: runtime: Add new devm functions
  spi: atmel-quadspi: Fix unbalanced pm_runtime by using devm_ API

 drivers/base/power/runtime.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/spi/atmel-quadspi.c  | 18 +++++-------------
 include/linux/pm_runtime.h   |  4 ++++
 3 files changed, 45 insertions(+), 13 deletions(-)

-- 
2.48.1

[PATCH v5 1/2] pm: runtime: Add new devm functions

[Bence Csókás]

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);
+
 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

Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

[Rafael J. Wysocki]

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
>
>
>

Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

[Csókás Bence]

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

Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

[Rafael J. Wysocki]

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.

Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

[Csókás Bence]

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

Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

[Rafael J. Wysocki]

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_?

Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

[Csókás Bence]

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

Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

[Rafael J. Wysocki]

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.

[PATCH v5 2/2] spi: atmel-quadspi: Fix unbalanced pm_runtime by using devm_ API

[Bence Csókás]

Fix unbalanced PM in error path of `atmel_qspi_probe()`
by using `devm_pm_runtime_*()` functions.

Reported-by: Alexander Dahl <ada@thorsis.com>
Closes: https://lore.kernel.org/linux-spi/20250110-paycheck-irregular-bcddab1276c7@thorsis.com/
Fixes: 5af42209a4d2 ("spi: atmel-quadspi: Add support for sama7g5 QSPI")
Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---
 drivers/spi/atmel-quadspi.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index d8c9be64d006..ddd12600a6f1 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -1439,22 +1439,18 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
 	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
+	devm_pm_runtime_set_active(&pdev->dev);
+	devm_pm_runtime_enable(&pdev->dev);
+	devm_pm_runtime_get_noresume(&pdev->dev);
 
 	err = atmel_qspi_init(aq);
 	if (err)
 		goto dma_release;
 
 	err = spi_register_controller(ctrl);
-	if (err) {
-		pm_runtime_put_noidle(&pdev->dev);
-		pm_runtime_disable(&pdev->dev);
-		pm_runtime_set_suspended(&pdev->dev);
-		pm_runtime_dont_use_autosuspend(&pdev->dev);
+	if (err)
 		goto dma_release;
-	}
+
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
 
@@ -1533,10 +1529,6 @@ static void atmel_qspi_remove(struct platform_device *pdev)
 		 */
 		dev_warn(&pdev->dev, "Failed to resume device on remove\n");
 	}
-
-	pm_runtime_disable(&pdev->dev);
-	pm_runtime_dont_use_autosuspend(&pdev->dev);
-	pm_runtime_put_noidle(&pdev->dev);
 }
 
 static int __maybe_unused atmel_qspi_suspend(struct device *dev)
-- 
2.48.1