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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

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