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

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

[Chen Yu]

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

-- 
2.7.4

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

[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 <yu.c.chen@intel.com>
---
 kernel/power/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 68d3ebc..a359bc5 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -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

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

[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 <higuita@gmx.net>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/base/power/main.c    |  4 +++-
 include/linux/sched/sysctl.h |  4 ++++
 kernel/sysctl.c              | 16 ++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index e44944f..7f990c2 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -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);
diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
index 22db1e6..1b7f536 100644
--- a/include/linux/sched/sysctl.h
+++ b/include/linux/sched/sysctl.h
@@ -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;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b43d0b2..bc8f01d 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -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",
-- 
2.7.4

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Pavel Machek]

Hi!

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

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

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

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

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Rafael J. Wysocki]

On Thu, Aug 11, 2016 at 8:54 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> 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.
>
> 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

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

[Rafael J. Wysocki]

On Thu, Aug 11, 2016 at 2:29 PM, Chen Yu <yu.c.chen@intel.com> wrote:
> The DPM watchdog is implemented only when PM_SLEEP is
> set, so make it depending on the latter.
>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>

Looks good to me.

> ---
>  kernel/power/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 68d3ebc..a359bc5 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Chen Yu]

Hi,
On Thu, Aug 11, 2016 at 08:54:42PM +0200, Pavel Machek wrote:
> Hi!
> 
> > 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.
> 
> 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?
> 
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

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Pavel Machek]

Hi!

> > > 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.
> > 
> > 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?
> > 
> 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Chen, Yu C]

Hi,


> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> 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!
> 
> > > > 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.
> > >
> > > 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?
> > >
> > 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.
OK. I'll do that, thanks.

Yu

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Rafael J. Wysocki]

On Fri, Aug 12, 2016 at 8:33 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > > 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.
>> >
>> > 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?
>> >
>> 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.

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

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Chen Yu]

On Fri, Aug 12, 2016 at 01:25:04PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 12, 2016 at 8:33 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> >> > > 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.
> >> >
> >> > 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?
> >> >
> >> 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.
> 
> 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:
 
Index: linux/kernel/power/Kconfig
===================================================================
--- linux.orig/kernel/power/Kconfig
+++ 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
Index: linux/drivers/base/power/main.c
===================================================================
--- linux.orig/drivers/base/power/main.c
+++ 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)
Index: linux/Documentation/kernel-parameters.txt
===================================================================
--- linux.orig/Documentation/kernel-parameters.txt
+++ 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.

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Pavel Machek]

On Wed 2016-08-17 11:43:48, Chen Yu wrote:
> On Fri, Aug 12, 2016 at 01:25:04PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Aug 12, 2016 at 8:33 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > > Hi!
> > >
> > >> > > 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.
> > >> >
> > >> > 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?
> > >> >
> > >> 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.
> > 
> > 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.

> Index: linux/kernel/power/Kconfig
> ===================================================================
> --- linux.orig/kernel/power/Kconfig
> +++ 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
>

I like this part.

> +++ 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Chen, Yu C]

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> 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
> 
> On Wed 2016-08-17 11:43:48, Chen Yu wrote:
> > On Fri, Aug 12, 2016 at 01:25:04PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, Aug 12, 2016 at 8:33 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > > > Hi!
> > > >
> > > >> > > 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.
> > > >> >
> > > >> > 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?
> > > >> >
> > > >> 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.
> > >
> > > 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.
> 
> > Index: linux/kernel/power/Kconfig
> >
> ================================================================
> ===
> > --- linux.orig/kernel/power/Kconfig
> > +++ 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
> >
> 
> I like this part.
> 
> > +++ 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?
> 
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

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Pavel Machek]

Hi!

> > > OK, I've modified the patch to the following version, Rafael, Pavel
> > > could you take a glance at it, thanks:
> > 
> > Certainly looks better.
> > 
> > > Index: linux/kernel/power/Kconfig
> > >
> > ================================================================
> > ===
> > > --- linux.orig/kernel/power/Kconfig
> > > +++ 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
> > >
> > 
> > I like this part.
> > 
> > > +++ 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?
> > 
> 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Rafael J. Wysocki]

On Thu, Aug 18, 2016 at 8:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > > OK, I've modified the patch to the following version, Rafael, Pavel
>> > > could you take a glance at it, thanks:
>> >
>> > Certainly looks better.
>> >
>> > > Index: linux/kernel/power/Kconfig
>> > >
>> > ================================================================
>> > ===
>> > > --- linux.orig/kernel/power/Kconfig
>> > > +++ 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
>> > >
>> >
>> > I like this part.
>> >
>> > > +++ 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?
>> >
>> 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

Re: [PATCH 0/2][RFC] PM / sleep: Expose DPM watchdog timeout to sysfs

[Pavel Machek]

Hi!

> >> > > +++ 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?
> >> >
> >> 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html