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

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

[Wolfram Sang]

After discussing this mail thread [1] again, we concluded that giving
userspace enough time to prepare is our favourite option. So, do not
keep the time value when suspended but reset it when resuming.

[1] https://patchwork.kernel.org/patch/10252209/

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Fabrizio: can you agree to that? The R-Car BSP team and we (the R-Car upstream
team) would prefer it this way (knowing it is also not perfect).

 drivers/watchdog/renesas_wdt.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index b570962e84f3..9f2307bf727b 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -48,7 +48,6 @@ struct rwdt_priv {
 	void __iomem *base;
 	struct watchdog_device wdev;
 	unsigned long clk_rate;
-	u16 time_left;
 	u8 cks;
 };
 
@@ -263,10 +262,9 @@ static int __maybe_unused rwdt_suspend(struct device *dev)
 {
 	struct rwdt_priv *priv = dev_get_drvdata(dev);
 
-	if (watchdog_active(&priv->wdev)) {
-		priv->time_left = readw(priv->base + RWTCNT);
+	if (watchdog_active(&priv->wdev))
 		rwdt_stop(&priv->wdev);
-	}
+
 	return 0;
 }
 
@@ -274,10 +272,9 @@ static int __maybe_unused rwdt_resume(struct device *dev)
 {
 	struct rwdt_priv *priv = dev_get_drvdata(dev);
 
-	if (watchdog_active(&priv->wdev)) {
+	if (watchdog_active(&priv->wdev))
 		rwdt_start(&priv->wdev);
-		rwdt_write(priv, priv->time_left, RWTCNT);
-	}
+
 	return 0;
 }
 
-- 
2.11.0

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

[Geert Uytterhoeven]

On Tue, Dec 4, 2018 at 1:02 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> After discussing this mail thread [1] again, we concluded that giving
> userspace enough time to prepare is our favourite option. So, do not
> keep the time value when suspended but reset it when resuming.
>
> [1] https://patchwork.kernel.org/patch/10252209/
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Fabrizio Castro]

Hello Wolfram,

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: 04 December 2018 12:02
> Subject: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume
>
> After discussing this mail thread [1] again, we concluded that giving
> userspace enough time to prepare is our favourite option. So, do not
> keep the time value when suspended but reset it when resuming.
>
> [1] https://patchwork.kernel.org/patch/10252209/
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Fabrizio: can you agree to that? The R-Car BSP team and we (the R-Car upstream
> team) would prefer it this way (knowing it is also not perfect).

I am not too sure, as the system relies on the watchdog to fix problems that won't resolve
on their own, therefore if you have an application made of two threads, one very critical
for the health of the system (but unfortunately buggy, which means it is destined to not
ping watchdog on time), and the other thread takes care of putting the system to sleep,
you may find yourself in a place where the system doesn't work properly (as the critical
thread won't work as expected) and never restarts (because the other thread works properly
and putting the system to sleep resets the watchdog).
Sometimes the line between policies and mechanisms is not a clear cut, I think this patch
looks more like a policy decision, but I may be wrong.

Cheers,
Fab

>
>  drivers/watchdog/renesas_wdt.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index b570962e84f3..9f2307bf727b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -48,7 +48,6 @@ struct rwdt_priv {
>  void __iomem *base;
>  struct watchdog_device wdev;
>  unsigned long clk_rate;
> -u16 time_left;
>  u8 cks;
>  };
>
> @@ -263,10 +262,9 @@ static int __maybe_unused rwdt_suspend(struct device *dev)
>  {
>  struct rwdt_priv *priv = dev_get_drvdata(dev);
>
> -if (watchdog_active(&priv->wdev)) {
> -priv->time_left = readw(priv->base + RWTCNT);
> +if (watchdog_active(&priv->wdev))
>  rwdt_stop(&priv->wdev);
> -}
> +
>  return 0;
>  }
>
> @@ -274,10 +272,9 @@ static int __maybe_unused rwdt_resume(struct device *dev)
>  {
>  struct rwdt_priv *priv = dev_get_drvdata(dev);
>
> -if (watchdog_active(&priv->wdev)) {
> +if (watchdog_active(&priv->wdev))
>  rwdt_start(&priv->wdev);
> -rwdt_write(priv, priv->time_left, RWTCNT);
> -}
> +
>  return 0;
>  }
>
> --
> 2.11.0



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

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

[Guenter Roeck]

On 12/4/18 4:01 AM, Wolfram Sang wrote:
> After discussing this mail thread [1] again, we concluded that giving
> userspace enough time to prepare is our favourite option. So, do not
> keep the time value when suspended but reset it when resuming.
> 
> [1] https://patchwork.kernel.org/patch/10252209/
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Above exchange says it all, no need to repeat.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Fabrizio: can you agree to that? The R-Car BSP team and we (the R-Car upstream
> team) would prefer it this way (knowing it is also not perfect).
> 
>   drivers/watchdog/renesas_wdt.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index b570962e84f3..9f2307bf727b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -48,7 +48,6 @@ struct rwdt_priv {
>   	void __iomem *base;
>   	struct watchdog_device wdev;
>   	unsigned long clk_rate;
> -	u16 time_left;
>   	u8 cks;
>   };
>   
> @@ -263,10 +262,9 @@ static int __maybe_unused rwdt_suspend(struct device *dev)
>   {
>   	struct rwdt_priv *priv = dev_get_drvdata(dev);
>   
> -	if (watchdog_active(&priv->wdev)) {
> -		priv->time_left = readw(priv->base + RWTCNT);
> +	if (watchdog_active(&priv->wdev))
>   		rwdt_stop(&priv->wdev);
> -	}
> +
>   	return 0;
>   }
>   
> @@ -274,10 +272,9 @@ static int __maybe_unused rwdt_resume(struct device *dev)
>   {
>   	struct rwdt_priv *priv = dev_get_drvdata(dev);
>   
> -	if (watchdog_active(&priv->wdev)) {
> +	if (watchdog_active(&priv->wdev))
>   		rwdt_start(&priv->wdev);
> -		rwdt_write(priv, priv->time_left, RWTCNT);
> -	}
> +
>   	return 0;
>   }
>   
> 

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

[Wolfram Sang]

Hi Guenter, all,

> > After discussing this mail thread [1] again, we concluded that giving
> > userspace enough time to prepare is our favourite option. So, do not
> > keep the time value when suspended but reset it when resuming.
> > 
> > [1] https://patchwork.kernel.org/patch/10252209/
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Above exchange says it all, no need to repeat.
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks.

I can relate to the policy argument, though. Regardless of this patch, I
wonder if we can make it configurable from userspace. A draft:

#define	WDIOF_RESUME_OPTS	0x0800

#define	WDIOS_RESUME_KEEP	0x0008
#define	WDIOS_RESUME_RESET	0x0010

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?

Thanks,

   Wolfram

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

[Guenter Roeck]

On 12/7/18 1:45 PM, Wolfram Sang wrote:
> Hi Guenter, all,
> 
>>> After discussing this mail thread [1] again, we concluded that giving
>>> userspace enough time to prepare is our favourite option. So, do not
>>> keep the time value when suspended but reset it when resuming.
>>>
>>> [1] https://patchwork.kernel.org/patch/10252209/
>>>
>>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>
>> Above exchange says it all, no need to repeat.
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks.
> 
> I can relate to the policy argument, though. Regardless of this patch, I
> wonder if we can make it configurable from userspace. A draft:
> 
> #define	WDIOF_RESUME_OPTS	0x0800
> 
> #define	WDIOS_RESUME_KEEP	0x0008
> #define	WDIOS_RESUME_RESET	0x0010
> 
> 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.

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.

Guenter

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

[Wolfram Sang]

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:
> > 
> > #define	WDIOF_RESUME_OPTS	0x0800
> > 
> > #define	WDIOS_RESUME_KEEP	0x0008
> > #define	WDIOS_RESUME_RESET	0x0010
> > 
> > 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.

> 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
IMHO, and then we can build the WDIOF_RESUME_KEEP option on top of it,
if needed. But for clarity, the first step seems to be a good idea in
any case, I'd say.

Thanks,

   Wolfram

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

[Guenter Roeck]

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:
>>>
>>> #define	WDIOF_RESUME_OPTS	0x0800
>>>
>>> #define	WDIOS_RESUME_KEEP	0x0008
>>> #define	WDIOS_RESUME_RESET	0x0010
>>>
>>> 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
was close to expire on suspend, we want it to expire for good on resume ?
Make the watchdog during a suspend/resume cycle more stringent that during
normal operation, effectively letting it expire early (or earlier) ?

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.

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

How would adding two flag bits (or one, for that matter) change the
current (undefined) behavior ?
Also, what exactly would be the regression ? Existing drivers would
not change their behavior, neither with one or two or three flag bits.

Thanks,
Guenter

> IMHO, and then we can build the WDIOF_RESUME_KEEP option on top of it,
> if needed. But for clarity, the first step seems to be a good idea in
> any case, I'd say.
>  > Thanks,
> 
>     Wolfram
> 
> 

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]


> I understand, but what is the use case behind it ? If the watchdog

I'll leave this discussion to Fabrizio...

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

... because this (a documented default) is exactly what *I* am
interested in :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Fabrizio Castro]

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.

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

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

I hope this helps.

Fab

>
> How would adding two flag bits (or one, for that matter) change the
> current (undefined) behavior ?
> Also, what exactly would be the regression ? Existing drivers would
> not change their behavior, neither with one or two or three flag bits.
>
> Thanks,
> Guenter
>
> > IMHO, and then we can build the WDIOF_RESUME_KEEP option on top of it,
> > if needed. But for clarity, the first step seems to be a good idea in
> > any case, I'd say.
> >  > Thanks,
> >
> >     Wolfram
> >
> >



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

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

[Guenter Roeck]

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

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

[Fabrizio Castro]

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.

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

[Fabrizio Castro]

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.

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

[Fabrizio Castro]

Hello Wolfram,

> From: Wolfram Sang <wsa@the-dreams.de>
> Sent: 07 December 2018 21:45
> Subject: Re: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume
>
> Hi Guenter, all,
>
> > > After discussing this mail thread [1] again, we concluded that giving
> > > userspace enough time to prepare is our favourite option. So, do not
> > > keep the time value when suspended but reset it when resuming.
> > >
> > > [1] https://patchwork.kernel.org/patch/10252209/
> > >
> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> > Above exchange says it all, no need to repeat.
> >
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> Thanks.
>
> 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?

This probably allows for user customization and yet could keep the current
behaviour in place, therefore I think it's a good start. Maybe once every single
driver makes an informed decision during suspend-resume we could drop one
of the flags?

Thanks,
Fab

>
> Thanks,
>
>    Wolfram



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

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

[Fabrizio Castro]

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: 04 December 2018 12:02
> Subject: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume
>
> After discussing this mail thread [1] again, we concluded that giving
> userspace enough time to prepare is our favourite option. So, do not
> keep the time value when suspended but reset it when resuming.
>
> [1] https://patchwork.kernel.org/patch/10252209/
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> ---
>
> Fabrizio: can you agree to that? The R-Car BSP team and we (the R-Car upstream
> team) would prefer it this way (knowing it is also not perfect).
>
>  drivers/watchdog/renesas_wdt.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index b570962e84f3..9f2307bf727b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -48,7 +48,6 @@ struct rwdt_priv {
>  void __iomem *base;
>  struct watchdog_device wdev;
>  unsigned long clk_rate;
> -u16 time_left;
>  u8 cks;
>  };
>
> @@ -263,10 +262,9 @@ static int __maybe_unused rwdt_suspend(struct device *dev)
>  {
>  struct rwdt_priv *priv = dev_get_drvdata(dev);
>
> -if (watchdog_active(&priv->wdev)) {
> -priv->time_left = readw(priv->base + RWTCNT);
> +if (watchdog_active(&priv->wdev))
>  rwdt_stop(&priv->wdev);
> -}
> +
>  return 0;
>  }
>
> @@ -274,10 +272,9 @@ static int __maybe_unused rwdt_resume(struct device *dev)
>  {
>  struct rwdt_priv *priv = dev_get_drvdata(dev);
>
> -if (watchdog_active(&priv->wdev)) {
> +if (watchdog_active(&priv->wdev))
>  rwdt_start(&priv->wdev);
> -rwdt_write(priv, priv->time_left, RWTCNT);
> -}
> +
>  return 0;
>  }
>
> --
> 2.11.0



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