| Message ID | 20181204120146.1923-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] watchdog: renesas_wdt: don't keep timer value during suspend/resume | expand |
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
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.
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; > } > >
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
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
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
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 > >
> 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 :)
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.
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.
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
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.
> 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.
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; }
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(-)