Re: [PATCH v2 5/5] watchdog: qcom: Register pretimeout interrupt as NMI

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Calling WDIOC_SETTIMEOUT or WDIOC_SETPRETIMEOUT on an active watchdog triggers an 'Unbalanced enable for IRQ' kernel warning.
- [Medium] Unbinding the driver or encountering a probe failure when the hardware watchdog is already running from the bootloader triggers a WARN_ON in free_nmi().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but setting the timeout or pretimeout on a stopped watchdog unconditionally starts the hardware watchdog without updating the core state, eventually leading to an unexpected system reboot.
--

commit 90c184f58c3a63ad67e01452209c8185a52db049
Author: Mayank Rungta <mrungta@google.com>

watchdog: qcom: Register pretimeout interrupt as NMI

When a system is completely unresponsive due to an interrupt storm or
deadlocked CPU cores with standard interrupts disabled, a standard
watchdog pretimeout bark interrupt will fail to execute, preventing the
pretimeout governor from capturing CPU backtraces before the hardware
reset bite.

This commit updates the driver to attempt requesting the pretimeout
interrupt as an NMI. It adds cleanup actions and handles enabling and
disabling the NMI during start and stop.

> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 4eb1bf9790125..82eaf8786426f 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -52,6 +52,8 @@ struct qcom_wdt {
>  	unsigned long		rate;
>  	void __iomem		*base;
>  	const u32		*layout;
> +	int			irq;
> +	bool			is_nmi;
>  };
>  
>  static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)

[ ... ]

> @@ -74,6 +76,28 @@ static irqreturn_t qcom_wdt_isr(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +static void qcom_wdt_enable_irq(struct qcom_wdt *wdt)
> +{
> +	if (wdt->is_nmi && wdt->irq > 0)
> +		enable_nmi(wdt->irq);
> +}
> +
> +static void qcom_wdt_disable_irq(struct qcom_wdt *wdt)
> +{
> +	if (wdt->is_nmi && wdt->irq > 0)
> +		disable_nmi(wdt->irq);
> +}
> +
> +static void qcom_wdt_free_nmi(void *arg)
> +{
> +	struct qcom_wdt *wdt = arg;
> +
> +	if (watchdog_active(&wdt->wdd))
> +		qcom_wdt_disable_irq(wdt);
> +
> +	free_nmi(wdt->irq, &wdt->wdd);
> +}

[Severity: Medium]
Will this fail to disable the NMI if the watchdog was started by the
bootloader?

If the hardware watchdog is already running from the bootloader,
qcom_wdt_probe() calls qcom_wdt_start(), which enables the NMI and sets the
WDOG_HW_RUNNING flag, but the WDOG_ACTIVE flag remains unset:

drivers/watchdog/qcom-wdt.c:qcom_wdt_probe() {
    ...
    if (qcom_wdt_is_running(&wdt->wdd)) {
        qcom_wdt_start(&wdt->wdd);
        set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
    }
    ...
}

If the driver is later unbound or fails probe, qcom_wdt_free_nmi() only
checks watchdog_active(), which evaluates WDOG_ACTIVE.

Because the disable step is skipped, will free_nmi() be called while the NMI
is still enabled, resulting in a WARN_ON stack trace during cleanup?

>  static int qcom_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> @@ -84,6 +108,8 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
>  	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
>  	writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
> +
> +	qcom_wdt_enable_irq(wdt);
>  	return 0;
>  }

[Severity: Medium]
Does this code trigger an unbalanced enable warning when the timeout is changed?

When a user application changes the timeout via ioctl(WDIOC_SETTIMEOUT), the
core watchdog subsystem invokes qcom_wdt_set_timeout(), which unconditionally
calls qcom_wdt_start():

drivers/watchdog/qcom-wdt.c:qcom_wdt_set_timeout() {
    ...
    wdd->timeout = timeout;
    return qcom_wdt_start(wdd);
}

Because qcom_wdt_start() then blindly calls qcom_wdt_enable_irq() without
checking if the NMI is already enabled, will this trigger an 'Unbalanced
enable for IRQ' warning from the core IRQ subsystem?

[Severity: High]
This is a pre-existing issue, but does calling qcom_wdt_start() here
unconditionally start the hardware even if it was stopped by the user?

If a user application stops the watchdog and then sets the timeout,
qcom_wdt_set_timeout() blindly invokes qcom_wdt_start() as shown above.

This physically starts the hardware countdown without updating the core state.

Because the core state remains disabled, subsequent keepalive pings are
silently ignored by the core. Will this lead to an un-pinged hardware
watchdog that eventually unexpectedly reboots the system?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-qcom-wdt-nmi-series-v2-0-363979fe6b6b@google.com?part=5