Re: [PATCH v6 4/4] watchdog: vt8500: Add a restart handler

[Alexey Charkov]

On Fri, Nov 13, 2026 at 4:44 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 11/12/26 10:55, Alexey Charkov wrote:
> > Reset the SoC through the watchdog when the machine's own restart hook
> > leaves it running: the PMSR write in vt8500_restart() has no effect on
> > WM8950, so a reboot there ends in "Reboot failed -- System halted"
> > unless a watchdog armed from userspace happens to fire.
> >
> > Arm the match a millisecond ahead of the current count, which leaves
> > ample margin for the register writes to land.
> >
> > Register at priority 128, the default for restart handlers and also the
> > priority of the ARM machine restart hook. That hook is registered first,
> > from setup_arch(), and handlers of equal priority run in registration
> > order, so this one runs right after it and only matters where the hook
> > fails to reset the SoC.
>
> This isn't really the idea of restart handlers. The sequence should be well
> defined. That is what the priorities are for. Why not give it a slightly
> lower priority ?

My thinking was along the lines of "I don't care who of you guys does
the job, as long as it's done". But now that you've pointed it out, a
watchdog reset is probably more rude than some other method, and also
requires waiting for the timer to expire, so I'll probably switch it
to 0 to make the "safety net" connotation explicit.

> > Signed-off-by: Alexey Charkov <alchark@xxxxxxxxx>
> > ---
> >   drivers/watchdog/vt8500-wdt.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/watchdog/vt8500-wdt.c b/drivers/watchdog/vt8500-wdt.c
> > index e0f838fd1d50..0ce1e2b06410 100644
> > --- a/drivers/watchdog/vt8500-wdt.c
> > +++ b/drivers/watchdog/vt8500-wdt.c
> > @@ -25,6 +25,9 @@
> >
> >   #define VT8500_WDT_DEFAULT_TIMEOUT  30      /* seconds */
> >
> > +/* How far ahead of the current count to arm the match for a restart */
> > +#define VT8500_WDT_RESTART_TICKS     (VT8500_TIMER_HZ / 1000)        /* 1 ms */
> > +
> >   static bool nowayout = WATCHDOG_NOWAYOUT;
> >   module_param(nowayout, bool, 0);
> >   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> > @@ -75,9 +78,18 @@ static int vt8500_watchdog_stop(struct watchdog_device *wdd)
> >       return 0;
> >   }
> >
> > +static int vt8500_watchdog_restart(struct watchdog_device *wdd,
> > +                                unsigned long action, void *data)
> > +{
> > +     struct vt8500_wdt_info *info = watchdog_get_drvdata(wdd);
> > +
> > +     return vt8500_watchdog_arm(info, VT8500_WDT_RESTART_TICKS);
>
> This sets a 1-ms timer and doesn't wait for it to fire, so the next
> restart handler (if it exists) will be called. That seems odd,
> and may introduce unexpected behavior. Is this intentional ? If so,
> it should be explained.

Not intentional, thanks for pointing it out. I'll add a wait in the
next version.

Thank you for the review!

Best regards,
Alexey