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

[Greg Kroah-Hartman]

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@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?
> >
>  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