[PATCH] power: suspend: Add suspend timeout handler

[PATCH] power: suspend: Add suspend timeout handler

[Joseph Jang]

From: josephjang <josephjang@google.com>

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@google.com>
---
 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.
+
 config DPM_WATCHDOG
 	bool "Device suspend/resume watchdog"
 	depends on PM_DEBUG && PSTORE && EXPERT
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 40f86ec4ab30..f25b8a47583e 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -575,6 +575,69 @@ void __pm_pr_dbg(bool defer, const char *fmt, ...)
 static inline void pm_print_times_init(void) {}
 #endif /* CONFIG_PM_SLEEP_DEBUG */
 
+#ifdef CONFIG_PM_SLEEP_MONITOR
+/* If set, devices will stuck at suspend for verification */
+static bool pm_hang_enabled;
+
+static int pm_notify_test(struct notifier_block *nb,
+			     unsigned long mode, void *_unused)
+{
+	pr_info("Jump into infinite loop now\n");
+
+	/* Suspend thread stuck at a loop forever */
+	for (;;)
+		;
+
+	pr_info("Fail to stuck at loop\n");
+
+	return 0;
+}
+
+static struct notifier_block pm_notify_nb = {
+	.notifier_call = pm_notify_test,
+};
+
+static ssize_t pm_hang_show(struct kobject *kobj, struct kobj_attribute *attr,
+			     char *buf)
+{
+	return snprintf(buf, 10, "%d\n", pm_hang_enabled);
+}
+
+static ssize_t pm_hang_store(struct kobject *kobj, struct kobj_attribute *attr,
+			      const char *buf, size_t n)
+{
+	unsigned long val;
+	int result;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val > 1)
+		return -EINVAL;
+
+	pm_hang_enabled = !!val;
+
+	if (pm_hang_enabled == true) {
+
+		result = register_pm_notifier(&pm_notify_nb);
+		if (result)
+			pr_warn("Can not register suspend notifier, return %d\n",
+				result);
+
+	} else {
+
+		result = unregister_pm_notifier(&pm_notify_nb);
+		if (result)
+			pr_warn("Can not unregister suspend notifier, return %d\n",
+				result);
+	}
+
+	return n;
+}
+
+power_attr(pm_hang);
+#endif
+
 struct kobject *power_kobj;
 
 /**
@@ -909,6 +972,9 @@ static struct attribute * g[] = {
 	&pm_wakeup_irq_attr.attr,
 	&pm_debug_messages_attr.attr,
 #endif
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	&pm_hang_attr.attr,
+#endif
 #endif
 #ifdef CONFIG_FREEZER
 	&pm_freeze_timeout_attr.attr,
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 8b1bb5ee7e5d..6f2679cfd9d1 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -30,6 +30,12 @@
 #include <trace/events/power.h>
 #include <linux/compiler.h>
 #include <linux/moduleparam.h>
+#ifdef CONFIG_PM_SLEEP_MONITOR
+#include <linux/sched/debug.h>
+#include <linux/kthread.h>
+#include <linux/sched.h>
+#include <uapi/linux/sched/types.h>
+#endif
 
 #include "power.h"
 
@@ -61,6 +67,133 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
 enum s2idle_states __read_mostly s2idle_state;
 static DEFINE_RAW_SPINLOCK(s2idle_lock);
 
+#ifdef CONFIG_PM_SLEEP_MONITOR
+/* Suspend monitor thread toggle reason */
+enum toggle_reason {
+	TOGGLE_NONE,
+	TOGGLE_START,
+	TOGGLE_STOP,
+};
+
+#define SUSPEND_TIMER_TIMEOUT_MS 30000
+static struct task_struct *ksuspend_mon_tsk;
+static DECLARE_WAIT_QUEUE_HEAD(power_suspend_waitqueue);
+static enum toggle_reason suspend_mon_toggle;
+static DEFINE_MUTEX(suspend_mon_lock);
+
+static void start_suspend_mon(void)
+{
+	mutex_lock(&suspend_mon_lock);
+	suspend_mon_toggle = TOGGLE_START;
+	mutex_unlock(&suspend_mon_lock);
+	wake_up(&power_suspend_waitqueue);
+}
+
+static void stop_suspend_mon(void)
+{
+	mutex_lock(&suspend_mon_lock);
+	suspend_mon_toggle = TOGGLE_STOP;
+	mutex_unlock(&suspend_mon_lock);
+	wake_up(&power_suspend_waitqueue);
+}
+
+static void suspend_timeout(int timeout_count)
+{
+	char *null_pointer = NULL;
+
+	pr_info("Suspend monitor timeout (timer is %d seconds)\n",
+		(SUSPEND_TIMER_TIMEOUT_MS/1000));
+
+	show_state_filter(TASK_UNINTERRUPTIBLE);
+
+	if (timeout_count < 2)
+		return;
+
+	if (is_console_suspended())
+		resume_console();
+
+	pr_info("Trigger a panic\n");
+
+	/* Trigger a NULL pointer dereference */
+	*null_pointer = 'a';
+
+	/* Should not reach here */
+	pr_err("Trigger panic failed!\n");
+}
+
+static int suspend_monitor_kthread(void *arg)
+{
+	long err;
+	struct sched_param param = {.sched_priority
+		= MAX_RT_PRIO-1};
+	static int timeout_count;
+	static long timeout;
+
+	pr_info("Init ksuspend_mon thread\n");
+
+	sched_setscheduler(current, SCHED_FIFO, &param);
+
+	timeout_count = 0;
+	timeout = MAX_SCHEDULE_TIMEOUT;
+
+	do {
+		/* Wait suspend timer timeout */
+		err = wait_event_interruptible_timeout(
+			power_suspend_waitqueue,
+			(suspend_mon_toggle != TOGGLE_NONE),
+			timeout);
+
+		mutex_lock(&suspend_mon_lock);
+		/* suspend monitor state change */
+		if (suspend_mon_toggle != TOGGLE_NONE) {
+			if (suspend_mon_toggle == TOGGLE_START) {
+				timeout = msecs_to_jiffies(
+					SUSPEND_TIMER_TIMEOUT_MS);
+				pr_info("Start suspend monitor\n");
+			} else if (suspend_mon_toggle == TOGGLE_STOP) {
+				timeout = MAX_SCHEDULE_TIMEOUT;
+				timeout_count = 0;
+				pr_info("Stop suspend monitor\n");
+			}
+			suspend_mon_toggle = TOGGLE_NONE;
+			mutex_unlock(&suspend_mon_lock);
+			continue;
+		}
+		mutex_unlock(&suspend_mon_lock);
+
+		/* suspend monitor event handler */
+		if (err == 0) {
+			timeout_count++;
+			suspend_timeout(timeout_count);
+		} else if (err == -ERESTARTSYS) {
+			pr_info("Exit ksuspend_mon!");
+			break;
+		}
+	} while (1);
+
+	return 0;
+}
+
+static void init_suspend_monitor_thread(void)
+{
+	int ret;
+
+	ksuspend_mon_tsk = kthread_create(suspend_monitor_kthread,
+		NULL, "ksuspend_mon");
+	if (IS_ERR(ksuspend_mon_tsk)) {
+		ret = PTR_ERR(ksuspend_mon_tsk);
+		ksuspend_mon_tsk = NULL;
+		pr_err("Create suspend_monitor_kthread failed! ret = %d\n",
+			ret);
+		return;
+	}
+
+	suspend_mon_toggle = TOGGLE_NONE;
+	wake_up_process(ksuspend_mon_tsk);
+
+}
+#endif
+
 /**
  * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
  *
@@ -89,6 +222,10 @@ static void s2idle_enter(void)
 {
 	trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
 
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	stop_suspend_mon();
+#endif
+
 	raw_spin_lock_irq(&s2idle_lock);
 	if (pm_wakeup_pending())
 		goto out;
@@ -114,6 +251,10 @@ static void s2idle_enter(void)
 	s2idle_state = S2IDLE_STATE_NONE;
 	raw_spin_unlock_irq(&s2idle_lock);
 
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	start_suspend_mon();
+#endif
+
 	trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, false);
 }
 
@@ -179,6 +320,9 @@ void __init pm_states_init(void)
 	 * initialize mem_sleep_states[] accordingly here.
 	 */
 	mem_sleep_states[PM_SUSPEND_TO_IDLE] = mem_sleep_labels[PM_SUSPEND_TO_IDLE];
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	init_suspend_monitor_thread();
+#endif
 }
 
 static int __init mem_sleep_default_setup(char *str)
@@ -426,6 +570,10 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 	if (error || suspend_test(TEST_CPUS))
 		goto Enable_cpus;
 
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	stop_suspend_mon();
+#endif
+
 	arch_suspend_disable_irqs();
 	BUG_ON(!irqs_disabled());
 
@@ -451,6 +599,10 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 	arch_suspend_enable_irqs();
 	BUG_ON(irqs_disabled());
 
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	start_suspend_mon();
+#endif
+
  Enable_cpus:
 	suspend_enable_secondary_cpus();
 
@@ -610,6 +762,11 @@ int pm_suspend(suspend_state_t state)
 		return -EINVAL;
 
 	pr_info("suspend entry (%s)\n", mem_sleep_labels[state]);
+
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	start_suspend_mon();
+#endif
+
 	error = enter_state(state);
 	if (error) {
 		suspend_stats.fail++;
@@ -617,6 +774,11 @@ int pm_suspend(suspend_state_t state)
 	} else {
 		suspend_stats.success++;
 	}
+
+#ifdef CONFIG_PM_SLEEP_MONITOR
+	stop_suspend_mon();
+#endif
+
 	pr_info("suspend exit\n");
 	return error;
 }
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9b75f6bfc333..58db8762eeda 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2259,6 +2259,11 @@ module_param_named(console_suspend, console_suspend_enabled,
 MODULE_PARM_DESC(console_suspend, "suspend console during suspend"
 	" and hibernate operations");
 
+int is_console_suspended(void)
+{
+	return console_suspended;
+}
+
 /**
  * suspend_console - suspend the console subsystem
  *
-- 
2.29.0.rc1.297.gfa9743e501-goog

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

[Greg Kroah-Hartman]

On Fri, Oct 16, 2020 at 11:51:09AM +0800, Joseph Jang wrote:
> From: josephjang <josephjang@google.com>

Please use your name as spelled out like you did above in the email
header.

> 
> 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@google.com>

Need a blank line before the signed-off-by: and again, spell your name
the same way.



> ---
>  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);

For global functions, how about:
	bool console_is_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.

Ouch, are you sure you want to panic?


> +
>  config DPM_WATCHDOG
>  	bool "Device suspend/resume watchdog"
>  	depends on PM_DEBUG && PSTORE && EXPERT
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 40f86ec4ab30..f25b8a47583e 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -575,6 +575,69 @@ void __pm_pr_dbg(bool defer, const char *fmt, ...)
>  static inline void pm_print_times_init(void) {}
>  #endif /* CONFIG_PM_SLEEP_DEBUG */
>  
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +/* If set, devices will stuck at suspend for verification */
> +static bool pm_hang_enabled;
> +
> +static int pm_notify_test(struct notifier_block *nb,
> +			     unsigned long mode, void *_unused)
> +{
> +	pr_info("Jump into infinite loop now\n");

Why do you have debugging code still enabled?

> +
> +	/* Suspend thread stuck at a loop forever */
> +	for (;;)
> +		;
> +

Don't busy spin, that will burn power.


> +	pr_info("Fail to stuck at loop\n");

And how can this happen?

> +
> +	return 0;
> +}
> +
> +static struct notifier_block pm_notify_nb = {
> +	.notifier_call = pm_notify_test,
> +};
> +
> +static ssize_t pm_hang_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			     char *buf)
> +{
> +	return snprintf(buf, 10, "%d\n", pm_hang_enabled);
> +}
> +
> +static ssize_t pm_hang_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			      const char *buf, size_t n)
> +{
> +	unsigned long val;
> +	int result;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	if (val > 1)
> +		return -EINVAL;
> +
> +	pm_hang_enabled = !!val;
> +
> +	if (pm_hang_enabled == true) {
> +
> +		result = register_pm_notifier(&pm_notify_nb);
> +		if (result)
> +			pr_warn("Can not register suspend notifier, return %d\n",
> +				result);
> +
> +	} else {
> +
> +		result = unregister_pm_notifier(&pm_notify_nb);
> +		if (result)
> +			pr_warn("Can not unregister suspend notifier, return %d\n",
> +				result);
> +	}
> +
> +	return n;
> +}
> +
> +power_attr(pm_hang);
> +#endif
> +
>  struct kobject *power_kobj;
>  
>  /**
> @@ -909,6 +972,9 @@ static struct attribute * g[] = {
>  	&pm_wakeup_irq_attr.attr,
>  	&pm_debug_messages_attr.attr,
>  #endif
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +	&pm_hang_attr.attr,

You added a sysfs file, but no Documentation/ABI/ update?  That's not
ok.


> +#endif
>  #endif
>  #ifdef CONFIG_FREEZER
>  	&pm_freeze_timeout_attr.attr,
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -30,6 +30,12 @@
>  #include <trace/events/power.h>
>  #include <linux/compiler.h>
>  #include <linux/moduleparam.h>
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +#include <linux/sched/debug.h>
> +#include <linux/kthread.h>
> +#include <linux/sched.h>
> +#include <uapi/linux/sched/types.h>
> +#endif

Don't #ifdef include files.

And why the uapi file?

>  
>  #include "power.h"
>  
> @@ -61,6 +67,133 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
>  enum s2idle_states __read_mostly s2idle_state;
>  static DEFINE_RAW_SPINLOCK(s2idle_lock);
>  
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +/* Suspend monitor thread toggle reason */
> +enum toggle_reason {
> +	TOGGLE_NONE,
> +	TOGGLE_START,
> +	TOGGLE_STOP,
> +};
> +
> +#define SUSPEND_TIMER_TIMEOUT_MS 30000
> +static struct task_struct *ksuspend_mon_tsk;
> +static DECLARE_WAIT_QUEUE_HEAD(power_suspend_waitqueue);
> +static enum toggle_reason suspend_mon_toggle;
> +static DEFINE_MUTEX(suspend_mon_lock);
> +
> +static void start_suspend_mon(void)
> +{
> +	mutex_lock(&suspend_mon_lock);
> +	suspend_mon_toggle = TOGGLE_START;
> +	mutex_unlock(&suspend_mon_lock);

Why do you need a lock for a single integer?

> +	wake_up(&power_suspend_waitqueue);
> +}
> +
> +static void stop_suspend_mon(void)
> +{
> +	mutex_lock(&suspend_mon_lock);
> +	suspend_mon_toggle = TOGGLE_STOP;
> +	mutex_unlock(&suspend_mon_lock);
> +	wake_up(&power_suspend_waitqueue);
> +}
> +
> +static void suspend_timeout(int timeout_count)
> +{
> +	char *null_pointer = NULL;
> +
> +	pr_info("Suspend monitor timeout (timer is %d seconds)\n",
> +		(SUSPEND_TIMER_TIMEOUT_MS/1000));
> +
> +	show_state_filter(TASK_UNINTERRUPTIBLE);
> +
> +	if (timeout_count < 2)
> +		return;
> +
> +	if (is_console_suspended())
> +		resume_console();
> +
> +	pr_info("Trigger a panic\n");

Again, debugging code?

> +
> +	/* Trigger a NULL pointer dereference */
> +	*null_pointer = 'a';

Are you sure this will work on all platforms?  We do have a panic
function if you really want to do that.

> +
> +	/* Should not reach here */
> +	pr_err("Trigger panic failed!\n");
> +}
> +
> +static int suspend_monitor_kthread(void *arg)
> +{
> +	long err;
> +	struct sched_param param = {.sched_priority
> +		= MAX_RT_PRIO-1};

Ick, no, call the scheduler functions properly, don't do this "by hand"
ever.

> +	static int timeout_count;
> +	static long timeout;
> +
> +	pr_info("Init ksuspend_mon thread\n");

Again, debugging code :(

> +
> +	sched_setscheduler(current, SCHED_FIFO, &param);
> +
> +	timeout_count = 0;
> +	timeout = MAX_SCHEDULE_TIMEOUT;
> +
> +	do {
> +		/* Wait suspend timer timeout */
> +		err = wait_event_interruptible_timeout(
> +			power_suspend_waitqueue,
> +			(suspend_mon_toggle != TOGGLE_NONE),
> +			timeout);
> +
> +		mutex_lock(&suspend_mon_lock);
> +		/* suspend monitor state change */
> +		if (suspend_mon_toggle != TOGGLE_NONE) {
> +			if (suspend_mon_toggle == TOGGLE_START) {
> +				timeout = msecs_to_jiffies(
> +					SUSPEND_TIMER_TIMEOUT_MS);
> +				pr_info("Start suspend monitor\n");
> +			} else if (suspend_mon_toggle == TOGGLE_STOP) {
> +				timeout = MAX_SCHEDULE_TIMEOUT;
> +				timeout_count = 0;
> +				pr_info("Stop suspend monitor\n");
> +			}
> +			suspend_mon_toggle = TOGGLE_NONE;
> +			mutex_unlock(&suspend_mon_lock);
> +			continue;
> +		}
> +		mutex_unlock(&suspend_mon_lock);
> +
> +		/* suspend monitor event handler */
> +		if (err == 0) {
> +			timeout_count++;
> +			suspend_timeout(timeout_count);
> +		} else if (err == -ERESTARTSYS) {
> +			pr_info("Exit ksuspend_mon!");
> +			break;
> +		}
> +	} while (1);
> +
> +	return 0;
> +}
> +
> +static void init_suspend_monitor_thread(void)
> +{
> +	int ret;
> +
> +	ksuspend_mon_tsk = kthread_create(suspend_monitor_kthread,
> +		NULL, "ksuspend_mon");
> +	if (IS_ERR(ksuspend_mon_tsk)) {
> +		ret = PTR_ERR(ksuspend_mon_tsk);
> +		ksuspend_mon_tsk = NULL;
> +		pr_err("Create suspend_monitor_kthread failed! ret = %d\n",
> +			ret);
> +		return;
> +	}
> +
> +	suspend_mon_toggle = TOGGLE_NONE;
> +	wake_up_process(ksuspend_mon_tsk);
> +
> +}
> +#endif
> +
>  /**
>   * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
>   *
> @@ -89,6 +222,10 @@ static void s2idle_enter(void)
>  {
>  	trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
>  
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +	stop_suspend_mon();
> +#endif

Do not put #ifdef in .c files, that's not the proper kernel coding
style.  Especially for single function calls.

I've stopped here...

greg k-h

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

[Petr Mladek]

On Fri 2020-10-16 11:51:09, Joseph Jang wrote:
> From: josephjang <josephjang@google.com>
> 
> 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.

A better solution would be to resume instead of panic().

> Note: Can use following command to simulate suspend hang for testing.
>     adb shell echo 1 > /sys/power/pm_hang

This looks dangerous. It adds a simple way to panic() the system.

First, it should get enabled separately. e.g.
CONFIG_TEST_PM_SLEEP_MONITOR.

Second, I would add it as a module that might get loaded
and unloaded.

> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> +static int suspend_monitor_kthread(void *arg)
> +{
> +	long err;
> +	struct sched_param param = {.sched_priority
> +		= MAX_RT_PRIO-1};
> +	static int timeout_count;
> +	static long timeout;
> +
> +	pr_info("Init ksuspend_mon thread\n");
> +
> +	sched_setscheduler(current, SCHED_FIFO, &param);
> +
> +	timeout_count = 0;
> +	timeout = MAX_SCHEDULE_TIMEOUT;
> +
> +	do {
> +		/* Wait suspend timer timeout */
> +		err = wait_event_interruptible_timeout(
> +			power_suspend_waitqueue,
> +			(suspend_mon_toggle != TOGGLE_NONE),
> +			timeout);
> +
> +		mutex_lock(&suspend_mon_lock);
> +		/* suspend monitor state change */
> +		if (suspend_mon_toggle != TOGGLE_NONE) {
> +			if (suspend_mon_toggle == TOGGLE_START) {
> +				timeout = msecs_to_jiffies(
> +					SUSPEND_TIMER_TIMEOUT_MS);
> +				pr_info("Start suspend monitor\n");
> +			} else if (suspend_mon_toggle == TOGGLE_STOP) {
> +				timeout = MAX_SCHEDULE_TIMEOUT;
> +				timeout_count = 0;
> +				pr_info("Stop suspend monitor\n");
> +			}
> +			suspend_mon_toggle = TOGGLE_NONE;
> +			mutex_unlock(&suspend_mon_lock);
> +			continue;
> +		}
> +		mutex_unlock(&suspend_mon_lock);
> +
> +		/* suspend monitor event handler */
> +		if (err == 0) {
> +			timeout_count++;
> +			suspend_timeout(timeout_count);
> +		} else if (err == -ERESTARTSYS) {
> +			pr_info("Exit ksuspend_mon!");
> +			break;
> +		}
> +	} while (1);
> +
> +	return 0;
> +}

Using kthread looks like an overkill to me. I wonder how this actually
works when the kthreads get freezed. It might be enough to implement
just a timer callback. Start the timer in start_suspend_mon() and
delete it in stop_suspend_mon(). Or do I miss anything?

Anyway, the kthread implementation looks a but hairy. If you really
need to use kthread, I suggest to use kthread_worker API. You would
need to run an init work to setup the RT scheduling. Then you
could just call kthread_queue_delayed_work(()
and kthread_cancel_delayed_work_sync() to start and stop
the monitor.


> @@ -114,6 +251,10 @@ static void s2idle_enter(void)
>  	s2idle_state = S2IDLE_STATE_NONE;
>  	raw_spin_unlock_irq(&s2idle_lock);
>  
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +	start_suspend_mon();
> +#endif

It is better to solve this by defining start_suspend_mon() as empty
function when the config option is disabled. For example, see
how  vgacon_text_force() is defined in console.h.

Best Regards,
Petr

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

[Greg Kroah-Hartman]

On Fri, Oct 16, 2020 at 04:58:38PM +0800, Joseph Jang wrote:
> Thank you Greg's promptly reply.

<snip>

You just sent html email, which got rejected by all of the mailing
lists :(

Please fix your email client to be sand and resend.

thanks,

greg k-h

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

[Joseph Jang]

Thank you Greg's promptly reply.

let me try to explain detail in following. Sorry I forgot to switch to
plain text mode in gmail.

On Fri, Oct 16, 2020 at 11:51:09AM +0800, Joseph Jang wrote:
> From: josephjang <josephjang@google.com>

Please use your name as spelled out like you did above in the email
header.

Sure, I will update the patch again like following.

From b3afca8f3ee1d587c188b830d94c683fe66cbfb3 Mon Sep 17 00:00:00 2001
From: Joseph Jang <josephjang@google.com>
Date: Fri, 16 Oct 2020 16:31:38 +0800
Subject: [PATCH] power: suspend: Add suspend timeout handler

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.


>
> 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@google.com>

Need a blank line before the signed-off-by: and again, spell your name
the same way.

Sure, I will update the patch again like following.

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: Joseph Jang <josephjang@google.com>
---


> ---
>  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);

For global functions, how about:
        bool console_is_suspended(void);
?

Agree, I will update like following.
 extern bool console_suspend_enabled;
+extern int console_is_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.

Ouch, are you sure you want to panic?

Yes, we would like to trigger kernel panic to reboot system and
prevent system hang there.


> +
>  config DPM_WATCHDOG
>       bool "Device suspend/resume watchdog"
>       depends on PM_DEBUG && PSTORE && EXPERT
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 40f86ec4ab30..f25b8a47583e 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -575,6 +575,69 @@ void __pm_pr_dbg(bool defer, const char *fmt, ...)
>  static inline void pm_print_times_init(void) {}
>  #endif /* CONFIG_PM_SLEEP_DEBUG */
>
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +/* If set, devices will stuck at suspend for verification */
> +static bool pm_hang_enabled;
> +
> +static int pm_notify_test(struct notifier_block *nb,
> +                          unsigned long mode, void *_unused)
> +{
> +     pr_info("Jump into infinite loop now\n");

Why do you have debugging code still enabled?

Because we want to make sure system hang was cause by our test code here.


> +
> +     /* Suspend thread stuck at a loop forever */
> +     for (;;)
> +             ;
> +

Don't busy spin, that will burn power.

Okay, I will add msleep() to prevent system burn power during testing.

+static int pm_notify_test(struct notifier_block *nb,
+     unsigned long mode, void *_unused)
+{
+ pr_info("Jump into infinite loop now\n");
+
+ /* Suspend thread stuck at a loop forever */
+ for (;;)
+ msleep(100);
+
+ pr_info("Fail to stuck at loop\n");
+
+ return 0;
+}


> +     pr_info("Fail to stuck at loop\n");

And how can this happen?

I think we should not see this log unless the for loop has problem.


Sure, I add some description in the document like following.
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -420,3 +420,18 @@ Description:
  disables it.  Reads from the file return the current value.
  The default is "1" if the build-time "SUSPEND_SKIP_SYNC" config
  flag is unset, or "0" otherwise.
+
+What: /sys/power/pm_hang
+Date: Oct 2020
+Contact: Joseph Jang <josephjang@google.com>
+Description:
+ The /sys/power/pm_hang file controls the variable pm_hang_enabled
+ which controls whether you need to simulate system hang during
+ suspend.
+
+ Writing a "1" to this file will set the pm_hang_enabled to "1" and
+ register a pm_notify function pm_notify_test() to simulate system
+ hang during suspend.
+ Writing a "0" to this file will set the pm_hang_enabled to "0" and
+ unregister a pm_notify function pm_notify_test().
+ Reads from this file return the current value of pm_hang_enabled.

> +#endif
>  #endif
>  #ifdef CONFIG_FREEZER
>       &pm_freeze_timeout_attr.attr,
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -30,6 +30,12 @@
>  #include <trace/events/power.h>
>  #include <linux/compiler.h>
>  #include <linux/moduleparam.h>
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +#include <linux/sched/debug.h>
> +#include <linux/kthread.h>
> +#include <linux/sched.h>
> +#include <uapi/linux/sched/types.h>
> +#endif

Don't #ifdef include files.

And why the uapi file?


I refer to linux-stable/kernel/rcu/tree.c source code.
In order to prevent build error for MAX_RT_PRIO, I need to include
this header file
<uapi/linux/sched/types.h>.

>
>  #include "power.h"
>
> @@ -61,6 +67,133 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
>  enum s2idle_states __read_mostly s2idle_state;
>  static DEFINE_RAW_SPINLOCK(s2idle_lock);
>
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +/* Suspend monitor thread toggle reason */
> +enum toggle_reason {
> +     TOGGLE_NONE,
> +     TOGGLE_START,
> +     TOGGLE_STOP,
> +};
> +
> +#define SUSPEND_TIMER_TIMEOUT_MS 30000
> +static struct task_struct *ksuspend_mon_tsk;
> +static DECLARE_WAIT_QUEUE_HEAD(power_suspend_waitqueue);
> +static enum toggle_reason suspend_mon_toggle;
> +static DEFINE_MUTEX(suspend_mon_lock);
> +
> +static void start_suspend_mon(void)
> +{
> +     mutex_lock(&suspend_mon_lock);
> +     suspend_mon_toggle = TOGGLE_START;
> +     mutex_unlock(&suspend_mon_lock);

Why do you need a lock for a single integer?

Since we have two kernel thread may reference variable "suspend_mon_toggle",
one is suspend thread, another is suspend_monitor_kthread. We use it
to start/stop
suspend monitor.

Yes, in order to notify user this kernel panic was triggered by
suspend_timeout(), I add debug code to prevent confused.


> +
> +     /* Trigger a NULL pointer dereference */
> +     *null_pointer = 'a';

Are you sure this will work on all platforms?  We do have a panic
function if you really want to do that.

I agree to change to use panic() like following.
+static void suspend_timeout(int timeout_count)
+{
+ pr_info("Suspend monitor timeout (timer is %d seconds)\n",
+ (SUSPEND_TIMER_TIMEOUT_MS/1000));
+
+ show_state_filter(TASK_UNINTERRUPTIBLE);
+
+ if (timeout_count < 2)
+ return;
+
+ if (console_is_suspended())
+ resume_console();
+
+ /* Trigger a NULL pointer dereference */
+ panic("suspend timeout and panic");
+}

> +
> +     /* Should not reach here */
> +     pr_err("Trigger panic failed!\n");
> +}
> +
> +static int suspend_monitor_kthread(void *arg)
> +{
> +     long err;
> +     struct sched_param param = {.sched_priority
> +             = MAX_RT_PRIO-1};

Ick, no, call the scheduler functions properly, don't do this "by hand"

ever.
I refer to kernel/sched/core.c and they use MAX_RT_PRIO as thread
priority directly.
If you have other suggestion, I am willing to try.


> +     static int timeout_count;
> +     static long timeout;
> +
> +     pr_info("Init ksuspend_mon thread\n");

Again, debugging code :(

In order to make sure ksuspend_mon thread has been initialize, I add log here.
If you think that is not necessary, I can remove it.

Sure, I got it, could you help to review if we can use IS_ENABLED()
like following?
+ if (IS_ENABLED(CONFIG_PM_SLEEP_MONITOR))
+ stop_suspend_mon();
+


Thank you,
Joseph.


Joseph Jang <josephjang@google.com> 於 2020年10月16日 週五 下午4:58寫道：
>
> Thank you Greg's promptly reply.
>
> let me try to explain detail in following.
>
> On Fri, Oct 16, 2020 at 11:51:09AM +0800, Joseph Jang wrote:
> > From: josephjang <josephjang@google.com>
>
> Please use your name as spelled out like you did above in the email
> header.
>
> Sure, I will update the patch again like following.
>
> From b3afca8f3ee1d587c188b830d94c683fe66cbfb3 Mon Sep 17 00:00:00 2001
> From: Joseph Jang <josephjang@google.com>
> Date: Fri, 16 Oct 2020 16:31:38 +0800
> Subject: [PATCH] power: suspend: Add suspend timeout handler
>
> 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.
>
>
> >
> > 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@google.com>
>
> Need a blank line before the signed-off-by: and again, spell your name
> the same way.
>
> Sure, I will update the patch again like following.
>
> 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: Joseph Jang <josephjang@google.com>
> ---
>
>
> > ---
> >  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);
>
> For global functions, how about:
>         bool console_is_suspended(void);
> ?
>
> Agree, I will update like following.
>  extern bool console_suspend_enabled;
> +extern int console_is_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.
>
> Ouch, are you sure you want to panic?
>
> Yes, we would like to trigger kernel panic to reboot system and prevent system hang there.
>
>
> > +
> >  config DPM_WATCHDOG
> >       bool "Device suspend/resume watchdog"
> >       depends on PM_DEBUG && PSTORE && EXPERT
> > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > index 40f86ec4ab30..f25b8a47583e 100644
> > --- a/kernel/power/main.c
> > +++ b/kernel/power/main.c
> > @@ -575,6 +575,69 @@ void __pm_pr_dbg(bool defer, const char *fmt, ...)
> >  static inline void pm_print_times_init(void) {}
> >  #endif /* CONFIG_PM_SLEEP_DEBUG */
> >
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +/* If set, devices will stuck at suspend for verification */
> > +static bool pm_hang_enabled;
> > +
> > +static int pm_notify_test(struct notifier_block *nb,
> > +                          unsigned long mode, void *_unused)
> > +{
> > +     pr_info("Jump into infinite loop now\n");
>
> Why do you have debugging code still enabled?
>
> Because we want to make sure system hang was cause by our test code here.
>
>
> > +
> > +     /* Suspend thread stuck at a loop forever */
> > +     for (;;)
> > +             ;
> > +
>
> Don't busy spin, that will burn power.
>
> Okay, I will add msleep() to prevent system burn power during testing.
>
> +static int pm_notify_test(struct notifier_block *nb,
> +     unsigned long mode, void *_unused)
> +{
> + pr_info("Jump into infinite loop now\n");
> +
> + /* Suspend thread stuck at a loop forever */
> + for (;;)
> + msleep(100);
> +
> + pr_info("Fail to stuck at loop\n");
> +
> + return 0;
> +}
>
>
> > +     pr_info("Fail to stuck at loop\n");
>
> And how can this happen?
>
> I think we should not see this log unless the for loop has problem.
>
>
>
> > +
> > +     return 0;
> > +}
> > +
> > +static struct notifier_block pm_notify_nb = {
> > +     .notifier_call = pm_notify_test,
> > +};
> > +
> > +static ssize_t pm_hang_show(struct kobject *kobj, struct kobj_attribute *attr,
> > +                          char *buf)
> > +{
> > +     return snprintf(buf, 10, "%d\n", pm_hang_enabled);
> > +}
> > +
> > +static ssize_t pm_hang_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +                           const char *buf, size_t n)
> > +{
> > +     unsigned long val;
> > +     int result;
> > +
> > +     if (kstrtoul(buf, 10, &val))
> > +             return -EINVAL;
> > +
> > +     if (val > 1)
> > +             return -EINVAL;
> > +
> > +     pm_hang_enabled = !!val;
> > +
> > +     if (pm_hang_enabled == true) {
> > +
> > +             result = register_pm_notifier(&pm_notify_nb);
> > +             if (result)
> > +                     pr_warn("Can not register suspend notifier, return %d\n",
> > +                             result);
> > +
> > +     } else {
> > +
> > +             result = unregister_pm_notifier(&pm_notify_nb);
> > +             if (result)
> > +                     pr_warn("Can not unregister suspend notifier, return %d\n",
> > +                             result);
> > +     }
> > +
> > +     return n;
> > +}
> > +
> > +power_attr(pm_hang);
> > +#endif
> > +
> >  struct kobject *power_kobj;
> >
> >  /**
> > @@ -909,6 +972,9 @@ static struct attribute * g[] = {
> >       &pm_wakeup_irq_attr.attr,
> >       &pm_debug_messages_attr.attr,
> >  #endif
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +     &pm_hang_attr.attr,
>
> You added a sysfs file, but no Documentation/ABI/ update?  That's not
> ok.
>
>
> Sure, I add some description in the document like following.
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -420,3 +420,18 @@ Description:
>   disables it.  Reads from the file return the current value.
>   The default is "1" if the build-time "SUSPEND_SKIP_SYNC" config
>   flag is unset, or "0" otherwise.
> +
> +What: /sys/power/pm_hang
> +Date: Oct 2020
> +Contact: Joseph Jang <josephjang@google.com>
> +Description:
> + The /sys/power/pm_hang file controls the variable pm_hang_enabled
> + which controls whether you need to simulate system hang during
> + suspend.
> +
> + Writing a "1" to this file will set the pm_hang_enabled to "1" and
> + register a pm_notify function pm_notify_test() to simulate system
> + hang during suspend.
> + Writing a "0" to this file will set the pm_hang_enabled to "0" and
> + unregister a pm_notify function pm_notify_test().
> + Reads from this file return the current value of pm_hang_enabled.
>
> > +#endif
> >  #endif
> >  #ifdef CONFIG_FREEZER
> >       &pm_freeze_timeout_attr.attr,
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -30,6 +30,12 @@
> >  #include <trace/events/power.h>
> >  #include <linux/compiler.h>
> >  #include <linux/moduleparam.h>
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +#include <linux/sched/debug.h>
> > +#include <linux/kthread.h>
> > +#include <linux/sched.h>
> > +#include <uapi/linux/sched/types.h>
> > +#endif
>
> Don't #ifdef include files.
>
> And why the uapi file?
>
>
> I refer to linux-stable/kernel/rcu/tree.c source code.
> In order to prevent build error for MAX_RT_PRIO, I need to include this header file
> <uapi/linux/sched/types.h>.
>
> >
> >  #include "power.h"
> >
> > @@ -61,6 +67,133 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
> >  enum s2idle_states __read_mostly s2idle_state;
> >  static DEFINE_RAW_SPINLOCK(s2idle_lock);
> >
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +/* Suspend monitor thread toggle reason */
> > +enum toggle_reason {
> > +     TOGGLE_NONE,
> > +     TOGGLE_START,
> > +     TOGGLE_STOP,
> > +};
> > +
> > +#define SUSPEND_TIMER_TIMEOUT_MS 30000
> > +static struct task_struct *ksuspend_mon_tsk;
> > +static DECLARE_WAIT_QUEUE_HEAD(power_suspend_waitqueue);
> > +static enum toggle_reason suspend_mon_toggle;
> > +static DEFINE_MUTEX(suspend_mon_lock);
> > +
> > +static void start_suspend_mon(void)
> > +{
> > +     mutex_lock(&suspend_mon_lock);
> > +     suspend_mon_toggle = TOGGLE_START;
> > +     mutex_unlock(&suspend_mon_lock);
>
> Why do you need a lock for a single integer?
>
> Since we have two kernel thread may reference variable "suspend_mon_toggle",
> one is suspend thread, another is suspend_monitor_kthread. We use it to start/stop
> suspend monitor.
>
>
>
> > +     wake_up(&power_suspend_waitqueue);
> > +}
> > +
> > +static void stop_suspend_mon(void)
> > +{
> > +     mutex_lock(&suspend_mon_lock);
> > +     suspend_mon_toggle = TOGGLE_STOP;
> > +     mutex_unlock(&suspend_mon_lock);
> > +     wake_up(&power_suspend_waitqueue);
> > +}
> > +
> > +static void suspend_timeout(int timeout_count)
> > +{
> > +     char *null_pointer = NULL;
> > +
> > +     pr_info("Suspend monitor timeout (timer is %d seconds)\n",
> > +             (SUSPEND_TIMER_TIMEOUT_MS/1000));
> > +
> > +     show_state_filter(TASK_UNINTERRUPTIBLE);
> > +
> > +     if (timeout_count < 2)
> > +             return;
> > +
> > +     if (is_console_suspended())
> > +             resume_console();
> > +
> > +     pr_info("Trigger a panic\n");
>
> Again, debugging code?
>
> Yes, in order to notify user this kernel panic was triggered by
> suspend_timeout(), I add debug code to prevent confused.
>
>
> > +
> > +     /* Trigger a NULL pointer dereference */
> > +     *null_pointer = 'a';
>
> Are you sure this will work on all platforms?  We do have a panic
> function if you really want to do that.
>
> I agree to change to use panic() like following.
> +static void suspend_timeout(int timeout_count)
> +{
> + pr_info("Suspend monitor timeout (timer is %d seconds)\n",
> + (SUSPEND_TIMER_TIMEOUT_MS/1000));
> +
> + show_state_filter(TASK_UNINTERRUPTIBLE);
> +
> + if (timeout_count < 2)
> + return;
> +
> + if (console_is_suspended())
> + resume_console();
> +
> + /* Trigger a NULL pointer dereference */
> + panic("suspend timeout and panic");
> +}
>
> > +
> > +     /* Should not reach here */
> > +     pr_err("Trigger panic failed!\n");
> > +}
> > +
> > +static int suspend_monitor_kthread(void *arg)
> > +{
> > +     long err;
> > +     struct sched_param param = {.sched_priority
> > +             = MAX_RT_PRIO-1};
>
> Ick, no, call the scheduler functions properly, don't do this "by hand"
>
> ever.
> I refer to kernel/sched/core.c and they use MAX_RT_PRIO as thread priority directly.
> If you have other suggestion, I am willing to try.
>
>
> > +     static int timeout_count;
> > +     static long timeout;
> > +
> > +     pr_info("Init ksuspend_mon thread\n");
>
> Again, debugging code :(
>
> In order to make sure ksuspend_mon thread has been initialize, I add log here.
> If you think that is not necessary, I can remove it.
>
>
>
>
> > +
> > +     sched_setscheduler(current, SCHED_FIFO, &param);
> > +
> > +     timeout_count = 0;
> > +     timeout = MAX_SCHEDULE_TIMEOUT;
> > +
> > +     do {
> > +             /* Wait suspend timer timeout */
> > +             err = wait_event_interruptible_timeout(
> > +                     power_suspend_waitqueue,
> > +                     (suspend_mon_toggle != TOGGLE_NONE),
> > +                     timeout);
> > +
> > +             mutex_lock(&suspend_mon_lock);
> > +             /* suspend monitor state change */
> > +             if (suspend_mon_toggle != TOGGLE_NONE) {
> > +                     if (suspend_mon_toggle == TOGGLE_START) {
> > +                             timeout = msecs_to_jiffies(
> > +                                     SUSPEND_TIMER_TIMEOUT_MS);
> > +                             pr_info("Start suspend monitor\n");
> > +                     } else if (suspend_mon_toggle == TOGGLE_STOP) {
> > +                             timeout = MAX_SCHEDULE_TIMEOUT;
> > +                             timeout_count = 0;
> > +                             pr_info("Stop suspend monitor\n");
> > +                     }
> > +                     suspend_mon_toggle = TOGGLE_NONE;
> > +                     mutex_unlock(&suspend_mon_lock);
> > +                     continue;
> > +             }
> > +             mutex_unlock(&suspend_mon_lock);
> > +
> > +             /* suspend monitor event handler */
> > +             if (err == 0) {
> > +                     timeout_count++;
> > +                     suspend_timeout(timeout_count);
> > +             } else if (err == -ERESTARTSYS) {
> > +                     pr_info("Exit ksuspend_mon!");
> > +                     break;
> > +             }
> > +     } while (1);
> > +
> > +     return 0;
> > +}
> > +
> > +static void init_suspend_monitor_thread(void)
> > +{
> > +     int ret;
> > +
> > +     ksuspend_mon_tsk = kthread_create(suspend_monitor_kthread,
> > +             NULL, "ksuspend_mon");
> > +     if (IS_ERR(ksuspend_mon_tsk)) {
> > +             ret = PTR_ERR(ksuspend_mon_tsk);
> > +             ksuspend_mon_tsk = NULL;
> > +             pr_err("Create suspend_monitor_kthread failed! ret = %d\n",
> > +                     ret);
> > +             return;
> > +     }
> > +
> > +     suspend_mon_toggle = TOGGLE_NONE;
> > +     wake_up_process(ksuspend_mon_tsk);
> > +
> > +}
> > +#endif
> > +
> >  /**
> >   * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
> >   *
> > @@ -89,6 +222,10 @@ static void s2idle_enter(void)
> >  {
> >       trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
> >
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +     stop_suspend_mon();
> > +#endif
>
> Do not put #ifdef in .c files, that's not the proper kernel coding
> style.  Especially for single function calls.
>
> Sure, I got it, could you help to review if we can use IS_ENABLED() like following?
> + if (IS_ENABLED(CONFIG_PM_SLEEP_MONITOR))
> + stop_suspend_mon();
> +
>
>
> Thank you,
> Joseph.
>
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 於 2020年10月16日 週五 下午1:43寫道：
>>
>> On Fri, Oct 16, 2020 at 11:51:09AM +0800, Joseph Jang wrote:
>> > From: josephjang <josephjang@google.com>
>>
>> Please use your name as spelled out like you did above in the email
>> header.
>>
>> >
>> > 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@google.com>
>>
>> Need a blank line before the signed-off-by: and again, spell your name
>> the same way.
>>
>>
>>
>> > ---
>> >  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);
>>
>> For global functions, how about:
>>         bool console_is_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.
>>
>> Ouch, are you sure you want to panic?
>>
>>
>> > +
>> >  config DPM_WATCHDOG
>> >       bool "Device suspend/resume watchdog"
>> >       depends on PM_DEBUG && PSTORE && EXPERT
>> > diff --git a/kernel/power/main.c b/kernel/power/main.c
>> > index 40f86ec4ab30..f25b8a47583e 100644
>> > --- a/kernel/power/main.c
>> > +++ b/kernel/power/main.c
>> > @@ -575,6 +575,69 @@ void __pm_pr_dbg(bool defer, const char *fmt, ...)
>> >  static inline void pm_print_times_init(void) {}
>> >  #endif /* CONFIG_PM_SLEEP_DEBUG */
>> >
>> > +#ifdef CONFIG_PM_SLEEP_MONITOR
>> > +/* If set, devices will stuck at suspend for verification */
>> > +static bool pm_hang_enabled;
>> > +
>> > +static int pm_notify_test(struct notifier_block *nb,
>> > +                          unsigned long mode, void *_unused)
>> > +{
>> > +     pr_info("Jump into infinite loop now\n");
>>
>> Why do you have debugging code still enabled?
>>
>> > +
>> > +     /* Suspend thread stuck at a loop forever */
>> > +     for (;;)
>> > +             ;
>> > +
>>
>> Don't busy spin, that will burn power.
>>
>>
>> > +     pr_info("Fail to stuck at loop\n");
>>
>> And how can this happen?
>>
>> > +
>> > +     return 0;
>> > +}
>> > +
>> > +static struct notifier_block pm_notify_nb = {
>> > +     .notifier_call = pm_notify_test,
>> > +};
>> > +
>> > +static ssize_t pm_hang_show(struct kobject *kobj, struct kobj_attribute *attr,
>> > +                          char *buf)
>> > +{
>> > +     return snprintf(buf, 10, "%d\n", pm_hang_enabled);
>> > +}
>> > +
>> > +static ssize_t pm_hang_store(struct kobject *kobj, struct kobj_attribute *attr,
>> > +                           const char *buf, size_t n)
>> > +{
>> > +     unsigned long val;
>> > +     int result;
>> > +
>> > +     if (kstrtoul(buf, 10, &val))
>> > +             return -EINVAL;
>> > +
>> > +     if (val > 1)
>> > +             return -EINVAL;
>> > +
>> > +     pm_hang_enabled = !!val;
>> > +
>> > +     if (pm_hang_enabled == true) {
>> > +
>> > +             result = register_pm_notifier(&pm_notify_nb);
>> > +             if (result)
>> > +                     pr_warn("Can not register suspend notifier, return %d\n",
>> > +                             result);
>> > +
>> > +     } else {
>> > +
>> > +             result = unregister_pm_notifier(&pm_notify_nb);
>> > +             if (result)
>> > +                     pr_warn("Can not unregister suspend notifier, return %d\n",
>> > +                             result);
>> > +     }
>> > +
>> > +     return n;
>> > +}
>> > +
>> > +power_attr(pm_hang);
>> > +#endif
>> > +
>> >  struct kobject *power_kobj;
>> >
>> >  /**
>> > @@ -909,6 +972,9 @@ static struct attribute * g[] = {
>> >       &pm_wakeup_irq_attr.attr,
>> >       &pm_debug_messages_attr.attr,
>> >  #endif
>> > +#ifdef CONFIG_PM_SLEEP_MONITOR
>> > +     &pm_hang_attr.attr,
>>
>> You added a sysfs file, but no Documentation/ABI/ update?  That's not
>> ok.
>>
>>
>> > +#endif
>> >  #endif
>> >  #ifdef CONFIG_FREEZER
>> >       &pm_freeze_timeout_attr.attr,
>> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>> > index 8b1bb5ee7e5d..6f2679cfd9d1 100644
>> > --- a/kernel/power/suspend.c
>> > +++ b/kernel/power/suspend.c
>> > @@ -30,6 +30,12 @@
>> >  #include <trace/events/power.h>
>> >  #include <linux/compiler.h>
>> >  #include <linux/moduleparam.h>
>> > +#ifdef CONFIG_PM_SLEEP_MONITOR
>> > +#include <linux/sched/debug.h>
>> > +#include <linux/kthread.h>
>> > +#include <linux/sched.h>
>> > +#include <uapi/linux/sched/types.h>
>> > +#endif
>>
>> Don't #ifdef include files.
>>
>> And why the uapi file?
>>
>> >
>> >  #include "power.h"
>> >
>> > @@ -61,6 +67,133 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
>> >  enum s2idle_states __read_mostly s2idle_state;
>> >  static DEFINE_RAW_SPINLOCK(s2idle_lock);
>> >
>> > +#ifdef CONFIG_PM_SLEEP_MONITOR
>> > +/* Suspend monitor thread toggle reason */
>> > +enum toggle_reason {
>> > +     TOGGLE_NONE,
>> > +     TOGGLE_START,
>> > +     TOGGLE_STOP,
>> > +};
>> > +
>> > +#define SUSPEND_TIMER_TIMEOUT_MS 30000
>> > +static struct task_struct *ksuspend_mon_tsk;
>> > +static DECLARE_WAIT_QUEUE_HEAD(power_suspend_waitqueue);
>> > +static enum toggle_reason suspend_mon_toggle;
>> > +static DEFINE_MUTEX(suspend_mon_lock);
>> > +
>> > +static void start_suspend_mon(void)
>> > +{
>> > +     mutex_lock(&suspend_mon_lock);
>> > +     suspend_mon_toggle = TOGGLE_START;
>> > +     mutex_unlock(&suspend_mon_lock);
>>
>> Why do you need a lock for a single integer?
>>
>> > +     wake_up(&power_suspend_waitqueue);
>> > +}
>> > +
>> > +static void stop_suspend_mon(void)
>> > +{
>> > +     mutex_lock(&suspend_mon_lock);
>> > +     suspend_mon_toggle = TOGGLE_STOP;
>> > +     mutex_unlock(&suspend_mon_lock);
>> > +     wake_up(&power_suspend_waitqueue);
>> > +}
>> > +
>> > +static void suspend_timeout(int timeout_count)
>> > +{
>> > +     char *null_pointer = NULL;
>> > +
>> > +     pr_info("Suspend monitor timeout (timer is %d seconds)\n",
>> > +             (SUSPEND_TIMER_TIMEOUT_MS/1000));
>> > +
>> > +     show_state_filter(TASK_UNINTERRUPTIBLE);
>> > +
>> > +     if (timeout_count < 2)
>> > +             return;
>> > +
>> > +     if (is_console_suspended())
>> > +             resume_console();
>> > +
>> > +     pr_info("Trigger a panic\n");
>>
>> Again, debugging code?
>>
>> > +
>> > +     /* Trigger a NULL pointer dereference */
>> > +     *null_pointer = 'a';
>>
>> Are you sure this will work on all platforms?  We do have a panic
>> function if you really want to do that.
>>
>> > +
>> > +     /* Should not reach here */
>> > +     pr_err("Trigger panic failed!\n");
>> > +}
>> > +
>> > +static int suspend_monitor_kthread(void *arg)
>> > +{
>> > +     long err;
>> > +     struct sched_param param = {.sched_priority
>> > +             = MAX_RT_PRIO-1};
>>
>> Ick, no, call the scheduler functions properly, don't do this "by hand"
>> ever.
>>
>> > +     static int timeout_count;
>> > +     static long timeout;
>> > +
>> > +     pr_info("Init ksuspend_mon thread\n");
>>
>> Again, debugging code :(
>>
>> > +
>> > +     sched_setscheduler(current, SCHED_FIFO, &param);
>> > +
>> > +     timeout_count = 0;
>> > +     timeout = MAX_SCHEDULE_TIMEOUT;
>> > +
>> > +     do {
>> > +             /* Wait suspend timer timeout */
>> > +             err = wait_event_interruptible_timeout(
>> > +                     power_suspend_waitqueue,
>> > +                     (suspend_mon_toggle != TOGGLE_NONE),
>> > +                     timeout);
>> > +
>> > +             mutex_lock(&suspend_mon_lock);
>> > +             /* suspend monitor state change */
>> > +             if (suspend_mon_toggle != TOGGLE_NONE) {
>> > +                     if (suspend_mon_toggle == TOGGLE_START) {
>> > +                             timeout = msecs_to_jiffies(
>> > +                                     SUSPEND_TIMER_TIMEOUT_MS);
>> > +                             pr_info("Start suspend monitor\n");
>> > +                     } else if (suspend_mon_toggle == TOGGLE_STOP) {
>> > +                             timeout = MAX_SCHEDULE_TIMEOUT;
>> > +                             timeout_count = 0;
>> > +                             pr_info("Stop suspend monitor\n");
>> > +                     }
>> > +                     suspend_mon_toggle = TOGGLE_NONE;
>> > +                     mutex_unlock(&suspend_mon_lock);
>> > +                     continue;
>> > +             }
>> > +             mutex_unlock(&suspend_mon_lock);
>> > +
>> > +             /* suspend monitor event handler */
>> > +             if (err == 0) {
>> > +                     timeout_count++;
>> > +                     suspend_timeout(timeout_count);
>> > +             } else if (err == -ERESTARTSYS) {
>> > +                     pr_info("Exit ksuspend_mon!");
>> > +                     break;
>> > +             }
>> > +     } while (1);
>> > +
>> > +     return 0;
>> > +}
>> > +
>> > +static void init_suspend_monitor_thread(void)
>> > +{
>> > +     int ret;
>> > +
>> > +     ksuspend_mon_tsk = kthread_create(suspend_monitor_kthread,
>> > +             NULL, "ksuspend_mon");
>> > +     if (IS_ERR(ksuspend_mon_tsk)) {
>> > +             ret = PTR_ERR(ksuspend_mon_tsk);
>> > +             ksuspend_mon_tsk = NULL;
>> > +             pr_err("Create suspend_monitor_kthread failed! ret = %d\n",
>> > +                     ret);
>> > +             return;
>> > +     }
>> > +
>> > +     suspend_mon_toggle = TOGGLE_NONE;
>> > +     wake_up_process(ksuspend_mon_tsk);
>> > +
>> > +}
>> > +#endif
>> > +
>> >  /**
>> >   * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
>> >   *
>> > @@ -89,6 +222,10 @@ static void s2idle_enter(void)
>> >  {
>> >       trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
>> >
>> > +#ifdef CONFIG_PM_SLEEP_MONITOR
>> > +     stop_suspend_mon();
>> > +#endif
>>
>> Do not put #ifdef in .c files, that's not the proper kernel coding
>> style.  Especially for single function calls.
>>
>> I've stopped here...
>>
>> greg k-h
>
>
>
> --
> Embedded Software engineer



-- 
Embedded Software engineer

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

[Joseph Jang]

Thanks Petr promptly response.

On Fri 2020-10-16 11:51:09, Joseph Jang wrote:
> From: josephjang <josephjang@google.com>
>
> 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.

A better solution would be to resume instead of panic().

[Joseph] suspend_timeout() will trigger kernel panic() only when
suspend thread stuck (deadlock/hang) for 2*30 seconds.
At that moment, I don't know how to resume the suspend thread. So I
just could trigger panic to reboot system.
If you have better suggestions, I am willing to study it.


> Note: Can use following command to simulate suspend hang for testing.
>     adb shell echo 1 > /sys/power/pm_hang

This looks dangerous. It adds a simple way to panic() the system.

First, it should get enabled separately. e.g.
CONFIG_TEST_PM_SLEEP_MONITOR.

Second, I would add it as a module that might get loaded
and unloaded.

[Joseph] Agree to enable new compile flag for test module.
I think it is better to create separate patch for the new test module right?

> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
Using kthread looks like an overkill to me. I wonder how this actually
works when the kthreads get freezed. It might be enough to implement
just a timer callback. Start the timer in start_suspend_mon() and
delete it in stop_suspend_mon(). Or do I miss anything?


Anyway, the kthread implementation looks a but hairy. If you really
need to use kthread, I suggest to use kthread_worker API. You would
need to run an init work to setup the RT scheduling. Then you
could just call kthread_queue_delayed_work(()
and kthread_cancel_delayed_work_sync() to start and stop
the monitor.

[Joseph]
Actually, I had ever think we just need to use
add_timer()/del_timer_sync() for start_suspend_mon()/stop_suspend_mon() before.

But I am not sure if add_timer() may cause any performance impact in
suspend thread or not.
So I try to create a suspend monitor kthread and just flip the flag in
suspend thread.

Thank you,
Joseph.
> @@ -114,6 +251,10 @@ static void s2idle_enter(void)
>       s2idle_state = S2IDLE_STATE_NONE;
>       raw_spin_unlock_irq(&s2idle_lock);
>
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +     start_suspend_mon();
> +#endif

It is better to solve this by defining start_suspend_mon() as empty
function when the config option is disabled. For example, see
how  vgacon_text_force() is defined in console.h.

[Joseph] Thank you for good suggestions.
May I know if I could use IS_ENABLED() ?
if (IS_ENABLED(CONFIG_PM_SLEEP_MONITOR))
start_suspend_mon();


Best Regards,
Petr



Thank you,
Joseph.


Petr Mladek <pmladek@suse.com> 於 2020年10月16日 週五 下午5:01寫道：
>
> On Fri 2020-10-16 11:51:09, Joseph Jang wrote:
> > From: josephjang <josephjang@google.com>
> >
> > 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.
>
> A better solution would be to resume instead of panic().
>
> > Note: Can use following command to simulate suspend hang for testing.
> >     adb shell echo 1 > /sys/power/pm_hang
>
> This looks dangerous. It adds a simple way to panic() the system.
>
> First, it should get enabled separately. e.g.
> CONFIG_TEST_PM_SLEEP_MONITOR.
>
> Second, I would add it as a module that might get loaded
> and unloaded.
>
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > +static int suspend_monitor_kthread(void *arg)
> > +{
> > +     long err;
> > +     struct sched_param param = {.sched_priority
> > +             = MAX_RT_PRIO-1};
> > +     static int timeout_count;
> > +     static long timeout;
> > +
> > +     pr_info("Init ksuspend_mon thread\n");
> > +
> > +     sched_setscheduler(current, SCHED_FIFO, &param);
> > +
> > +     timeout_count = 0;
> > +     timeout = MAX_SCHEDULE_TIMEOUT;
> > +
> > +     do {
> > +             /* Wait suspend timer timeout */
> > +             err = wait_event_interruptible_timeout(
> > +                     power_suspend_waitqueue,
> > +                     (suspend_mon_toggle != TOGGLE_NONE),
> > +                     timeout);
> > +
> > +             mutex_lock(&suspend_mon_lock);
> > +             /* suspend monitor state change */
> > +             if (suspend_mon_toggle != TOGGLE_NONE) {
> > +                     if (suspend_mon_toggle == TOGGLE_START) {
> > +                             timeout = msecs_to_jiffies(
> > +                                     SUSPEND_TIMER_TIMEOUT_MS);
> > +                             pr_info("Start suspend monitor\n");
> > +                     } else if (suspend_mon_toggle == TOGGLE_STOP) {
> > +                             timeout = MAX_SCHEDULE_TIMEOUT;
> > +                             timeout_count = 0;
> > +                             pr_info("Stop suspend monitor\n");
> > +                     }
> > +                     suspend_mon_toggle = TOGGLE_NONE;
> > +                     mutex_unlock(&suspend_mon_lock);
> > +                     continue;
> > +             }
> > +             mutex_unlock(&suspend_mon_lock);
> > +
> > +             /* suspend monitor event handler */
> > +             if (err == 0) {
> > +                     timeout_count++;
> > +                     suspend_timeout(timeout_count);
> > +             } else if (err == -ERESTARTSYS) {
> > +                     pr_info("Exit ksuspend_mon!");
> > +                     break;
> > +             }
> > +     } while (1);
> > +
> > +     return 0;
> > +}
>
> Using kthread looks like an overkill to me. I wonder how this actually
> works when the kthreads get freezed. It might be enough to implement
> just a timer callback. Start the timer in start_suspend_mon() and
> delete it in stop_suspend_mon(). Or do I miss anything?
>
> Anyway, the kthread implementation looks a but hairy. If you really
> need to use kthread, I suggest to use kthread_worker API. You would
> need to run an init work to setup the RT scheduling. Then you
> could just call kthread_queue_delayed_work(()
> and kthread_cancel_delayed_work_sync() to start and stop
> the monitor.
>
>
> > @@ -114,6 +251,10 @@ static void s2idle_enter(void)
> >       s2idle_state = S2IDLE_STATE_NONE;
> >       raw_spin_unlock_irq(&s2idle_lock);
> >
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +     start_suspend_mon();
> > +#endif
>
> It is better to solve this by defining start_suspend_mon() as empty
> function when the config option is disabled. For example, see
> how  vgacon_text_force() is defined in console.h.
>
> Best Regards,
> Petr



-- 
Embedded Software engineer

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

[Rafael J. Wysocki]

On Fri, Oct 16, 2020 at 11:01 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2020-10-16 11:51:09, Joseph Jang wrote:
> > From: josephjang <josephjang@google.com>
> >
> > 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.
>
> A better solution would be to resume instead of panic().

Well, abort the suspend if it happens during suspend or continue if it
happens during resume,

But we have a suspend watchdog already, don't we?

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

[Rafael J. Wysocki]

On Fri, Oct 16, 2020 at 5:51 AM Joseph Jang <josephjang@google.com> wrote:
>
> From: josephjang <josephjang@google.com>
>
> 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@google.com>
> ---
>  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

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

[Greg KH]

Note, your response is not threading well, just hit 'reply' in your
email client...

On Tue, Oct 20, 2020 at 09:01:05AM +0000, josephjang@google.com wrote:
> > On Tue, Oct 20, 2020 at 08:15:38AM +0000, josephjang@google.com wrote:
> > > > 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@google.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.
> 
> > Only put .h files that are needed by different .c files in the
> > include/linux/ directory.  Otherwise it should be local to where the .c
> > file is.
> > Great, use that!
> 
> > > But we really hit the suspend hang issue that DPM_WATCHDOG cannot cover.
> 
> > What issue is that?
> 
> > > 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.
> 
> > How will a user know which they should use?
> 
> > Why not just fix whatever is wrong with the watchdog code instead of
> > creating a new one?
> 
> > > > 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.
> 
> > Then fix that!
> 
> > > I think PM_SLEEP_MONITOR is an extension debug feature from
> > DPM_WATCHDOG.
> 
> > Please just fix the watchdog, as obviously it is not working properly.
> > Don't create something new because of that.
> 
> > thanks,
> 
> > greg k-h
> 
> Thank you Greq for promptly responding.
> I am okay to fix the DPM_WATCHDOG feature, but would like to have quick sync
> up before start.
> Could you help?
> 
> 
> 1. Can we change the kernel config name ?
> DPM_WATCHDOG stands for Device Power Management.
> We propose PM_SLEEP_MONITOR is to cover Core PM and Device PM.

That's fine.

> 2. Can we create a new data structure instead of using the following struct
> dpm_watchdog?
> struct dpm_watchdog {
> 	struct device		*dev;
> 	struct task_struct	*tsk;
> 	struct timer_list	timer;
> };

Why not use the existing one?

> 
> I list some reasons by following ...
> 
> static int device_resume(struct device *dev, pm_message_t state, bool async)
> {
> 	pm_callback_t callback = NULL;
> 	const char *info = NULL;
> 	int error = 0;
> 	DECLARE_DPM_WATCHDOG_ON_STACK(wd);  <== dpm_watchdog use stack memory for
> watchdog timer struct, but sleep timer use global memory.

Then move it off of the stack.

> 
> ...<SNIP>
> 
> 	if (!dpm_wait_for_superior(dev, async))
> 		goto Complete;
> 
> 	dpm_watchdog_set(&wd, dev);  <== dpm_watchdog need "struct device", but
> sleep timer doesn't need it.

That's fine, what are you trying to optimize for?


> 	device_lock(dev);
> 
> ...<SNIP>
> 
>  Unlock:
> 	device_unlock(dev);
> 	dpm_watchdog_clear(&wd);
> 
>  Complete:
> 	complete_all(&dev->power.completion);
> 
> 	TRACE_RESUME(error);
> 
> 	return error;
> }

Submit a patch that shows what you are doing and we will be glad to
review that.

thanks,

greg k-h

[PATCH] power: suspend: Add suspend timeout handler

[josephjang]

> On Tue, Oct 20, 2020 at 08:15:38AM +0000, josephjang@google.com wrote:
> > > 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@google.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.

> Only put .h files that are needed by different .c files in the
> include/linux/ directory.  Otherwise it should be local to where the .c
> file is.
> Great, use that!

> > But we really hit the suspend hang issue that DPM_WATCHDOG cannot cover.

> What issue is that?

> > 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.

> How will a user know which they should use?

> Why not just fix whatever is wrong with the watchdog code instead of
> creating a new one?

> > > 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.

> Then fix that!

> > I think PM_SLEEP_MONITOR is an extension debug feature from  
> DPM_WATCHDOG.

> Please just fix the watchdog, as obviously it is not working properly.
> Don't create something new because of that.

> thanks,

> greg k-h

Thank you Greq for promptly responding.
I am okay to fix the DPM_WATCHDOG feature, but would like to have quick  
sync up before start.
Could you help?


1. Can we change the kernel config name ?
DPM_WATCHDOG stands for Device Power Management.
We propose PM_SLEEP_MONITOR is to cover Core PM and Device PM.


2. Can we create a new data structure instead of using the following struct  
dpm_watchdog?
struct dpm_watchdog {
	struct device		*dev;
	struct task_struct	*tsk;
	struct timer_list	timer;
};

I list some reasons by following ...

static int device_resume(struct device *dev, pm_message_t state, bool async)
{
	pm_callback_t callback = NULL;
	const char *info = NULL;
	int error = 0;
	DECLARE_DPM_WATCHDOG_ON_STACK(wd);  <== dpm_watchdog use stack memory for  
watchdog timer struct, but sleep timer use global memory.

...<SNIP>

	if (!dpm_wait_for_superior(dev, async))
		goto Complete;

	dpm_watchdog_set(&wd, dev);  <== dpm_watchdog need "struct device", but  
sleep timer doesn't need it.
	device_lock(dev);

...<SNIP>

  Unlock:
	device_unlock(dev);
	dpm_watchdog_clear(&wd);

  Complete:
	complete_all(&dev->power.completion);

	TRACE_RESUME(error);

	return error;
}

Thank you,
Joseph.

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

[Greg KH]

On Tue, Oct 20, 2020 at 08:15:38AM +0000, josephjang@google.com wrote:
> > 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@google.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.

Only put .h files that are needed by different .c files in the
include/linux/ directory.  Otherwise it should be local to where the .c
file is.

> > > --- /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.

Great, use that!

> But we really hit the suspend hang issue that DPM_WATCHDOG cannot cover.

What issue is that?

> 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.

How will a user know which they should use?

Why not just fix whatever is wrong with the watchdog code instead of
creating a new one?

> > 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.

Then fix that!

> I think PM_SLEEP_MONITOR is an extension debug feature from DPM_WATCHDOG.

Please just fix the watchdog, as obviously it is not working properly.
Don't create something new because of that.

thanks,

greg k-h

[PATCH] power: suspend: Add suspend timeout handler

[josephjang]

> 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@google.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.

[PATCH] power: suspend: Add suspend timeout handler

[josephjang]

> >
> > On Fri, Oct 16, 2020 at 3:22 PM <josephjang@google.com> wrote:
> > >
> > > Thank you Rafael's promptly response.
> > >
> > > > On Fri, Oct 16, 2020 at 5:51 AM Joseph Jang <josephjang@google.com>  
> wrote:
> > > > >
> > > > > From: josephjang <josephjang@google.com>
> > > > >
> > > > > 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@google.com>
> > > > > ---
> > > > >  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.


Since kernel/power/suspend.c doesn't need "struct device *dev" like  
following.
Can we create a new suspend_timer to cover PM core and Device PM hang  
issues?

/**
  * dpm_watchdog_set - Enable pm watchdog for given device.
  * @wd: Watchdog. Must be allocated on the stack.
  * @dev: Device to handle.
  */
static void dpm_watchdog_set(struct dpm_watchdog *wd, struct device *dev)
{
	struct timer_list *timer = &wd->timer;

	wd->dev = dev;
	wd->tsk = current;
...



Thank you,
Joseph.

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

[Joseph Jang]

Yes, I agree.

Rafael J. Wysocki <rafael@kernel.org> 於 2020年10月16日 週五 下午9:24寫道：
>
> On Fri, Oct 16, 2020 at 3:22 PM <josephjang@google.com> wrote:
> >
> > Thank you Rafael's promptly response.
> >
> > > On Fri, Oct 16, 2020 at 5:51 AM Joseph Jang <josephjang@google.com> wrote:
> > > >
> > > > From: josephjang <josephjang@google.com>
> > > >
> > > > 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@google.com>
> > > > ---
> > > >  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

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

[Rafael J. Wysocki]

On Fri, Oct 16, 2020 at 3:22 PM <josephjang@google.com> wrote:
>
> Thank you Rafael's promptly response.
>
> > On Fri, Oct 16, 2020 at 5:51 AM Joseph Jang <josephjang@google.com> wrote:
> > >
> > > From: josephjang <josephjang@google.com>
> > >
> > > 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@google.com>
> > > ---
> > >  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.

[PATCH] power: suspend: Add suspend timeout handler

[josephjang]

Thank you Rafael's promptly response.

> On Fri, Oct 16, 2020 at 5:51 AM Joseph Jang <josephjang@google.com> wrote:
> >
> > From: josephjang <josephjang@google.com>
> >
> > 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@google.com>
> > ---
> >  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).
Our suspend timeout monitor can cover PM core and Device PM hang issues.

Thank you,
Joseph.

[PATCH] power: suspend: Add suspend timeout handler

[josephjang]

Thank you Greg's promptly reply.

let me try to explain detail in following.

On Fri, Oct 16, 2020 at 11:51:09AM +0800, Joseph Jang wrote:
> From: josephjang <josephjang@google.com>

Please use your name as spelled out like you did above in the email
header.

Sure, I will update the patch again like following.
 From b3afca8f3ee1d587c188b830d94c683fe66cbfb3 Mon Sep 17 00:00:00 2001
From: Joseph Jang <josephjang@google.com>
Date: Fri, 16 Oct 2020 16:31:38 +0800
Subject: [PATCH] power: suspend: Add suspend timeout handler

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.


> 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@google.com>

Need a blank line before the signed-off-by: and again, spell your name
the same way.

Sure, I will update the patch again like following.

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: Joseph Jang <josephjang@google.com>
---

> ---
>   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);

For global functions, how about:
         bool console_is_suspended(void);
?

Agree, I will update like following.
  extern bool console_suspend_enabled;
+extern int console_is_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.

Ouch, are you sure you want to panic?
Yes, we would like to trigger kernel panic to reboot system and prevent  
system hang there.

> +
>   config DPM_WATCHDOG
>        bool "Device suspend/resume watchdog"
>        depends on PM_DEBUG && PSTORE && EXPERT
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 40f86ec4ab30..f25b8a47583e 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -575,6 +575,69 @@ void __pm_pr_dbg(bool defer, const char *fmt, ...)
>   static inline void pm_print_times_init(void) {}
>   #endif /* CONFIG_PM_SLEEP_DEBUG */

> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +/* If set, devices will stuck at suspend for verification */
> +static bool pm_hang_enabled;
> +
> +static int pm_notify_test(struct notifier_block *nb,
> +                          unsigned long mode, void *_unused)
> +{
> +     pr_info("Jump into infinite loop now\n");

Why do you have debugging code still enabled?
Because we want to make sure system hang was cause by our test code here.

> +
> +     /* Suspend thread stuck at a loop forever */
> +     for (;;)
> +             ;
> +

Don't busy spin, that will burn power.
Okay, I will add msleep() to prevent system burn power during testing.
+static int pm_notify_test(struct notifier_block *nb,
+     unsigned long mode, void *_unused)
+{
+ pr_info("Jump into infinite loop now\n");
+
+ /* Suspend thread stuck at a loop forever */
+ for (;;)
+ msleep(100);
+
+ pr_info("Fail to stuck at loop\n");
+
+ return 0;
+}

> +     pr_info("Fail to stuck at loop\n");

And how can this happen?
I think we should not see this log unless the for loop has problem.

Sure, I add some description in the document like following.
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -420,3 +420,18 @@ Description:
   disables it.  Reads from the file return the current value.
   The default is "1" if the build-time "SUSPEND_SKIP_SYNC" config
   flag is unset, or "0" otherwise.
+
+What: /sys/power/pm_hang
+Date: Oct 2020
+Contact: Joseph Jang <josephjang@google.com>
+Description:
+ The /sys/power/pm_hang file controls the variable pm_hang_enabled
+ which controls whether you need to simulate system hang during
+ suspend.
+
+ Writing a "1" to this file will set the pm_hang_enabled to "1" and
+ register a pm_notify function pm_notify_test() to simulate system
+ hang during suspend.
+ Writing a "0" to this file will set the pm_hang_enabled to "0" and
+ unregister a pm_notify function pm_notify_test().
+ Reads from this file return the current value of pm_hang_enabled.
> +#endif
>   #endif
>   #ifdef CONFIG_FREEZER
>        &pm_freeze_timeout_attr.attr,
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -30,6 +30,12 @@
>   #include <trace/events/power.h>
>   #include <linux/compiler.h>
>   #include <linux/moduleparam.h>
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +#include <linux/sched/debug.h>
> +#include <linux/kthread.h>
> +#include <linux/sched.h>
> +#include <uapi/linux/sched/types.h>
> +#endif

Don't #ifdef include files.

And why the uapi file?

I refer to linux-stable/kernel/rcu/tree.c source code.
In order to prevent build error for MAX_RT_PRIO, I need to include this  
header file
<uapi/linux/sched/types.h>.

>   #include "power.h"

> @@ -61,6 +67,133 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
>   enum s2idle_states __read_mostly s2idle_state;
>   static DEFINE_RAW_SPINLOCK(s2idle_lock);

> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +/* Suspend monitor thread toggle reason */
> +enum toggle_reason {
> +     TOGGLE_NONE,
> +     TOGGLE_START,
> +     TOGGLE_STOP,
> +};
> +
> +#define SUSPEND_TIMER_TIMEOUT_MS 30000
> +static struct task_struct *ksuspend_mon_tsk;
> +static DECLARE_WAIT_QUEUE_HEAD(power_suspend_waitqueue);
> +static enum toggle_reason suspend_mon_toggle;
> +static DEFINE_MUTEX(suspend_mon_lock);
> +
> +static void start_suspend_mon(void)
> +{
> +     mutex_lock(&suspend_mon_lock);
> +     suspend_mon_toggle = TOGGLE_START;
> +     mutex_unlock(&suspend_mon_lock);

Why do you need a lock for a single integer?
Since we have two kernel thread may reference variable "suspend_mon_toggle",
one is suspend thread, another is suspend_monitor_kthread. We use it to  
start/stop
suspend monitor.
Yes, in order to notify user this kernel panic was triggered by
suspend_timeout(), I add debug code to prevent confused.

> +
> +     /* Trigger a NULL pointer dereference */
> +     *null_pointer = 'a';

Are you sure this will work on all platforms?  We do have a panic
function if you really want to do that.
I agree to change to use panic() like following.
+static void suspend_timeout(int timeout_count)
+{
+ pr_info("Suspend monitor timeout (timer is %d seconds)\n",
+ (SUSPEND_TIMER_TIMEOUT_MS/1000));
+
+ show_state_filter(TASK_UNINTERRUPTIBLE);
+
+ if (timeout_count < 2)
+ return;
+
+ if (console_is_suspended())
+ resume_console();
+
+ /* Trigger a NULL pointer dereference */
+ panic("suspend timeout and panic");
+}
> +
> +     /* Should not reach here */
> +     pr_err("Trigger panic failed!\n");
> +}
> +
> +static int suspend_monitor_kthread(void *arg)
> +{
> +     long err;
> +     struct sched_param param = {.sched_priority
> +             = MAX_RT_PRIO-1};

Ick, no, call the scheduler functions properly, don't do this "by hand"
ever.
I refer to kernel/sched/core.c and they use MAX_RT_PRIO as thread priority  
directly.
If you have other suggestion, I am willing to try.

> +     static int timeout_count;
> +     static long timeout;
> +
> +     pr_info("Init ksuspend_mon thread\n");

Again, debugging code :(
In order to make sure ksuspend_mon thread has been initialize, I add log  
here.
If you think that is not necessary, I can remove it.
Sure, I got it, could you help to review if we can use IS_ENABLED() like  
following?
+ if (IS_ENABLED(CONFIG_PM_SLEEP_MONITOR))
+ stop_suspend_mon();
+

Thank you,
Joseph.

[PATCH] power: suspend: Add suspend timeout handler

[josephjang]

Thank you Petr for promptly reply.

> On Fri 2020-10-16 11:51:09, Joseph Jang wrote:
> > From: josephjang <josephjang@google.com>
> >
> > 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.

> A better solution would be to resume instead of panic().


[Joseph] suspend_timeout() will trigger kernel panic() only when
suspend thread stuck (deadlock/hang) for 2*30 seconds.
At that moment, I don't know how to resume the suspend thread. So I
just could trigger panic to reboot system.
If you have better suggestions, I am willing to study it.

> > Note: Can use following command to simulate suspend hang for testing.
> >     adb shell echo 1 > /sys/power/pm_hang

> This looks dangerous. It adds a simple way to panic() the system.

> First, it should get enabled separately. e.g.
> CONFIG_TEST_PM_SLEEP_MONITOR.

> Second, I would add it as a module that might get loaded
> and unloaded.


[Joseph] Agree to enable new compile flag for test module.
I think it is better to create separate patch for the new test module right?

> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> Using kthread looks like an overkill to me. I wonder how this actually
> works when the kthreads get freezed. It might be enough to implement
> just a timer callback. Start the timer in start_suspend_mon() and
> delete it in stop_suspend_mon(). Or do I miss anything?

> Anyway, the kthread implementation looks a but hairy. If you really
> need to use kthread, I suggest to use kthread_worker API. You would
> need to run an init work to setup the RT scheduling. Then you
> could just call kthread_queue_delayed_work(()
> and kthread_cancel_delayed_work_sync() to start and stop
> the monitor.



[Joseph]
Actually, I had ever think we just need to use
add_timer()/del_timer_sync() for start_suspend_mon()/stop_suspend_mon()  
before.

But I am not sure if add_timer() may cause any performance impact in
suspend thread or not.
So I try to create a suspend monitor kthread and just flip the flag in
suspend thread.


> > @@ -114,6 +251,10 @@ static void s2idle_enter(void)
> >       s2idle_state = S2IDLE_STATE_NONE;
> >       raw_spin_unlock_irq(&s2idle_lock);
> >
> > +#ifdef CONFIG_PM_SLEEP_MONITOR
> > +     start_suspend_mon();
> > +#endif

> It is better to solve this by defining start_suspend_mon() as empty
> function when the config option is disabled. For example, see
> how  vgacon_text_force() is defined in console.h.


[Joseph] Thank you for good suggestions.
May I know if I could use IS_ENABLED() ?
if (IS_ENABLED(CONFIG_PM_SLEEP_MONITOR))
     start_suspend_mon();

> Best Regards,
> Petr