linux-kernel - Re: [PATCH v1 1/1] pinctrl: wpcm450: Correct the fwnode_irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Sep 2022 13:01:32 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jonathan Neuschäfer <j.neuschaefer@....net>
Cc:     Linus Walleij <linus.walleij@...aro.org>, openbmc@...ts.ozlabs.org,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] pinctrl: wpcm450: Correct the fwnode_irq_get()
 return value check

On Wed, Sep 07, 2022 at 11:04:40PM +0200, Jonathan Neuschäfer wrote:
> On Mon, Sep 05, 2022 at 10:14:08PM +0300, Andy Shevchenko wrote:
> > fwnode_irq_get() may return all possible signed values, such as Linux
> > error code. Fix the code to handle this properly.
> 
> It would be good to note explicitly here what a return value of zero
> means, i.e., as the documentation of of_irq_get says, "IRQ mapping
> failure", and why it should result in skipping this IRQ.

Not that I'm fun of duplicating documentation in the commit message,
but it won't take much in this case.

...

> >  		for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {
> > -			int irq = fwnode_irq_get(child, i);
> > +			int irq;
> >  
> > +			irq = fwnode_irq_get(child, i);

> (Unneccesary churn, but I'll allow it)

The point here is to see that we actually check something that we just got
from somewhere else. It's slightly better for reading and maintaining the
code as I explained in [1].

And there is a difference to the cases like

static int foo(struct platform_device *pdev, ...)
{
	struct device *dev = &pdev->dev;
	...
}

when we know ahead that if pdev is NULL, something is _so_ wrong that
it's not even our issue.

[1]: https://lore.kernel.org/lkml/CAHp75Vda5KX5pVrNeueQEODoEy405eTb9SYJtts-Lm9jMNocHQ@mail.gmail.com/

> >  			if (irq < 0)
> >  				break;
> > +			if (!irq)
> > +				continue;
> 
> Since irq == 0 seems to be an error condition, the following seems fine
> to me, and simpler:
> 
> -			if (irq < 0)
> +			if (irq <= 0)
>  				break;

Not sure it's the same by two reasons:
 1) break != continue;
 2) we might need to convert 0 to error if we ever go to report this

So, to me mapping error shouldn't be fatal to continue, but I would
like to hear your interpretation since you know this case much better
than me.

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko