On 12/10/18 1:37 AM, Fabrizio Castro wrote:
Hello Guenter,
From: Guenter Roeck <groeck7@xxxxxxxxx> 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
