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

[Fabrizio Castro <fabrizio.castro@bp.renesas.com>]

Hello Guenter,

> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: 10 December 2018 14:24
> Subject: Re: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume
>
> 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.

Exactly

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

Alright then, in which case I agree with the original purpose of this patch.

Thanks,
Fab

>
> Thanks,
> Guenter


[https://www2.renesas.eu/media/email/unicef.jpg]

This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.