On 1/19/22 6:02 PM, Uwe Kleine-König wrote:
[...]
This patch is based on the former Andy Shevchenko's patch:
https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux....
Currently platform_get_irq_optional() returns an error code even if IRQ resource simply has not been found. It prevents the callers from being error code agnostic in their error handling:
ret = platform_get_irq_optional(...); if (ret < 0 && ret != -ENXIO) return ret; // respect deferred probe if (ret > 0) ...we get an IRQ...
All other *_optional() APIs seem to return 0 or NULL in case an optional resource is not available. Let's follow this good example, so that the callers would look like:
ret = platform_get_irq_optional(...); if (ret < 0) return ret; if (ret > 0) ...we get an IRQ...
Reported-by: Matthias Schiffer matthias.schiffer@ew.tq-group.com Signed-off-by: Sergey Shtylyov s.shtylyov@omp.ru
[...]
Please don't merge this as yet, I'm going thru this patch once again and have already found some sloppy code. :-/
Who would you expect to merge this? I would have expected Greg, but he
Me too, it's his area, the message was addressed to Greg KH...
seems to have given up this thread.
You instill too much uncertainty in him. :-)
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c index 7450904e330a..fdc63bfa5be4 100644 --- a/drivers/char/ipmi/bt-bmc.c +++ b/drivers/char/ipmi/bt-bmc.c @@ -382,12 +382,14 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc, bt_bmc->irq = platform_get_irq_optional(pdev, 0); if (bt_bmc->irq < 0) return bt_bmc->irq;
- if (!bt_bmc->irq)
return 0;Hm, this is sloppy. Will recast and rebase to the -next branch.
I didn't think about what you mean with sloppy, but the code is equivalent to
if (bt_bmc->irq <= 0) return bt_bmc->irq;
Exactly.
[...]
diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c index 2ccd1db5e98f..0d1bdd27cd78 100644 --- a/drivers/edac/xgene_edac.c +++ b/drivers/edac/xgene_edac.c @@ -1917,7 +1917,7 @@ static int xgene_edac_probe(struct platform_device *pdev)
for (i = 0; i < 3; i++) { irq = platform_get_irq_optional(pdev, i);Is *_optinal() even correct here?
_optinal isn't correct, _optional maybe is. :-)
No. :-)
Anyhow, look at e26124cd5f7099949109608845bba9e9bf96599c, the driver was fixed not to print two error messages and the wrong option was picked.
I think this patch is wrong...
if (irq < 0) {
if (irq <= 0) { dev_err(&pdev->dev, "No IRQ resource\n");
This is what needed to be thrown overboard... :-)
rc = -EINVAL; goto out_err;What's wrong here is that the return code is hardcoded ...
This is wrong as well -- kills the deferred probing. I have 2 separate patches for this driver now... just need some time to get 'em ready...
[...]
index bdf924b73e47..51289700a7ac 100644 --- a/drivers/power/supply/mp2629_charger.c +++ b/drivers/power/supply/mp2629_charger.c @@ -581,9 +581,9 @@ static int mp2629_charger_probe(struct platform_device *pdev) platform_set_drvdata(pdev, charger);
irq = platform_get_irq_optional(to_platform_device(dev->parent), 0);
Again, is *_optional() even correct here?
- if (irq < 0) {
- if (irq <= 0) { dev_err(dev, "get irq fail: %d\n", irq);
return irq;
return irq < 0 ? irq : -ENXIO;Ack, could be simplified by switching to platform_get_irq().
Have a draft patch...
Best regards Uwe
MBR, Sergey