Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

[Sergey Shtylyov <s.shtylyov@omp.ru>]

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.intel.com/
>>>
>>> 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