Re: [PATCH] power: suspend: Add suspend timeout handler

[Joseph Jang]

Yes, I agree.

Rafael J. Wysocki <rafael@xxxxxxxxxx> 於 2020年10月16日 週五 下午9:24寫道：
>
> On Fri, Oct 16, 2020 at 3:22 PM <josephjang@xxxxxxxxxx> wrote:
> >
> > Thank you Rafael's promptly response.
> >
> > > On Fri, Oct 16, 2020 at 5:51 AM Joseph Jang <josephjang@xxxxxxxxxx> wrote:
> > > >
> > > > From: josephjang <josephjang@xxxxxxxxxx>
> > > >
> > > > Add suspend timeout handler to prevent device stuck during suspend/
> > > > resume process. Suspend timeout handler will dump disk sleep task
> > > > at first round timeout and trigger kernel panic at second round timeout.
> > > > The default timer for each round is 30 seconds.
> > > >
> > > > Note: Can use following command to simulate suspend hang for testing.
> > > >     adb shell echo 1 > /sys/power/pm_hang
> > > >     adb shell echo mem > /sys/power/state
> > > > Signed-off-by: josephjang <josephjang@xxxxxxxxxx>
> > > > ---
> > > >  include/linux/console.h |   1 +
> > > >  kernel/power/Kconfig    |   9 +++
> > > >  kernel/power/main.c     |  66 ++++++++++++++++
> > > >  kernel/power/suspend.c  | 162 ++++++++++++++++++++++++++++++++++++++++
> > > >  kernel/printk/printk.c  |   5 ++
> > > >  5 files changed, 243 insertions(+)
> > > >
> > > > diff --git a/include/linux/console.h b/include/linux/console.h
> > > > index 0670d3491e0e..ac468c602c0b 100644
> > > > --- a/include/linux/console.h
> > > > +++ b/include/linux/console.h
> > > > @@ -192,6 +192,7 @@ static inline void console_sysfs_notify(void)
> > > >  { }
> > > >  #endif
> > > >  extern bool console_suspend_enabled;
> > > > +extern int is_console_suspended(void);
> > > >
> > > >  /* Suspend and resume console messages over PM events */
> > > >  extern void suspend_console(void);
> > > > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > > > index a7320f07689d..52b7a181b6d8 100644
> > > > --- a/kernel/power/Kconfig
> > > > +++ b/kernel/power/Kconfig
> > > > @@ -207,6 +207,15 @@ config PM_SLEEP_DEBUG
> > > >         def_bool y
> > > >         depends on PM_DEBUG && PM_SLEEP
> > > >
> > > > +config PM_SLEEP_MONITOR
> > > > +       bool "Linux kernel suspend/resume process monitor"
> > > > +       depends on PM_SLEEP
> > > > +       help
> > > > +       This option will enable suspend/resume monitor to prevent device
> > > > +       stuck during suspend/resume process. Suspend timeout handler
> > > will
> > > > +       dump disk sleep task at first round timeout and trigger kernel
> > > panic
> > > > +       at second round timeout. The default timer for each round is 30
> > > seconds.
> > > > +
> >
> > > The facility associated with the Kconfig entry right below is supposed
> > > to do exactly the same thing.
> >
> > > What's the reason to add another one?  What is missing?
> >
> >
> >
> > > >  config DPM_WATCHDOG
> > > >         bool "Device suspend/resume watchdog"
> > > >         depends on PM_DEBUG && PSTORE && EXPERT
> >
> > Because we found some suspend hand issue that cannot be detected by
> > "CONFIG_DPM_WATCHDOG" (which is focus on device PM).
>
> What's that issue?
>
> > Our suspend timeout monitor can cover PM core and Device PM hang issues.
>
> Then I'd suggest to extend the existing watchdog instead of adding a
> whole new implementation.



-- 
Embedded Software engineer