| Message ID | 20240205170747.19748-1-vimal.kumar32@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] PM / sleep: Mechanism to find source aborting kernel suspend transition | expand |
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 <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 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 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 <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 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
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(); }