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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0iS20uPhqNOnkj36rTBGQF3fecF6Hq4JU4=wz4pSzrFyg@mail.gmail.com>
Date: Thu, 27 Mar 2025 15:14:22 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Csókás Bence <csokas.bence@...lan.hu>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	Varshini Rajendran <varshini.rajendran@...rochip.com>, Tudor Ambarus <tudor.ambarus@...aro.org>, 
	Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Len Brown <len.brown@...el.com>, 
	Pavel Machek <pavel@....cz>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Danilo Krummrich <dakr@...nel.org>, Alexander Dahl <ada@...rsis.com>, 
	Nicolas Ferre <nicolas.ferre@...rochip.com>, 
	Alexandre Belloni <alexandre.belloni@...tlin.com>, Claudiu Beznea <claudiu.beznea@...on.dev>, 
	Pavel Machek <pavel@...nel.org>
Subject: Re: [PATCH v5 1/2] pm: runtime: Add new devm functions

On Thu, Mar 27, 2025 at 2:24 PM Csókás Bence <csokas.bence@...lan.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_?