Re: [PATCH v3] watchdog: orion_wdt: Propagate errors from optional IRQ lookup

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

Pre-existing issues:
- [High] Probe deferral of optional IRQs causes a running watchdog to reset the system.
- [High] Race condition in driver removal and error paths allows IRQ handler to access unclocked registers.
--

--- Patch [1]: [PATCH v3] watchdog: orion_wdt: Propagate errors from optional IRQ lookup ---
commit 18f3c178650daa4e1cc407d2909bc28318084819
Author: bui duc phuc <[email protected]>

watchdog: orion_wdt: Propagate errors from optional IRQ lookup

This patch updates the orion_wdt driver to propagate errors returned by
platform_get_irq_optional() instead of continuing without an IRQ. It routes
errors like -EPROBE_DEFER and -EINVAL to the disable_clk error handling path.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index 1802634cc7cda..0ea946823b0a7 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -606,6 +606,10 @@ static int orion_wdt_probe(struct platform_device *pdev)
>  
>  	/* Request the IRQ only after the watchdog is disabled */
>  	irq = platform_get_irq_optional(pdev, 0);
> +	if (irq < 0 && irq != -ENXIO) {
> +		ret = irq;
> +		goto disable_clk;

[Severity: High]
This is a pre-existing issue, but does returning -EPROBE_DEFER here cause an
unintended system reboot if the hardware watchdog was left running by the
bootloader?

If orion_wdt_probe() detects the hardware is running and sets
WDOG_HW_RUNNING, but then defers probe here, the watchdog core won't
register the device or ping it. The driver jumps to the disable_clk label,
but on many Armada platforms the shared clock doesn't physically stop.
The timer will continue ticking and eventually forcefully reset the SoC.

> +	}
>  	if (irq > 0) {
>  		/*
>  		 * Not all supported platforms specify an interrupt for the

[ ... ]

> @@ -619,6 +623,10 @@ static int orion_wdt_probe(struct platform_device *pdev)
>  
>  	/* Optional 2nd interrupt for pretimeout */
>  	irq = platform_get_irq_optional(pdev, 1);
> +	if (irq < 0 && irq != -ENXIO) {
> +		ret = irq;
> +		goto disable_clk;

[Severity: High]
This isn't a bug introduced by this patch, but does the disable_clk error
path leave a window for a fatal bus fault?

The driver manages interrupts using devm_request_irq(), meaning devres
cleanup runs after orion_wdt_probe() returns. By manually calling
clk_disable_unprepare() in the disable_clk path before the IRQ is
unregistered, the interrupts remain active while the clock is gated.

If the pre-timeout interrupt fires during this window, orion_wdt_pre_irq()
will access registers of a clock-gated peripheral, which causes a
Synchronous External Abort on ARM architectures. The same race exists in
orion_wdt_remove().

> +	}
>  	if (irq > 0) {
>  		orion_wdt_info.options |= WDIOF_PRETIMEOUT;
>  		ret = devm_request_irq(&pdev->dev, irq, orion_wdt_pre_irq,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1