Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2021/12/11 19:25, Sergey Shtylyov wrote:
> Hello!
> 
> On 11.12.2021 2:45, Damien Le Moal wrote:
> 
> [...]
>>>>> platform_get_irq() will print a message when it fails.
>>>>> No need to repeat this.
>>>>>
>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills
>>>>> out a big WARN() in such case.
>>>>
>>>> The reason you should be able to remove the "if (!irq)" test is that
>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc
>>>> says. But looking at platform_get_irq_optional(), which is called by
>>>> platform_get_irq(), the out label is:
>>>>
>>>> 	WARN(ret == 0, "0 is an invalid IRQ number\n");
>>>> 	return ret;
>>>>
>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to
>>>> return -ENXIO:
>>>>
>>>> 	if (WARN(ret == 0, "0 is an invalid IRQ number\n"))
>>>> 		return -ENXIO;
>>>> 	return ret;
>>>
>>>     My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this
>>> but returns -EINVAL instead.
>>
>> Thinking more about this, shouldn't this change go into platform_get_irq()
>> instead of platform_get_irq_optional() ?
> 
>     Why? platform_get_irq() currently just calls platform_get_irq_optional()...
> 
>> The way I see it, I think that the intended behavior for
>> platform_get_irq_optional() is:
>> 1) If have IRQ, return it, always > 0
>> 2) If no IRQ, return 0
> 
>     That does include the IRQ0 case, right?

IRQ 0 being invalid, I think that case should be dealt with internally within
platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus
be case (3), an error.

> 
>> 3) If error, return < 0
>> no ?
> 
>    I completely agree, I (after thinking a bit) have no issues with that...
> 
>> And for platform_get_irq(), case (2) becomes an error.
>> Is this the intended semantic ?
> 
>     I don't see how it's different from the current behavior. But we can do 
> that as well, I just don't see whether it's really better...

The problem I see is that the current behavior is unclear: what does
platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think
it should be the latter rather than the former. Note that the function could
return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away,
but then I do not see any difference between platform_get_irq_optional() and
platform_get_irq().

If the preferred API semantic is to allow returning IRQ 0 with a warning, then
the kdoc comments of platform_get_irq_optional() and platform_get_irq() are
totally broken, and the code for many drivers is probably wrong too.

> 
>> I am really not sure here as the functions kdoc description and the code do not
>> match. Which one is correct ?
> 
>     It seems both are wrong. :-)
> 
> [...]
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research