Re: [PATCH v2 3/3] arm64: dts: nuvoton: Add WDT node for MA35D1 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Read-modify-write operations on REG_WDT_CTL do not mask write-1-to-clear (W1C) flags, leading to unintentional clearing of pending interrupts or reset flags.
--

--- Patch [3]: [PATCH v2 3/3] arm64: dts: nuvoton: Add WDT node for MA35D1 SoC ---
commit a67dd4dd104040028c3121676f3993b8ca0f94db
Author: Zi-Yu Chen <zychennvt@gmail.com>

arm64: dts: nuvoton: Add WDT node for MA35D1 SoC

This commit adds the Watchdog Timer (WDT) device node to the Nuvoton
MA35D1 device tree. It defines the register space, interrupt, and clock
for the watchdog controller.

> 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]
Does this read-modify-write clear pending write-1-to-clear (W1C) flags?

If IF, WKF, or RSTF bits are set when this reads REG_WDT_CTL, writing the
value back without masking those bits will unintentionally clear them, which
could result in dropped interrupts or wakeup events.

[ ... ]
> +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 update silently drop pending interrupts?

By performing a read-modify-write without masking out the W1C bits (such as
IF or WKF), any currently pending flags will be written back as 1s and cleared
before they can be processed.

[ ... ]
> +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)) {
> +		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]
Could this lose a race with a wakeup event?

Writing back the modified control register without clearing the W1C bits first
will unintentionally acknowledge and drop any wakeup or interrupt events that
occurred just before the read.

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