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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] The `ma35d1_wdt->dev` pointer is never initialized, leading to NULL pointer dereferences in logging functions.
- [High] Read-modify-write operations on REG_WDT_CTL inadvertently clear W1C (write-one-to-clear) status flags.
- [High] Missing clock domain synchronization (`ma35d1_wdt_wait_sync`) after writes to `REG_WDT_CTL` can lead to dropped register writes.
- [High] Driver overwrites `wdt_dev->timeout`, breaking the software heartbeat extension.
- [High] Missing cleanup for wake IRQ allocated by dev_pm_set_wake_irq(), leading to a memory leak and permanent device unbind/bind failures.
- [High] The driver fails to check if the watchdog was already enabled by the bootloader, causing unexpected system reboots during boot.
- [Medium] Unsigned integer comparison against zero renders IRQ error handling dead code.
- [Medium] Integer overflow in `target_ticks` calculation triggers unexpected system resets for large timeouts.
- [Low] Unnecessary dependency: selects `MFD_SYSCON` without using it.
--

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

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/Kconfig b/drivers/watchdog/Kconfig
> index 08cb8612d41fe..28d8d3d12c27f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -720,6 +720,17 @@ 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
> +	select MFD_SYSCON

[Severity: Low]
Is select MFD_SYSCON necessary here?

The driver maps its MMIO region directly and doesn't appear to use MFD or
syscon APIs.

[ ... ]
> --- /dev/null
> +++ b/drivers/watchdog/ma35d1_wdt.c
[ ... ]
> +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;
> +	static const uint8_t toutsel_shifts[] = { 4,  6,  8,  10, 12,
> +						  14, 16, 18, 20 };
> +
> +	target_ticks = (unsigned long)timeout * ma35d1_wdt->clk_rate;

[Severity: Medium]
Can this calculation overflow on 32-bit platforms where unsigned long is
32 bits?

If a user configures a large timeout, timeout * clk_rate might truncate
before it can be clamped, resulting in a tiny hardware timeout.

> +
> +	for (i = 0; i < ARRAY_SIZE(toutsel_shifts); i++) {
> +		if ((1UL << toutsel_shifts[i]) >= target_ticks)
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(toutsel_shifts))
> +		i = ARRAY_SIZE(toutsel_shifts) - 1;
> +
> +	scoped_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 sequence accidentally clear write-one-to-clear
status flags like IF, WKF, or RSTF?

Writing back a 1 to these bits when they are concurrently set by hardware
would acknowledge and clear them.

> +
> +	/* To avoid truncation errors (0 seconds) during division. */
> +	wdt_dev->timeout = (1UL << toutsel_shifts[i]) / ma35d1_wdt->clk_rate;

[Severity: High]
Does overwriting wdt_dev->timeout with the truncated hardware timeout
break the software heartbeat extension?

If userspace requests a timeout larger than the hardware maximum, the core
delegates to the software ping worker. Modifying wdt_dev->timeout here
causes the core to adopt the small timeout instead.

> +	if (wdt_dev->timeout == 0)
> +		wdt_dev->timeout = 1;
> +
> +	dev_dbg(ma35d1_wdt->dev, "wdt_dev->timeout = %d\n", wdt_dev->timeout);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	unsigned int val;
> +
> +	struct ma35d1_wdt_dev *ma35d1_wdt = watchdog_get_drvdata(wdt_dev);
> +	void __iomem *wdt_base = ma35d1_wdt->wdt_base;
> +	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]
Can the back-to-back writes to REG_WDT_CTL drop register updates?

ma35d1_wdt_set_timeout() writes to REG_WDT_CTL but doesn't call
ma35d1_wdt_wait_sync() afterwards. When ma35d1_wdt_start() immediately
writes to REG_WDT_CTL again, it might happen before the synchronization
bit clears.

[ ... ]
> +static int ma35d1_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ma35d1_wdt_dev *ma35d1_wdt;
> +	unsigned long clk_rate, val;
> +	int ret;
> +
> +	ma35d1_wdt = devm_kzalloc(dev, sizeof(*ma35d1_wdt), GFP_KERNEL);
> +	if (!ma35d1_wdt)
> +		return -ENOMEM;

[Severity: High]
It looks like ma35d1_wdt->dev is never assigned to dev during probe.

If a synchronization timeout occurs later in ma35d1_wdt_stop(), will
dev_err(ma35d1_wdt->dev, ...) dereference this uninitialized pointer?

[ ... ]
> +	ma35d1_wdt->irq = platform_get_irq(pdev, 0);
> +	if (ma35d1_wdt->irq < 0)
> +		return dev_err_probe(dev, ma35d1_wdt->irq,
> +				     "failed to get irq\n");

[Severity: Medium]
Since the irq field in struct ma35d1_wdt_dev is declared as an unsigned u32,
will this < 0 check ever evaluate to true?

If platform_get_irq() returns a negative error code like -ENXIO, the error
handling would be skipped and proceed to devm_request_irq() with a huge
positive IRQ number.

[ ... ]
> +	if (device_property_read_bool(dev, "wakeup-source")) {
> +		ret = devm_device_init_wakeup(dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = dev_pm_set_wake_irq(dev, ma35d1_wdt->irq);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Because the driver uses dev_pm_set_wake_irq() without a devm_ wrapper and
lacks a remove function, does this leak the PM wake IRQ structure?

If the driver is unbound and rebound via sysfs, the attached wake IRQ is
never cleaned up, which could cause subsequent binds to fail.

[ ... ]
> +	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);
> +	}
> +
> +	watchdog_set_drvdata(&ma35d1_wdt->wdt_dev, ma35d1_wdt);

[Severity: High]
Does this code need to check the WDTEN bit to handle a running watchdog
handed off by the bootloader?

If the hardware is already ticking and WDOG_HW_RUNNING is not set, the
watchdog core won't know to ping the hardware before userspace connects,
which could lead to spurious resets during boot.

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