Re: [PATCH] watchdog: bcm7038: Add a restart callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] watchdog: bcm7038_wdt: missing spinlock causes restart failure on panic
--

commit 6002003a9fe5a5243c56c551564e6ccb85d21046
Author: Linus Walleij <linusw@kernel.org>

watchdog: bcm7038: Add a restart callback
    
The watchdog in BCM6846 can reset the system, but bcm7038_wdt
does not provide a restart callback. Add this. The vendor kernel
also uses a short watchdog timeout to reset this SoC.

> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
> index e038dd66b8198..1637e32ac02e8 100644
> --- a/drivers/watchdog/bcm7038_wdt.c
> +++ b/drivers/watchdog/bcm7038_wdt.c
[ ... ]
> @@ -113,6 +115,32 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
>  	return time_left / wdt->rate;
>  }
>  
> +static int bcm7038_wdt_restart(struct watchdog_device *wdog,
> +			       unsigned long action, void *data)
> +{
> +	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> +	unsigned long flags;
> +
> +	/*
> +	 * Prevent runtime watchdog operations from interfering on
> +	 * uniprocessor systems where restart handlers are preemptible.
> +	 */
> +	local_irq_save(flags);
> +
> +	/* Stop the timer before changing its reload value. */
> +	bcm7038_wdt_stop(wdog);

[Severity: High]
Can this restart handler execute out of sequence with other watchdog
operations?

If a panic or SysRq interrupt preempts a ping or stop operation exactly
between its two writes to WDT_CMD_REG, could the hardware state machine
become corrupted?

For example, if bcm7038_wdt_ping() is interrupted:

drivers/watchdog/bcm7038_wdt.c:bcm7038_wdt_ping() {
	...
	bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
	<--- interrupted by panic/SysRq

And then bcm7038_wdt_restart() executes in the interrupt context, it will
call bcm7038_wdt_stop() which writes WDT_STOP_1 to the hardware that was
expecting WDT_START_2.

Would this cause the watchdog to ignore the reset command and hang the
system during reboot or panic? Additionally, on SMP, could an
smp_send_stop() IPI from a panicking CPU halt another CPU mid-sequence in
the same way?

> +	/* The watchdog does not fire with a count below two. */
> +	bcm7038_wdt_write(2, wdt->base + WDT_TIMEOUT_REG);
> +	bcm7038_wdt_ping(wdog);
> +
> +	/* Flush the writes and wait for the reset to take effect. */
> +	bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
> +	mdelay(500);
> +	local_irq_restore(flags);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-genexis-xg6846-watchdog-reset-v1-1-9577d65223d5@kernel.org?part=1