On Fri, 26 Aug 2022 at 19:12, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
