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

[email protected] 於 2026/7/24 下午 05:24 寫道:
> 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.
Not applicable. SYNC only used for the change of WDTEN.

> - [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.
Fixed. Added a check for watchdog_hw_running.

> - [High] Driver fails to synchronize the hardware timeout with the software configuration when inheriting a running watchdog during probe.
Fixed. Added set_timeout() before devm_watchdog_register_device().

> - [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.
Not applicable. As noted in the cover letter, this does not affect the W1C flags.

> - [Medium] In ma35d1_wdt_resume(), the driver writes to REG_WDT_RSTCNT before ensuring the control register changes have synchronized.
Not applicable. These two operations work independently and do not affect each other.

> - [Low] Spelling mistake in Kconfig help text.
Fixed. Corrected the spelling mistake in the Kconfig help text.

> --
> 
> --- Patch [2]: [PATCH v2 2/3] watchdog: Add Nuvoton MA35D1 watchdog driver support ---
> commit 31ed5d4971c74d4f75935b7f98c40e226cdc3521
> Author: Zi-Yu Chen <[email protected]>
> 
> 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?
> 

Regards,
Zi-Yu Chen