mutex deadlock of dpm_resume() in low memory situation

Could you look into this issue ?

Quoted 1 line · Youngmin Nam

Could you look into this issue ?

Can you submit a patch that resolves the issue for you, as you have a way to actually test this out? That would be the quickest way to get it resolved, and to help confirm that this is even an issue at all.

Quoted 1 line · Youngmin Nam

Could you look into this issue ?

Can you submit a patch that resolves the issue for you, as you have a way to actually test this out? That would be the quickest way to get it resolved, and to help confirm that this is even an issue at all.

Quoted 6 lines · CET Greg KH

Quoted 1 line · Youngmin Nam

Could you look into this issue ?

Can you submit a patch that resolves the issue for you, as you have a way to actually test this out? That would be the quickest way to get it resolved, and to help confirm that this is even an issue at all.

Something like the appended patch should be sufficient to address this AFAICS.

I haven't tested it yet (will do so shortly), so all of the usual disclaimers apply.

I think that it can be split into 2 patches, but for easier testing here it goes in one piece.

Quoted 10 lines · CET Rafael J. Wysocki

Quoted 6 lines · CET Greg KH

Quoted 1 line · Youngmin Nam

Could you look into this issue ?

Can you submit a patch that resolves the issue for you, as you have a way to actually test this out? That would be the quickest way to get it resolved, and to help confirm that this is even an issue at all.

Something like the appended patch should be sufficient to address this AFAICS.

Quoted 5 lines

I haven't tested it yet (will do so shortly), so all of the usual disclaimers apply.

I think that it can be split into 2 patches, but for easier testing here it goes in one piece.

Well, please scratch this, it will not handle "async" devices properly.

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · CET Rafael J. Wysocki

Quoted 6 lines · CET Greg KH

Quoted 1 line · Youngmin Nam

Could you look into this issue ?

Can you submit a patch that resolves the issue for you, as you have a way to actually test this out? That would be the quickest way to get it resolved, and to help confirm that this is even an issue at all.

Something like the appended patch should be sufficient to address this AFAICS.

Quoted 7 lines

Quoted 5 lines

I haven't tested it yet (will do so shortly), so all of the usual disclaimers apply.

I think that it can be split into 2 patches, but for easier testing here it goes in one piece.

Well, please scratch this, it will not handle "async" devices properly.

This

https://lore.kernel.org/linux-pm/6019796.lOV4Wx5bFT@kreacher/

should address the issue properly (it has been lightly tested).

Please give it a go and let me know if it works for you (on top of 6.7-rc7).

From: Rafael J. Wysocki <[email protected]>

In preparation for subsequent changes, introduce a specialized variant of async_schedule_dev() that will not invoke the argument function synchronously when it cannot be scheduled for asynchronous execution.

The new function, async_schedule_dev_nocall(), will be used for fixing possible deadlocks in the system-wide power management core code.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |   12 ++++++++----
 include/linux/async.h     |    2 ++
 kernel/async.c            |   29 +++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

Index: linux-pm/kernel/async.c
===================================================================

kernel/async.c +6 -0

@@ -244,6 +244,35 @@ async_cookie_t async_schedule_node(async
 EXPORT_SYMBOL_GPL(async_schedule_node);
 
 /**
+ * async_schedule_dev_nocall - A simplified variant of async_schedule_dev()
+ * @func: function to execute asynchronously
+ * @dev: device argument to be passed to function
+ *
+ * @dev is used as both the argument for the function and to provide NUMA
+ * context for where to run the function.

Quoted 34 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

In preparation for subsequent changes, introduce a specialized variant of async_schedule_dev() that will not invoke the argument function synchronously when it cannot be scheduled for asynchronous execution.

The new function, async_schedule_dev_nocall(), will be used for fixing possible deadlocks in the system-wide power management core code.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |   12 ++++++++----
 include/linux/async.h     |    2 ++
 kernel/async.c            |   29 +++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

Index: linux-pm/kernel/async.c
===================================================================

kernel/async.c +10 -0

@@ -244,6 +244,35 @@ async_cookie_t async_schedule_node(async
 EXPORT_SYMBOL_GPL(async_schedule_node);
 
 /**
+ * async_schedule_dev_nocall - A simplified variant of async_schedule_dev()
+ * @func: function to execute asynchronously
+ * @dev: device argument to be passed to function
+ *
+ * @dev is used as both the argument for the function and to provide NUMA
+ * context for where to run the function.
+ *
+ * If the asynchronous execution of @func is scheduled successfully, return
+ * true. Otherwise, do nothing and return false, unlike async_schedule_dev()
+ * that will run the function synchronously then.

kernel/async.c async_cookie_t async_schedule_node(async · line 262

Quoted 6 lines

kernel/async.c +6 -0

+ */
+bool async_schedule_dev_nocall(async_func_t func, struct device *dev)
+{
+     struct async_entry *entry;
+
+     entry = kzalloc(sizeof(struct async_entry), GFP_KERNEL);

Is GFP_KERNEL intended here ?

I think it's not safe since will
be called from device_resume_noirq() .

Quoted 7 lines

kernel/async.c async_cookie_t async_schedule_node(async · line 262

Quoted 5 lines

kernel/async.c +5 -0

+bool async_schedule_dev_nocall(async_func_t func, struct device *dev)
+{
+     struct async_entry *entry;
+
+     entry = kzalloc(sizeof(struct async_entry), GFP_KERNEL);

Is GFP_KERNEL intended here ?

Yes, it is.

PM will be the only user of this, at least for now, and it all runs in process context.

Quoted 2 lines

I think it's not safe since will
be called from device_resume_noirq() .

device_resume_noirq() runs in process context too.

The name is somewhat confusing (sorry about that) and it means that hardirq handlers (for the majority of IRQs) don't run in that resume phase, but interrupts are enabled locally on all CPUs (this is required for wakeup handling, among other things).

Quoted 16 lines · Rafael J. Wysocki

Quoted 7 lines

kernel/async.c async_cookie_t async_schedule_node(async · line 262

Quoted 5 lines

kernel/async.c +5 -0

+bool async_schedule_dev_nocall(async_func_t func, struct device *dev)
+{
+     struct async_entry *entry;
+
+     entry = kzalloc(sizeof(struct async_entry), GFP_KERNEL);

Is GFP_KERNEL intended here ?

Yes, it is.

PM will be the only user of this, at least for now, and it all runs in process context.

Quoted 2 lines

I think it's not safe since will
be called from device_resume_noirq() .

Quoted 6 lines

device_resume_noirq() runs in process context too.

The name is somewhat confusing (sorry about that) and it means that hardirq handlers (for the majority of IRQs) don't run in that resume phase, but interrupts are enabled locally on all CPUs (this is required for wakeup handling, among other things).

Then my concern would be: if among devices with disabled IRQs are disk devices? Seems there are disk devices as well, and because GFP_KERNEL can start reclaiming memory by doing disk IO (write dirty pages for example), with disk driver interrupts disabled reclaiming process can not finish.

I do not see how such possible infinite waiting for disk IO scenario is prevented here, did I miss something?

Quoted 31 lines · Stanislaw Gruszka

Quoted 16 lines · Rafael J. Wysocki

Quoted 7 lines

kernel/async.c async_cookie_t async_schedule_node(async · line 262

Quoted 5 lines

kernel/async.c +5 -0

+bool async_schedule_dev_nocall(async_func_t func, struct device *dev)
+{
+     struct async_entry *entry;
+
+     entry = kzalloc(sizeof(struct async_entry), GFP_KERNEL);

Is GFP_KERNEL intended here ?

Yes, it is.

PM will be the only user of this, at least for now, and it all runs in process context.

Quoted 2 lines

I think it's not safe since will
be called from device_resume_noirq() .

Quoted 6 lines

device_resume_noirq() runs in process context too.

The name is somewhat confusing (sorry about that) and it means that hardirq handlers (for the majority of IRQs) don't run in that resume phase, but interrupts are enabled locally on all CPUs (this is required for wakeup handling, among other things).

Then my concern would be: if among devices with disabled IRQs are disk devices? Seems there are disk devices as well, and because GFP_KERNEL can start reclaiming memory by doing disk IO (write dirty pages for example), with disk driver interrupts disabled reclaiming process can not finish.

Quoted 2 lines

I do not see how such possible infinite waiting for disk IO scenario is prevented here, did I miss something?

Well, it is not a concern, because the suspend code already prevents the mm subsystem from trying too hard to find free memory. See the pm_restrict_gfp_mask() call in enter_state().

Quoted 36 lines · Rafael J. Wysocki

Quoted 33 lines · Stanislaw Gruszka

Quoted 16 lines · Rafael J. Wysocki

Quoted 7 lines

kernel/async.c async_cookie_t async_schedule_node(async · line 262

Quoted 5 lines

kernel/async.c +5 -0

+bool async_schedule_dev_nocall(async_func_t func, struct device *dev)
+{
+     struct async_entry *entry;
+
+     entry = kzalloc(sizeof(struct async_entry), GFP_KERNEL);

Is GFP_KERNEL intended here ?

Yes, it is.

PM will be the only user of this, at least for now, and it all runs in process context.

Quoted 2 lines

I think it's not safe since will
be called from device_resume_noirq() .

Quoted 6 lines

device_resume_noirq() runs in process context too.

The name is somewhat confusing (sorry about that) and it means that hardirq handlers (for the majority of IRQs) don't run in that resume phase, but interrupts are enabled locally on all CPUs (this is required for wakeup handling, among other things).

Then my concern would be: if among devices with disabled IRQs are disk devices? Seems there are disk devices as well, and because GFP_KERNEL can start reclaiming memory by doing disk IO (write dirty pages for example), with disk driver interrupts disabled reclaiming process can not finish.

I do not see how such possible infinite waiting for disk IO scenario is prevented here, did I miss something?

Quoted 3 lines

Well, it is not a concern, because the suspend code already prevents the mm subsystem from trying too hard to find free memory. See the pm_restrict_gfp_mask() call in enter_state().

So that I missed :-) Thanks for explanations.

Reviewed-by Stanislaw Gruszka <[email protected]> for the series.

From: Rafael J. Wysocki <[email protected]>

It is reported that in low-memory situations the system-wide resume core code deadlocks, because async_schedule_dev() executes its argument function synchronously if it cannot allocate memory (an not only then) and that function attempts to acquire a mutex that is already held.

Address this by changing the code in question to use async_schedule_dev_nocall() for scheduling the asynchronous execution of device suspend and resume functions and to directly run them synchronously if async_schedule_dev_nocall() returns false.

Fixes 09beebd8f93b ("PM: sleep: core: Switch back to async_schedule_dev()")

Link https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/ open here

Reported-by Youngmin Nam <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---

The commit pointed to by the Fixes: tag is the last one that modified the code in question, even though the bug had been there already before.

Still, the fix will not apply to the code before that commit.

---
 drivers/base/power/main.c |  148 +++++++++++++++++++++-------------------------
 1 file changed, 68 insertions(+), 80 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +26 -12

@@ -579,7 +579,7 @@ bool dev_pm_skip_resume(struct device *d
 }
 
 /**
- * device_resume_noirq - Execute a "noirq resume" callback for given device.
+ * __device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.

@@ -587,7 +587,7 @@ bool dev_pm_skip_resume(struct device *d
  * The driver of @dev will not receive interrupts while this function is being
  * executed.
  */
-static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
+static void __device_resume_noirq(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -655,7 +655,13 @@ Skip:
 Out:
        complete_all(&dev->power.completion);
        TRACE_RESUME(error);
-       return error;
+
+       if (error) {
+               suspend_stats.failed_resume_noirq++;
+               dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
+       }
 }
 
 static bool is_async(struct device *dev)

@@ -668,11 +674,15 @@ static bool dpm_async_fn(struct device *
 {
        reinit_completion(&dev->power.completion);
 
-       if (is_async(dev)) {
-               get_device(dev);
-               async_schedule_dev(func, dev);
+       if (!is_async(dev))
+               return false;
+
+       get_device(dev);
+
+       if (async_schedule_dev_nocall(func, dev))
                return true;
-       }
+
+       put_device(dev);
 
        return false;
 }

@@ -680,15 +690,19 @@ static bool dpm_async_fn(struct device *
 static void async_resume_noirq(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
-
-       error = device_resume_noirq(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
 
+       __device_resume_noirq(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume_noirq(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume_noirq))
+               return;
+
+       __device_resume_noirq(dev, pm_transition, false);
+}
+
 static void dpm_noirq_resume_devices(pm_message_t state)
 {
        struct device *dev;

@@ -698,14 +712,6 @@ static void dpm_noirq_resume_devices(pm_
        mutex_lock(&dpm_list_mtx);

drivers/base/power/main.c +19 -19

        while (!list_empty(&dpm_noirq_list)) {
                dev = to_device(dpm_noirq_list.next);
                get_device(dev);

@@ -713,17 +719,7 @@ static void dpm_noirq_resume_devices(pm_
 
                mutex_unlock(&dpm_list_mtx);
 
-               if (!is_async(dev)) {
-                       int error;
-
-                       error = device_resume_noirq(dev, state, false);
-                       if (error) {
-                               suspend_stats.failed_resume_noirq++;
-                               dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
-                               dpm_save_failed_dev(dev_name(dev));
-                               pm_dev_err(dev, state, " noirq", error);
-                       }
-               }
+               device_resume_noirq(dev);
 
                put_device(dev);
 

@@ -751,14 +747,14 @@ void dpm_resume_noirq(pm_message_t state
 }
 
 /**
- * device_resume_early - Execute an "early resume" callback for given device.
+ * __device_resume_early - Execute an "early resume" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.
  *
  * Runtime PM is disabled for @dev while this function is being executed.
  */
-static int device_resume_early(struct device *dev, pm_message_t state, bool async)
+static void __device_resume_early(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -811,21 +807,31 @@ Out:
 
        pm_runtime_enable(dev);
        complete_all(&dev->power.completion);
-       return error;
+
+       if (error) {
+               suspend_stats.failed_resume_early++;
+               dpm_save_failed_step(SUSPEND_RESUME_EARLY);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async early" : " early", error);
+       }
 }
 
 static void async_resume_early(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
-
-       error = device_resume_early(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
 
+       __device_resume_early(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume_early(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume_early))
+               return;
+
+       __device_resume_early(dev, pm_transition, false);
+}
+
 /**
  * dpm_resume_early - Execute "early resume" callbacks for all devices.
  * @state: PM transition of the system being carried out.

@@ -839,14 +845,6 @@ void dpm_resume_early(pm_message_t state
        mutex_lock(&dpm_list_mtx);

drivers/base/power/main.c +18 -18

        while (!list_empty(&dpm_late_early_list)) {
                dev = to_device(dpm_late_early_list.next);
                get_device(dev);

@@ -854,17 +852,7 @@ void dpm_resume_early(pm_message_t state
 
                mutex_unlock(&dpm_list_mtx);
 
-               if (!is_async(dev)) {
-                       int error;
-
-                       error = device_resume_early(dev, state, false);
-                       if (error) {
-                               suspend_stats.failed_resume_early++;
-                               dpm_save_failed_step(SUSPEND_RESUME_EARLY);
-                               dpm_save_failed_dev(dev_name(dev));
-                               pm_dev_err(dev, state, " early", error);
-                       }
-               }
+               device_resume_early(dev);
 
                put_device(dev);
 

@@ -888,12 +876,12 @@ void dpm_resume_start(pm_message_t state
 EXPORT_SYMBOL_GPL(dpm_resume_start);
 
 /**
- * device_resume - Execute "resume" callbacks for given device.
+ * __device_resume - Execute "resume" callbacks for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.
  */
-static int device_resume(struct device *dev, pm_message_t state, bool async)
+static void __device_resume(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -975,20 +963,30 @@ static int device_resume(struct device *
 
        TRACE_RESUME(error);
 
-       return error;
+       if (error) {
+               suspend_stats.failed_resume++;
+               dpm_save_failed_step(SUSPEND_RESUME);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async" : "", error);
+       }
 }
 
 static void async_resume(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
 
-       error = device_resume(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
+       __device_resume(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume))
+               return;
+
+       __device_resume(dev, pm_transition, false);
+}
+
 /**
  * dpm_resume - Execute "resume" callbacks for non-sysdev devices.
  * @state: PM transition of the system being carried out.

drivers/base/power/main.c +2 -9

        while (!list_empty(&dpm_suspended_list)) {
                dev = to_device(dpm_suspended_list.next);
+
                get_device(dev);
-               if (!is_async(dev)) {
-                       int error;
 
-                       mutex_unlock(&dpm_list_mtx);
+               mutex_unlock(&dpm_list_mtx);
 
-                       error = device_resume(dev, state, false);
-                       if (error) {
-                               suspend_stats.failed_resume++;
-                               dpm_save_failed_step(SUSPEND_RESUME);
-                               dpm_save_failed_dev(dev_name(dev));
-                               pm_dev_err(dev, state, "", error);

Quoted 19 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

It is reported that in low-memory situations the system-wide resume core code deadlocks, because async_schedule_dev() executes its argument function synchronously if it cannot allocate memory (an not only then) and that function attempts to acquire a mutex that is already held.

Address this by changing the code in question to use async_schedule_dev_nocall() for scheduling the asynchronous execution of device suspend and resume functions and to directly run them synchronously if async_schedule_dev_nocall() returns false.

Fixes 09beebd8f93b ("PM: sleep: core: Switch back to async_schedule_dev()")

Link https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/ open here

Reported-by Youngmin Nam <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Quoted 4 lines

The commit pointed to by the Fixes: tag is the last one that modified the code in question, even though the bug had been there already before.

Still, the fix will not apply to the code before that commit.

An option could be to just do "Cc: [email protected] # v5.7+" instead of pointing to a commit with a Fixes tag.

Quoted 91 lines

---
 drivers/base/power/main.c |  148 +++++++++++++++++++++-------------------------
 1 file changed, 68 insertions(+), 80 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +26 -12

@@ -579,7 +579,7 @@ bool dev_pm_skip_resume(struct device *d
 }
 
 /**
- * device_resume_noirq - Execute a "noirq resume" callback for given device.
+ * __device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.

@@ -587,7 +587,7 @@ bool dev_pm_skip_resume(struct device *d
  * The driver of @dev will not receive interrupts while this function is being
  * executed.
  */
-static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
+static void __device_resume_noirq(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -655,7 +655,13 @@ Skip:
 Out:
        complete_all(&dev->power.completion);
        TRACE_RESUME(error);
-       return error;
+
+       if (error) {
+               suspend_stats.failed_resume_noirq++;
+               dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
+       }
 }
 
 static bool is_async(struct device *dev)

@@ -668,11 +674,15 @@ static bool dpm_async_fn(struct device *
 {
        reinit_completion(&dev->power.completion);
 
-       if (is_async(dev)) {
-               get_device(dev);
-               async_schedule_dev(func, dev);
+       if (!is_async(dev))
+               return false;
+
+       get_device(dev);
+
+       if (async_schedule_dev_nocall(func, dev))
                return true;
-       }
+
+       put_device(dev);
 
        return false;
 }

@@ -680,15 +690,19 @@ static bool dpm_async_fn(struct device *
 static void async_resume_noirq(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
-
-       error = device_resume_noirq(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
 
+       __device_resume_noirq(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume_noirq(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume_noirq))
+               return;
+
+       __device_resume_noirq(dev, pm_transition, false);
+}
+
 static void dpm_noirq_resume_devices(pm_message_t state)
 {
        struct device *dev;

@@ -698,14 +712,6 @@ static void dpm_noirq_resume_devices(pm_
        mutex_lock(&dpm_list_mtx);
        pm_transition = state;
 

drivers/base/power/main.c static void dpm_noirq_resume_devices(pm_ · line 708

Quoted 8 lines

drivers/base/power/main.c +0 -8

-       /*
-        * Advanced the async threads upfront,
-        * in case the starting of async threads is
-        * delayed by non-async resuming devices.
-        */
-       list_for_each_entry(dev, &dpm_noirq_list, power.entry)
-               dpm_async_fn(dev, async_resume_noirq);
-

If I understand correctly, this means that we are no longer going to run the async devices upfront, right?

Depending on how devices get ordered in the dpm_noirq_list, it sounds like the above could have a negative impact on the total resume time!?

Of course, if all devices would be async capable this wouldn't be a problem...

Quoted 79 lines

drivers/base/power/main.c +19 -19

        while (!list_empty(&dpm_noirq_list)) {
                dev = to_device(dpm_noirq_list.next);
                get_device(dev);

@@ -713,17 +719,7 @@ static void dpm_noirq_resume_devices(pm_
 
                mutex_unlock(&dpm_list_mtx);
 
-               if (!is_async(dev)) {
-                       int error;
-
-                       error = device_resume_noirq(dev, state, false);
-                       if (error) {
-                               suspend_stats.failed_resume_noirq++;
-                               dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
-                               dpm_save_failed_dev(dev_name(dev));
-                               pm_dev_err(dev, state, " noirq", error);
-                       }
-               }
+               device_resume_noirq(dev);
 
                put_device(dev);
 

@@ -751,14 +747,14 @@ void dpm_resume_noirq(pm_message_t state
 }
 
 /**
- * device_resume_early - Execute an "early resume" callback for given device.
+ * __device_resume_early - Execute an "early resume" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.
  *
  * Runtime PM is disabled for @dev while this function is being executed.
  */
-static int device_resume_early(struct device *dev, pm_message_t state, bool async)
+static void __device_resume_early(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -811,21 +807,31 @@ Out:
 
        pm_runtime_enable(dev);
        complete_all(&dev->power.completion);
-       return error;
+
+       if (error) {
+               suspend_stats.failed_resume_early++;
+               dpm_save_failed_step(SUSPEND_RESUME_EARLY);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async early" : " early", error);
+       }
 }
 
 static void async_resume_early(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
-
-       error = device_resume_early(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
 
+       __device_resume_early(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume_early(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume_early))
+               return;
+
+       __device_resume_early(dev, pm_transition, false);
+}
+
 /**
  * dpm_resume_early - Execute "early resume" callbacks for all devices.
  * @state: PM transition of the system being carried out.

@@ -839,14 +845,6 @@ void dpm_resume_early(pm_message_t state
        mutex_lock(&dpm_list_mtx);

drivers/base/power/main.c void dpm_resume_early(pm_message_t state · line 849

Quoted 10 lines

drivers/base/power/main.c +0 -8

        pm_transition = state;
 
-       /*
-        * Advanced the async threads upfront,
-        * in case the starting of async threads is
-        * delayed by non-async resuming devices.
-        */
-       list_for_each_entry(dev, &dpm_late_early_list, power.entry)
-               dpm_async_fn(dev, async_resume_early);
-

Ditto.

Quoted 74 lines

drivers/base/power/main.c +18 -18

        while (!list_empty(&dpm_late_early_list)) {
                dev = to_device(dpm_late_early_list.next);
                get_device(dev);

@@ -854,17 +852,7 @@ void dpm_resume_early(pm_message_t state
 
                mutex_unlock(&dpm_list_mtx);
 
-               if (!is_async(dev)) {
-                       int error;
-
-                       error = device_resume_early(dev, state, false);
-                       if (error) {
-                               suspend_stats.failed_resume_early++;
-                               dpm_save_failed_step(SUSPEND_RESUME_EARLY);
-                               dpm_save_failed_dev(dev_name(dev));
-                               pm_dev_err(dev, state, " early", error);
-                       }
-               }
+               device_resume_early(dev);
 
                put_device(dev);
 

@@ -888,12 +876,12 @@ void dpm_resume_start(pm_message_t state
 EXPORT_SYMBOL_GPL(dpm_resume_start);
 
 /**
- * device_resume - Execute "resume" callbacks for given device.
+ * __device_resume - Execute "resume" callbacks for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.
  */
-static int device_resume(struct device *dev, pm_message_t state, bool async)
+static void __device_resume(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -975,20 +963,30 @@ static int device_resume(struct device *
 
        TRACE_RESUME(error);
 
-       return error;
+       if (error) {
+               suspend_stats.failed_resume++;
+               dpm_save_failed_step(SUSPEND_RESUME);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async" : "", error);
+       }
 }
 
 static void async_resume(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
 
-       error = device_resume(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
+       __device_resume(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume))
+               return;
+
+       __device_resume(dev, pm_transition, false);
+}
+
 /**
  * dpm_resume - Execute "resume" callbacks for non-sysdev devices.
  * @state: PM transition of the system being carried out.

drivers/base/power/main.c void dpm_resume(pm_message_t state) · line 1013

Quoted 6 lines

drivers/base/power/main.c +0 -3

@@ -1008,27 +1006,17 @@ void dpm_resume(pm_message_t state)
        pm_transition = state;
        async_error = 0;
 
-       list_for_each_entry(dev, &dpm_suspended_list, power.entry)
-               dpm_async_fn(dev, async_resume);
-

Ditto.

Quoted 21 lines

drivers/base/power/main.c +5 -10

        while (!list_empty(&dpm_suspended_list)) {
                dev = to_device(dpm_suspended_list.next);
+
                get_device(dev);
-               if (!is_async(dev)) {
-                       int error;
 
-                       mutex_unlock(&dpm_list_mtx);
+               mutex_unlock(&dpm_list_mtx);
 
-                       error = device_resume(dev, state, false);
-                       if (error) {
-                               suspend_stats.failed_resume++;
-                               dpm_save_failed_step(SUSPEND_RESUME);
-                               dpm_save_failed_dev(dev_name(dev));
-                               pm_dev_err(dev, state, "", error);
-                       }
+               device_resume(dev);
+
+               mutex_lock(&dpm_list_mtx);
 

drivers/base/power/main.c void dpm_resume(pm_message_t state) · line 1022

Quoted 5 lines

drivers/base/power/main.c +0 -2

-                       mutex_lock(&dpm_list_mtx);
-               }
                if (!list_empty(&dev->power.entry))
                        list_move_tail(&dev->power.entry, &dpm_prepared_list);
 

Other than the potential issue I pointed out, the code as such looks good to me!

Quoted 24 lines · Ulf Hansson

Quoted 22 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

It is reported that in low-memory situations the system-wide resume core code deadlocks, because async_schedule_dev() executes its argument function synchronously if it cannot allocate memory (an not only then) and that function attempts to acquire a mutex that is already held.

Address this by changing the code in question to use async_schedule_dev_nocall() for scheduling the asynchronous execution of device suspend and resume functions and to directly run them synchronously if async_schedule_dev_nocall() returns false.

Fixes 09beebd8f93b ("PM: sleep: core: Switch back to async_schedule_dev()")

Link https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/ open here

Reported-by Youngmin Nam <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---

The commit pointed to by the Fixes: tag is the last one that modified the code in question, even though the bug had been there already before.

Quoted 4 lines

Quoted 1 line

Still, the fix will not apply to the code before that commit.

An option could be to just do "Cc: [email protected] # v5.7+" instead of pointing to a commit with a Fixes tag.

Right, but one can argue that every commit with a "Cc: stable" tag is a fix, so it should carry a Fixes: tag too anyway.

Quoted 92 lines

Quoted 92 lines

---
 drivers/base/power/main.c |  148 +++++++++++++++++++++-------------------------
 1 file changed, 68 insertions(+), 80 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +26 -12

@@ -579,7 +579,7 @@ bool dev_pm_skip_resume(struct device *d
 }
 
 /**
- * device_resume_noirq - Execute a "noirq resume" callback for given device.
+ * __device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.

@@ -587,7 +587,7 @@ bool dev_pm_skip_resume(struct device *d
  * The driver of @dev will not receive interrupts while this function is being
  * executed.
  */
-static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
+static void __device_resume_noirq(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -655,7 +655,13 @@ Skip:
 Out:
        complete_all(&dev->power.completion);
        TRACE_RESUME(error);
-       return error;
+
+       if (error) {
+               suspend_stats.failed_resume_noirq++;
+               dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
+       }
 }
 
 static bool is_async(struct device *dev)

@@ -668,11 +674,15 @@ static bool dpm_async_fn(struct device *
 {
        reinit_completion(&dev->power.completion);
 
-       if (is_async(dev)) {
-               get_device(dev);
-               async_schedule_dev(func, dev);
+       if (!is_async(dev))
+               return false;
+
+       get_device(dev);
+
+       if (async_schedule_dev_nocall(func, dev))
                return true;
-       }
+
+       put_device(dev);
 
        return false;
 }

@@ -680,15 +690,19 @@ static bool dpm_async_fn(struct device *
 static void async_resume_noirq(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
-
-       error = device_resume_noirq(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
 
+       __device_resume_noirq(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume_noirq(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume_noirq))
+               return;
+
+       __device_resume_noirq(dev, pm_transition, false);
+}
+
 static void dpm_noirq_resume_devices(pm_message_t state)
 {
        struct device *dev;

@@ -698,14 +712,6 @@ static void dpm_noirq_resume_devices(pm_
        mutex_lock(&dpm_list_mtx);
        pm_transition = state;
 

Quoted 11 lines

drivers/base/power/main.c static void dpm_noirq_resume_devices(pm_ · line 708

Quoted 8 lines

drivers/base/power/main.c +0 -8

-       /*
-        * Advanced the async threads upfront,
-        * in case the starting of async threads is
-        * delayed by non-async resuming devices.
-        */
-       list_for_each_entry(dev, &dpm_noirq_list, power.entry)
-               dpm_async_fn(dev, async_resume_noirq);
-

If I understand correctly, this means that we are no longer going to run the async devices upfront, right?

Right.

Quoted 2 lines

Depending on how devices get ordered in the dpm_noirq_list, it sounds like the above could have a negative impact on the total resume time!?

It could, but it is unclear at this time whether or not it will.

Quoted 2 lines

Of course, if all devices would be async capable this wouldn't be a problem...

Sure.

So the existing behavior can be restored with the help of an additional device flag, but I didn't decide to add such a flag just yet.

I'll probably do it in 6.9, unless the performance impact is serious enough, in which case it can be added earlier.

I still would prefer to get to a point at which the suspend and resume paths are analogous (from the async POV) and that's what happens after this patch, so I'd say that IMO it is better to address any performance regressions on top of it.

Quoted 26 lines · Rafael J. Wysocki

Quoted 24 lines · Ulf Hansson

Quoted 22 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

It is reported that in low-memory situations the system-wide resume core code deadlocks, because async_schedule_dev() executes its argument function synchronously if it cannot allocate memory (an not only then) and that function attempts to acquire a mutex that is already held.

Address this by changing the code in question to use async_schedule_dev_nocall() for scheduling the asynchronous execution of device suspend and resume functions and to directly run them synchronously if async_schedule_dev_nocall() returns false.

Fixes 09beebd8f93b ("PM: sleep: core: Switch back to async_schedule_dev()")

Link https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/ open here

Reported-by Youngmin Nam <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---

The commit pointed to by the Fixes: tag is the last one that modified the code in question, even though the bug had been there already before.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Still, the fix will not apply to the code before that commit.

An option could be to just do "Cc: [email protected] # v5.7+" instead of pointing to a commit with a Fixes tag.

Right, but one can argue that every commit with a "Cc: stable" tag is a fix, so it should carry a Fixes: tag too anyway.

Yes, certainly. But in this case it's more questionable as it's not really fixing the commit it points out.

Note that, I have no strong opinion here, but maybe Greg has a preferred way?

Quoted 120 lines

Quoted 92 lines

Quoted 92 lines

---
 drivers/base/power/main.c |  148 +++++++++++++++++++++-------------------------
 1 file changed, 68 insertions(+), 80 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +26 -12

@@ -579,7 +579,7 @@ bool dev_pm_skip_resume(struct device *d
 }
 
 /**
- * device_resume_noirq - Execute a "noirq resume" callback for given device.
+ * __device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being resumed asynchronously.

@@ -587,7 +587,7 @@ bool dev_pm_skip_resume(struct device *d
  * The driver of @dev will not receive interrupts while this function is being
  * executed.
  */
-static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
+static void __device_resume_noirq(struct device *dev, pm_message_t state, bool async)
 {
        pm_callback_t callback = NULL;
        const char *info = NULL;

@@ -655,7 +655,13 @@ Skip:
 Out:
        complete_all(&dev->power.completion);
        TRACE_RESUME(error);
-       return error;
+
+       if (error) {
+               suspend_stats.failed_resume_noirq++;
+               dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
+               dpm_save_failed_dev(dev_name(dev));
+               pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
+       }
 }
 
 static bool is_async(struct device *dev)

@@ -668,11 +674,15 @@ static bool dpm_async_fn(struct device *
 {
        reinit_completion(&dev->power.completion);
 
-       if (is_async(dev)) {
-               get_device(dev);
-               async_schedule_dev(func, dev);
+       if (!is_async(dev))
+               return false;
+
+       get_device(dev);
+
+       if (async_schedule_dev_nocall(func, dev))
                return true;
-       }
+
+       put_device(dev);
 
        return false;
 }

@@ -680,15 +690,19 @@ static bool dpm_async_fn(struct device *
 static void async_resume_noirq(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
-       int error;
-
-       error = device_resume_noirq(dev, pm_transition, true);
-       if (error)
-               pm_dev_err(dev, pm_transition, " async", error);
 
+       __device_resume_noirq(dev, pm_transition, true);
        put_device(dev);
 }
 
+static void device_resume_noirq(struct device *dev)
+{
+       if (dpm_async_fn(dev, async_resume_noirq))
+               return;
+
+       __device_resume_noirq(dev, pm_transition, false);
+}
+
 static void dpm_noirq_resume_devices(pm_message_t state)
 {
        struct device *dev;

@@ -698,14 +712,6 @@ static void dpm_noirq_resume_devices(pm_
        mutex_lock(&dpm_list_mtx);
        pm_transition = state;
 

Quoted 11 lines

drivers/base/power/main.c static void dpm_noirq_resume_devices(pm_ · line 708

Quoted 8 lines

drivers/base/power/main.c +0 -8

-       /*
-        * Advanced the async threads upfront,
-        * in case the starting of async threads is
-        * delayed by non-async resuming devices.
-        */
-       list_for_each_entry(dev, &dpm_noirq_list, power.entry)
-               dpm_async_fn(dev, async_resume_noirq);
-

If I understand correctly, this means that we are no longer going to run the async devices upfront, right?

Right.

Quoted 2 lines

Depending on how devices get ordered in the dpm_noirq_list, it sounds like the above could have a negative impact on the total resume time!?

It could, but it is unclear at this time whether or not it will.

Quoted 2 lines

Of course, if all devices would be async capable this wouldn't be a problem...

Sure.

So the existing behavior can be restored with the help of an additional device flag, but I didn't decide to add such a flag just yet.

Quoted 7 lines

I'll probably do it in 6.9, unless the performance impact is serious enough, in which case it can be added earlier.

I still would prefer to get to a point at which the suspend and resume paths are analogous (from the async POV) and that's what happens after this patch, so I'd say that IMO it is better to address any performance regressions on top of it.

Fair enough!

[...]

Feel free to add my Reviewed-by for the series!

Quoted 31 lines · Rafael J. Wysocki

Quoted 28 lines · Ulf Hansson

Quoted 17 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

It is reported that in low-memory situations the system-wide resume core code deadlocks, because async_schedule_dev() executes its argument function synchronously if it cannot allocate memory (an not only then) and that function attempts to acquire a mutex that is already held.

Address this by changing the code in question to use async_schedule_dev_nocall() for scheduling the asynchronous execution of device suspend and resume functions and to directly run them synchronously if async_schedule_dev_nocall() returns false.

Fixes 09beebd8f93b ("PM: sleep: core: Switch back to async_schedule_dev()")

Link https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/ open here

Reported-by Youngmin Nam <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

Quoted 6 lines

---

The commit pointed to by the Fixes: tag is the last one that modified the code in question, even though the bug had been there already before.

Still, the fix will not apply to the code before that commit.

An option could be to just do "Cc: [email protected] # v5.7+" instead of pointing to a commit with a Fixes tag.

Quoted 2 lines

Right, but one can argue that every commit with a "Cc: stable" tag is a fix, so it should carry a Fixes: tag too anyway.

Yes, certainly. But in this case it's more questionable as it's not really fixing the commit it points out.

Note that, I have no strong opinion here, but maybe Greg has a preferred way?

Hi Everyone,

As reported here

https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/

the device suspend-resume code running during system-wide PM transitions deadlock on low memory, because it attempts to acquire a mutex that's already held by it in those cases.

This series addresses the issue by changing the resume code behavior to directly run the device PM functions synchronously if they cannot be scheduled for asynchronous executions (patch [3/3]).

For this purpose, the async code is rearranged (patch [1/3]) and a new variant of async_schedule_dev() is introduced (patch [2/3]).

Quoted 15 lines · Rafael J. Wysocki

Hi Everyone,

As reported here

https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/

the device suspend-resume code running during system-wide PM transitions deadlock on low memory, because it attempts to acquire a mutex that's already held by it in those cases.

This series addresses the issue by changing the resume code behavior to directly run the device PM functions synchronously if they cannot be scheduled for asynchronous executions (patch [3/3]).

Quoted 2 lines

For this purpose, the async code is rearranged (patch [1/3]) and a new variant of async_schedule_dev() is introduced (patch [2/3]).

Given the lack of negative feedback, I've queued up this series for 6.8-rc1.

Please let me know if there are any issues with that.

Thanks!

Quoted 16 lines · Rafael J. Wysocki

Quoted 15 lines · Rafael J. Wysocki

Hi Everyone,

As reported here

https://lore.kernel.org/linux-pm/ZYvjiqX6EsL15moe@perf/

the device suspend-resume code running during system-wide PM transitions deadlock on low memory, because it attempts to acquire a mutex that's already held by it in those cases.

This series addresses the issue by changing the resume code behavior to directly run the device PM functions synchronously if they cannot be scheduled for asynchronous executions (patch [3/3]).

Quoted 9 lines

Quoted 2 lines

For this purpose, the async code is rearranged (patch [1/3]) and a new variant of async_schedule_dev() is introduced (patch [2/3]).

Given the lack of negative feedback, I've queued up this series for 6.8-rc1.

Please let me know if there are any issues with that.

Thanks!

Hi Rafael

We haven't seen any regression issue under our stress test.

So, feel free to add

Tested-by Youngmin Nam <[email protected]>