[alsa-devel] [PATCH 1/2] ASoC: nuc900: Fix platform_get_irq() error checking some more

[Arvind Yadav]

Hi Dan,


On Monday 11 December 2017 02:10 PM, Dan Carpenter wrote:
> On Sun, Dec 10, 2017 at 08:10:26AM +0530, arvindY wrote:
>>
>> On Sunday 10 December 2017 07:22 AM, Dan Carpenter wrote:
>>> On Sat, Dec 09, 2017 at 06:27:32PM +0100, Alexandre Belloni wrote:
>>>>>> diff --git a/sound/soc/nuc900/nuc900-ac97.c b/sound/soc/nuc900/nuc900-ac97.c
>>>>>> index 5e4fbd2d3479..71fce7c85c93 100644
>>>>>> --- a/sound/soc/nuc900/nuc900-ac97.c
>>>>>> +++ b/sound/soc/nuc900/nuc900-ac97.c
>>>>>> @@ -345,11 +345,10 @@ static int nuc900_ac97_drvprobe(struct platform_device *pdev)
>>>>>>     		goto out;
>>>>>>     	}
>>>>>> -	nuc900_audio->irq_num = platform_get_irq(pdev, 0);
>>>>>> -	if (nuc900_audio->irq_num <= 0) {
>>>>>> -		ret = nuc900_audio->irq_num < 0 ? nuc900_audio->irq_num : -EBUSY;
>>>>>> +	ret = platform_get_irq(pdev, 0);
>>>>>> +	if (ret < 0)
>>>> The <= 0 was ok, see:
>>>> https://lkml.org/lkml/2017/11/18/41
>>>>
>>> Yeah, but is it ever going to return 0?  That seems like a design error
>>> and also really crap commenting if so
>> yes, It can return 0 on sprac platform and If you see the return of
>> platform_get_irq() 'return r ? r->start : -ENXIO;'. It should be
>> 'return r && r->start? r->start : -ENXIO;'. We can not add checks here,
>> Because There's a bunch of platforms in the kernel they still use IRQ0 as
>> valid.
>> I have separate mails where few maintainer ask me to add check for 0 and few
>> not.
>> Adding check for 0 will never harm.
> What you're saying doesn't make sense.
I am following a below link. Where they have point out irq 0 is not valid.
https://lwn.net/Articles/470820/
>
> You *can't* treat 0 as an error on Sparc because that's a valid IRQ.  In
> fact, it seems like if you want to write portable code you should never
> treated zero as an error.
>
> It doesn't make sense that someone would register an IRQ resource with
> an invalid IRQ number.  In other words, r->start is never going to be
> zero on a platform where that's invalid.
>
> So I'm pretty sure "if (ret < 0) " is the correct way to write code and
> "if (ret <= 0) " is incorrect but generally harmless except perhaps in
> limited situations on SPARC or other similar arches.
>
> regards,
> dan carpenter
>
~arvind