Re: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume
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 Above exchange says it all, no need to repeat. Reviewed-by: Guenter Roeck 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
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 > > Above exchange says it all, no need to repeat. > > Reviewed-by: Guenter Roeck 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
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
Above exchange says it all, no need to repeat.
Reviewed-by: Guenter Roeck
---
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(>wdev)) {
- priv->time_left = readw(priv->base + RWTCNT);
+ if (watchdog_active(>wdev))
rwdt_stop(>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(>wdev)) {
+ if (watchdog_active(>wdev))
rwdt_start(>wdev);
- rwdt_write(priv, priv->time_left, RWTCNT);
- }
+
return 0;
}
RE: [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume
Hello Wolfram,
> From: Wolfram Sang
> 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
> ---
>
> 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(>wdev)) {
> -priv->time_left = readw(priv->base + RWTCNT);
> +if (watchdog_active(>wdev))
> rwdt_stop(>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(>wdev)) {
> +if (watchdog_active(>wdev))
> rwdt_start(>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
On Tue, Dec 4, 2018 at 1:02 PM 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
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
[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
---
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(>wdev)) {
- priv->time_left = readw(priv->base + RWTCNT);
+ if (watchdog_active(>wdev))
rwdt_stop(>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(>wdev)) {
+ if (watchdog_active(>wdev))
rwdt_start(>wdev);
- rwdt_write(priv, priv->time_left, RWTCNT);
- }
+
return 0;
}
--
2.11.0
