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