| Message ID | 20230818074509.295220-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next,v2,RESEND] I2C: Fix return value check for devm_pinctrl_get() | expand |
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) { ...
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
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
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.
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
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
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); }
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(-)