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

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

[Ruan Jinjie]

Though the devm_pinctrl_get() function returns error pointers when
CONFIG_PINCTRL is defined and NULL otherwise. NULL is returned
on purpose for devm_pinctrl_get(). When PINCTRL is disabled NULL becomes
a valid pinctrl cookie which can be passed to the other stub functions.
With this drivers using pinctrl can get through their probe function
without an error when PINCTRL is disabled.

The same approach is taken by the clk and regulator API. It is correct
to test the return value of devm_pinctrl_get() with IS_ERR(). So update
the checks accordingly.

Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery")
Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional")
Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
v2:
- Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving them behind.
- Update the commit title and message.
---
 drivers/i2c/busses/i2c-at91-master.c | 2 +-
 drivers/i2c/busses/i2c-imx.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index 94cff1cd527e..2bf1df5ef473 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -831,7 +831,7 @@ static int at91_init_twi_recovery_gpio(struct platform_device *pdev,
 	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
 
 	rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
-	if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
+	if (IS_ERR(rinfo->pinctrl)) {
 		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
 		return PTR_ERR(rinfo->pinctrl);
 	}
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 10e89586ca72..05d55893f04e 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1388,7 +1388,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
 
 	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);
 	}
-- 
2.34.1


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

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

[Russell King (Oracle)]

On Fri, Aug 18, 2023 at 03:45:08PM +0800, Ruan Jinjie wrote:
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 10e89586ca72..05d55893f04e 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1388,7 +1388,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>  	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>  
>  	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.

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

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

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

[Linus Walleij]

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.

Yours,
Linus Walleij

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

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

[Andi Shyti]

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.

Andi

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

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

[Russell King (Oracle)]

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

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

[Andi Shyti]

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

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

[Ruan Jinjie]

On 2023/8/19 22:45, Andi Shyti wrote:
> 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.

I'll do as what Russel suggested. Thank you!

> 
> Andi

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