| 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
| ||
|
Message-ID: <5941318.DvuYhMxLoT@rafael.j.wysocki>
Date: Tue, 18 Nov 2025 13:45:13 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ulf Hansson <ulf.hansson@...aro.org>, Rose Wu <ya-jou.wu@...iatek.com>
Cc: linux-pm@...r.kernel.org, rafael.j.wysocki@...el.com,
regressions@...ts.linux.dev, linux-kernel@...r.kernel.org,
wsd_upstream <wsd_upstream@...iatek.com>, linux-mediatek@...ts.infradead.org,
"artis. chiu" <artis.chiu@...iatek.com>,
"Johnny-cc. Kao" <Johnny-cc.Kao@...iatek.com>
Subject:
[PATCH v3] PM: sleep: core: Fix runtime PM enabling in device_resume_early()
On Tuesday, November 18, 2025 1:26:06 PM CET Rafael J. Wysocki wrote:
> On Tue, Nov 18, 2025 at 1:18 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> >
> > On Tue, 18 Nov 2025 at 12:48, Rafael J. Wysocki <rafael@...nel.org> wrote:
> > >
> > > On Tuesday, November 18, 2025 9:31:08 AM CET Rose Wu wrote:
> > > > Hi,
> > > >
> > > > On Mon, 2025-11-17 at 19:57 +0100, Rafael J. Wysocki wrote:
> > > > >
> > > > > Make two changes to address this problem.
> > > > >
> > > > > First, reorder device_suspend_late() to only disable runtime PM for a
> > > > > device if the power.is_late_suspended flag is going to be set for it.
> > > > > In all of the other cases, disabling runtime PM for the device is not
> > > > > in fact necessary.
> > > > >
> > > > > Second, make device_resume_early() only enable runtime PM for the
> > > > > devices with the power.is_late_suspended flag set.
> > > > >
> > > >
> > > > My concern is with the error path in device_suspend_late().
> > > > If a device fails its dpm_run_callback(), it appears that its
> > > > power.is_late_suspended flag is not set, potentially leaving its runtime
> > > > PM disabled during the resume sequence.
> > >
> > > Right, pm_runtime_enable() is missing in the error path after calling
> > > dpm_run_callback().
> > >
> > > ---
> > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > >
> > > Runtime PM should only be enabled in device_resume_early() if it has
> > > been disabled for the given device by device_suspend_late(). Otherwise,
> > > it may cause runtime PM callbacks to run prematurely in some cases
> > > which leads to further functional issues.
> > >
> > > Make two changes to address this problem.
> > >
> > > First, reorder device_suspend_late() to only disable runtime PM for a
> > > device when it is going to look for the device's callback. In all of
> > > the other cases, disabling runtime PM for the device is not in fact
> > > necessary. However, if the device's callback returns an error and the
> > > power.is_late_suspended flag is not going to be set, enable runtime
> > > PM so it only remains disabled when power.is_late_suspended is set.
> > >
> > > Second, make device_resume_early() only enable runtime PM for the
> > > devices with the power.is_late_suspended flag set.
> > >
> > > Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
> > > Reported-by: Rose Wu <ya-jou.wu@...iatek.com>
> > > Closes: https://lore.kernel.org/linux-pm/70b25dca6f8c2756d78f076f4a7dee7edaaffc33.camel@mediatek.com/
> > > Cc: 6.16+ <stable@...r.kernel.org> # 6.16+
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > ---
> > >
> > > v1 -> v2: Add pm_runtime_enable() to device_suspend_late() error path (Rose).
> > >
> > > ---
> > > drivers/base/power/main.c | 15 ++++++++-------
> > > 1 file changed, 8 insertions(+), 7 deletions(-)
> > >
> > > --- a/drivers/base/power/main.c
> > > +++ b/drivers/base/power/main.c
> > > @@ -941,11 +941,11 @@ Run:
> > >
> > > Skip:
> > > dev->power.is_late_suspended = false;
> > > + pm_runtime_enable(dev);
> > >
> > > Out:
> > > TRACE_RESUME(error);
> > >
> > > - pm_runtime_enable(dev);
> > > complete_all(&dev->power.completion);
> > >
> > > if (error) {
> > > @@ -1630,12 +1630,6 @@ static void device_suspend_late(struct d
> > > TRACE_DEVICE(dev);
> > > TRACE_SUSPEND(0);
> > >
> > > - /*
> > > - * Disable runtime PM for the device without checking if there is a
> > > - * pending resume request for it.
> > > - */
> > > - __pm_runtime_disable(dev, false);
> > > -
> > > dpm_wait_for_subordinate(dev, async);
> > >
> > > if (READ_ONCE(async_error))
> > > @@ -1649,6 +1643,12 @@ static void device_suspend_late(struct d
> > > if (dev->power.syscore || dev->power.direct_complete)
> > > goto Complete;
> > >
> > > + /*
> > > + * Disable runtime PM for the device without checking if there is a
> > > + * pending resume request for it.
> > > + */
> > > + __pm_runtime_disable(dev, false);
> > > +
> >
> > Moving this here means we are going to keep runtime pm enabled for
> > syscore devices during system wide suspend/resume. That's quite a
> > change in behaviour for a fix for a regression, I think. Not saying
> > that it can't work though.
>
> syscore devices can be a special case, but I thought it wouldn't be
> necessary to special-case them.
>
> Do you actually know about any of them needing special casing?
In any case, below is a v3 that special-cases syscore devices. Fortunately,
it is not much more complicated than the v2.
Thanks!
---
From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
Runtime PM should only be enabled in device_resume_early() if it has
been disabled for the given device by device_suspend_late(). Otherwise,
it may cause runtime PM callbacks to run prematurely in some cases
which leads to further functional issues.
Make two changes to address this problem.
First, reorder device_suspend_late() to only disable runtime PM for a
device when it is going to look for the device's callback or if the
device is a "syscore" one. In all of the other cases, disabling runtime
PM for the device is not in fact necessary. However, if the device's
callback returns an error and the power.is_late_suspended flag is not
going to be set, enable runtime PM so it only remains disabled when
power.is_late_suspended is set.
Second, make device_resume_early() only enable runtime PM for the
devices with the power.is_late_suspended flag set.
Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
Reported-by: Rose Wu <ya-jou.wu@...iatek.com>
Closes: https://lore.kernel.org/linux-pm/70b25dca6f8c2756d78f076f4a7dee7edaaffc33.camel@mediatek.com/
Cc: 6.16+ <stable@...r.kernel.org> # 6.16+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
---
v2 -> v1:
* Also set is_late_suspended for syscore devices to avoid skipping runtime PM
disabling for them (Ulf).
* Update the changelog to reflect the above.
v1 -> v2:
* Add pm_runtime_enable() to device_suspend_late() error path (Rose).
* Update the changelog to reflect the above.
---
drivers/base/power/main.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -903,7 +903,10 @@ static void device_resume_early(struct d
TRACE_DEVICE(dev);
TRACE_RESUME(0);
- if (dev->power.syscore || dev->power.direct_complete)
+ if (dev->power.syscore)
+ goto Skip;
+
+ if (dev->power.direct_complete)
goto Out;
if (!dev->power.is_late_suspended)
@@ -941,11 +944,11 @@ Run:
Skip:
dev->power.is_late_suspended = false;
+ pm_runtime_enable(dev);
Out:
TRACE_RESUME(error);
- pm_runtime_enable(dev);
complete_all(&dev->power.completion);
if (error) {
@@ -1630,12 +1633,6 @@ static void device_suspend_late(struct d
TRACE_DEVICE(dev);
TRACE_SUSPEND(0);
- /*
- * Disable runtime PM for the device without checking if there is a
- * pending resume request for it.
- */
- __pm_runtime_disable(dev, false);
-
dpm_wait_for_subordinate(dev, async);
if (READ_ONCE(async_error))
@@ -1646,9 +1643,18 @@ static void device_suspend_late(struct d
goto Complete;
}
- if (dev->power.syscore || dev->power.direct_complete)
+ if (dev->power.direct_complete)
goto Complete;
+ /*
+ * Disable runtime PM for the device without checking if there is a
+ * pending resume request for it.
+ */
+ __pm_runtime_disable(dev, false);
+
+ if (dev->power.syscore)
+ goto Skip;
+
if (dev->pm_domain) {
info = "late power domain ";
callback = pm_late_early_op(&dev->pm_domain->ops, state);
@@ -1679,6 +1685,7 @@ Run:
WRITE_ONCE(async_error, error);
dpm_save_failed_dev(dev_name(dev));
pm_dev_err(dev, state, async ? " async late" : " late", error);
+ pm_runtime_enable(dev);
goto Complete;
}
dpm_propagate_wakeup_to_parent(dev);
Powered by blists - more mailing lists