watchdog: qcom: Propagate errors from optional IRQ lookup
Subsystems (2)
PATCH v2 1/1 watchdog: qcom: Propagate errors from optional IRQ lookup
From: bui duc phuc <[email protected]>
platform_get_irq_optional() returns a positive IRQ number on success or a negative error code on failure. For an optional IRQ, -ENXIO indicates that no IRQ is available, while other errors should be propagated.
Instead of only checking for -EPROBE_DEFER, propagate all error codes returned by platform_get_irq_optional() other than -ENXIO, so that failures are properly reported to the caller.
---
Link v1 :
https://lore.kernel.org/all/[email protected]/
Changes in v2 :
- Update pretimeout handling based on Guenter's review.
drivers/watchdog/qcom-wdt.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
drivers/watchdog/qcom-wdt.c +12 -10
@@ -289,18 +289,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 ) { - ret =devm_request_irq (dev, irq, qcom_wdt_isr,0 , -"wdt_bark" , &wdt->wdd); -if (ret) -return ret; +if (data->pretimeout) { + irq =platform_get_irq_optional (pdev,0 ); +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 ; + wdt->wdd.info = &qcom_wdt_pt_info; + wdt->wdd.pretimeout =1 ; + } }else { -if (irq == -EPROBE_DEFER ) -return -EPROBE_DEFER ; - wdt->wdd.info = &qcom_wdt_info; }
Signature · 1 line
2.43.0
[email protected]
AI reviewThank 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…
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.
drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 309
Quoted 11 lines
drivers/watchdog/qcom-wdt.c +2
-1@@ -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.
drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 304
Quoted 11 lines
drivers/watchdog/qcom-wdt.c +11
-0+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?
Quoted 6 lines
drivers/watchdog/qcom-wdt.c +0
-3}else { -if (irq == -EPROBE_DEFER ) -return -EPROBE_DEFER ; - wdt->wdd.info = &qcom_wdt_info; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 18 lines
[Severity: High]
Does this code leavewdt->wdd.infouninitialized ifdata->pretimeoutis 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-EINVALbecausewdd->infois NULL?drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 307
Quoted 6 lines
} else {drivers/watchdog/qcom-wdt.c +0
-3-if (irq == -EPROBE_DEFER ) -return -EPROBE_DEFER ; - wdt->wdd.info = &qcom_wdt_info; }
Quoted 2 lines
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Oh, it looks like this approach is correct.
Option C (default-then-override, only look up the IRQ when pretimeout is supported):
----------------------wdt->wdd.info = &qcom_wdt_info;
if (data->pretimeout) {
irq = platform_get_irq_optional(pdev, 0);
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;
}
}-----------------------
drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 293
Quoted 9 lines
/* check if there is pretimeout support */ irq = platform_get_irq_optional(pdev, 0);drivers/watchdog/qcom-wdt.c +2
-5-if (data->pretimeout && irq >0 ) { - ret =devm_request_irq (dev, irq, qcom_wdt_isr,0 , -"wdt_bark" , &wdt->wdd); -if (ret) -return ret; +if (data->pretimeout) { + irq =platform_get_irq_optional (pdev,0 );
Sorry, I missed removing the old irq = platform_get_irq_optional(pdev, 0); line. I will send v3 shortly.