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

[Pavel Machek <pavel@ucw.cz>]

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