PM / sleep: Mechanism to find source aborting kernel suspend transition

PATCH Superseded Vimal Kumar 2 participants 4 messages Feb 5, 2024 – Feb 7, 2024

Subsystems (4)

Driver core Suspend to RAM Hibernation Power management core

lore

Share & tools

lore

Vimal Kumar

author

2024-02-05 17:08 UTC #0 lore raw

PATCH v4 1/1 PM / sleep: Mechanism to find source aborting kernel suspend transition

Sometimes kernel suspend transitions can be aborted unconditionally by manipulating pm_abort_suspend value using "hard" wakeup triggers or through "pm_system_wakeup()".

There is no way to trace the source path of module or subsystem which aborted the suspend transitions. This change will create a list of wakeup sources aborting suspend in progress through "hard" events as well as subsytems aborting suspend using "pm_system_wakeup()".

Example: Existing suspend failure logs:

[  349.708359] PM: Some devices failed to suspend, or early wake event detected
[  350.327842] PM: suspend exit

Suspend failure logs with this change:

[  518.761835] PM: Some devices failed to suspend, or early wake event detected
[  519.486939] PM: wakeup source or subsystem uart_suspend_port aborted suspend
[  519.500594] PM: suspend exit

Here we can clearly identify the module triggerring abort suspend.

Co-developed-by Chinmoy Ghosh <[email protected]>

Signed-off-by Chinmoy Ghosh <[email protected]>

Co-developed-by Mintu Patel <[email protected]>

Signed-off-by Mintu Patel <[email protected]>

Co-developed-by Vishal Badole <[email protected]>

Signed-off-by Vishal Badole <[email protected]>

Signed-off-by Vimal Kumar <[email protected]>

---
Changes in v4:
- Changed GFP_KERNEL flag to GFP_ATOMIC
- Changed mutex_lock to raw_spin_lock

---
 drivers/base/power/wakeup.c | 100 +++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

drivers/base/power/wakeup.c +10 -0

index a917219feea6..b04794557eef 100644
@@ -73,6 +73,16 @@ static struct wakeup_source deleted_ws = {
 
 static DEFINE_IDA(wakeup_ida);
 
+#ifdef CONFIG_PM_DEBUG
+static DEFINE_RAW_SPINLOCK(pm_abort_suspend_list_lock);
+
+struct pm_abort_suspend_source {
+	struct list_head list;
+	char *source_triggering_abort_suspend;
+};
+static LIST_HEAD(pm_abort_suspend_list);
+#endif
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

@@ -575,6 +585,54 @@ static void wakeup_source_activate(struct wakeup_source *ws)

	trace_wakeup_source_activate(ws->name, cec);

}

+#ifdef CONFIG_PM_DEBUG
+/**
+ * pm_abort_suspend_list_clear - Clear pm_abort_suspend_list.
+ *
+ * The pm_abort_suspend_list will be cleared when system PM exits.
+ */
+static void pm_abort_suspend_list_clear(void)
+{
+ unsigned long flags;
+ struct pm_abort_suspend_source *info, *tmp;
+
+ raw_spin_lock_irqsave(&pm_abort_suspend_list_lock, flags);
+ list_for_each_entry_safe(info, tmp, &pm_abort_suspend_list, list) {
+ list_del(&info->list);
+ kfree(info);
+ }
+ raw_spin_unlock_irqrestore(&pm_abort_suspend_list_lock, flags);
+}
+
+/**
+ * pm_abort_suspend_source_add - Update pm_abort_suspend_list
+ * @source_name: Wakeup_source or function aborting suspend transitions.
+ *
+ * Add the source name responsible for updating the abort_suspend flag in the
+ * pm_abort_suspend_list.
+ */
+static void pm_abort_suspend_source_add(const char *source_name)
+{
+ unsigned long flags;
+ struct pm_abort_suspend_source *info;
+
+ info = kmalloc(sizeof(*info), GFP_ATOMIC);
+ if (!info)
+ return;
+
+ INIT_LIST_HEAD(&info->list);
+ info->source_triggering_abort_suspend = kstrdup(source_name, GFP_ATOMIC);
+ if (!info->source_triggering_abort_suspend) {
+ kfree(info);
+ return;
+ }
+
+ raw_spin_lock_irqsave(&pm_abort_suspend_list_lock, flags);
+ list_add_tail(&info->list, &pm_abort_suspend_list);
+ raw_spin_unlock_irqrestore(&pm_abort_suspend_list_lock, flags);
+}
+#endif
+
/**
* wakeup_source_report_event - Report wakeup event using the given source.
* @ws: Wakeup source to report the event for.
@@ -590,8 +648,13 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)

	if (!ws->active)
		wakeup_source_activate(ws);

- if (hard)
+ if (hard) {
+#ifdef CONFIG_PM_DEBUG
+ if (pm_suspend_target_state != PM_SUSPEND_ON)
+ pm_abort_suspend_source_add(ws->name);
+#endif

		pm_system_wakeup();

+ }
}

/**
@@ -893,12 +956,47 @@ bool pm_wakeup_pending(void)

		pm_print_active_wakeup_sources();
	}

+#ifdef CONFIG_PM_DEBUG
+ if (atomic_read(&pm_abort_suspend) > 0) {
+ struct pm_abort_suspend_source *info;
+
+ raw_spin_lock_irqsave(&pm_abort_suspend_list_lock, flags);
+ list_for_each_entry(info, &pm_abort_suspend_list, list) {
+ pm_pr_dbg("wakeup source or subsystem %s aborted suspend\n",
+ info->source_triggering_abort_suspend);
+ }
+ raw_spin_unlock_irqrestore(&pm_abort_suspend_list_lock, flags);
+ pm_abort_suspend_list_clear();
+ }
+#endif
+

	return ret || atomic_read(&pm_abort_suspend) > 0;

}
EXPORT_SYMBOL_GPL(pm_wakeup_pending);

void pm_system_wakeup(void)
{
+
+#ifdef CONFIG_PM_DEBUG
+#ifdef CONFIG_DEBUG_INFO
+ if (pm_suspend_target_state != PM_SUSPEND_ON) {
+ char *source_name = kasprintf(GFP_ATOMIC,
+ "%ps",
+ __builtin_return_address(0));
+ if (!source_name)
+ goto exit;
+
+ if (strcmp(source_name, "pm_wakeup_ws_event"))
+ pm_abort_suspend_source_add(source_name);
+
+ kfree(source_name);
+ }
+exit:
+#else
+ if (pm_suspend_target_state != PM_SUSPEND_ON)
+ pm_pr_dbg("Some wakeup source or subsystem aborted suspend\n");
+#endif
+#endif

	atomic_inc(&pm_abort_suspend);
	s2idle_wake();

}
--
2.25.1

Greg Kroah-Hartman

maintainer

2024-02-05 19:33 UTC #1 lore raw

Quoted 27 lines · Vimal Kumar

Sometimes kernel suspend transitions can be aborted unconditionally by manipulating pm_abort_suspend value using "hard" wakeup triggers or through "pm_system_wakeup()".
There is no way to trace the source path of module or subsystem which aborted the suspend transitions. This change will create a list of wakeup sources aborting suspend in progress through "hard" events as well as subsytems aborting suspend using "pm_system_wakeup()".
Example: Existing suspend failure logs:
[  349.708359] PM: Some devices failed to suspend, or early wake event detected
[  350.327842] PM: suspend exit
Suspend failure logs with this change:
[  518.761835] PM: Some devices failed to suspend, or early wake event detected
[  519.486939] PM: wakeup source or subsystem uart_suspend_port aborted suspend
[  519.500594] PM: suspend exit
Here we can clearly identify the module triggerring abort suspend.
Co-developed-by Chinmoy Ghosh <[email protected]>
Signed-off-by Chinmoy Ghosh <[email protected]>
Co-developed-by Mintu Patel <[email protected]>
Signed-off-by Mintu Patel <[email protected]>
Co-developed-by Vishal Badole <[email protected]>
Signed-off-by Vishal Badole <[email protected]>
Signed-off-by Vimal Kumar <[email protected]>

Quoted 4 lines

---
Changes in v4:
- Changed GFP_KERNEL flag to GFP_ATOMIC
- Changed mutex_lock to raw_spin_lock

why raw?

Quoted 26 lines

---
 drivers/base/power/wakeup.c | 100 +++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

drivers/base/power/wakeup.c +10 -0

index a917219feea6..b04794557eef 100644
@@ -73,6 +73,16 @@ static struct wakeup_source deleted_ws = {
 
 static DEFINE_IDA(wakeup_ida);
 
+#ifdef CONFIG_PM_DEBUG
+static DEFINE_RAW_SPINLOCK(pm_abort_suspend_list_lock);
+
+struct pm_abort_suspend_source {
+	struct list_head list;
+	char *source_triggering_abort_suspend;
+};
+static LIST_HEAD(pm_abort_suspend_list);
+#endif
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 588

Quoted 4 lines

drivers/base/power/wakeup.c +1 -0

@@ -575,6 +585,54 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 	trace_wakeup_source_activate(ws->name, cec);
 }
 
+#ifdef CONFIG_PM_DEBUG

Please do not add #ifdef to .c files, this makes this file even messier.

drivers/base/power/wakeup.c static void wakeup_source_report_event(struct wakeup_source *ws, bool hard) · line 655

Quoted 10 lines

drivers/base/power/wakeup.c +5 -1

@@ -590,8 +648,13 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
 	if (!ws->active)
 		wakeup_source_activate(ws);
 
-	if (hard)
+	if (hard) {
+#ifdef CONFIG_PM_DEBUG
+		if (pm_suspend_target_state != PM_SUSPEND_ON)
+			pm_abort_suspend_source_add(ws->name);
+#endif

Especially for stuff like this, if you write your .h files properly, no #ifdef are needed.

Quoted 40 lines

drivers/base/power/wakeup.c +26 -0

 		pm_system_wakeup();
+	}
 }
 
 /**

@@ -893,12 +956,47 @@ bool pm_wakeup_pending(void)
 		pm_print_active_wakeup_sources();
 	}
 
+#ifdef CONFIG_PM_DEBUG
+	if (atomic_read(&pm_abort_suspend) > 0) {
+		struct pm_abort_suspend_source *info;
+
+		raw_spin_lock_irqsave(&pm_abort_suspend_list_lock, flags);
+		list_for_each_entry(info, &pm_abort_suspend_list, list) {
+			pm_pr_dbg("wakeup source or subsystem %s aborted suspend\n",
+					info->source_triggering_abort_suspend);
+		}
+		raw_spin_unlock_irqrestore(&pm_abort_suspend_list_lock, flags);
+		pm_abort_suspend_list_clear();
+	}
+#endif
+
 	return ret || atomic_read(&pm_abort_suspend) > 0;
 }
 EXPORT_SYMBOL_GPL(pm_wakeup_pending);
 
 void pm_system_wakeup(void)
 {
+
+#ifdef CONFIG_PM_DEBUG
+#ifdef CONFIG_DEBUG_INFO
+	if (pm_suspend_target_state != PM_SUSPEND_ON) {
+		char *source_name = kasprintf(GFP_ATOMIC,
+					"%ps",
+					__builtin_return_address(0));
+		if (!source_name)
+			goto exit;
+
+		if (strcmp(source_name, "pm_wakeup_ws_event"))

drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 999

Quoted 10 lines

drivers/base/power/wakeup.c +10 -0

+			pm_abort_suspend_source_add(source_name);
+
+		kfree(source_name);
+	}
+exit:
+#else
+	if (pm_suspend_target_state != PM_SUSPEND_ON)
+		pm_pr_dbg("Some wakeup source or subsystem aborted suspend\n");
+#endif
+#endif

Would you want to maintain this #ifdef nesting mess for the next 20 years? Please do not do this.

thanks,

greg k-h

Vimal Kumar

author

2024-02-07 03:55 UTC #2 lore raw

Quoted 33 lines · Greg Kroah-Hartman

Quoted 27 lines · Vimal Kumar
Sometimes kernel suspend transitions can be aborted unconditionally by manipulating pm_abort_suspend value using "hard" wakeup triggers or through "pm_system_wakeup()".
There is no way to trace the source path of module or subsystem which aborted the suspend transitions. This change will create a list of wakeup sources aborting suspend in progress through "hard" events as well as subsytems aborting suspend using "pm_system_wakeup()".
Example: Existing suspend failure logs:
[  349.708359] PM: Some devices failed to suspend, or early wake event detected
[  350.327842] PM: suspend exit
Suspend failure logs with this change:
[  518.761835] PM: Some devices failed to suspend, or early wake event detected
[  519.486939] PM: wakeup source or subsystem uart_suspend_port aborted suspend
[  519.500594] PM: suspend exit
Here we can clearly identify the module triggerring abort suspend.
Co-developed-by Chinmoy Ghosh <[email protected]>
Signed-off-by Chinmoy Ghosh <[email protected]>
Co-developed-by Mintu Patel <[email protected]>
Signed-off-by Mintu Patel <[email protected]>
Co-developed-by Vishal Badole <[email protected]>
Signed-off-by Vishal Badole <[email protected]>
Signed-off-by Vimal Kumar <[email protected]>
Quoted 4 lines
---
Changes in v4:
- Changed GFP_KERNEL flag to GFP_ATOMIC
- Changed mutex_lock to raw_spin_lock

Quoted 2 lines

why raw?

As mutex_lock might sleep, we need to use lock that is suitable for usages in atomic context. raw_spin_lock is already being used for other list in
this driver, so I used the same. If suggested we can switch to spin_lock_irqsave as well.

Quoted 38 lines

Quoted 27 lines

---
 drivers/base/power/wakeup.c | 100 +++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

drivers/base/power/wakeup.c +10 -0

index a917219feea6..b04794557eef 100644
@@ -73,6 +73,16 @@ static struct wakeup_source deleted_ws = {
 
 static DEFINE_IDA(wakeup_ida);
 
+#ifdef CONFIG_PM_DEBUG
+static DEFINE_RAW_SPINLOCK(pm_abort_suspend_list_lock);
+
+struct pm_abort_suspend_source {
+	struct list_head list;
+	char *source_triggering_abort_suspend;
+};
+static LIST_HEAD(pm_abort_suspend_list);
+#endif
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 588

Quoted 4 lines

drivers/base/power/wakeup.c +1 -0

@@ -575,6 +585,54 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 	trace_wakeup_source_activate(ws->name, cec);
 }
 
+#ifdef CONFIG_PM_DEBUG

Please do not add #ifdef to .c files, this makes this file even messier.

Quoted 4 lines

drivers/base/power/wakeup.c +0 -0

@@ -590,8 +648,13 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
 	if (!ws->active)
 		wakeup_source_activate(ws);

Quoted 10 lines

drivers/base/power/wakeup.c static void wakeup_source_report_event(struct wakeup_source *ws, bool hard) · line 655
Quoted 6 lines
drivers/base/power/wakeup.c +5 -1
-	if (hard)
+	if (hard) {
+#ifdef CONFIG_PM_DEBUG
+		if (pm_suspend_target_state != PM_SUSPEND_ON)
+			pm_abort_suspend_source_add(ws->name);
+#endif
Especially for stuff like this, if you write your .h files properly, no #ifdef are needed.

Thanks. We can move it to .h file.

Quoted 48 lines

drivers/base/power/wakeup.c +31 -0

 		pm_system_wakeup();
+	}
 }
 
 /**

@@ -893,12 +956,47 @@ bool pm_wakeup_pending(void)
 		pm_print_active_wakeup_sources();
 	}
 
+#ifdef CONFIG_PM_DEBUG
+	if (atomic_read(&pm_abort_suspend) > 0) {
+		struct pm_abort_suspend_source *info;
+
+		raw_spin_lock_irqsave(&pm_abort_suspend_list_lock, flags);
+		list_for_each_entry(info, &pm_abort_suspend_list, list) {
+			pm_pr_dbg("wakeup source or subsystem %s aborted suspend\n",
+					info->source_triggering_abort_suspend);
+		}
+		raw_spin_unlock_irqrestore(&pm_abort_suspend_list_lock, flags);
+		pm_abort_suspend_list_clear();
+	}
+#endif
+
 	return ret || atomic_read(&pm_abort_suspend) > 0;
 }
 EXPORT_SYMBOL_GPL(pm_wakeup_pending);
 
 void pm_system_wakeup(void)
 {
+
+#ifdef CONFIG_PM_DEBUG
+#ifdef CONFIG_DEBUG_INFO
+	if (pm_suspend_target_state != PM_SUSPEND_ON) {
+		char *source_name = kasprintf(GFP_ATOMIC,
+					"%ps",
+					__builtin_return_address(0));
+		if (!source_name)
+			goto exit;
+
+		if (strcmp(source_name, "pm_wakeup_ws_event"))
+			pm_abort_suspend_source_add(source_name);
+
+		kfree(source_name);
+	}
+exit:

Quoted 9 lines

drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 999
Quoted 5 lines
drivers/base/power/wakeup.c +5 -0
+#else
+	if (pm_suspend_target_state != PM_SUSPEND_ON)
+		pm_pr_dbg("Some wakeup source or subsystem aborted suspend\n");
+#endif
+#endif
Would you want to maintain this #ifdef nesting mess for the next 20 years? Please do not do this.

I was hoping if we can remove the "CONFIG_PM_DEBUG" as this functionality can exist by default as well. We can avoid nesting and usage for CONFIG_DEBUG_INFO move to .h files.

Please share your perspective on this.

Quoted 3 lines

thanks,
greg k-h

Warm Regards,
Vimal Kumar

Greg Kroah-Hartman

maintainer

2024-02-07 10:25 UTC #3 lore raw

Quoted 34 lines · Vimal Kumar

Quoted 33 lines · Greg Kroah-Hartman
Quoted 31 lines · Vimal Kumar
Sometimes kernel suspend transitions can be aborted unconditionally by manipulating pm_abort_suspend value using "hard" wakeup triggers or through "pm_system_wakeup()".
There is no way to trace the source path of module or subsystem which aborted the suspend transitions. This change will create a list of wakeup sources aborting suspend in progress through "hard" events as well as subsytems aborting suspend using "pm_system_wakeup()".
Example: Existing suspend failure logs:
[  349.708359] PM: Some devices failed to suspend, or early wake event detected
[  350.327842] PM: suspend exit
Suspend failure logs with this change:
[  518.761835] PM: Some devices failed to suspend, or early wake event detected
[  519.486939] PM: wakeup source or subsystem uart_suspend_port aborted suspend
[  519.500594] PM: suspend exit
Here we can clearly identify the module triggerring abort suspend.
Co-developed-by Chinmoy Ghosh <[email protected]>
Signed-off-by Chinmoy Ghosh <[email protected]>
Co-developed-by Mintu Patel <[email protected]>
Signed-off-by Mintu Patel <[email protected]>
Co-developed-by Vishal Badole <[email protected]>
Signed-off-by Vishal Badole <[email protected]>
Signed-off-by Vimal Kumar <[email protected]>
---
Changes in v4:
- Changed GFP_KERNEL flag to GFP_ATOMIC
- Changed mutex_lock to raw_spin_lock

Quoted 4 lines

Quoted 2 lines
why raw?
As mutex_lock might sleep, we need to use lock that is suitable for usages in atomic context. raw_spin_lock is already being used for other list in
this driver, so I used the same. If suggested we can switch to spin_lock_irqsave as well.

You need a really good reason, and a documented one, as to why to use a raw spinlock. If not, please just use a normal one (or irqsave if it can be grabbed in irq context.)

Quoted 11 lines

Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+exit:
+#else
+	if (pm_suspend_target_state != PM_SUSPEND_ON)
+		pm_pr_dbg("Some wakeup source or subsystem aborted suspend\n");
+#endif
+#endif
Would you want to maintain this #ifdef nesting mess for the next 20 years? Please do not do this.
I was hoping if we can remove the "CONFIG_PM_DEBUG" as this functionality can exist by default as well.

Then submit changes for that if that is what you want :)

thanks,

greg k-h