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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Aug 18, 2023 at 09:20:34PM +0200, Andi Shyti wrote:
> Hi,
> 
> On Fri, Aug 18, 2023 at 06:42:11PM +0200, Linus Walleij wrote:
> > On Fri, Aug 18, 2023 at 10:20 AM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > > On Fri, Aug 18, 2023 at 03:45:08PM +0800, Ruan Jinjie wrote:
> > 
> > > >       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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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