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

[Guenter Roeck <linux@roeck-us.net>]

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 ?

> 
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>   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.

Thanks,
Guenter

> +}
> +
>   static const struct watchdog_ops vt8500_watchdog_ops = {
>   	.start			= vt8500_watchdog_start,
>   	.stop			= vt8500_watchdog_stop,
> +	.restart		= vt8500_watchdog_restart,
>   };
>   
>   static const struct watchdog_info vt8500_watchdog_info = {
> @@ -107,6 +119,7 @@ static int vt8500_wdt_probe(struct auxiliary_device *auxdev,
>   	info = container_of(auxdev, struct vt8500_wdt_info, auxdev);
>   	watchdog_set_drvdata(wdd, info);
>   	watchdog_set_nowayout(wdd, nowayout);
> +	watchdog_set_restart_priority(wdd, 128);
>   
>   	/* Let the core feed a watchdog left running, e.g. across kexec */
>   	if (readl(info->regbase + VT8500_WDT_EN_REG) & VT8500_WDT_EN)
>