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

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

[Andy Shevchenko]

fwnode_irq_get() may return all possible signed values, such as Linux
error code. Fix the code to handle this properly.

Fixes: a1d1e0e3d80a ("pinctrl: nuvoton: Add driver for WPCM450")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c b/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
index 0dbeb91f0bf2..8193b92da403 100644
--- a/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
+++ b/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
@@ -1081,10 +1081,13 @@ static int wpcm450_gpio_register(struct platform_device *pdev,
 
 		girq->num_parents = 0;
 		for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {
-			int irq = fwnode_irq_get(child, i);
+			int irq;
 
+			irq = fwnode_irq_get(child, i);
 			if (irq < 0)
 				break;
+			if (!irq)
+				continue;
 
 			girq->parents[i] = irq;
 			girq->num_parents++;
-- 
2.35.1

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

[Jonathan Neuschäfer]

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

Hello,

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.

> Fixes: a1d1e0e3d80a ("pinctrl: nuvoton: Add driver for WPCM450")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/nuvoton/pinctrl-wpcm450.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c b/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
> index 0dbeb91f0bf2..8193b92da403 100644
> --- a/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
> +++ b/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
> @@ -1081,10 +1081,13 @@ static int wpcm450_gpio_register(struct platform_device *pdev,
>  
>  		girq->num_parents = 0;
>  		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)

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



Thanks,
Jonathan

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

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

[Andy Shevchenko]

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

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

[Jonathan Neuschäfer]

[-- 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 --]