Re: [PATCH 2/2] watchdog: stm32_iwdg: Propagate -EPROBE_DEFER from IRQ lookup

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

New issues:
- [High] Unexpected system reset due to probe deferral while the hardware watchdog is already running.

Pre-existing issues:
- [High] Leaked wake_irq configuration on driver unbind or probe failure causes permanent reprobe failures.
- [High] The driver requests the IRQ before initializing the driver data used by the interrupt handler, creating a race condition that can result in a NULL pointer dereference.
--

--- Patch [2]: [PATCH 2/2] watchdog: stm32_iwdg: Propagate -EPROBE_DEFER from IRQ lookup ---
commit dc7c8342d9ed50f0391276fabd2f26a71397adae
Author: bui duc phuc <[email protected]>

watchdog: stm32_iwdg: Propagate -EPROBE_DEFER from IRQ lookup

platform_get_irq_optional() does not return 0. It returns a positive IRQ
number on success or a negative error code on failure. Return -EPROBE_DEFER
from platform_get_irq_optional() so the driver is re-probed.

> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
> index b356a272ff9a0..102f510849578 100644
> --- a/drivers/watchdog/stm32_iwdg.c
> +++ b/drivers/watchdog/stm32_iwdg.c
> @@ -287,7 +287,9 @@ static int stm32_iwdg_irq_init(struct platform_device *pdev,
>  		return 0;
>  
>  	irq = platform_get_irq_optional(pdev, 0);
> -	if (irq <= 0)
> +	if (irq == -EPROBE_DEFER)
> +		return irq;

[Severity: High]
If the hardware watchdog is already running, won't returning -EPROBE_DEFER
here cause a regression by triggering an unexpected system reset?

If the bootloader started the watchdog, the hardware timer is actively
counting down. Because this deferral happens before the driver registers
with the watchdog core or pings the device, the probe will abort. Since
deferred probing can take an unpredictable amount of time, can the watchdog
expire and reset the system before the driver is successfully reprobed?

> +	if (irq < 0)
>  		return 0;
>  
>  	if (of_property_read_bool(np, "wakeup-source")) {
>  		ret = devm_device_init_wakeup(dev);
>  		if (ret)
>  			return ret;
>  
>  		ret = dev_pm_set_wake_irq(dev, irq);

[Severity: High]
This is a pre-existing issue, but does this leak the wake_irq configuration
on driver unbind or probe failure?

Since dev_pm_set_wake_irq() is not device-managed and lacks an explicit
cleanup via dev_pm_clear_wake_irq(), dev->power.wakeirq remains populated.
If a later initialization step fails or the driver is unbound, won't any
subsequent probe attempt hit a warning for an already initialized wake irq
and return -EEXIST, permanently failing reprobes?

>  		if (ret)
>  			return ret;
>  	}
>  
>  	ret = devm_request_irq(dev, irq, stm32_iwdg_isr, 0,
>  			       dev_name(dev), wdd);

[Severity: High]
This is a pre-existing issue, but can this cause a NULL pointer dereference
if the interrupt fires immediately?

In stm32_iwdg_probe(), the interrupt is requested here before driver data
is initialized:

	ret = stm32_iwdg_irq_init(pdev, wdt);
	if (ret)
		return ret;

	watchdog_set_drvdata(wdd, wdt);

If the interrupt fires before watchdog_set_drvdata() is executed, the
interrupt handler will read NULL driver data:

stm32_iwdg_isr() {
	struct watchdog_device *wdd = wdog_arg;
	struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd);
	u32 reg;

	reg = reg_read(wdt->regs, IWDG_EWCR);

Since wdt is NULL, won't dereferencing wdt->regs crash the kernel?

>  	if (ret)
>  		return ret;
>  
>  	wdd->info = &stm32_iwdg_preinfo;
>  	return 0;
>  }

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