watchdog: msc313e: Fix issues Sashiko reported

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware watchdog is not properly stopped during system suspend if it was started by the bootloader but not opened by userspace.
- [High] The resume callback ignores the return value of msc313e_wdt_start().
- [High] Potential division by zero during max_timeout calculation.
- [High] Clock reference count leak and unintended hardware activation in msc313e_wdt_settimeout.
- [Low] Struct initialization uses commas instead of semicolons.

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_suspend()

    if (watchdog_active(&priv->wdev))
        msc313e_wdt_stop(&priv->wdev);

[Severity: High]
This is a pre-existing issue, but does this bypass stopping the hardware watchdog if userspace hasn't opened the device?

If the hardware was started by the bootloader but not yet opened by userspace, the ping worker is frozen during suspend. Since watchdog_active() checks if userspace has opened it, will the active hardware timer continue running and trigger a spurious system reset during system suspend?

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_resume()

    if (watchdog_active(&priv->wdev))
        msc313e_wdt_start(&priv->wdev);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of msc313e_wdt_start()?

If the clock enable fails inside msc313e_wdt_start(), the function will silently discard the error. Will subsequent register operations in the watchdog then access unclocked memory and cause a system crash?

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_probe()

    priv->wdev.info = &msc313e_wdt_ident,
    priv->wdev.ops = &msc313e_wdt_ops,
    priv->wdev.parent = dev;

[Severity: Low]
This isn't a bug introduced by this patch, but are these assignments using comma operators intentionally instead of semicolons?

While syntactically valid, this violates standard kernel coding style and could cause unintended behavior during refactoring.

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_probe()

    priv->wdev.min_timeout = MSC313E_WDT_MIN_TIMEOUT;
    priv->wdev.max_timeout = U32_MAX / clk_get_rate(priv->clk);
    priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;

[Severity: High]
This is a pre-existing issue, but could this calculation result in a division by zero panic?

If clk_get_rate() returns 0 for an uninitialized clock, using it directly as a denominator here will trigger a division by zero exception during probe.

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_settimeout()

    wdev->timeout = new_time;

    return msc313e_wdt_start(wdev);

[Severity: High]
This is a pre-existing issue, but does unconditionally calling msc313e_wdt_start() here cause a clock reference leak or unintended hardware activation?

If the watchdog is already active, this would increment the clock reference count redundantly. If it is stopped, it inadvertently starts the hardware timer without the watchdog core tracking it, which could cause an unexpected system reset.

msc313e_wdt_probe() doesn't set the driver data for the platform device. As a result, dev_get_drvdata() in msc313e_wdt_suspend() and msc313e_wdt_resume() will return NULL, leading to a NULL pointer dereference afterward.

Set the platform device driver data in msc313e_wdt_probe().

Fixes e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")

Signed-off-by Tzung-Bi Shih <[email protected]>

---
 drivers/watchdog/msc313e_wdt.c | 1 +
 1 file changed, 1 insertion(+)

drivers/watchdog/msc313e_wdt.c +1 -0

index d962589e2c55..f69d66971c41 100644
@@ -124,6 +124,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
 
 	watchdog_set_drvdata(&priv->wdev, priv);
+	platform_set_drvdata(pdev, priv);
 
 	watchdog_init_timeout(&priv->wdev, timeout, dev);
 	watchdog_stop_on_reboot(&priv->wdev);

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Clock reference count is leaked on the probe error path if watchdog registration fails.

Pre-existing issues:
- [High] Repeatedly setting the watchdog timeout leaks clock prepare and enable references, potentially leading to refcount overflow.
- [High] System will unexpectedly reset during suspend if the hardware watchdog is running but was never opened by userspace.
- [High] Potential division by zero if clk_get_rate() returns 0.
--

commit 363d0038f19004fef94c7716c644171a89070cb1
Author: Tzung-Bi Shih <[email protected]>
Subject: watchdog: msc313e: Enable clock before accessing hardware registers

msc313e_wdt_probe() reads from hardware registers without ensuring the required clock is enabled. This patch calls clk_prepare_enable() before reading WDT registers and leaves the clock enabled if the WDT is running.

Quoted 12 lines

drivers/watchdog/msc313e_wdt.c +1 -0

index 3b62650375628..b89b1cddaa5e8 100644
@@ -98,6 +98,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
  {
  	struct device *dev = &pdev->dev;
  	struct msc313e_wdt_priv *priv;
+	int ret;
 
  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
  	if (!priv)

[ ... ]

drivers/watchdog/msc313e_wdt.c static int msc313e_wdt_probe(struct platform_device *pdev) · line 121

Quoted 2 lines

drivers/watchdog/msc313e_wdt.c +0 -0

@@ -120,9 +121,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
  	priv->wdev.max_timeout = U32_MAX / clk_get_rate(priv->clk);

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause a division by zero panic during device probe if clk_get_rate() returns 0?

Quoted 11 lines

drivers/watchdog/msc313e_wdt.c +6 -1

  	priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
 
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
  	/* If the period is non-zero the WDT is running */
-	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16))
+	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16)) {
  		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
+		/*

drivers/watchdog/msc313e_wdt.c static int msc313e_wdt_probe(struct platform_device *pdev) · line 140

Quoted 10 lines

drivers/watchdog/msc313e_wdt.c +7 -0

+		 * Keep the clock enabled. The watchdog core will skip the next
+		 * start() and a future stop() will balance the CCF reference
+		 * count.
+		 */
+	} else {
+		clk_disable_unprepare(priv->clk);
+	}
 
  	watchdog_set_drvdata(&priv->wdev, priv);
  	platform_set_drvdata(pdev, priv);

[Severity: Medium]
If the hardware watchdog is already running at boot, the driver intentionally leaves the clock enabled here. However, if devm_watchdog_register_device() subsequently fails:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_probe() {
     ...
	return devm_watchdog_register_device(dev, &priv->wdev);
}

Does this regression leak the clock reference count on the error path if device registration fails, since the prepared clock is never disabled?

[Severity: High]
This is a pre-existing issue, but repeatedly setting the watchdog timeout leaks clock prepare and enable references, potentially leading to refcount overflow.

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_settimeout() {
     ...
	return msc313e_wdt_start(wdev);
}

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_start() {
     ...
	err = clk_prepare_enable(priv->clk);
     ...
}

Since the set_timeout callback directly wraps msc313e_wdt_start(), does this sequence unconditionally increment the clock refcount every time the timeout is updated, without a corresponding decrement?

[Severity: High]
This isn't a bug introduced by this patch, but will the system unexpectedly reset during suspend if the hardware watchdog is running but was never opened by userspace?

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_suspend() {
     ...
	if (watchdog_active(&priv->wdev))
		msc313e_wdt_stop(&priv->wdev);
     ...
}

If the watchdog was left running by the bootloader (WDOG_HW_RUNNING is set) but never opened by userspace, watchdog_active() is false and the driver skips stopping it. Meanwhile, the watchdog core suspends its background ping worker.

Since the hardware timer continues counting down without being pinged, will it expire and reset the suspended system?

Quoted 5 lines · On 8/26/26 22:01, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Clock reference count is leaked on the probe error path if watchdog registration fails.

That seems real.

Quoted 5 lines

Pre-existing issues:
- [High] Repeatedly setting the watchdog timeout leaks clock prepare and enable references, potentially leading to refcount overflow.
- [High] System will unexpectedly reset during suspend if the hardware watchdog is running but was never opened by userspace.
- [High] Potential division by zero if clk_get_rate() returns 0.

Quoted 6 lines

drivers/watchdog/msc313e_wdt.c static int msc313e_wdt_probe(struct platform_device *pdev) · line 121

Quoted 2 lines

drivers/watchdog/msc313e_wdt.c +0 -0

@@ -120,9 +121,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
  	priv->wdev.max_timeout = U32_MAX / clk_get_rate(priv->clk);

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause a division by zero panic during device probe if clk_get_rate() returns 0?

Quoted 31 lines

Quoted 10 lines

drivers/watchdog/msc313e_wdt.c +5 -1

  	priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
  	/* If the period is non-zero the WDT is running */
-	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16))
+	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16)) {
  		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);

drivers/watchdog/msc313e_wdt.c static int msc313e_wdt_probe(struct platform_device *pdev) · line 140

Quoted 10 lines

drivers/watchdog/msc313e_wdt.c +8 -0

+		/*
+		 * Keep the clock enabled. The watchdog core will skip the next
+		 * start() and a future stop() will balance the CCF reference
+		 * count.
+		 */
+	} else {
+		clk_disable_unprepare(priv->clk);
+	}
  	watchdog_set_drvdata(&priv->wdev, priv);
  	platform_set_drvdata(pdev, priv);

[Severity: Medium]
If the hardware watchdog is already running at boot, the driver intentionally leaves the clock enabled here. However, if devm_watchdog_register_device() subsequently fails:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_probe() {
     ...
	return devm_watchdog_register_device(dev, &priv->wdev);
}

Quoted 3 lines

Does this regression leak the clock reference count on the error path if device registration fails, since the prepared clock is never disabled?

Yes, that will need to be fixed.

Quoted 16 lines

[Severity: High]
This is a pre-existing issue, but repeatedly setting the watchdog timeout leaks clock prepare and enable references, potentially leading to refcount overflow.

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_settimeout() {
     ...
	return msc313e_wdt_start(wdev);
}

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_start() {
     ...
	err = clk_prepare_enable(priv->clk);
     ...
}

Quoted 4 lines

Since the set_timeout callback directly wraps msc313e_wdt_start(), does this sequence unconditionally increment the clock refcount every time the timeout is updated, without a corresponding decrement?

Worse, it starts the watchdog even if it isn't running. Separate problem, though.

Quoted 12 lines

[Severity: High]
This isn't a bug introduced by this patch, but will the system unexpectedly reset during suspend if the hardware watchdog is running but was never opened by userspace?

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_suspend() {
     ...
	if (watchdog_active(&priv->wdev))
		msc313e_wdt_stop(&priv->wdev);
     ...
}

Quoted 7 lines

If the watchdog was left running by the bootloader (WDOG_HW_RUNNING is set) but never opened by userspace, watchdog_active() is false and the driver skips stopping it. Meanwhile, the watchdog core suspends its background ping worker.

Since the hardware timer continues counting down without being pinged, will it expire and reset the suspended system?