[PATCH 0/2] watchdog: msc313e: Fix issues Sashiko reported

[PATCH 0/2] watchdog: msc313e: Fix issues Sashiko reported

[Tzung-Bi Shih]

The series fixes the two issues reported by Sashiko in [1].

[1] https://lore.kernel.org/all/20260826062035.7645D1F000E9@smtp.kernel.org/

Tzung-Bi Shih (2):
  watchdog: msc313e: Fix NULL pointer dereference in PM callbacks
  watchdog: msc313e: Enable clock before accessing hardware registers

 drivers/watchdog/msc313e_wdt.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.55.0.887.g758fc8c411-goog

[PATCH 1/2] watchdog: msc313e: Fix NULL pointer dereference in PM callbacks

[Tzung-Bi Shih]

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 <tzungbi@kernel.org>
---
 drivers/watchdog/msc313e_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index d962589e2c55..f69d66971c41 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -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);
-- 
2.55.0.887.g758fc8c411-goog

[PATCH 2/2] watchdog: msc313e: Enable clock before accessing hardware registers

[Tzung-Bi Shih]

msc313e_wdt_probe() reads from hardware registers without ensuring the
required clock is enabled.  Furthermore, if the bootloader leaves the
watchdog running, msc313e_wdt_probe() sets WDOG_HW_RUNNING without
increasing the clock's reference count.

While the clock is currently supplied as a fixed clock by the device
tree (`xtal_div2` in arch/arm/boot/dts/sigmastar/mstar-v7.dtsi) which
masks the physical issue, this still violates the API usage.

Call clk_prepare_enable() before reading WDT registers.  If the WDT is
running, leave the clock enabled so the CCF reference counter is
balanced.

Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/watchdog/msc313e_wdt.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index f69d66971c41..b279ef645430 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -97,6 +97,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)
@@ -119,9 +120,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 	priv->wdev.max_timeout = U32_MAX / clk_get_rate(priv->clk);
 	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);
+		/*
+		 * 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);
-- 
2.55.0.887.g758fc8c411-goog

Re: [PATCH 1/2] watchdog: msc313e: Fix NULL pointer dereference in PM callbacks

[sashiko-bot]

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.
--

commit c2fa09163e4f9bd75f052d03568c2d22f22158f0
Author: Tzung-Bi Shih <tzungbi@kernel.org>

watchdog: msc313e: Fix NULL pointer dereference in PM callbacks

The commit fixes a NULL pointer dereference in the power management callbacks
of the msc313e watchdog driver. It ensures that the driver data is properly
set for the platform device in the probe function so it can be retrieved
during suspend and resume.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 90171431fc594..3b62650375628 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -125,6 +125,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);

While reviewing this code, I noticed a few pre-existing issues in the driver:

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827044700.554333-1-tzungbi@kernel.org?part=1

Re: [PATCH 2/2] watchdog: msc313e: Enable clock before accessing hardware registers

[sashiko-bot]

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 <tzungbi@kernel.org>
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.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 3b62650375628..b89b1cddaa5e8 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -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)

[ ... ]

> @@ -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?

>  	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);
> +		/*
> +		 * 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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827044700.554333-1-tzungbi@kernel.org?part=2

Re: [PATCH 1/2] watchdog: msc313e: Fix NULL pointer dereference in PM callbacks

[Guenter Roeck]

On Thu, Aug 27, 2026 at 04:46:59AM +0000, Tzung-Bi Shih wrote:
> 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 <tzungbi@kernel.org>

Applied.

Thanks,
Guenter

> ---
>  drivers/watchdog/msc313e_wdt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index d962589e2c55..f69d66971c41 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -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);

Re: [PATCH 2/2] watchdog: msc313e: Enable clock before accessing hardware registers

[Guenter Roeck]

On 8/26/26 22:01, sashiko-bot@kernel.org 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.

> 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 <tzungbi@kernel.org>
> 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.
> 
>> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
>> index 3b62650375628..b89b1cddaa5e8 100644
>> --- a/drivers/watchdog/msc313e_wdt.c
>> +++ b/drivers/watchdog/msc313e_wdt.c
>> @@ -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)
> 
> [ ... ]
> 
>> @@ -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?
> 
>>   	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);
>> +		/*
>> +		 * 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?
> 
Yes, that will need to be fixed.

> 
> [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?
> 

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

> 
> [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?
> 

Re: [PATCH 2/2] watchdog: msc313e: Enable clock before accessing hardware registers

[Tzung-Bi Shih]

On Fri, Aug 28, 2026 at 07:48:18AM -0700, Guenter Roeck wrote:
> On 8/26/26 22:01, sashiko-bot@kernel.org 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.

Fixed it in v2.

https://lore.kernel.org/all/20260828161348.13212-5-tzungbi@kernel.org

> 
> > 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.

Proposed fixes for them in the series [1].

[1] https://lore.kernel.org/all/20260828161348.13212-1-tzungbi@kernel.org

> > > @@ -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?

https://lore.kernel.org/all/20260828161348.13212-3-tzungbi@kernel.org

> > 
> > >   	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);
> > > +		/*
> > > +		 * 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?
> > 
> Yes, that will need to be fixed.

https://lore.kernel.org/all/20260828161348.13212-5-tzungbi@kernel.org

> > [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?
> > 
> 
> Worse, it starts the watchdog even if it isn't running. Separate problem, though.

https://lore.kernel.org/all/20260828161348.13212-4-tzungbi@kernel.org

> > [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?

https://lore.kernel.org/all/20260828161348.13212-6-tzungbi@kernel.org

Re: [PATCH 1/2] watchdog: msc313e: Fix NULL pointer dereference in PM callbacks

[Tzung-Bi Shih]

On Thu, Aug 27, 2026 at 04:57:46AM +0000, sashiko-bot@kernel.org wrote:
> 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.

Proposed fixes for them in the series [1].

[1] https://lore.kernel.org/all/20260828161348.13212-1-tzungbi@kernel.org

> 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?

https://lore.kernel.org/all/20260828161348.13212-6-tzungbi@kernel.org

> 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?

https://lore.kernel.org/all/20260828161348.13212-9-tzungbi@kernel.org

> 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.

https://lore.kernel.org/all/20260828161348.13212-10-tzungbi@kernel.org

> 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.

https://lore.kernel.org/all/20260828161348.13212-3-tzungbi@kernel.org

> 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.

https://lore.kernel.org/all/20260828161348.13212-4-tzungbi@kernel.org