Re: [PATCH v2] watchdog: qcom: Propagate errors from optional IRQ lookup

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing initialization of `wdt->wdd.info` when the optional pretimeout IRQ is absent but pretimeout is supported.
- [Low] Redundant sequential calls to `platform_get_irq_optional()`.
--

--- Patch [1]: [PATCH v2] watchdog: qcom: Propagate errors from optional IRQ lookup ---
commit bf01bac4df56d04f8ab925257d4dfcb55e1e6adf
Author: bui duc phuc <[email protected]>

watchdog: qcom: Propagate errors from optional IRQ lookup

This commit modifies the qcom watchdog driver to propagate errors returned
by platform_get_irq_optional(), rather than only checking for -EPROBE_DEFER.

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

> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index a425902b9f68d..5c16420751631 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -305,18 +305,20 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  
>  	/* check if there is pretimeout support */
>  	irq = platform_get_irq_optional(pdev, 0);
> -	if (data->pretimeout && irq > 0) {
> +	if (data->pretimeout) {
> +		irq = platform_get_irq_optional(pdev, 0);

[Severity: Low]
This isn't a bug, but is the first call to platform_get_irq_optional() still
needed?

It appears that if data->pretimeout is true, the result is immediately 
overwritten by the second call. If data->pretimeout is false, the irq value is
ignored by the else branch.

> +		if (irq < 0 && irq != -ENXIO)
> +			return irq;
> +		if (irq > 0) {
> +			ret = devm_request_irq(dev, irq, qcom_wdt_isr, 0,
> +					       "wdt_bark", &wdt->wdd);
> +			if (ret)
> +				return ret;
> +
> +			wdt->wdd.info = &qcom_wdt_pt_info;
> +			wdt->wdd.pretimeout = 1;
> +		}

[Severity: High]
Does this code leave wdt->wdd.info uninitialized if data->pretimeout is true
but the optional IRQ is missing?

If platform_get_irq_optional() returns -ENXIO, the if (irq > 0) branch is
skipped. Since this all occurs inside the if (data->pretimeout) branch, the
outer else block is bypassed.

Could this cause devm_watchdog_register_device() to later fail with -EINVAL
because wdd->info is NULL?

>  	} else {
> -		if (irq == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> -
>  		wdt->wdd.info = &qcom_wdt_info;
>  	}

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