| Message ID | 20220826135638.3751835-1-saproj@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: ftwdt010_wdt: add _restart() function | expand |
On Fri, Aug 26, 2022 at 04:56:38PM +0300, Sergei Antonov wrote: > Add a missing _restart function to watchdog_ops. The restart function isn't "missing" because it is optional. > FTWDT010 resets system when it is started with timeout = 0. > > Signed-off-by: Sergei Antonov <saproj@gmail.com> > --- > drivers/watchdog/ftwdt010_wdt.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c > index 21dcc7765688..9eaaaca1094d 100644 > --- a/drivers/watchdog/ftwdt010_wdt.c > +++ b/drivers/watchdog/ftwdt010_wdt.c > @@ -93,6 +93,13 @@ static int ftwdt010_wdt_set_timeout(struct watchdog_device *wdd, > return 0; > } > > +static int ftwdt010_wdt_restart(struct watchdog_device *wdd, > + unsigned long action, void *data) > +{ > + wdd->timeout = 0; > + return ftwdt010_wdt_start(wdd); While technically correct, this crates the impression that ftwdt010_wdt_start() might return an error - which it never does. Given that, I would prefer ftwdt010_wdt_start(wdd); return 0; Also, did you make sure that interrupt support does not interfer with restart ? We don't want to get an interrupt when the watchdog fires due to a call to the restart handler. Thanks, Guenter > +} > + > static irqreturn_t ftwdt010_wdt_interrupt(int irq, void *data) > { > struct ftwdt010_wdt *gwdt = data; > @@ -107,6 +114,7 @@ static const struct watchdog_ops ftwdt010_wdt_ops = { > .stop = ftwdt010_wdt_stop, > .ping = ftwdt010_wdt_ping, > .set_timeout = ftwdt010_wdt_set_timeout, > + .restart = ftwdt010_wdt_restart, > .owner = THIS_MODULE, > }; > > -- > 2.34.1 >
On Fri, 26 Aug 2022 at 19:12, Guenter Roeck <linux@roeck-us.net> wrote: > > On Fri, Aug 26, 2022 at 04:56:38PM +0300, Sergei Antonov wrote: > > Add a missing _restart function to watchdog_ops. > > The restart function isn't "missing" because it is optional. > > > FTWDT010 resets system when it is started with timeout = 0. > > > > Signed-off-by: Sergei Antonov <saproj@gmail.com> > > --- > > drivers/watchdog/ftwdt010_wdt.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c > > index 21dcc7765688..9eaaaca1094d 100644 > > --- a/drivers/watchdog/ftwdt010_wdt.c > > +++ b/drivers/watchdog/ftwdt010_wdt.c > > @@ -93,6 +93,13 @@ static int ftwdt010_wdt_set_timeout(struct watchdog_device *wdd, > > return 0; > > } > > > > +static int ftwdt010_wdt_restart(struct watchdog_device *wdd, > > + unsigned long action, void *data) > > +{ > > + wdd->timeout = 0; > > + return ftwdt010_wdt_start(wdd); > > While technically correct, this crates the impression that > ftwdt010_wdt_start() might return an error - which it never does. > Given that, I would prefer > ftwdt010_wdt_start(wdd); > return 0; Why loose the return value? My code lets it propagate. It is potentially non-zero. > Also, did you make sure that interrupt support does not interfer with > restart ? We don't want to get an interrupt when the watchdog fires due > to a call to the restart handler. No, I did not test watchdog interrupt on my hardware (don't' know yet how to set it up correctly). I only tested that restart restarts the hardware as expected. Now that I think of it, it may be more precise to do gwdt->has_irq = false; before calling ftwdt010_wdt_start() So that WDCR_WDINTR flag will not be set and the chip will not trigger an interrupt.
On Fri, Aug 26, 2022 at 07:56:35PM +0300, Sergei Antonov wrote: > On Fri, 26 Aug 2022 at 19:12, Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Fri, Aug 26, 2022 at 04:56:38PM +0300, Sergei Antonov wrote: > > > Add a missing _restart function to watchdog_ops. > > > > The restart function isn't "missing" because it is optional. > > > > > FTWDT010 resets system when it is started with timeout = 0. > > > > > > Signed-off-by: Sergei Antonov <saproj@gmail.com> > > > --- > > > drivers/watchdog/ftwdt010_wdt.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c > > > index 21dcc7765688..9eaaaca1094d 100644 > > > --- a/drivers/watchdog/ftwdt010_wdt.c > > > +++ b/drivers/watchdog/ftwdt010_wdt.c > > > @@ -93,6 +93,13 @@ static int ftwdt010_wdt_set_timeout(struct watchdog_device *wdd, > > > return 0; > > > } > > > > > > +static int ftwdt010_wdt_restart(struct watchdog_device *wdd, > > > + unsigned long action, void *data) > > > +{ > > > + wdd->timeout = 0; > > > + return ftwdt010_wdt_start(wdd); > > > > While technically correct, this crates the impression that > > ftwdt010_wdt_start() might return an error - which it never does. > > Given that, I would prefer > > ftwdt010_wdt_start(wdd); > > return 0; > > Why loose the return value? My code lets it propagate. It is > potentially non-zero. > No, it never is. Look at the code. That is the whole point. > > Also, did you make sure that interrupt support does not interfer with > > restart ? We don't want to get an interrupt when the watchdog fires due > > to a call to the restart handler. > > No, I did not test watchdog interrupt on my hardware (don't' know yet > how to set it up correctly). I only tested that restart restarts the > hardware as expected. > Now that I think of it, it may be more precise to do > gwdt->has_irq = false; > before calling ftwdt010_wdt_start() > So that WDCR_WDINTR flag will not be set and the chip will not trigger > an interrupt. In that case I'd rather see a separate function which is called from both ftwdt010_wdt_start() and the restart function and has timeout and the interrupt flag as parameters. That would also address the never-happening error return. Thanks, Guenter
On Fri, 26 Aug 2022 at 20:30, Guenter Roeck <linux@roeck-us.net> wrote: > > > > +static int ftwdt010_wdt_restart(struct watchdog_device *wdd, > > > > + unsigned long action, void *data) > > > > +{ > > > > + wdd->timeout = 0; > > > > + return ftwdt010_wdt_start(wdd); > > > > > > While technically correct, this crates the impression that > > > ftwdt010_wdt_start() might return an error - which it never does. > > > Given that, I would prefer > > > ftwdt010_wdt_start(wdd); > > > return 0; > > > > Why loose the return value? My code lets it propagate. It is > > potentially non-zero. > > > No, it never is. Look at the code. That is the whole point. Of course I looked at ftwdt010_wdt_start() and saw it always returning 0. By "potentially" I meant it returning errors too in the future. > > > Also, did you make sure that interrupt support does not interfer with > > > restart ? We don't want to get an interrupt when the watchdog fires due > > > to a call to the restart handler. > > > > No, I did not test watchdog interrupt on my hardware (don't' know yet > > how to set it up correctly). I only tested that restart restarts the > > hardware as expected. > > Now that I think of it, it may be more precise to do > > gwdt->has_irq = false; > > before calling ftwdt010_wdt_start() > > So that WDCR_WDINTR flag will not be set and the chip will not trigger > > an interrupt. > > In that case I'd rather see a separate function which is called from both > ftwdt010_wdt_start() and the restart function and has timeout and the interrupt > flag as parameters. That would also address the never-happening error return. OK. Submitting a new patch.
diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c index 21dcc7765688..9eaaaca1094d 100644 --- a/drivers/watchdog/ftwdt010_wdt.c +++ b/drivers/watchdog/ftwdt010_wdt.c @@ -93,6 +93,13 @@ static int ftwdt010_wdt_set_timeout(struct watchdog_device *wdd, return 0; } +static int ftwdt010_wdt_restart(struct watchdog_device *wdd, + unsigned long action, void *data) +{ + wdd->timeout = 0; + return ftwdt010_wdt_start(wdd); +} + static irqreturn_t ftwdt010_wdt_interrupt(int irq, void *data) { struct ftwdt010_wdt *gwdt = data; @@ -107,6 +114,7 @@ static const struct watchdog_ops ftwdt010_wdt_ops = { .stop = ftwdt010_wdt_stop, .ping = ftwdt010_wdt_ping, .set_timeout = ftwdt010_wdt_set_timeout, + .restart = ftwdt010_wdt_restart, .owner = THIS_MODULE, };
Add a missing _restart function to watchdog_ops. FTWDT010 resets system when it is started with timeout = 0. Signed-off-by: Sergei Antonov <saproj@gmail.com> --- drivers/watchdog/ftwdt010_wdt.c | 8 ++++++++ 1 file changed, 8 insertions(+)