Re: [PATCHv2 1/3] 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:

Pre-existing issues:
- [High] Race Condition / NULL Pointer Dereference in IRQ Handler
- [High] Broken hardware running detection silently disables SAM9X60 watchdog
- [Medium] Missing Implementation of Claimed Fix
--

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

watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

This commit aims to fix three issues in the sama5d4 watchdog driver: an
unsafe IRQF_SHARED combination, unconditional IRQ_HANDLED returns on shared
lines, and a hardcoded timeout. It modifies the IRQ handler to return
IRQ_NONE for spurious interrupts and passes the configured timeout to the
init function.

>     1. Unsafe IRQF_SHARED | IRQF_NO_SUSPEND combination: The watchdog
>        interrupt is a dedicated peripheral line, not shared with other
>        devices.

[Severity: Medium]
This is a pre-existing issue, but the commit message states this
combination is being removed, yet the patch does not appear to change the
devm_request_irq() flags in the driver.

Was the code change for this specific item accidentally omitted from the
patch?

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 13e72918338a2..c7d6f877537c3 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c

[ ... ]

> @@ -311,7 +312,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}

[Severity: High]
This is a pre-existing issue, but does this hardware running check use the
correct bit for SAM9X60 devices?

In sama5d4_wdt_probe(), the code hardcodes AT91_WDT_WDDIS when reading the
status:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_probe() {
	...
	reg = wdt_read(wdt, AT91_WDT_MR);
	if (!(reg & AT91_WDT_WDDIS)) {
		wdt->mr &= ~AT91_WDT_WDDIS;
		set_bit(WDOG_HW_RUNNING, &wdd->status);
	}
	...
}

For SAM9X60, shouldn't this check AT91_SAM9X60_WDDIS instead?

Additionally, the wdt_enabled macro is hardcoded to check AT91_WDT_WDDIS
against wdt->mr. Since wdt->mr is initialized with AT91_SAM9X60_WDDIS for
SAM9X60 devices, the macro evaluates to true.

When wdt_enabled evaluates to true in sama5d4_wdt_init(), the block intended
to preserve the running state is skipped. The driver then writes wdt->mr
(which has the disable bit set) to the hardware. 

Does this silently disable the watchdog on SAM9X60 while leaving the core
believing it is running?

> -	ret = sama5d4_wdt_init(wdt);
> +	ret = sama5d4_wdt_init(wdt, wdd->timeout);
>  	if (ret)
>  		return ret;

[Severity: High]
This is also a pre-existing issue, but is there a race condition between
devm_request_irq() and platform_set_drvdata() in this probe function?

Earlier in sama5d4_wdt_probe(), devm_request_irq() is called with
IRQF_SHARED to register sama5d4_wdt_irq_handler():

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

If the line is shared and active, or if a spurious interrupt fires before
probe finishes, sama5d4_wdt_irq_handler() will be invoked:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_irq_handler() {
	struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
	u32 reg;

	if (wdt->sam9x60_support)
	...
}

However, platform_set_drvdata(pdev, wdt) is not called until the very
end of sama5d4_wdt_probe(). If the interrupt handler fires before this,
platform_get_drvdata() will return NULL, causing an immediate NULL pointer
dereference when accessing wdt->sam9x60_support.

Could platform_set_drvdata() be moved earlier in the probe sequence,
or devm_request_irq() moved later?

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