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

[email protected] 於 2026/8/4 下午 03:15 寫道:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Missing wait for hardware synchronization (`SYNC` bit) after writing to `REG_WDT_CTL` leads to dropped writes and hardware races.
Not applicable.
According to the MA35D1 TRM, hardware synchronization (SYNC bit) is only required when the WDTEN (Watchdog Enable) bit status is toggled.
Other control bit updates do not require waiting for SYNC."

> - [High] Read-modify-write operations on `REG_WDT_CTL` inadvertently clear Write-1-to-Clear (W1C) status flags.
Not applicable.
INTEN and WKEN are only enabled during PM suspend when watchdog wakeup is configured. During normal runtime, INTEN and WKEN are disabled, 
so IF and WKF remain 0. Therefore, read-modify-write operations during runtime will not inadvertently clear active flags.

> - [Medium] The interrupt handler unconditionally clears all flags and returns IRQ_HANDLED without verifying the interrupt source.
Not applicable.
The WDT interrupt line is dedicated (non-shared) and strictly utilized as a wakeup source during PM suspend.
Unconditionally clearing the flags upon interrupt entry ensures the wakeup/interrupt state is properly reset when exiting sleep.

> --
> 
> --- Patch [2]: [PATCH v3 2/3] watchdog: Add Nuvoton MA35D1 watchdog driver support ---
> commit b3e6fc51e35611680a67e3e8fcf506e2ac5f75cb
> Author: Zi-Yu Chen <[email protected]>
> 
> watchdog: Add Nuvoton MA35D1 watchdog driver support
> 
> Add driver support for the Watchdog Timer (WDT) integrated in Nuvoton
> MA35D1 SoC. This driver provides standard watchdog functionality,
> including timeout configuration, ping, and system reset support via
> the watchdog framework.
> 
>> diff --git a/drivers/watchdog/ma35d1_wdt.c b/drivers/watchdog/ma35d1_wdt.c
>> new file mode 100644
>> index 0000000000000..119d58f338bf0
>> --- /dev/null
>> +++ b/drivers/watchdog/ma35d1_wdt.c
> 
> [ ... ]
> 
>> +static int ma35d1_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> +				  unsigned int timeout)
>> +{
> 
> [ ... ]
> 
>> +	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);
> 
> [Severity: High]
> Does this read-modify-write of REG_WDT_CTL inadvertently clear W1C flags?
> 
> If bits like IF, WKF, and RSTF are write-1-to-clear, writing back a value
> that has them set will unintentionally clear pending interrupts, wakeups, or
> reset status. Should these W1C bits be masked out from val before it is
> written back?
> 
> Also, should there be a call to ma35d1_wdt_wait_sync() here?
> 
> When ma35d1_wdt_start() calls this function, it modifies REG_WDT_CTL, returns,
> and then immediately modifies REG_WDT_CTL again. Could this second write
> overlap with the clock domain synchronization of the first write and cause
> hardware races?
> 
>> +
>> +	ma35d1_wdt_ping(wdt_dev);
>> +	return 0;
>> +}
>> +
>> +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]
> This read-modify-write shares the same potential W1C flag clearing issue
> mentioned in ma35d1_wdt_set_timeout().
> 
>> +	ret = ma35d1_wdt_wait_sync(ma35d1_wdt);
>> +	if (ret) {
>> +		dev_err(wdt_dev->parent, "Wait for WDTEN SYNC timeout!\n");
>> +		return ret;
>> +	}
>> +
>> +	writel_relaxed(RESET_COUNTER, wdt_base + REG_WDT_RSTCNT);
>> +
>> +	return 0;
>> +}
> 
> [ ... ]
> 
>> +static irqreturn_t ma35d1_wdt_isr(int irq, void *dev_id)
>> +{
>> +	struct ma35d1_wdt_dev *ma35d1_wdt = dev_id;
>> +	unsigned int val;
>> +
>> +	/* Clear the flag if set */
>> +	guard(spinlock)(&ma35d1_wdt->lock);
>> +	val = readl_relaxed(ma35d1_wdt->wdt_base + REG_WDT_CTL);
>> +	writel_relaxed(val, ma35d1_wdt->wdt_base + REG_WDT_CTL);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> [Severity: Medium]
> Does this interrupt handler need to verify the interrupt source before
> returning IRQ_HANDLED?
> 
> As written, this unconditionally claims the interrupt, which bypasses the
> kernel's spurious interrupt detection if the interrupt was actually
> generated by another device sharing the line.
> 
> Additionally, would writing back the unmodified value of REG_WDT_CTL clear
> all W1C flags at once, including RSTF, which might destroy status
> information needed elsewhere?
> 
> [ ... ]
> 
>> +static int ma35d1_wdt_probe(struct platform_device *pdev)
>> +{
> 
> [ ... ]
> 
>> +	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]
> Does this write to clear RSTF also require a call to ma35d1_wdt_wait_sync()?
> 
> The code proceeds to call ma35d1_wdt_set_timeout() shortly after this, which
> will write to REG_WDT_CTL again. Could this lead to dropped writes or hardware
> races if the synchronization from this first write isn't complete?
> 
>> +
>> +	if (val & WDTEN)
>> +		set_bit(WDOG_HW_RUNNING, &ma35d1_wdt->wdt_dev.status);
>> +
>> +	watchdog_set_drvdata(&ma35d1_wdt->wdt_dev, ma35d1_wdt);
>> +	watchdog_set_nowayout(&ma35d1_wdt->wdt_dev, nowayout);
>> +	watchdog_init_timeout(&ma35d1_wdt->wdt_dev, timeout, &pdev->dev);
>> +
>> +	ma35d1_wdt_set_timeout(&ma35d1_wdt->wdt_dev,
>> +			       ma35d1_wdt->wdt_dev.timeout);
> 
> [ ... ]
> 
>> +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) ||
>> +	    watchdog_hw_running(&ma35d1_wdt->wdt_dev)) {
>> +		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);
> 
> [Severity: High]
> This read-modify-write shares the same potential W1C flag clearing issue.
> 
> If a pending wakeup flag is set during the read, could it be inadvertently
> cleared right before the system sleeps?
> 
>> +		ret = ma35d1_wdt_wait_sync(ma35d1_wdt);
>> +		if (ret) {
>> +			dev_err(dev, "Wait for WDTEN SYNC timeout!\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> 

Regards,
Zi-Yu Chen