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(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index a917219feea6..b04794557eef 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -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
On Mon, Feb 05, 2024 at 10:37:45PM +0530, Vimal Kumar wrote:
> 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
why raw?
> ---
> drivers/base/power/wakeup.c | 100 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 99 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index a917219feea6..b04794557eef 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -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
Please do not add #ifdef to .c files, this makes this file even messier.
> @@ -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.
> 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
Would you want to maintain this #ifdef nesting mess for the next 20
years? Please do not do this.
thanks,
greg k-h
On Mon, Feb 05, 2024 at 07:33:17PM +0000, Greg Kroah-Hartman wrote:
> On Mon, Feb 05, 2024 at 10:37:45PM +0530, Vimal Kumar wrote:
> > 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
>
> 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.
>
> > ---
> > drivers/base/power/wakeup.c | 100 +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 99 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index a917219feea6..b04794557eef 100644
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -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
>
> Please do not add #ifdef to .c files, this makes this file even messier.
>
> > @@ -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.
>
Thanks. We can move it to .h file.
>
>
>
> > 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
>
> 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.
> thanks,
>
> greg k-h
Warm Regards,
Vimal Kumar
On Wed, Feb 07, 2024 at 09:24:57AM +0530, Vimal Kumar wrote:
> On Mon, Feb 05, 2024 at 07:33:17PM +0000, Greg Kroah-Hartman wrote:
> > On Mon, Feb 05, 2024 at 10:37:45PM +0530, Vimal Kumar wrote:
> > > 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
> >
> > 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.)
> > > +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