Re: [PATCH v2 2/3] watchdog: Add Nuvoton MA35D1 watchdog driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Writes to REG_WDT_CTL are missing required hardware synchronization waits (SYNC bit), risking dropped writes and hardware state corruption.
- [High] The watchdog timer is not paused during system suspend if it was running from the bootloader but not yet opened by userspace, leading to unintended system resets.
- [High] Driver fails to synchronize the hardware timeout with the software configuration when inheriting a running watchdog during probe.
- [High] Read-modify-write operations on REG_WDT_CTL inadvertently clear W1C (Write-1-to-Clear) status flags, leading to lost interrupts and wakeup events.
- [Medium] In ma35d1_wdt_resume(), the driver writes to REG_WDT_RSTCNT before ensuring the control register changes have synchronized.
- [Low] Spelling mistake in Kconfig help text.
--

--- Patch [2]: [PATCH v2 2/3] watchdog: Add Nuvoton MA35D1 watchdog driver support ---
commit 31ed5d4971c74d4f75935b7f98c40e226cdc3521
Author: Zi-Yu Chen <zychennvt@gmail.com>

watchdog: Add Nuvoton MA35D1 watchdog driver support
    
This commit adds a new watchdog driver for the Nuvoton MA35D1 SoC. It provides
support for timeout configuration, ping, and system reset functionality via
the standard watchdog framework.

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 08cb8612d41fe..972740fd36a69 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -720,6 +720,16 @@ config STMP3XXX_RTC_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stmp3xxx_rtc_wdt.
>  
> +config MA35D1_WDT
> +	tristate "Nuvoton MA35D1 Watchdog Timer"
> +	depends on ARCH_MA35 || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here to include support for the watchdog timer
> +	  for the Nuvoton MA35D1 series microprosser.

[Severity: Low]
Is "microprosser" a typo here? Did you mean "microprocessor"?

> diff --git a/drivers/watchdog/ma35d1_wdt.c b/drivers/watchdog/ma35d1_wdt.c
> new file mode 100644
> index 0000000000000..e5247e55dfee4
> --- /dev/null
> +++ b/drivers/watchdog/ma35d1_wdt.c

[ ... ]

> +static int ma35d1_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +	struct ma35d1_wdt_dev *ma35d1_wdt = watchdog_get_drvdata(wdt_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	guard(spinlock_irqsave)(&ma35d1_wdt->lock);
> +	val = readl_relaxed(ma35d1_wdt->wdt_base + REG_WDT_CTL);
> +	val &= ~WDTEN;
> +	writel_relaxed(val, ma35d1_wdt->wdt_base + REG_WDT_CTL);

[Severity: High]
Will this read-modify-write inadvertently clear Write-1-to-Clear (W1C) flags
like IF, WKF, or RSTF? 

Looking at ma35d1_wdt_isr(), it appears that reading a 1 and writing it back
clears the flag. Should these W1C flags be explicitly masked out when doing
read-modify-write operations on REG_WDT_CTL here, and similarly in other
functions like ma35d1_wdt_set_timeout(), ma35d1_wdt_start(), and
ma35d1_wdt_suspend()?

[ ... ]

> +static int ma35d1_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +				  unsigned int timeout)
> +{
> +	struct ma35d1_wdt_dev *ma35d1_wdt = watchdog_get_drvdata(wdt_dev);
> +	unsigned long target_ticks;
> +	unsigned int val, i;

[ ... ]

> +	guard(spinlock_irqsave)(&ma35d1_wdt->lock);
> +	val = readl_relaxed(ma35d1_wdt->wdt_base + REG_WDT_CTL);
> +	val &= ~TOUTSEL;
> +	val |= FIELD_PREP(TOUTSEL, i);
> +	writel_relaxed(val, ma35d1_wdt->wdt_base + REG_WDT_CTL);
> +
> +	ma35d1_wdt_ping(wdt_dev);
> +	return 0;
> +}

[Severity: High]
Does this writel_relaxed() require a call to ma35d1_wdt_wait_sync() before
proceeding? 

Without waiting for the SYNC bit, could the immediate ping command be ignored
by the hardware or cause hardware state corruption?

> +static int ma35d1_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	struct ma35d1_wdt_dev *ma35d1_wdt = watchdog_get_drvdata(wdt_dev);
> +	void __iomem *wdt_base = ma35d1_wdt->wdt_base;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = ma35d1_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
> +	if (ret < 0)
> +		return ret;
> +
> +	guard(spinlock_irqsave)(&ma35d1_wdt->lock);
> +	val = readl_relaxed(wdt_base + REG_WDT_CTL);
> +	val |= (WDTEN | RSTEN);
> +
> +	writel_relaxed(val, wdt_base + REG_WDT_CTL);

[Severity: High]
Because ma35d1_wdt_set_timeout() writes to REG_WDT_CTL without waiting for
sync, and then ma35d1_wdt_start() immediately performs another read-modify-write
on the same register, could this back-to-back unsynchronized access cause
dropped writes and prevent the watchdog from starting properly?

[ ... ]

> +static int ma35d1_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ma35d1_wdt_dev *ma35d1_wdt;

[ ... ]

> +	val = readl_relaxed(ma35d1_wdt->wdt_base + REG_WDT_CTL);
> +	if (val & RSTF) {
> +		ma35d1_wdt->wdt_dev.bootstatus = WDIOF_CARDRESET;
> +		writel_relaxed(val, ma35d1_wdt->wdt_base + REG_WDT_CTL);
> +	}

[Severity: High]
Should there be a wait for hardware synchronization after this write clears
the RSTF bit?

> +
> +	if (val & WDTEN)
> +		set_bit(WDOG_HW_RUNNING, &ma35d1_wdt->wdt_dev.status);

[Severity: High]
If the bootloader left the watchdog running with a very short timeout, could
the system reset before the core's ping worker can keep it alive?

Should this also call ma35d1_wdt_set_timeout() to ensure the hardware's
timeout matches the software configuration when taking over a running watchdog?

[ ... ]

> +static int ma35d1_wdt_suspend(struct device *dev)
> +{
> +	struct ma35d1_wdt_dev *ma35d1_wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&ma35d1_wdt->wdt_dev)) {

[Severity: High]
Does this need to check watchdog_hw_running() as well?

If the bootloader started the watchdog (WDOG_HW_RUNNING is set) but userspace
hasn't opened the device yet (WDOG_ACTIVE is false), this check will skip
pausing the hardware. Could the timer then expire and unexpectedly reset the
system during sleep?

[ ... ]

> +static int ma35d1_wdt_resume(struct device *dev)
> +{
> +	struct ma35d1_wdt_dev *ma35d1_wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&ma35d1_wdt->wdt_dev)) {

[Severity: High]
Similar to suspend, should this also check watchdog_hw_running() to ensure
a hardware-running watchdog is properly resumed?

> +		u32 val;
> +		int ret;
> +
> +		guard(spinlock_irqsave)(&ma35d1_wdt->lock);
> +		val = readl_relaxed(ma35d1_wdt->wdt_base + REG_WDT_CTL);
> +
> +		if (device_may_wakeup(dev)) {
> +			val |= RSTEN;
> +			val &= ~(INTEN | WKEN);
> +		} else {
> +			val |= (WDTEN | RSTEN);
> +		}
> +		writel_relaxed(val, ma35d1_wdt->wdt_base + REG_WDT_CTL);
> +		writel_relaxed(RESET_COUNTER,
> +			       ma35d1_wdt->wdt_base + REG_WDT_RSTCNT);
> +		ret = ma35d1_wdt_wait_sync(ma35d1_wdt);

[Severity: Medium]
Is it safe to issue the ping command (writing to REG_WDT_RSTCNT) before
waiting for the control register changes to synchronize?

In ma35d1_wdt_start(), the ping happens after ma35d1_wdt_wait_sync(). Could
doing it before sync here cause the ping to be ignored by the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724091427.1689980-1-zychennvt@gmail.com?part=2