Messages in this thread | | | Date | Wed, 7 Feb 2024 10:25:02 +0000 | From | Greg Kroah-Hartman <> | 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@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
| |