On Fri, Aug 26, 2022 at 07:56:35PM +0300, Sergei Antonov wrote:
> 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.
>
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
