Re: [PATCH 1/2] watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dropping IRQF_SHARED causes a probe regression on SAM9X60 and SAM9X7 platforms where the watchdog IRQ is genuinely shared.

Pre-existing issues:
- [High] A race condition during probe can cause a NULL pointer dereference in the IRQ handler because the IRQ is requested before the platform drvdata is set.
- [High] The driver incorrectly uses the SAMA5D4 watchdog disable bit (`AT91_WDT_WDDIS`) instead of the SAM9X60 watchdog disable bit (`AT91_SAM9X60_WDDIS`) when checking if the hardware is running.
--

commit 10beb96fcac3b8fc975881a372fcfe158d7e1630
Author: Rosen Penev <rosenp@gmail.com>

watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

This commit modifies the sama5d4 watchdog driver to drop the shared IRQ
flag, handle spurious interrupts appropriately by returning IRQ_NONE, and
use the configured timeout during initialization instead of a hardcoded
16-second default.

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c

[ ... ]

>  	if (wdt->need_irq) {
>  		ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
> -				       IRQF_SHARED | IRQF_IRQPOLL |
> -				       IRQF_NO_SUSPEND, pdev->name, pdev);
> +				       IRQF_IRQPOLL | IRQF_NO_SUSPEND,
> +				       pdev->name, pdev);

[Severity: High]
Will dropping IRQF_SHARED cause probe failures on SAM9X60 and SAM9X7
platforms?  On these devices, the watchdog uses the System IRQ, which is
shared with other critical peripherals like the RTC, RTT, PMC, and PIT.  If
another driver has already requested the shared line, won't
devm_request_irq() fail with -EBUSY and disable the watchdog entirely?

[Severity: High]
This is a pre-existing issue, but can this sequence lead to a NULL pointer
dereference?

The IRQ is requested and enabled here, but platform_set_drvdata() isn't
called until the end of sama5d4_wdt_probe().  If an interrupt fires
immediately (for instance, from a shared device like the RTC on SAM9X60),
sama5d4_wdt_irq_handler() will execute:

sama5d4_wdt_irq_handler() {
        struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
        u32 reg;

        if (wdt->sam9x60_support)
        ...
}

Since platform_get_drvdata() returns NULL before the driver data is set,
won't this crash when attempting to access wdt->sam9x60_support?

>  		if (ret) {
>  			dev_err(dev, "cannot register interrupt handler\n");
>  			return ret;

[ ... ]

>  	reg = wdt_read(wdt, AT91_WDT_MR);
>  	if (!(reg & AT91_WDT_WDDIS)) {
>  		wdt->mr &= ~AT91_WDT_WDDIS;
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}
>  
> -	ret = sama5d4_wdt_init(wdt);
> +	ret = sama5d4_wdt_init(wdt, wdd->timeout);

[Severity: High]
This is a pre-existing issue, but does this logic correctly identify if the
hardware is running on SAM9X60 platforms?

The driver checks AT91_WDT_WDDIS (which is bit 15) unconditionally here and
in the wdt_enabled macro.  However, SAM9X60 uses AT91_SAM9X60_WDDIS 
(which is bit 12).

Since the SAM9X60 initial wdt->mr only has bit 12 set, won't this check
always evaluate to true, bypassing the safe watchdog initialization block
inside sama5d4_wdt_init() and potentially leaving the system exposed to
spurious resets?

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