Re: [PATCH] watchdog: sama5d4_wdt: use platform_get_irq()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Assigning the signed result of `platform_get_irq()` to an unsigned `u32` variable causes the `irq < 0` error check to always evaluate to false, breaking the driver's fallback mechanism.
- [Medium] Using `platform_get_irq()` instead of `platform_get_irq_optional()` for an optional IRQ causes spurious error messages to be logged.

Pre-existing issues:
- [High] The driver registers a shared interrupt handler (`IRQF_SHARED`) before setting the platform driver data, creating a race condition that can lead to a NULL pointer dereference.
--

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

watchdog: sama5d4_wdt: use platform_get_irq()

irq_of_parse_and_map() requires irq_dispose_mapping() on failure. Don't
bother with it as platform_get_irq doesn't need it.

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 13e72918338a2..d9b212557ce36 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -286,8 +286,8 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	if (wdt->need_irq) {
> -		irq = irq_of_parse_and_map(dev->of_node, 0);
> -		if (!irq) {
> +		irq = platform_get_irq(pdev, 0);
> +		if (irq < 0) {

[Severity: High]
Since irq is declared as an unsigned u32 earlier in sama5d4_wdt_probe(), won't
this irq < 0 check always evaluate to false?

platform_get_irq() returns a negative error code (like -ENXIO) on failure,
which will be implicitly cast to a large positive integer. Instead of
gracefully falling back by setting wdt->need_irq = false, won't the driver
proceed to call devm_request_irq() with an invalid IRQ number and fail the
probe?

[Severity: Medium]
Will this log spurious error messages when the IRQ is genuinely absent?

The driver treats the IRQ as optional and handles its absence gracefully.
However, platform_get_irq() internally calls dev_err_probe(), printing a
core-level error message to the kernel log when an IRQ is not found. Should
this use platform_get_irq_optional() instead to avoid confusing user-visible
error logs during boot?

>  			dev_warn(dev, "failed to get IRQ from DT\n");
>  			wdt->need_irq = false;
>  		}

[Severity: High]
This is a pre-existing issue, but does requesting a shared IRQ before setting
the platform driver data create a race condition?

A few lines later in sama5d4_wdt_probe(), the driver requests the IRQ:

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

Because the IRQ is requested with IRQF_SHARED, another device sharing the line
or a spurious interrupt could cause sama5d4_wdt_irq_handler() to execute
immediately.

The interrupt handler calls platform_get_drvdata() and dereferences it:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_irq_handler() {
	struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
	...
	if (wdt->sam9x60_support)
	...
}

However, platform_set_drvdata(pdev, wdt) is only called at the very end of
sama5d4_wdt_probe(). If the interrupt fires within this window, won't
platform_get_drvdata() return NULL and cause a NULL pointer dereference?

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