Re: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume

[Guenter Roeck <linux@roeck-us.net>]

On 12/10/18 1:37 AM, Fabrizio Castro wrote:
> Hello Guenter,
> 
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: 09 December 2018 18:13
>> Subject: Re: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume
>>
>> On 12/9/18 8:36 AM, Wolfram Sang wrote:
>>> Hi Guenter,
>>>
>>>>> I can relate to the policy argument, though. Regardless of this patch, I
>>>>> wonder if we can make it configurable from userspace. A draft:
>>>>>
>>>>> #defineWDIOF_RESUME_OPTS0x0800
>>>>>
>>>>> #defineWDIOS_RESUME_KEEP0x0008
>>>>> #defineWDIOS_RESUME_RESET0x0010
>>>>>
>>>>> and then in watchdog_ioctl() under WDIOC_SETOPTIONS:
>>>>>
>>>>> if (!(wdd->info->options & WDIOF_RESUME_OPTS))
>>>>> err = -EOPNOTSUPP;
>>>>> goto break;
>>>>>
>>>>> if (val & WDIOS_RESUME_KEEP)
>>>>> wdd->status |= WDOG_KEEP_TIMER_WHEN_RESUME;
>>>>>
>>>>> if (val & WDIOS_RESUME_RESET)
>>>>> wdd->status ~= ~WDOG_KEEP_TIMER_WHEN_RESUME;
>>>>>
>>>>> So, drivers with WDIOF_RESUME_OPTS could act on the
>>>>> WDOG_KEEP_TIMER_WHEN_RESUME flag.
>>>>>
>>>>> Opinions?
>>>>>
>>>>
>>>> Not entirely sure I understand the use case.
>>>
>>> Well, as I mentioned before, I can understand the "isn't this policy?"
>>> question from Fabrizio. Would be good to hear his opinion on this.
>>>
>> I understand, but what is the use case behind it ? If the watchdog
> 
> Is there a documented use case for resetting the counter at resume? I don't
> think the documentation is clear about this, therefore we need to think ahead
> 
>> was close to expire on suspend, we want it to expire for good on resume ?
> 
> That decision is up to user space, isn't it? The decision to go to sleep comes from
> user space, therefore user space should consider the time left on the counter before
> going to sleep (or ask the system to ping the watchdog at resume), and do what's
> best for the health of the system, from kernel space we don't know if the user application
> is behaving as expected or not, therefore we don't know what's best for the system.
> Why don't we let user space decide?
> I guess Wolfram proposal goes in the right direction?
> 
>> Make the watchdog during a suspend/resume cycle more stringent that during
>> normal operation, effectively letting it expire early (or earlier) ?
> 
> As the decision to go to sleep comes from user space, I don't think we can say that
> letting the watchdog expire on resume is more stringent (or unfair) than during
> normal operation, if the system is healthy user space should consider the delay
> introduced by going to sleep and waking up and it should make sure that there is
> enough time left on the watchdog timer before asking the system to go to sleep.
> 

Should it ? Does it ? Is there any watchdog daemon out there which sends a final
ping to the watchdog just before suspend or immediately after resume ?

>>
>> I'd rather clarify in the documentation that watchdog drivers are expected
>> to ping the watchdog after resume, ie that restarting the watchdog after
>> resume should be handled like starting the watchdog.
> 
> Let me understand this a little bit better, if you have a use case where you don't
> want to automatically ping the watchdog at resume you can't go to sleep?
> 

Yes, the normal use case. The point of a watchdog is to recover from a fatal
system failure. For a normal use of a watchdog, especially one that involves
suspend/resume and is thus not time critical, that behavior should be relaxed,
not stringent, and under no circumstances should result in an unnecessary /
unexpected system reboot.

>>
>>>> Having said that, if we were to add this option, I think only a single
>>>> flag would be needed - WDIOF_RESUME_KEEP. All we need to do is declare
>>>> that a ping on resume shall be the default. Anything else would result
>>>> in undefined per-driver default behavior.
>>>
>>> I would very much love that. To be honest, I thought we already are in
>>> the undefined per-driver behaviour; this is why I added two flags, to
>>> not cause regressions. Declaring a default would be a great first step
> 
> I agree with Wolfram, to me it looks like this is undefined per-driver
> behaviour already
> 

Point well made, but that is primarily a documentation deficiency: If the
expected behavior was well documented, we would not have this argument.

At this point I would be happy to accept a patch clarifying the documentation.
Unless I get guidance from Wim suggesting otherwise, going forward I won't
accept any watchdog drivers which do not implement resetting the timer on
resume.

Thanks,
Guenter