PM / sleep: Expose DPM watchdog timeout to sysfs

RFC PATCH NAK Chen Yu 3 participants 16 messages Aug 11, 2016 – Aug 19, 2016

Subsystems (6)

Documentation Driver core Suspend to RAM Hibernation +2 more subsystems

lore

Share & tools

lore

Chen Yu

author

2016-08-11 12:21 UTC #0 lore raw

cover PM / sleep: Expose DPM watchdog timeout to sysfs

Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.

The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.

Chen Yu (2):
PM / sleep: Make DPM watchdog depend on PM_SLEEP PM / Sleep: Expose DPM watchdog timeout to sysfs

 drivers/base/power/main.c    |  4 +++-
 include/linux/sched/sysctl.h |  4 ++++
 kernel/power/Kconfig         |  2 +-
 kernel/sysctl.c              | 16 ++++++++++++++++
 4 files changed, 24 insertions(+), 2 deletions(-)

Signature · 1 line

2.7.4

Chen Yu

author

2016-08-11 12:21 UTC #1 lore raw

RFC PATCH 1/2 PM / sleep: Make DPM watchdog depend on PM_SLEEP

The DPM watchdog is implemented only when PM_SLEEP is set, so make it depending on the latter.

Signed-off-by Chen Yu <[email protected]>

---
 kernel/power/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/power/Kconfig +1 -1

index 68d3ebc..a359bc5 100644
@@ -186,7 +186,7 @@ config PM_SLEEP_DEBUG
 
 config DPM_WATCHDOG
 	bool "Device suspend/resume watchdog"
-	depends on PM_DEBUG && PSTORE
+	depends on PM_DEBUG && PSTORE && PM_SLEEP
 	---help---
 	  Sets up a watchdog timer to capture drivers that are
 	  locked up attempting to suspend/resume a device.

Signature · 1 line

2.7.4

Rafael J. Wysocki

maintainer

2016-08-12 00:40 UTC #2 lore raw

Re: [PATCH 1/2][RFC] PM / sleep: Make DPM watchdog depend on PM_SLEEP

Quoted 4 lines · Chen Yu

The DPM watchdog is implemented only when PM_SLEEP is set, so make it depending on the latter.
Signed-off-by Chen Yu <[email protected]>

Looks good to me.

Quoted 24 lines

---
 kernel/power/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/power/Kconfig +1 -2

index 68d3ebc..a359bc5 100644
@@ -186,7 +186,7 @@ config PM_SLEEP_DEBUG
 
 config DPM_WATCHDOG
        bool "Device suspend/resume watchdog"
-       depends on PM_DEBUG && PSTORE
+       depends on PM_DEBUG && PSTORE && PM_SLEEP
        ---help---
          Sets up a watchdog timer to capture drivers that are
          locked up attempting to suspend/resume a device.
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Chen Yu

author

2016-08-11 12:21 UTC #3 lore raw

RFC PATCH 2/2 PM / sleep: Expose DPM watchdog timeout to sysfs

In reply to Chen Yu

Default suspend/resume watchdog timeout has once been modified from 12 to 60 seconds, in case some harddisks have firmware problems and take too long time to resume from suspend. And currently we have a new report that this delay may takes more than 60 seconds. So for future possible scenario, this patch exposes the watchdog timeout value to sysfs, and user can customize it to accommodate their use case, without recompiling the kernel.

Link https://bugzilla.kernel.org/show_bug.cgi?id=117971

Reported-by Higuita <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/main.c    |  4 +++-
 include/linux/sched/sysctl.h |  4 ++++
 kernel/sysctl.c              | 16 ++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

drivers/base/power/main.c +3 -1

index e44944f..7f990c2 100644
@@ -407,6 +407,8 @@ struct dpm_watchdog {
 #define DECLARE_DPM_WATCHDOG_ON_STACK(wd) \
 	struct dpm_watchdog wd
 
+int __read_mostly sysctl_dpm_timeout_secs = CONFIG_DPM_WATCHDOG_TIMEOUT;
+
 /**
  * dpm_watchdog_handler - Driver suspend / resume watchdog handler.
  * @data: Watchdog object address.

@@ -439,7 +441,7 @@ static void dpm_watchdog_set(struct dpm_watchdog *wd, struct device *dev)
 
 	init_timer_on_stack(timer);
 	/* use same timeout value for both suspend and resume */
-	timer->expires = jiffies + HZ * CONFIG_DPM_WATCHDOG_TIMEOUT;
+	timer->expires = jiffies + HZ * sysctl_dpm_timeout_secs;
 	timer->function = dpm_watchdog_handler;
 	timer->data = (unsigned long)wd;
 	add_timer(timer);

include/linux/sched/sysctl.h +4 -0

index 22db1e6..1b7f536 100644
@@ -14,6 +14,10 @@ extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
 enum { sysctl_hung_task_timeout_secs = 0 };
 #endif
 
+#ifdef CONFIG_DPM_WATCHDOG
+extern int sysctl_dpm_timeout_secs;
+#endif
+
 extern unsigned int sysctl_sched_latency;
 extern unsigned int sysctl_sched_min_granularity;
 extern unsigned int sysctl_sched_wakeup_granularity;

kernel/sysctl.c +16 -0

index b43d0b2..bc8f01d 100644
@@ -149,6 +149,11 @@ static const int cap_last_cap = CAP_LAST_CAP;
 static unsigned long hung_task_timeout_max = (LONG_MAX/HZ);
 #endif
 
+/* this is needed for the proc_dointvec_minmax for sysctl_dpm_timeout_secs */
+#ifdef CONFIG_DPM_WATCHDOG
+static int dpm_timeout_max = 120;
+#endif
+
 #ifdef CONFIG_INOTIFY_USER
 #include <linux/inotify.h>
 #endif

@@ -1084,6 +1089,17 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &neg_one,
 	},
 #endif
+#ifdef CONFIG_DPM_WATCHDOG
+	{
+		.procname	= "dpm_timeout_secs",
+		.data		= &sysctl_dpm_timeout_secs,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &one,
+		.extra2		= &dpm_timeout_max,
+	},
+#endif
 #ifdef CONFIG_COMPAT
 	{
 		.procname	= "compat-log",

Signature · 1 line

2.7.4

Pavel Machek

NAK

2016-08-11 18:54 UTC #4 lore raw

In reply to Chen Yu

Hi!

Quoted 9 lines

Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.

Quoted 3 lines

The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.

Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.

We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.

Or maybe CONFIG_DPM_WATCHDOG should contain numeric value that user has to select?

And I'm pretty certain new user interfaces need to be documented.

NAK.
Pavel

Quoted 10 lines

Chen Yu (2):
PM / sleep: Make DPM watchdog depend on PM_SLEEP PM / Sleep: Expose DPM watchdog timeout to sysfs
 drivers/base/power/main.c    |  4 +++-
 include/linux/sched/sysctl.h |  4 ++++
 kernel/power/Kconfig         |  2 +-
 kernel/sysctl.c              | 16 ++++++++++++++++
 4 files changed, 24 insertions(+), 2 deletions(-)

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Rafael J. Wysocki

maintainer

2016-08-12 00:39 UTC #5 lore raw

Quoted 15 lines · Pavel Machek

Hi!
Quoted 9 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
Quoted 3 lines
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.

Quoted 5 lines

Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.
We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG".

But this doesn't introduce CONFIG_DPM_WATCHDOG. It makes it depend on CONFIG_PM_SLEEP which is quite reasonable, because that watchdog is only used during sleep transitions.

Thanks,
Rafael

Chen Yu

author

2016-08-12 02:44 UTC #6 lore raw

In reply to Pavel Machek

Hi,

Quoted 18 lines · Pavel Machek

Hi!
Quoted 9 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
Quoted 3 lines
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.
Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.

Quoted 8 lines

We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.
Or maybe CONFIG_DPM_WATCHDOG should contain numeric value that user has to select?

Yes, if people select it then they have the risk to break their system, and the original thought of the patch is to behave like a diagnosis to make it easier for the users to figure it out, how much time it takes to suspend/resume a bogus peripheral, without recomping the kernel. -- currently the timeout value for CONFIG_DPM_WATCHDOG can be adjusted by menuconfig, but bug reporter might have to recompile the kernel to confirm, and it takes some time to get a feedback from them, so...

thanks,
Yu

Pavel Machek

2016-08-12 06:33 UTC #7 lore raw

Hi!

Quoted 24 lines

Quoted 16 lines
Quoted 9 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
Quoted 3 lines
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.
Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.
Quoted 8 lines
We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.
Or maybe CONFIG_DPM_WATCHDOG should contain numeric value that user has to select?

Quoted 7 lines

Yes, if people select it then they have the risk to break their system, and the original thought of the patch is to behave like a diagnosis to make it easier for the users to figure it out, how much time it takes to suspend/resume a bogus peripheral, without recomping the kernel. -- currently the timeout value for CONFIG_DPM_WATCHDOG can be adjusted by menuconfig, but bug reporter might have to recompile the kernel to confirm, and it takes some time to get a feedback from them, so...

Please don't add sysfs knobs for this. People should not need to adjust sysfs files to get working kernel.

You can for example make the default 120 seconds.
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Chen, Yu C

author

2016-08-12 10:49 UTC #8 lore raw

Hi,

Quoted 41 lines

-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Friday, August 12, 2016 2:33 PM
To: Chen, Yu C
Cc: Linux PM List; Rafael J. Wysocki; Greg Kroah-Hartman; Len Brown; Takashi Iwai; Benoit Goby
Subject: Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs
Hi!
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking
CONFIG_PM_SLEEP.
Quoted 10 lines
Quoted 10 lines
Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.
We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.
Or maybe CONFIG_DPM_WATCHDOG should contain numeric value that
user
Quoted 6 lines
Quoted 2 lines
has to select?
Yes, if people select it then they have the risk to break their system, and the original thought of the patch is to behave like a diagnosis to make it easier for the users to figure it out, how much time it takes to suspend/resume a bogus peripheral, without recomping the
kernel.

Quoted 8 lines

Quoted 3 lines
-- currently the timeout value for CONFIG_DPM_WATCHDOG can be adjusted by menuconfig, but bug reporter might have to recompile the kernel to confirm, and it takes some time to get a feedback from them, so...
Please don't add sysfs knobs for this. People should not need to adjust sysfs files to get working kernel.
You can for example make the default 120 seconds.

OK. I'll do that, thanks.

Rafael J. Wysocki

maintainer

2016-08-12 11:25 UTC #9 lore raw

In reply to Pavel Machek

Quoted 34 lines · Pavel Machek

Hi!
Quoted 24 lines
Quoted 16 lines
Quoted 9 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
Quoted 3 lines
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.
Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.
Quoted 8 lines
We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.
Or maybe CONFIG_DPM_WATCHDOG should contain numeric value that user has to select?
Quoted 7 lines
Yes, if people select it then they have the risk to break their system, and the original thought of the patch is to behave like a diagnosis to make it easier for the users to figure it out, how much time it takes to suspend/resume a bogus peripheral, without recomping the kernel. -- currently the timeout value for CONFIG_DPM_WATCHDOG can be adjusted by menuconfig, but bug reporter might have to recompile the kernel to confirm, and it takes some time to get a feedback from them, so...

Quoted 4 lines

Please don't add sysfs knobs for this. People should not need to adjust sysfs files to get working kernel.
You can for example make the default 120 seconds.

Plus, IMO it would be good to be able to disable this thing from the kernel command line entirely in case 2 minutes is still too little time for somebody.

Thanks,
Rafael

Chen Yu

author

2016-08-17 03:35 UTC #10 lore raw

Quoted 35 lines · Rafael J. Wysocki

Quoted 34 lines · Pavel Machek
Hi!
Quoted 31 lines
Quoted 16 lines
Quoted 9 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
Quoted 3 lines
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.
Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.
Quoted 8 lines
We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.
Or maybe CONFIG_DPM_WATCHDOG should contain numeric value that user has to select?
Yes, if people select it then they have the risk to break their system, and the original thought of the patch is to behave like a diagnosis to make it easier for the users to figure it out, how much time it takes to suspend/resume a bogus peripheral, without recomping the kernel. -- currently the timeout value for CONFIG_DPM_WATCHDOG can be adjusted by menuconfig, but bug reporter might have to recompile the kernel to confirm, and it takes some time to get a feedback from them, so...

Quoted 9 lines

Quoted 4 lines
Please don't add sysfs knobs for this. People should not need to adjust sysfs files to get working kernel.
You can for example make the default 120 seconds.
Plus, IMO it would be good to be able to disable this thing from the kernel command line entirely in case 2 minutes is still too little time for somebody.

OK, I've modified the patch to the following version, Rafael, Pavel could you take a glance at it, thanks:

kernel/power/Kconfig +1 -1

Index: linux/kernel/power/Kconfig
===================================================================
@@ -197,7 +197,7 @@ config DPM_WATCHDOG
 config DPM_WATCHDOG_TIMEOUT
 	int "Watchdog timeout in seconds"
 	range 1 120
-	default 60
+	default 120
 	depends on DPM_WATCHDOG
 
 config PM_TRACE

drivers/base/power/main.c +16 -0

Index: linux/drivers/base/power/main.c
===================================================================
@@ -407,6 +407,8 @@ struct dpm_watchdog {
 #define DECLARE_DPM_WATCHDOG_ON_STACK(wd) \
 	struct dpm_watchdog wd
 
+static unsigned long __read_mostly dpm_wd_disabled;
+
 /**
  * dpm_watchdog_handler - Driver suspend / resume watchdog handler.
  * @data: Watchdog object address.

@@ -434,6 +436,9 @@ static void dpm_watchdog_set(struct dpm_
 {
 	struct timer_list *timer = &wd->timer;
 
+	if (dpm_wd_disabled)
+		return;
+
 	wd->dev = dev;
 	wd->tsk = current;

@@ -453,9 +458,20 @@ static void dpm_watchdog_clear(struct dp
 {
 	struct timer_list *timer = &wd->timer;
 
+	if (dpm_wd_disabled)
+		return;
+
 	del_timer_sync(timer);
 	destroy_timer_on_stack(timer);
 }
+
+static int __init no_dpm_wd_setup(char *str)
+{
+	dpm_wd_disabled = 1;
+	return 1;
+}
+__setup("no_dpm_watchdog", no_dpm_wd_setup);
+
 #else
 #define DECLARE_DPM_WATCHDOG_ON_STACK(wd)
 #define dpm_watchdog_set(x, y)

Documentation/kernel-parameters.txt +2 -0

Index: linux/Documentation/kernel-parameters.txt
===================================================================
@@ -2749,6 +2749,8 @@ bytes respectively. Such letter suffixes
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
                         soft-lockup and NMI watchdog (hard-lockup).
 
+	no_dpm_watchdog	[KNL] Disable the device suspend/resume watchdog.
+
 	nowb		[ARM]
 
 	nox2apic	[X86-64,APIC] Do not enable x2APIC mode.

Pavel Machek

2016-08-18 12:06 UTC #11 lore raw

Quoted 39 lines · Chen Yu

Quoted 38 lines · Rafael J. Wysocki
Quoted 37 lines · Pavel Machek
Hi!
Quoted 24 lines
Quoted 16 lines
Quoted 9 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
Quoted 3 lines
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking CONFIG_PM_SLEEP.
Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.
Quoted 8 lines
We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.
Or maybe CONFIG_DPM_WATCHDOG should contain numeric value that user has to select?
Quoted 7 lines
Yes, if people select it then they have the risk to break their system, and the original thought of the patch is to behave like a diagnosis to make it easier for the users to figure it out, how much time it takes to suspend/resume a bogus peripheral, without recomping the kernel. -- currently the timeout value for CONFIG_DPM_WATCHDOG can be adjusted by menuconfig, but bug reporter might have to recompile the kernel to confirm, and it takes some time to get a feedback from them, so...
Please don't add sysfs knobs for this. People should not need to adjust sysfs files to get working kernel.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
You can for example make the default 120 seconds.
Plus, IMO it would be good to be able to disable this thing from the kernel command line entirely in case 2 minutes is still too little time for somebody.
OK, I've modified the patch to the following version, Rafael, Pavel could you take a glance at it, thanks:

Certainly looks better.

kernel/power/Kconfig config DPM_WATCHDOG · line 202

Quoted 12 lines

Index: linux/kernel/power/Kconfig
===================================================================
kernel/power/Kconfig +1 -1
@@ -197,7 +197,7 @@ config DPM_WATCHDOG
 config DPM_WATCHDOG_TIMEOUT
 	int "Watchdog timeout in seconds"
 	range 1 120
-	default 60
+	default 120
 	depends on DPM_WATCHDOG
 

I like this part.

kernel/power/Kconfig bytes respectively. Such letter suffixes · line 2756

Quoted 10 lines

kernel/power/Kconfig +3 -0

+++ linux/Documentation/kernel-parameters.txt

@@ -2749,6 +2749,8 @@ bytes respectively. Such letter suffixes
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
                         soft-lockup and NMI watchdog (hard-lockup).
 
+	no_dpm_watchdog	[KNL] Disable the device suspend/resume watchdog.
+
 	nowb		[ARM]
 
 	nox2apic	[X86-64,APIC] Do not enable x2APIC mode.

Do we really need the new cmdline option? Can we just tell people to disable DPM_WATCHDOG completely?

Is there reason to keep the DPM_WATCHDOG? Did it find some real bugs lately?

Thanks,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Chen, Yu C

author

2016-08-19 01:17 UTC #12 lore raw

Quoted 92 lines

-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Thursday, August 18, 2016 8:07 PM
To: Chen, Yu C
Cc: Rafael J. Wysocki; Linux PM List; Rafael J. Wysocki; Greg Kroah-Hartman; Len Brown; Takashi Iwai; Benoit Goby
Subject: Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs
Quoted 13 lines · Chen Yu
Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Pavel Machek
Hi!
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Recently we have a new report that, the harddisk can not resume on time due to firmware issues, and got a kernel panic because of DPM watchdog timeout. Since the default timeout has once been modified from 12 to 60 seconds, we might still encounter new case which requires a longer timeout, so expose the value to sysfs and let the users decide which value is appropriate, meanwhile this can also ease the debugging process.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
The first patch is to force DPM watchdog depending on CONFIG_PM_SLEEP, thus the second patch which does the actual work, can use CONFIG_DPM_WATCHDOG safely without checking
CONFIG_PM_SLEEP.
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Kernel should just work. User should not have to configure random knobs to have working suspend/hibernation.
We do not want "CONFIG_BREAK_SUSPEND" so I believe we don't want "CONFIG_DPM_WATCHDOG". If normal users select it and it breaks their system, make it depend on "CONFIG_EXPERT" or hide it in some other way or maybe remove it from Kconfig altogether.
Or maybe CONFIG_DPM_WATCHDOG should contain numeric value
that
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
user has to select?
Yes, if people select it then they have the risk to break their system, and the original thought of the patch is to behave like a diagnosis to make it easier for the users to figure it out, how much time it takes to suspend/resume a bogus peripheral, without
recomping the kernel.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
-- currently the timeout value for CONFIG_DPM_WATCHDOG can be adjusted by menuconfig, but bug reporter might have to recompile the kernel to confirm, and it takes some time to get a feedback from
them, so...
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Please don't add sysfs knobs for this. People should not need to adjust sysfs files to get working kernel.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
You can for example make the default 120 seconds.
Plus, IMO it would be good to be able to disable this thing from the kernel command line entirely in case 2 minutes is still too little time for somebody.
OK, I've modified the patch to the following version, Rafael, Pavel could you take a glance at it, thanks:
Certainly looks better.
Quoted 2 lines
Index: linux/kernel/power/Kconfig
================================================================
===
kernel/power/Kconfig config DPM_WATCHDOG config
Quoted 3 lines
kernel/power/Kconfig +0 -0
@@ -197,7 +197,7 @@ config DPM_WATCHDOG  config
DPM_WATCHDOG_TIMEOUT
kernel/power/Kconfig config DPM_WATCHDOG · line 202
Quoted 6 lines
kernel/power/Kconfig +1 -1
 	int "Watchdog timeout in seconds"
 	range 1 120
-	default 60
+	default 120
 	depends on DPM_WATCHDOG
 
I like this part.
kernel/power/Kconfig bytes respectively. Such letter suffixes · line 2752
Quoted 6 lines
kernel/power/Kconfig +2 -0
+++ linux/Documentation/kernel-parameters.txt
@@ -2749,6 +2749,8 @@ bytes respectively. Such letter suffixes
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
                         soft-lockup and NMI watchdog (hard-lockup).
 
+	no_dpm_watchdog	[KNL] Disable the device suspend/resume
watchdog.
Quoted 3 lines
kernel/power/Kconfig +1 -0
+
 	nowb		[ARM]
 

Quoted 7 lines

kernel/power/Kconfig bytes respectively. Such letter suffixes · line 2756
Quoted 1 line
kernel/power/Kconfig +0 -0
 	nox2apic	[X86-64,APIC] Do not enable x2APIC mode.
Do we really need the new cmdline option? Can we just tell people to disable DPM_WATCHDOG completely?
Is there reason to keep the DPM_WATCHDOG? Did it find some real bugs lately?

We can tell users to disable the DPM_WATCHDOG, maybe some android people would like to use this feature to track/confirm hardware/firmware issues.
I'm ok with only adjusting the timeout from 60 to 120.

Thanks,
Yu

Pavel Machek

2016-08-19 01:49 UTC #13 lore raw

Hi!

Quoted 34 lines

Quoted 34 lines
Quoted 2 lines
OK, I've modified the patch to the following version, Rafael, Pavel could you take a glance at it, thanks:
Certainly looks better.
Quoted 2 lines
Index: linux/kernel/power/Kconfig
================================================================
===
kernel/power/Kconfig config DPM_WATCHDOG config
Quoted 3 lines
kernel/power/Kconfig +0 -0
@@ -197,7 +197,7 @@ config DPM_WATCHDOG  config
DPM_WATCHDOG_TIMEOUT
kernel/power/Kconfig config DPM_WATCHDOG · line 202
Quoted 6 lines
kernel/power/Kconfig +1 -1
 	int "Watchdog timeout in seconds"
 	range 1 120
-	default 60
+	default 120
 	depends on DPM_WATCHDOG
 
I like this part.
kernel/power/Kconfig bytes respectively. Such letter suffixes · line 2752
Quoted 6 lines
kernel/power/Kconfig +2 -0
+++ linux/Documentation/kernel-parameters.txt
@@ -2749,6 +2749,8 @@ bytes respectively. Such letter suffixes
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
                         soft-lockup and NMI watchdog (hard-lockup).
 
+	no_dpm_watchdog	[KNL] Disable the device suspend/resume
watchdog.
Quoted 4 lines
kernel/power/Kconfig +1 -0
+
 	nowb		[ARM]
 
 	nox2apic	[X86-64,APIC] Do not enable x2APIC mode.

Quoted 8 lines

Quoted 5 lines
Do we really need the new cmdline option? Can we just tell people to disable DPM_WATCHDOG completely?
Is there reason to keep the DPM_WATCHDOG? Did it find some real bugs lately?
We can tell users to disable the DPM_WATCHDOG, maybe some android people would like to use this feature to track/confirm hardware/firmware issues.
I'm ok with only adjusting the timeout from 60 to 120.

I believe that's the best solution for now. (Android people often claim commandline is hard for them to use...)

Thanks,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Rafael J. Wysocki

maintainer

2016-08-19 01:22 UTC #14 lore raw

Quoted 39 lines · Pavel Machek

Hi!
Quoted 37 lines
Quoted 37 lines
Quoted 2 lines
OK, I've modified the patch to the following version, Rafael, Pavel could you take a glance at it, thanks:
Certainly looks better.
Quoted 2 lines
Index: linux/kernel/power/Kconfig
================================================================
===
kernel/power/Kconfig config DPM_WATCHDOG config
Quoted 3 lines
kernel/power/Kconfig +0 -0
@@ -197,7 +197,7 @@ config DPM_WATCHDOG  config
DPM_WATCHDOG_TIMEOUT
kernel/power/Kconfig config DPM_WATCHDOG · line 202
Quoted 6 lines
kernel/power/Kconfig +1 -1
  int "Watchdog timeout in seconds"
  range 1 120
- default 60
+ default 120
  depends on DPM_WATCHDOG
 
I like this part.
kernel/power/Kconfig bytes respectively. Such letter suffixes · line 2752
Quoted 6 lines
kernel/power/Kconfig +2 -0
+++ linux/Documentation/kernel-parameters.txt
@@ -2749,6 +2749,8 @@ bytes respectively. Such letter suffixes
  nowatchdog      [KNL] Disable both lockup detectors, i.e.
                         soft-lockup and NMI watchdog (hard-lockup).
 
+ no_dpm_watchdog [KNL] Disable the device suspend/resume
watchdog.
kernel/power/Kconfig bytes respectively. Such letter suffixes · line 2756
Quoted 4 lines
kernel/power/Kconfig +1 -0
+
  nowb            [ARM]
 
  nox2apic        [X86-64,APIC] Do not enable x2APIC mode.
Do we really need the new cmdline option? Can we just tell people to disable DPM_WATCHDOG completely?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Is there reason to keep the DPM_WATCHDOG? Did it find some real bugs lately?
We can tell users to disable the DPM_WATCHDOG, maybe some android people would like to use this feature to track/confirm hardware/firmware issues.
I'm ok with only adjusting the timeout from 60 to 120.
I believe that's the best solution for now. (Android people often claim commandline is hard for them to use...)

So maybe DPM_WATCHDOG should depend on EXPERT?

I really don't want the general-purpose distros to enable it and leave users without a way to turn it off.

Thanks,
Rafael

Pavel Machek

2016-08-19 00:59 UTC #15 lore raw

Hi!

Quoted 17 lines

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes · line 2752
Quoted 6 lines
+++ linux/Documentation/kernel-parameters.txt
Documentation/kernel-parameters.txt +1 -0
@@ -2749,6 +2749,8 @@ bytes respectively. Such letter suffixes
  nowatchdog      [KNL] Disable both lockup detectors, i.e.
                         soft-lockup and NMI watchdog (hard-lockup).
 
+ no_dpm_watchdog [KNL] Disable the device suspend/resume
watchdog.
Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes · line 2756
Quoted 4 lines
Documentation/kernel-parameters.txt +1 -0
+
  nowb            [ARM]
 
  nox2apic        [X86-64,APIC] Do not enable x2APIC mode.
Do we really need the new cmdline option? Can we just tell people to disable DPM_WATCHDOG completely?
Is there reason to keep the DPM_WATCHDOG? Did it find some real bugs lately?

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
We can tell users to disable the DPM_WATCHDOG, maybe some android people would like to use this feature to track/confirm hardware/firmware issues.
I'm ok with only adjusting the timeout from 60 to 120.
I believe that's the best solution for now. (Android people often claim commandline is hard for them to use...)
So maybe DPM_WATCHDOG should depend on EXPERT?

I'd say so, yes.

Best regards,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html