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

[sashiko-bot@kernel.org]

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