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] [day] [month] [year] [list]

Message-ID: <CAJZ5v0g2cjZwJYjd7Jt-dDDaxjCPwmkAzRZM1HDT4tod7RiUUw@mail.gmail.com>
Date: Thu, 27 Mar 2025 17:45:26 +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 5:11 PM Csókás Bence <csokas.bence@...lan.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.