Re: [PATCH v4] PM / sleep: Mechanism to find source aborting kernel suspend transition

[Greg Kroah-Hartman]

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@xxxxxxxxx>
> Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@xxxxxxxxx>
> Co-developed-by: Mintu Patel <mintupatel89@xxxxxxxxx>
> Signed-off-by: Mintu Patel <mintupatel89@xxxxxxxxx>
> Co-developed-by: Vishal Badole <badolevishal1116@xxxxxxxxx>
> Signed-off-by: Vishal Badole <badolevishal1116@xxxxxxxxx>
> Signed-off-by: Vimal Kumar <vimal.kumar32@xxxxxxxxx>
> ---
> 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