Re: [PATCH] watchdog/sama5d4: set platform driver data before requesting IRQ

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared interrupt handler unconditionally returns IRQ_HANDLED even when it determines that the interrupt was not generated by the watchdog device.
- [High] The driver requests an interrupt using both IRQF_SHARED and IRQF_NO_SUSPEND, which is an unsafe combination.
- [High] The physical hardware timeout is unconditionally reset to a hardcoded 16 seconds during initialization and resume, ignoring any configured device tree or user space timeout.
--

commit 7ce180f45b6396bc468527a9890ba1852c13eb65
Author: Rosen Penev <rosenp@gmail.com>

watchdog/sama5d4: set platform driver data before requesting IRQ

sama5d4_wdt_probe() requests a shared IRQ before calling
platform_set_drvdata(). Because the IRQ is shared, the handler
can fire immediately, causing a NULL pointer crash when reading
the missing driver data. This patch moves the drvdata setup earlier.

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 704b786cc2ec6..dcf5b8ac9e0d2 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -268,6 +268,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  						: AT91_WDT_WDDIS;
>  
>  	watchdog_set_drvdata(wdd, wdt);
> +	platform_set_drvdata(pdev, wdt);

[Severity: High]
This isn't a bug introduced by this patch, but since this change fixes a 
crash in the shared IRQ handler, is it safe to request this IRQ with both 
IRQF_SHARED and IRQF_NO_SUSPEND?

According to the suspend documentation, IRQF_NO_SUSPEND affects the entire
IRQ line, keeping it enabled during system suspend. When the line is shared,
any interrupt triggered during suspend causes all sharing handlers to run:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_probe() {
        ...
        ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
                               IRQF_SHARED | IRQF_IRQPOLL |
                               IRQF_NO_SUSPEND, pdev->name, pdev);
        ...
}

Could other suspended devices sharing the line crash or cause bus faults if
they are unprepared to handle interrupts while powered down?

[Severity: High]
This is a pre-existing issue, but how does the handler deal with spurious
interrupts on this shared line?

The code unconditionally returns IRQ_HANDLED even when the interrupt was not
generated by the watchdog:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_irq_handler() {
        ...
        if (reg) {
                pr_crit("Atmel Watchdog Software Reset\n");
                emergency_restart();
                pr_crit("Reboot didn't succeed\n");
        }

        return IRQ_HANDLED;
}

Might this prevent the kernel's spurious interrupt detection from disabling
the IRQ line during an interrupt storm, potentially leading to a hard lockup
if a malfunctioning device shares the same line?

>  	regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(regs))
> @@ -316,8 +317,6 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	platform_set_drvdata(pdev, wdt);
> -
>  	dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
>  		 wdd->timeout, nowayout);

[Severity: High]
This is a pre-existing issue, but does the hardware timeout get
unconditionally overwritten with the default 16 seconds?

In sama5d4_wdt_init(), the hardware timeout is hardcoded to
WDT_DEFAULT_TIMEOUT (16 seconds), which seems to ignore the configured
wdd->timeout logged above:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_init() {
        ...
        val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT);

        if (wdt->sam9x60_support) {
                wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(val));
        } else {
                wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT));
                wdt->mr |= AT91_WDT_SET_WDV(val);
        }
        ...
}

Because sama5d4_wdt_init() is called unconditionally during both probe and
sama5d4_wdt_resume_early(), will a custom timeout configured via device tree
or userspace be lost? If a userspace daemon expects a 32s timeout and pings
at 20s intervals, it seems the unexpected 16s hardware timeout would cause
the system to unexpectedly reboot.

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