| 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: <CAJZ5v0jBQbDx2G0ZUy+Jx2yAdt_KCOJhOEbpQfLar=yWwvbhOA@mail.gmail.com>
Date: Thu, 17 Jul 2025 11:40:04 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Saravana Kannan <saravanak@...gle.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux PM <linux-pm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH v1] PM: sleep: Rearrange suspend/resume error handling in
the core
On Wed, Jul 16, 2025 at 11:52 PM Saravana Kannan <saravanak@...gle.com> wrote:
>
> On Wed, Jul 16, 2025 at 12:31 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Notice that device_suspend_noirq(), device_suspend_late() and
> > device_suspend() all set async_error on errors, so they don't really
> > need to return a value. Accordingly, make them all void and use
> > async_error in their callers instead of their return values.
> >
> > Moreover, since async_error is updated concurrently without locking
> > during asynchronous suspend and resume processing, use READ_ONCE() and
> > WRITE_ONCE() for accessing it in those places to ensure that all of the
> > accesses will be carried out as expected.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >
> > Based on the current linux-pm.git material in linux-next.
> >
> > ---
> > drivers/base/power/main.c | 79 ++++++++++++++++++++--------------------------
> > 1 file changed, 35 insertions(+), 44 deletions(-)
> >
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -767,7 +767,7 @@
> > TRACE_RESUME(error);
> >
> > if (error) {
> > - async_error = error;
> > + WRITE_ONCE(async_error, error);
> > dpm_save_failed_dev(dev_name(dev));
> > pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
> > }
> > @@ -824,7 +824,7 @@
> > mutex_unlock(&dpm_list_mtx);
> > async_synchronize_full();
> > dpm_show_time(starttime, state, 0, "noirq");
> > - if (async_error)
> > + if (READ_ONCE(async_error))
> > dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
> >
> > trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, false);
> > @@ -910,7 +910,7 @@
> > complete_all(&dev->power.completion);
> >
> > if (error) {
> > - async_error = error;
> > + WRITE_ONCE(async_error, error);
> > dpm_save_failed_dev(dev_name(dev));
> > pm_dev_err(dev, state, async ? " async early" : " early", error);
> > }
> > @@ -971,7 +971,7 @@
> > mutex_unlock(&dpm_list_mtx);
> > async_synchronize_full();
> > dpm_show_time(starttime, state, 0, "early");
> > - if (async_error)
> > + if (READ_ONCE(async_error))
> > dpm_save_failed_step(SUSPEND_RESUME_EARLY);
> >
> > trace_suspend_resume(TPS("dpm_resume_early"), state.event, false);
> > @@ -1086,7 +1086,7 @@
> > TRACE_RESUME(error);
> >
> > if (error) {
> > - async_error = error;
> > + WRITE_ONCE(async_error, error);
> > dpm_save_failed_dev(dev_name(dev));
> > pm_dev_err(dev, state, async ? " async" : "", error);
> > }
> > @@ -1150,7 +1150,7 @@
> > mutex_unlock(&dpm_list_mtx);
> > async_synchronize_full();
> > dpm_show_time(starttime, state, 0, NULL);
> > - if (async_error)
> > + if (READ_ONCE(async_error))
> > dpm_save_failed_step(SUSPEND_RESUME);
> >
> > cpufreq_resume();
> > @@ -1387,7 +1387,7 @@
> > * The driver of @dev will not receive interrupts while this function is being
> > * executed.
> > */
> > -static int device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
> > +static void device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
> > {
> > pm_callback_t callback = NULL;
> > const char *info = NULL;
> > @@ -1398,7 +1398,7 @@
> >
> > dpm_wait_for_subordinate(dev, async);
> >
> > - if (async_error)
> > + if (READ_ONCE(async_error))
> > goto Complete;
> >
> > if (dev->power.syscore || dev->power.direct_complete)
> > @@ -1431,7 +1431,7 @@
> > Run:
> > error = dpm_run_callback(callback, dev, state, info);
> > if (error) {
> > - async_error = error;
> > + WRITE_ONCE(async_error, error);
> > dpm_save_failed_dev(dev_name(dev));
> > pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
> > goto Complete;
> > @@ -1457,12 +1457,10 @@
> > complete_all(&dev->power.completion);
> > TRACE_SUSPEND(error);
> >
> > - if (error || async_error)
> > - return error;
> > + if (error || READ_ONCE(async_error))
> > + return;
> >
> > dpm_async_suspend_superior(dev, async_suspend_noirq);
> > -
> > - return 0;
> > }
> >
> > static void async_suspend_noirq(void *data, async_cookie_t cookie)
> > @@ -1477,7 +1475,7 @@
> > {
> > ktime_t starttime = ktime_get();
> > struct device *dev;
> > - int error = 0;
> > + int error;
>
> Are we still keeping around the error variable ... (question continues
> further down)
> >
> > trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
> >
> > @@ -1508,13 +1506,13 @@
> >
> > mutex_unlock(&dpm_list_mtx);
> >
> > - error = device_suspend_noirq(dev, state, false);
> > + device_suspend_noirq(dev, state, false);
> >
> > put_device(dev);
> >
> > mutex_lock(&dpm_list_mtx);
> >
> > - if (error || async_error) {
> > + if (READ_ONCE(async_error)) {
> > dpm_async_suspend_complete_all(&dpm_late_early_list);
> > /*
> > * Move all devices to the target list to resume them
> > @@ -1528,9 +1526,8 @@
> > mutex_unlock(&dpm_list_mtx);
> >
> > async_synchronize_full();
> > - if (!error)
> > - error = async_error;
> >
> > + error = READ_ONCE(async_error);
>
> Just to cache the value locally so that the value used for the "if()"
> check is the one that's sent to dpm_show_time()?
Generally, yes.
To be more precise, the READ_ONCE() is not really necessary after the
async_synchronize_full(), so "bare" async_error could be used going
forward, but then I would need to add a comment explaining this here
and in two other places, so I chose to just use the existing local
variable to store the value.
> Put another way, why can't we also delete the local "error" variable?
It could be deleted, but I preferred to make fewer changes in this patch.
> Assuming we need to keep "error":
>
> Reviewed-by: Saravana Kannan <saravanak@...gle.com>
Thanks!
Powered by blists - more mailing lists