| 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: <CAGETcx-ddJMua5_VMNofr2vZ9n5Oyo4iT6Bac825L8tFqqQsxg@mail.gmail.com>
Date: Wed, 16 Jul 2025 14:51:21 -0700
From: Saravana Kannan <saravanak@...gle.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: 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 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()?
Put another way, why can't we also delete the local "error" variable?
Assuming we need to keep "error":
Reviewed-by: Saravana Kannan <saravanak@...gle.com>
-Saravana
> if (error)
> dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
>
> @@ -1585,7 +1582,7 @@
> *
> * Runtime PM is disabled for @dev while this function is being executed.
> */
> -static int device_suspend_late(struct device *dev, pm_message_t state, bool async)
> +static void device_suspend_late(struct device *dev, pm_message_t state, bool async)
> {
> pm_callback_t callback = NULL;
> const char *info = NULL;
> @@ -1602,11 +1599,11 @@
>
> dpm_wait_for_subordinate(dev, async);
>
> - if (async_error)
> + if (READ_ONCE(async_error))
> goto Complete;
>
> if (pm_wakeup_pending()) {
> - async_error = -EBUSY;
> + WRITE_ONCE(async_error, -EBUSY);
> goto Complete;
> }
>
> @@ -1640,7 +1637,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 late" : " late", error);
> goto Complete;
> @@ -1654,12 +1651,10 @@
> TRACE_SUSPEND(error);
> complete_all(&dev->power.completion);
>
> - if (error || async_error)
> - return error;
> + if (error || READ_ONCE(async_error))
> + return;
>
> dpm_async_suspend_superior(dev, async_suspend_late);
> -
> - return 0;
> }
>
> static void async_suspend_late(void *data, async_cookie_t cookie)
> @@ -1678,7 +1673,7 @@
> {
> ktime_t starttime = ktime_get();
> struct device *dev;
> - int error = 0;
> + int error;
>
> trace_suspend_resume(TPS("dpm_suspend_late"), state.event, true);
>
> @@ -1711,13 +1706,13 @@
>
> mutex_unlock(&dpm_list_mtx);
>
> - error = device_suspend_late(dev, state, false);
> + device_suspend_late(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_suspended_list);
> /*
> * Move all devices to the target list to resume them
> @@ -1731,9 +1726,8 @@
> mutex_unlock(&dpm_list_mtx);
>
> async_synchronize_full();
> - if (!error)
> - error = async_error;
>
> + error = READ_ONCE(async_error);
> if (error) {
> dpm_save_failed_step(SUSPEND_SUSPEND_LATE);
> dpm_resume_early(resume_event(state));
> @@ -1822,7 +1816,7 @@
> * @state: PM transition of the system being carried out.
> * @async: If true, the device is being suspended asynchronously.
> */
> -static int device_suspend(struct device *dev, pm_message_t state, bool async)
> +static void device_suspend(struct device *dev, pm_message_t state, bool async)
> {
> pm_callback_t callback = NULL;
> const char *info = NULL;
> @@ -1834,7 +1828,7 @@
>
> dpm_wait_for_subordinate(dev, async);
>
> - if (async_error) {
> + if (READ_ONCE(async_error)) {
> dev->power.direct_complete = false;
> goto Complete;
> }
> @@ -1854,7 +1848,7 @@
>
> if (pm_wakeup_pending()) {
> dev->power.direct_complete = false;
> - async_error = -EBUSY;
> + WRITE_ONCE(async_error, -EBUSY);
> goto Complete;
> }
>
> @@ -1938,7 +1932,7 @@
>
> Complete:
> 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);
> }
> @@ -1946,12 +1940,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);
> -
> - return 0;
> }
>
> static void async_suspend(void *data, async_cookie_t cookie)
> @@ -1970,7 +1962,7 @@
> {
> ktime_t starttime = ktime_get();
> struct device *dev;
> - int error = 0;
> + int error;
>
> trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
> might_sleep();
> @@ -2005,13 +1997,13 @@
>
> mutex_unlock(&dpm_list_mtx);
>
> - error = device_suspend(dev, state, false);
> + device_suspend(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_prepared_list);
> /*
> * Move all devices to the target list to resume them
> @@ -2025,9 +2017,8 @@
> mutex_unlock(&dpm_list_mtx);
>
> async_synchronize_full();
> - if (!error)
> - error = async_error;
>
> + error = READ_ONCE(async_error);
> if (error)
> dpm_save_failed_step(SUSPEND_SUSPEND);
>
>
>
>
Powered by blists - more mailing lists