Message-ID: <000000000000011a8805b215d69a@google.com>
Date:   Tue, 20 Oct 2020 08:15:38 +0000
From:   <josephjang@...gle.com>
To:     gregkh@...uxfoundation.org, rafael@...nel.org, rjw@...ysocki.net,
        pavel@....cz, len.brown@...el.com, pmladek@...e.com,
        sergey.senozhatsky@...il.com, rostedt@...dmis.org
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        jonglin@...gle.com, woodylin@...gle.com, markcheng@...gle.com,
        josephjang@...gle.com
Subject: [PATCH] power: suspend: Add suspend timeout handler

> On Tue, Oct 20, 2020 at 02:22:26PM +0800, Joseph Jang wrote:
> > Add sleep timer and timeout handler to prevent device stuck during  
> suspend/
> > resume process. The 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 defined in
> > CONFIG_PM_SLEEP_TIMER_TIMEOUT.
> >
> > Signed-off-by: Joseph Jang <josephjang@...gle.com>
> > ---
> >  MAINTAINERS                   |  2 +
> >  include/linux/console.h       |  1 +
> >  include/linux/suspend_timer.h | 90 +++++++++++++++++++++++++++++++++++

> Why is this file in include/linux/ if you only ever call it from one .c
> file?

I just refer to include/linux/suspend.h and create a new header file in the  
same folder.
If you have a better location for the new header file, please feel free to  
let me know.

> > --- /dev/null
> > +++ b/include/linux/suspend_timer.h
> > @@ -0,0 +1,90 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _LINUX_SLEEP_TIMER_H
> > +#define _LINUX_SLEEP_TIMER_H
> > +
> > +#include <linux/sched/debug.h>
> > +
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +struct sleep_timer {
> > +     struct task_struct      *tsk;
> > +     struct timer_list       timer;
> > +};
> > +
> > +#define DECLARE_SLEEP_TIMER(st) > > +     struct sleep_timer st
> > +
> > +/**
> > + * init_sleep_timer - Initialize sleep timer.
> > + * @st: Sleep timer to initialize.
> > + * @func: Sleep timer timeout handler.
> > + */
> > +static void init_sleep_timer(struct sleep_timer *st, void (*func))
> > +{
> > +     struct timer_list *timer = &st->timer;
> > +
> > +     timer_setup(timer, func, 0);
> > +}
> > +
> > +/**
> > + * start_sleep_timer - Enable sleep timer to monitor suspend thread.
> > + * @st: Sleep timer to enable.
> > + */
> > +static void start_sleep_timer(struct sleep_timer *st)
> > +{
> > +     struct timer_list *timer = &st->timer;
> > +
> > +     st->tsk = current;
> > +
> > +     /* use same timeout value for both suspend and resume */
> > +     timer->expires = jiffies + HZ * CONFIG_PM_SLEEP_TIMER_TIMEOUT;
> > +     add_timer(timer);
> > +}
> > +
> > +/**
> > + * stop_sleep_timer - Disable sleep timer.
> > + * @st: sleep timer to disable.
> > + */
> > +static void stop_sleep_timer(struct sleep_timer *st)
> > +{
> > +     struct timer_list *timer = &st->timer;
> > +
> > +     del_timer_sync(timer);
> > +}
> > +
> > +/**
> > + * sleep_timeout_handler - sleep timer timeout handler.
> > + * @t: The timer list that sleep timer depends on.
> > + *
> > + * Called when suspend thread has timeout suspending or resuming.
> > + * Dump all uninterruptible tasks' call stack and call panic() to
> > + * reboot system in second round timeout.
> > + */
> > +static void sleep_timeout_handler(struct timer_list *t)
> > +{
> > +     struct sleep_timer *st = from_timer(st, t, timer);
> > +     static int timeout_count;
> > +
> > +     pr_info("Sleep timeout (timer is %d seconds)\n",
> > +             (CONFIG_PM_SLEEP_TIMER_TIMEOUT));
> > +     show_stack(st->tsk, NULL, KERN_EMERG);
> > +     show_state_filter(TASK_UNINTERRUPTIBLE);
> > +
> > +     if (timeout_count < 1) {
> > +             timeout_count++;
> > +             start_sleep_timer(st);
> > +             return;
> > +     }
> > +
> > +     if (console_is_suspended())
> > +             resume_console();
> > +
> > +     panic("Sleep timeout and panic\n");
> > +}
> > +#else
> > +#define DECLARE_SLEEP_TIMER(st)
> > +#define init_sleep_timer(x, y)
> > +#define start_sleep_timer(x)
> > +#define stop_sleep_timer(x)
> > +#endif
> > +
> > +#endif /* _LINUX_SLEEP_TIMER_H */
> > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > index a7320f07689d..9e2b274db0c1 100644
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -207,6 +207,21 @@ 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 sleep timer to prevent device stuck
> > +     during suspend/resume process. Sleep timeout handler will dump
> > +     disk sleep task at first round timeout and trigger kernel panic
> > +     at second round timeout. The timer for each round is defined in
> > +     CONFIG_PM_SLEEP_TIMER_TIMEOUT.

> I thought we already had a watchdog for all of this, why not just always
> add this to that code, for that config option?


Yes, we already have DPM_WATCHDOG to monitor device power management.
But we really hit the suspend hang issue that DPM_WATCHDOG cannot cover.
We propose a wide coverage debug feature like PM_SLEEP_MONITOR which
not only covers PM but also core PM hang issues.

And DPM_WATCHDOG is for device driver power management in  
drivers/base/power/main.c
and PM_SLEEP_MONITOR locate is for core power management in  
kernel/power/suspend.c.
I think it is fine for users to select whether they need device PM only or  
not.


> And why isn't the watchdog sufficient for you?  Why are you "open
> coding" a watchdog timer logic here at all???


Yes, we refer to DPM_WATCHDOG to extend the watchdog debugging for core PM.
Because we really hit a real case that was not covered by DPM_WATCHDOG.
I think PM_SLEEP_MONITOR is an extension debug feature from DPM_WATCHDOG.


> thanks,

> greg k-h


Thank you,
Joseph.

Powered by blists - more mailing lists