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.

Signed-off-by bui duc phuc <[email protected]>

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.

Signed-off-by bui duc phuc <[email protected]>

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.

Propagate errors such as -EPROBE_DEFER and -EINVAL instead of continuing probe without the IRQ.

Signed-off-by bui duc phuc <[email protected]>

---
 drivers/watchdog/aspeed_wdt.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/watchdog/aspeed_wdt.c +0 -0

index c9e79851504c..48454ac86074 100644

drivers/watchdog/aspeed_wdt.c +0 -0

 			ret = devm_request_irq(dev, irq, aspeed_wdt_irq,
 					       IRQF_SHARED, dev_name(dev),

drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_probe(struct platform_device *pdev) · line 434

Quoted 3 lines

drivers/watchdog/aspeed_wdt.c +2 -0

+             if (irq < 0 && irq != -ENXIO)
+                     return irq;
              if (irq > 0) {

This if () is no longer needed.

Hi Guenter,

Thank you for your review.

Quoted 4 lines

Quoted 3 lines

drivers/watchdog/aspeed_wdt.c +2 -0

+             if (irq < 0 && irq != -ENXIO)
+                     return irq;
               if (irq > 0) {

Quoted 2 lines

This if () is no longer needed.

If we remove the if (irq > 0) check, irq can be -ENXIO and would be passed to devm_request_irq().
I see that the irq argument of devm_request_irq() is an unsigned int. Would it be safe to pass -ENXIO to it in this case?

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

+                     return irq;
               if (irq > 0) {

This if () is no longer needed.

Quoted 5 lines

If we remove the if (irq > 0) check, irq can be -ENXIO and would be passed to devm_request_irq().
I see that the irq argument of devm_request_irq() is an unsigned int. Would it be safe to pass -ENXIO to it in this case?

No, sorry, I was wrong.

Hi Guenter,

Thank you for your feedback.

Quoted 11 lines

Quoted 11 lines

Quoted 5 lines

Quoted 2 lines

+                     return irq;
               if (irq > 0) {

This if () is no longer needed.

If we remove the if (irq > 0) check, irq can be -ENXIO and would be passed to devm_request_irq().
I see that the irq argument of devm_request_irq() is an unsigned int. Would it be safe to pass -ENXIO to it in this case?

Quoted 2 lines

No, sorry, I was wrong.

So this means there are no issues with this patch. Patches 2 and 3 have already been applied by you.
I will temporarily resend only patch 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.

Propagate errors such as -EPROBE_DEFER and -EINVAL instead of continuing probe without the IRQ.

Signed-off-by bui duc phuc <[email protected]>

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.

Signed-off-by bui duc phuc <[email protected]>

---
  drivers/watchdog/qcom-wdt.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/watchdog/qcom-wdt.c +0 -0

index 49bd04841f0c..a8eb1d8f24f0 100644

This is wrong. Check the if() path - the code ends up here if pretimeout == 0, even if irq > 0.

Hi Guenter,

Thanks for the review.

Quoted 4 lines

This is wrong. Check the if() path - the code ends up here if pretimeout == 0, even if irq > 0.

You're right. I misunderstood the condition in the if() path. I'll rewrite it as follows:

  irq = platform_get_irq_optional(pdev, 0);

if (data->pretimeout && irq > 0) {
.....
} else {
- if (irq == -EPROBE_DEFER)

-       return -EPROBE_DEFER;
   wdt->wdd.info = &qcom_wdt_info;

}

Best regards,
Phuc

Review

Quoted 3 lines

  irq = platform_get_irq_optional(pdev, 0);

(quoted diff) +2 -0

+if (irq < 0 && irq != -ENXIO)
+        return irq;

This is still wrong. If there is no pretimeout, it does not matter if there is an error.

Hi Guenter

Thank you for your review .

Quoted 6 lines

Review

Quoted 3 lines

   irq = platform_get_irq_optional(pdev, 0);

(quoted diff) +2 -0

+if (irq < 0 && irq != -ENXIO)
+        return irq;

This is still wrong. If there is no pretimeout, it does not matter if there is an error.

If checking data->pretimeout is required here, I'd propose one of the following approaches
let me know which one you'd prefer:

Option A (minimal diff, keep existing structure):
---------------
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;

         wdt->wdd.info = &qcom_wdt_pt_info;
         wdt->wdd.pretimeout = 1;
} else {
         if (data->pretimeout && irq < 0 && irq != -ENXIO)
                 return irq;

         wdt->wdd.info = &qcom_wdt_info;
}

------------------

Option B (check moved out, before the if/else):

------------------
irq = platform_get_irq_optional(pdev, 0);
if (data->pretimeout && irq < 0 && irq != -ENXIO)

         return irq;

if (data->pretimeout && 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;
} else {
         wdt->wdd.info = &qcom_wdt_info;
}

----------------------

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;
         }
}

-----------------------

Let me know which one you think fits best, or if you'd prefer something else.

I wpuld probably implement something like

        if (data->pretimeout) {
                irq = platform_get_irq_optional(pdev, 0);
                if (irq < 0 && irq != -ENXIO)
                        return irq;
                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;
        } else {
                wdt->wdd.info = &qcom_wdt_info;
        }

(which I think would be a combination of B and C) but ultimately it is POV and doesn't really matter.