Re: [PATCH -next v2 RESEND] I2C: Fix return value check for devm_pinctrl_get()

[Andi Shyti <andi.shyti@kernel.org>]

Hi Russel,

> > > > >       i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > > > -     if (!i2c_imx->pinctrl || IS_ERR(i2c_imx->pinctrl)) {
> > > > > +     if (IS_ERR(i2c_imx->pinctrl)) {
> > > > >               dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> > > > >               return PTR_ERR(i2c_imx->pinctrl);
> > > > >       }
> > > >
> > > > I haven't looked at the AT91 version, but... isn't the original code
> > > > entirely correct?
> > > >
> > > > If pinctrl is not available (thus devm_pinctrl_get() returns NULL) then
> > > > recovery can't work, because we can't switch the I2C pins between the
> > > > I2C controller and GPIO. So, isn't it quite correct to print
> > > > "can't get pinctrl, bus recovery not supported" because the I2C bus
> > > > can't be recovered without pinctrl?
> > > >
> > > > The PTR_ERR() is also fine - because if pinctrl is not present and
> > > > returns NULL, we'll end up returning zero, which is exactly what we
> > > > want.
> > > 
> > > Oh, you're probably absolutely right about that.
> > > 
> > > > The alternative would be to open code that, maybe with a more accurate
> > > > message:
> > > >
> > > >         if (!i2c_imx->pinctrl) {
> > > >                 dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
> > > >                 return 0;
> > > >         }
> > > >         if (IS_ERR(i2c_imx->pinctrl) {
> > > >                 ...
> > > 
> > > This is a way better patch. It makes the implicit explicit.
> > 
> > we could also use
> > 
> > 	if (IS_ERR_OR_NULL(i2c_imx->pinctrl))
> > 		...
> > 
> > without changing any logic in the driver.
> 
> IS_ERR_OR_NULL() - is a macro I personally hate, it causes a lot of
> trouble. I have mutt setup to mark IS_ERR_OR_NULL with a red background
> so it stands out in patches. It is utterly evil, and I really wish we
> could get rid of that damn macro.
> 
> It also looks wrong.
> 
> 	if (IS_ERR_OR_NULL(x))
> 		return PTR_ERR(x);
> 
> rings alarm bells for some people, because if x is NULL, then
> PTR_ERR(x) is zero.
> 
> While this may be what is intended in this case, for a great many
> places in the kernel, this is a bug. So I can guarantee that
> _someone_ will come along and want to "fix" that to make the NULL
> case return an error code, and in doing so end up breaking the
> driver.
> 
> So... no, just don't.
> 
> This is why having two if() statements are a good idea, and is
> what Linus means by "making the implicit explicit" - because it
> then becomes absolutely obvious what we want to do in the NULL
> case, and what we want to do in the error case.
> 
> There is none of this ambiguity that I point out above.

Yes, I fully agree, IS_ERR_OR_NULL() shoud be almost never be
used in an exit path (unless you are in a void function and few
other cases, like (borderline) this one).

I'm OK also if Ruan goes with what you suggested.

Andi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel