* [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition
@ 2023-12-10 10:03 Vimal Kumar
2023-12-10 10:28 ` Greg Kroah-Hartman
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Vimal Kumar @ 2023-12-10 10:03 UTC (permalink / raw
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Greg Kroah-Hartman,
linux-pm, linux-kernel
Cc: chinmoyghosh2001, badolevishal1116, mintupatel89, 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 <chinmoyghosh2001@gmail.com>
Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
---
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(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index a917219feea6..fa1e3369b0f6 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -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();
}
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index ef503088942d..2eef5817b825 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -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 */
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index fa3bf161d13f..49c830246930 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition
2023-12-10 10:03 [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition Vimal Kumar
@ 2023-12-10 10:28 ` Greg Kroah-Hartman
2023-12-10 10:28 ` Greg Kroah-Hartman
2023-12-10 10:35 ` Greg Kroah-Hartman
2 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2023-12-10 10:28 UTC (permalink / raw
To: Vimal Kumar
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm,
linux-kernel, chinmoyghosh2001, badolevishal1116, mintupatel89
On Sun, Dec 10, 2023 at 03:33:01PM +0530, Vimal Kumar wrote:
> +/**
> + * 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition
2023-12-10 10:03 [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition Vimal Kumar
2023-12-10 10:28 ` Greg Kroah-Hartman
@ 2023-12-10 10:28 ` Greg Kroah-Hartman
2023-12-10 10:35 ` Greg Kroah-Hartman
2 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2023-12-10 10:28 UTC (permalink / raw
To: Vimal Kumar
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm,
linux-kernel, chinmoyghosh2001, badolevishal1116, mintupatel89
On Sun, Dec 10, 2023 at 03:33:01PM +0530, Vimal Kumar wrote:
> +EXPORT_SYMBOL_GPL(abort_suspend_list_clear);
Why is this exported to modules?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition
2023-12-10 10:03 [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition Vimal Kumar
2023-12-10 10:28 ` Greg Kroah-Hartman
2023-12-10 10:28 ` Greg Kroah-Hartman
@ 2023-12-10 10:35 ` Greg Kroah-Hartman
2024-01-09 4:48 ` Vimal Kumar
2 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2023-12-10 10:35 UTC (permalink / raw
To: Vimal Kumar
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm,
linux-kernel, chinmoyghosh2001, badolevishal1116, mintupatel89
On Sun, Dec 10, 2023 at 03:33:01PM +0530, Vimal Kumar wrote:
> +#define MAX_SUSPEND_ABORT_LEN 256
What does this number mean?
> +static DEFINE_RAW_SPINLOCK(abort_suspend_lock);
Why is this a "raw" spinlock? What requires this?
> +
> +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);
How can this list head not be initialized already?
> +
> + 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);
> + }
After you print them all out, why not remove them from the list now?
Why wait until later?
> +
> 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?
> +
> + 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition
2023-12-10 10:35 ` Greg Kroah-Hartman
@ 2024-01-09 4:48 ` Vimal Kumar
0 siblings, 0 replies; 5+ messages in thread
From: Vimal Kumar @ 2024-01-09 4:48 UTC (permalink / raw
To: Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm,
linux-kernel, chinmoyghosh2001, badolevishal1116, mintupatel89
Apologies for delayed response.
On Sun, Dec 10, 2023 at 11:35:02AM +0100, Greg Kroah-Hartman wrote:
> On Sun, Dec 10, 2023 at 03:33:01PM +0530, Vimal Kumar wrote:
> > +#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.
>
> > +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.
>
> > +
> > +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);
Removed EXPORT_SYMBOL in next version and fixed the name.
> > +
> > +/**
> > + * 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);
>
> How can this list head not be initialized already?
This list is internal to the structure we are defining. Check is reduntant here.
>
> > +
> > + 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);
> > + }
>
> 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.
>
> > +
> > 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?
>
We have changed to dynamic allocation based on the required size in the next version.
> > +
> > + 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.
> 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.
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-09 4:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-10 10:03 [PATCH v2] PM / sleep: Mechanism to find source aborting kernel suspend transition Vimal Kumar
2023-12-10 10:28 ` Greg Kroah-Hartman
2023-12-10 10:28 ` Greg Kroah-Hartman
2023-12-10 10:35 ` Greg Kroah-Hartman
2024-01-09 4:48 ` Vimal Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).