PM / sleep: Mechanism to find source aborting kernel suspend transition
Subsystems (4)
PATCH v5 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.
---
Changes in v5:
- Removed CONFIG_PM_DEBUG
- Moved conditional directives to .h file
- Used spin_lock instead of raw_spin_lock
--- drivers/base/power/power.h | 14 ++++++ drivers/base/power/wakeup.c | 86 ++++++++++++++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-)
drivers/base/power/power.h +0 -0
@@ -168,3 +168,17 @@ static inline void device_pm_init(struct device *dev)
device_pm_sleep_init(dev); pm_runtime_init(dev);
}
+
+#ifdef CONFIG_DEBUG_INFO
+
+static inline char *pm_abort_suspend_source_name(void) +{
+ char *source_name = kasprintf(GFP_ATOMIC, "%pS", __builtin_return_address(0));
+ return source_name;
+}
+
+#else
+
+ static inline char *pm_abort_suspend_source_name(void) { return NULL; }
+
+#endif /* CONFIG_DEBUG_INFO */
drivers/base/power/wakeup.c +8 -0
@@ -73,6 +73,14 @@ static struct wakeup_source deleted_ws = {static DEFINE_IDA (wakeup_ida); +static DEFINE_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); +/** * wakeup_source_create - Create a struct wakeup_source object. * @name: Name of the new wakeup source.
@@ -575,6 +583,52 @@ static void wakeup_source_activate(struct wakeup_source *ws)
trace_wakeup_source_activate(ws->name, cec);
}
+/**
+ * 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;
+
+ 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);
+ }
+ 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;
+ }
+
+ spin_lock_irqsave(&pm_abort_suspend_list_lock, flags);
+ list_add_tail(&info->list, &pm_abort_suspend_list);
+ spin_unlock_irqrestore(&pm_abort_suspend_list_lock, flags);
+}
+
/**
* wakeup_source_report_event - Report wakeup event using the given source.
* @ws: Wakeup source to report the event for.
@@ -590,8 +644,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();
+ }
}
/**
@@ -893,12 +950,39 @@ bool pm_wakeup_pending(void)
pm_print_active_wakeup_sources(); }
+ if (atomic_read(&pm_abort_suspend) > 0) {
+ struct pm_abort_suspend_source *info;
+
+ 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);
+ }
+ spin_unlock_irqrestore(&pm_abort_suspend_list_lock, flags);
+ pm_abort_suspend_list_clear();
+ }
+
return ret || atomic_read(&pm_abort_suspend) > 0;
}
EXPORT_SYMBOL_GPL(pm_wakeup_pending);
void pm_system_wakeup(void)
{
+
+ if (pm_suspend_target_state != PM_SUSPEND_ON) {
+ char *source_name = pm_abort_suspend_source_name();
+
+ if (!source_name) {
+ pm_pr_dbg("Some wakeup source or subsystem aborted suspend\n");
+ goto exit;
+ }
+
+ if (strcmp(source_name, "pm_wakeup_ws_event"))
+ pm_abort_suspend_source_add(source_name);
+
+ kfree(source_name);
+ }
+exit:
atomic_inc(&pm_abort_suspend); s2idle_wake();
}
--
2.25.1
Greg Kroah-Hartman
maintainerQuoted 42 lines · Vimal Kumar
Sometimes kernel suspend transitions can be aborted unconditionally by manipulating
pm_abort_suspendvalue 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 exitSuspend 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 exitHere 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 v5:
- RemovedCONFIG_PM_DEBUG
- Moved conditional directives to .h file
- Usedspin_lockinstead ofraw_spin_lock--- drivers/base/power/power.h | 14 ++++++ drivers/base/power/wakeup.c | 86 ++++++++++++++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-)drivers/base/power/power.h +0
-0
drivers/base/power/power.h static inline void device_pm_init(struct device *dev) · line 176
Quoted 9 lines
drivers/base/power/power.h +6
-0@@ -168,3 +168,17 @@ static inline void device_pm_init(struct device *dev)device_pm_sleep_init (dev);pm_runtime_init (dev); } + +#ifdef CONFIG_DEBUG_INFO + +static inline char *pm_abort_suspend_source_name (void ) +{ +char *source_name =kasprintf (GFP_ATOMIC ,"%pS" ,__builtin_return_address (0 ));
This is a cool hack, but no error checking?
drivers/base/power/power.h static inline void device_pm_init(struct device *dev) · line 182
Quoted 6 lines
drivers/base/power/power.h +6
-0+ return source_name; +} + +#else + +static inline char *pm_abort_suspend_source_name (void ) {return NULL ; }
Odd indentation, why?
Quoted 75 lines
drivers/base/power/power.h +2
-0+ +#endif /* CONFIG_DEBUG_INFO */ drivers/base/power/wakeup.c +54
-0@@ -73,6 +73,14 @@ static struct wakeup_source deleted_ws = {static DEFINE_IDA (wakeup_ida); +static DEFINE_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); +/** * wakeup_source_create - Create a struct wakeup_source object. * @name: Name of the new wakeup source. @@ -575,6 +583,52 @@ static void wakeup_source_activate(struct wakeup_source *ws)trace_wakeup_source_activate (ws->name, cec); } +/** +* 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; + +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); + } +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 ; + } + +spin_lock_irqsave (&pm_abort_suspend_list_lock, flags); +list_add_tail (&info->list, &pm_abort_suspend_list); +spin_unlock_irqrestore (&pm_abort_suspend_list_lock, flags); +} +/** * wakeup_source_report_event - Report wakeup event using the given source. * @ws: Wakeup source to report the event for.
drivers/base/power/wakeup.c static void wakeup_source_report_event(struct wakeup_source *ws, bool hard) · line 648
Quoted 6 lines
drivers/base/power/wakeup.c +2
-1@@ -590,8 +644,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 )
Why is this state special?
Quoted 7 lines
drivers/base/power/wakeup.c +2
-0+pm_abort_suspend_source_add (ws->name);pm_system_wakeup (); + } }/**
drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 959
Quoted 10 lines
drivers/base/power/wakeup.c +7
-0@@ -893,12 +950,39 @@ bool pm_wakeup_pending(void)pm_print_active_wakeup_sources (); } +if (atomic_read (&pm_abort_suspend) >0 ) { +struct pm_abort_suspend_source *info; + +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);
You should put the %s in quotes or something to make it obvious this is a function name.
drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 962
Quoted 3 lines
drivers/base/power/wakeup.c +3
-0+ } +spin_unlock_irqrestore (&pm_abort_suspend_list_lock, flags); +pm_abort_suspend_list_clear ();
You just raced between printing the list and deleting it, why drop a lock just to grab it again? Why does this have to be a separate function at all?
Quoted 4 lines
drivers/base/power/wakeup.c +2
-0+ } +return ret ||atomic_read (&pm_abort_suspend) >0 ; }
drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 976
Quoted 10 lines
drivers/base/power/wakeup.c +6
-0EXPORT_SYMBOL_GPL (pm_wakeup_pending);void pm_system_wakeup (void ) { + +if (pm_suspend_target_state !=PM_SUSPEND_ON ) { +char *source_name =pm_abort_suspend_source_name (); + +if (!source_name) { +pm_pr_dbg ("Some wakeup source or subsystem aborted suspend\n" );
So if the config option is disabled, you will just get this message? That's not going to be very helpful. Or are you going to get it again as you already get that today?
And why are you relying on CONFIG_DEBUG_INFO, where is that documented?
drivers/base/power/wakeup.c bool pm_wakeup_pending(void) · line 980
Quoted 4 lines
drivers/base/power/wakeup.c +4
-0+ goto exit; + } + +if (strcmp (source_name,"pm_wakeup_ws_event" ))
You are relying on a function name here, that is not going to go well over time. What happens if they get out of sync? What will keep that in sync? What documents this? How is this going to be maintained properly over time?
What is going to use this new kernel log message?
thanks,
greg k-h