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

PATCH Superseded Vimal Kumar 2 participants 5 messages Dec 10, 2023 – Jan 9, 2024

Subsystems (4)

Driver core Suspend to RAM Hibernation Power management core

lore

Share & tools

lore

Vimal Kumar

author

2023-12-10 10:03 UTC #0 lore raw

PATCH v2 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 v2:
- Make pm_abort_suspend_list static.
- Added locks.
- Removed list_empty check as not needed.
- Fix Doc format.
- Fix function name as per global functions.
- Removed use of unlikely/likely.
- Removed extra logging for kmalloc failure.
- Use *info instead of struct name.

---
 drivers/base/power/wakeup.c | 82 ++++++++++++++++++++++++++++++++++++-
 include/linux/suspend.h     |  2 +
 kernel/power/suspend.c      |  1 +
 3 files changed, 84 insertions(+), 1 deletion(-)

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

index a917219feea6..fa1e3369b0f6 100644
@@ -73,6 +73,15 @@ static struct wakeup_source deleted_ws = {
 
 static DEFINE_IDA(wakeup_ida);
 
+#define MAX_SUSPEND_ABORT_LEN 256
+static DEFINE_RAW_SPINLOCK(abort_suspend_lock);
+
+struct pm_abort_suspend_source {
+	struct list_head list;
+	char *source_triggering_abort_suspend;
+};
+static LIST_HEAD(pm_abort_suspend_list);
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

@@ -575,6 +584,56 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 	trace_wakeup_source_activate(ws->name, cec);
 }
 
+/**
+ * abort_suspend_list_clear - Clear pm_abort_suspend_list.
+ *
+ * The pm_abort_suspend_list will be cleared when system PM exits.
+ */
+void abort_suspend_list_clear(void)
+{
+	struct pm_abort_suspend_source *info, *tmp;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&abort_suspend_lock, flags);
+	list_for_each_entry_safe(info, tmp, &pm_abort_suspend_list, list) {
+		list_del(&info->list);
+		kfree(info);
+	}
+	raw_spin_unlock_irqrestore(&abort_suspend_lock, flags);
+}
+EXPORT_SYMBOL_GPL(abort_suspend_list_clear);
+
+/**
+ * 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)
+{
+	struct pm_abort_suspend_source *info;
+	unsigned long flags;
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return;
+
+	/* Initialize the list within the struct if it's not already initialized */
+	if (list_empty(&info->list))
+		INIT_LIST_HEAD(&info->list);
+
+	info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
+	if (!info->source_triggering_abort_suspend) {
+		kfree(info);
+		return;
+	}
+
+	raw_spin_lock_irqsave(&abort_suspend_lock, flags);
+	list_add_tail(&info->list, &pm_abort_suspend_list);
+	raw_spin_unlock_irqrestore(&abort_suspend_lock, flags);
+}
+
 /**
  * wakeup_source_report_event - Report wakeup event using the given source.
  * @ws: Wakeup source to report the event for.

@@ -590,8 +649,11 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
 	if (!ws->active)
 		wakeup_source_activate(ws);
 
-	if (hard)
+	if (hard) {
+		if (pm_suspend_target_state != PM_SUSPEND_ON)
+			pm_abort_suspend_source_add(ws->name);
 		pm_system_wakeup();
+	}
 }
 
 /**

@@ -877,6 +939,7 @@ bool pm_wakeup_pending(void)
 {
 	unsigned long flags;
 	bool ret = false;
+	struct pm_abort_suspend_source *info;
 
 	raw_spin_lock_irqsave(&events_lock, flags);
 	if (events_check_enabled) {

@@ -893,12 +956,29 @@ bool pm_wakeup_pending(void)
 		pm_print_active_wakeup_sources();
 	}
 
+	if (atomic_read(&pm_abort_suspend) > 0) {
+		raw_spin_lock_irqsave(&abort_suspend_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(&abort_suspend_lock, flags);
+	}
+
 	return ret || atomic_read(&pm_abort_suspend) > 0;
 }
 EXPORT_SYMBOL_GPL(pm_wakeup_pending);
 
 void pm_system_wakeup(void)
 {
+	char buf[MAX_SUSPEND_ABORT_LEN];
+
+	if (pm_suspend_target_state != PM_SUSPEND_ON) {
+		sprintf(buf, "%ps", __builtin_return_address(0));
+		if (strcmp(buf, "pm_wakeup_ws_event"))
+			pm_abort_suspend_source_add(buf);
+	}
+
 	atomic_inc(&pm_abort_suspend);
 	s2idle_wake();
 }

include/linux/suspend.h +2 -0

index ef503088942d..2eef5817b825 100644
@@ -528,6 +528,7 @@ extern void pm_print_active_wakeup_sources(void);
 
 extern unsigned int lock_system_sleep(void);
 extern void unlock_system_sleep(unsigned int);
+extern void abort_suspend_list_clear(void);
 
 #else /* !CONFIG_PM_SLEEP */

@@ -556,6 +557,7 @@ static inline void pm_system_irq_wakeup(unsigned int irq_number) {}
 
 static inline unsigned int lock_system_sleep(void) { return 0; }
 static inline void unlock_system_sleep(unsigned int flags) {}
+static inline void abort_suspend_list_clear(void) {}
 
 #endif /* !CONFIG_PM_SLEEP */

kernel/power/suspend.c +1 -0

index fa3bf161d13f..49c830246930 100644
@@ -623,6 +623,7 @@ int pm_suspend(suspend_state_t state)
 	} else {
 		suspend_stats.success++;
 	}
+	abort_suspend_list_clear();
 	pr_info("suspend exit\n");
 	return error;
 }

Signature · 1 line

2.25.1

Greg Kroah-Hartman

maintainer

2023-12-10 10:28 UTC #1 lore raw

Quoted 2 lines · Vimal Kumar

+/**
+ * abort_suspend_list_clear - Clear pm_abort_suspend_list.

Again, a horrible global symbol name, please fix all of these up.

thanks,

greg k-h

Greg Kroah-Hartman

maintainer

2023-12-10 10:28 UTC #2 lore raw

In reply to Vimal Kumar

Quoted 1 line · Vimal Kumar

+EXPORT_SYMBOL_GPL(abort_suspend_list_clear);

Why is this exported to modules?

thanks,

greg k-h

Greg Kroah-Hartman

maintainer

2023-12-10 10:35 UTC #3 lore raw

In reply to Vimal Kumar

Quoted 1 line · Vimal Kumar

+#define MAX_SUSPEND_ABORT_LEN 256

What does this number mean?

Quoted 1 line

+static DEFINE_RAW_SPINLOCK(abort_suspend_lock);

Why is this a "raw" spinlock? What requires this?

Quoted 42 lines

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

+
+struct pm_abort_suspend_source {
+	struct list_head list;
+	char *source_triggering_abort_suspend;
+};
+static LIST_HEAD(pm_abort_suspend_list);
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

@@ -575,6 +584,56 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 	trace_wakeup_source_activate(ws->name, cec);
 }
 
+/**
+ * abort_suspend_list_clear - Clear pm_abort_suspend_list.
+ *
+ * The pm_abort_suspend_list will be cleared when system PM exits.
+ */
+void abort_suspend_list_clear(void)
+{
+	struct pm_abort_suspend_source *info, *tmp;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&abort_suspend_lock, flags);
+	list_for_each_entry_safe(info, tmp, &pm_abort_suspend_list, list) {
+		list_del(&info->list);
+		kfree(info);
+	}
+	raw_spin_unlock_irqrestore(&abort_suspend_lock, flags);
+}
+EXPORT_SYMBOL_GPL(abort_suspend_list_clear);
+
+/**
+ * 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)
+{

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

Quoted 10 lines

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

+	struct pm_abort_suspend_source *info;
+	unsigned long flags;
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return;
+
+	/* Initialize the list within the struct if it's not already initialized */
+	if (list_empty(&info->list))
+		INIT_LIST_HEAD(&info->list);

How can this list head not be initialized already?

Quoted 38 lines

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

+
+	info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
+	if (!info->source_triggering_abort_suspend) {
+		kfree(info);
+		return;
+	}
+
+	raw_spin_lock_irqsave(&abort_suspend_lock, flags);
+	list_add_tail(&info->list, &pm_abort_suspend_list);
+	raw_spin_unlock_irqrestore(&abort_suspend_lock, flags);
+}
+
 /**
  * wakeup_source_report_event - Report wakeup event using the given source.
  * @ws: Wakeup source to report the event for.

@@ -590,8 +649,11 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
 	if (!ws->active)
 		wakeup_source_activate(ws);
 
-	if (hard)
+	if (hard) {
+		if (pm_suspend_target_state != PM_SUSPEND_ON)
+			pm_abort_suspend_source_add(ws->name);
 		pm_system_wakeup();
+	}
 }
 
 /**

@@ -877,6 +939,7 @@ bool pm_wakeup_pending(void)
 {
 	unsigned long flags;
 	bool ret = false;
+	struct pm_abort_suspend_source *info;
 
 	raw_spin_lock_irqsave(&events_lock, flags);
 	if (events_check_enabled) {

@@ -893,12 +956,29 @@ bool pm_wakeup_pending(void)
 		pm_print_active_wakeup_sources();

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

Quoted 10 lines

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

 	}
 
+	if (atomic_read(&pm_abort_suspend) > 0) {
+		raw_spin_lock_irqsave(&abort_suspend_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(&abort_suspend_lock, flags);
+	}

After you print them all out, why not remove them from the list now? Why wait until later?

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

Quoted 8 lines

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

+
 	return ret || atomic_read(&pm_abort_suspend) > 0;
 }
 EXPORT_SYMBOL_GPL(pm_wakeup_pending);
 
 void pm_system_wakeup(void)
 {
+	char buf[MAX_SUSPEND_ABORT_LEN];

You never actually check to ensure that you do not overflow this value, right? And are you _SURE_ you can put a string this big on the stack?

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

Quoted 4 lines

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

+
+	if (pm_suspend_target_state != PM_SUSPEND_ON) {
+		sprintf(buf, "%ps", __builtin_return_address(0));
+		if (strcmp(buf, "pm_wakeup_ws_event"))

This is _VERY_ fragile, you are relying on a specific symbol to never change its name, which is not going to work in the long run, AND this will not work if you don't have symbols in your kernel, right?

How was this tested?

And again, why is this even needed, who will use it? What tools will consume it? Who will rely on it?

thanks,

greg k-h

Vimal Kumar

author

2024-01-09 04:49 UTC #4 lore raw

Apologies for delayed response.

Quoted 4 lines · Greg Kroah-Hartman

Quoted 1 line · Vimal Kumar
+#define MAX_SUSPEND_ABORT_LEN 256
What does this number mean?

It was for buffer size assuming function name won't go beyond that. We have removed this and modified to dynamic allocation in the next version.

Quoted 4 lines

Quoted 2 lines
+static DEFINE_RAW_SPINLOCK(abort_suspend_lock);
Why is this a "raw" spinlock? What requires this?

We tried to use same as one used for wakeup_source. We modified to mutex_lock in the next version.

Quoted 25 lines

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

+
+struct pm_abort_suspend_source {
+	struct list_head list;
+	char *source_triggering_abort_suspend;
+};
+static LIST_HEAD(pm_abort_suspend_list);
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

@@ -575,6 +584,56 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 	trace_wakeup_source_activate(ws->name, cec);
 }
 
+/**
+ * abort_suspend_list_clear - Clear pm_abort_suspend_list.
+ *
+ * The pm_abort_suspend_list will be cleared when system PM exits.
+ */
+void abort_suspend_list_clear(void)
+{
+	struct pm_abort_suspend_source *info, *tmp;
+	unsigned long flags;
+

Quoted 8 lines

drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 604
Quoted 8 lines
drivers/base/power/wakeup.c +8 -0
+	raw_spin_lock_irqsave(&abort_suspend_lock, flags);
+	list_for_each_entry_safe(info, tmp, &pm_abort_suspend_list, list) {
+		list_del(&info->list);
+		kfree(info);
+	}
+	raw_spin_unlock_irqrestore(&abort_suspend_lock, flags);
+}
+EXPORT_SYMBOL_GPL(abort_suspend_list_clear);

Removed EXPORT_SYMBOL in next version and fixed the name.

Quoted 14 lines

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

+
+/**
+ * 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)
+{
+	struct pm_abort_suspend_source *info;
+	unsigned long flags;
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);

Quoted 8 lines

drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 624
Quoted 6 lines
drivers/base/power/wakeup.c +6 -0
+	if (!info)
+		return;
+
+	/* Initialize the list within the struct if it's not already initialized */
+	if (list_empty(&info->list))
+		INIT_LIST_HEAD(&info->list);
How can this list head not be initialized already?

This list is internal to the structure we are defining. Check is reduntant here.

Quoted 41 lines

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

+
+	info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
+	if (!info->source_triggering_abort_suspend) {
+		kfree(info);
+		return;
+	}
+
+	raw_spin_lock_irqsave(&abort_suspend_lock, flags);
+	list_add_tail(&info->list, &pm_abort_suspend_list);
+	raw_spin_unlock_irqrestore(&abort_suspend_lock, flags);
+}
+
 /**
  * wakeup_source_report_event - Report wakeup event using the given source.
  * @ws: Wakeup source to report the event for.

@@ -590,8 +649,11 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
 	if (!ws->active)
 		wakeup_source_activate(ws);
 
-	if (hard)
+	if (hard) {
+		if (pm_suspend_target_state != PM_SUSPEND_ON)
+			pm_abort_suspend_source_add(ws->name);
 		pm_system_wakeup();
+	}
 }
 
 /**

@@ -877,6 +939,7 @@ bool pm_wakeup_pending(void)
 {
 	unsigned long flags;
 	bool ret = false;
+	struct pm_abort_suspend_source *info;
 
 	raw_spin_lock_irqsave(&events_lock, flags);
 	if (events_check_enabled) {

@@ -893,12 +956,29 @@ bool pm_wakeup_pending(void)
 		pm_print_active_wakeup_sources();
 	}

Quoted 11 lines

drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 966
Quoted 8 lines
drivers/base/power/wakeup.c +8 -0
+	if (atomic_read(&pm_abort_suspend) > 0) {
+		raw_spin_lock_irqsave(&abort_suspend_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(&abort_suspend_lock, flags);
+	}
After you print them all out, why not remove them from the list now? Why wait until later?

Yes, this can be done. We have changed it in the next version.

Quoted 6 lines

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

+
 	return ret || atomic_read(&pm_abort_suspend) > 0;
 }
 EXPORT_SYMBOL_GPL(pm_wakeup_pending);

Quoted 7 lines

drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 974
Quoted 3 lines
drivers/base/power/wakeup.c +1 -0
 void pm_system_wakeup(void)
 {
+	char buf[MAX_SUSPEND_ABORT_LEN];
You never actually check to ensure that you do not overflow this value, right? And are you _SURE_ you can put a string this big on the stack?

We have changed to dynamic allocation based on the required size in the next version.

Quoted 9 lines

drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 978
Quoted 4 lines
drivers/base/power/wakeup.c +4 -0
+
+	if (pm_suspend_target_state != PM_SUSPEND_ON) {
+		sprintf(buf, "%ps", __builtin_return_address(0));
+		if (strcmp(buf, "pm_wakeup_ws_event"))
This is _VERY_ fragile, you are relying on a specific symbol to never change its name, which is not going to work in the long run, AND this will not work if you don't have symbols in your kernel, right?

As this is void function, we are depending on this symbol. We have put the changes under debug flag in the next version.
If the symbol changes in future we need to adopt or if the pm_system_wakeup() modified to take some input that can be used later.

Quoted 2 lines

How was this tested?

As of now we invoked "pm_system_wakeup()" from one of the late_suspend calls and tested this functionality which listed source_name invoking abort suspend.

Quoted 6 lines

And again, why is this even needed, who will use it? What tools will consume it? Who will rely on it?
thanks,
greg k-h

Kernel developer can use this in debugging suspend failure while supporting system wide s2r.

Commit 8a537ece3d94 (PM / wakeup: Integrate mechanism to abort suspend transitions in progress) and commit 60d4553bdc1a0 allowed to invoke pm_system_wakeup() when wakeup_source_report_event() is called to signal a "hard" event to abort system suspend in progress. Also, suspend can be aborted by direct call to pm_system_wakeup().

In some scenario, we have encoundered a situation where system suspend was aborted using the above mechanism. However we faced a challenge in identifying the subsystem or module responsible suspend failures. This will prove valuable in debugging system suspend failures by providing details about the sources aborting suspend in progress.

Warm Regards,
Vimal Kumar