Re: [PATCH v1 1/1] pinctrl: wpcm450: Correct the fwnode_irq_get() return value check

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 2795 bytes --]

Hello,


On Thu, Sep 08, 2022 at 01:01:32PM +0300, Andy Shevchenko wrote:
> 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.

My problem with the description is that handling "all possible signed
values" is fairly meaningless: The code arguably did that already, it
did *something* for every possible value. The significant change of
your patch is that the value zero is handled differently.

IOW, what I miss is something along the lines of: "fwnode_irq_get can
return zero to indicate some errors. Handle this case like other errors."

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

Ok, fair enough.


> 
> > >  			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;

Right, hence why I asked for reasoning why zero should be handled
the way you propose to handle it.

>  2) we might need to convert 0 to error if we ever go to report this

Good point.

> 
> 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.

However: In wpcm450_gpio_register, there is currently no reporting for
mapping errors in this loop.

I'm fine with either break or continue.


Thanks,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]