drivers/base/power/main.c | 79 ++++++++++++++++++++-------------------------- 1 file changed, 35 insertions(+), 44 deletions(-)
From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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;
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);
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);On Wed, Jul 16, 2025 at 12:31 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@google.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);
>
>
>
>
On Wed, Jul 16, 2025 at 11:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jul 16, 2025 at 12:31 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@google.com>
Thanks!
© 2016 - 2025 Red Hat, Inc.