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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 7 Feb 2024 10:25:02 +0000
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Vimal Kumar <vimal.kumar32@...il.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Pavel Machek <pavel@....cz>,
	Len Brown <len.brown@...el.com>, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, chinmoyghosh2001@...il.com,
	badolevishal1116@...il.com, mintupatel89@...il.com
Subject: Re: [PATCH v4] PM / sleep: Mechanism to find source aborting kernel
 suspend transition

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