| Message ID | 20230817022018.3527570-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next,v2] I2C: Fix return value check for devm_pinctrl_get() | expand |
On Thu, Aug 17, 2023 at 10:20:17AM +0800, Ruan Jinjie wrote: > The devm_pinctrl_get() function returns error pointers and never > returns NULL. 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> > --- > 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 +- Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> Thank you!
On Thu, Aug 17, 2023 at 4:21 AM Ruan Jinjie <ruanjinjie@huawei.com> wrote: > The devm_pinctrl_get() function returns error pointers and never > returns NULL. 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> That's right. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thanks for fixing this Ruan! Yours, Linus Walleij
On 17/08/2023 at 04:20, Ruan Jinjie wrote: > The devm_pinctrl_get() function returns error pointers and never > returns NULL. Update the checks accordingly. > > Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery") Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional") > Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.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 >
> -----Original Message----- > From: Ruan Jinjie <ruanjinjie@huawei.com> > Sent: Wednesday, August 16, 2023 9:20 PM > To: linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Codrin > Ciubotariu <codrin.ciubotariu@microchip.com>; Andi Shyti > <andi.shyti@kernel.org>; Nicolas Ferre <nicolas.ferre@microchip.com>; > Alexandre Belloni <alexandre.belloni@bootlin.com>; Claudiu Beznea > <claudiu.beznea@tuxon.dev>; Oleksij Rempel <linux@rempel-privat.de>; > Pengutronix Kernel Team <kernel@pengutronix.de>; Shawn Guo > <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio > Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; > Wolfram Sang <wsa@kernel.org>; Linus Walleij <linus.walleij@linaro.org>; > Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Leo Li > <leoyang.li@nxp.com> > Cc: ruanjinjie@huawei.com > Subject: [PATCH -next v2] I2C: Fix return value check for devm_pinctrl_get() > > The devm_pinctrl_get() function returns error pointers and never returns > NULL. Update the checks accordingly. Not exactly. It can return NULL when CONFIG_PINCTRL is not defined. We probably should fix that API too. include/linux/pinctrl/consumer.h: static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev) { return NULL; } Regards, Leo > > 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> > --- > 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
Hi, Le 17/08/2023 à 19:30, Leo Li a écrit : >> The devm_pinctrl_get() function returns error pointers and never returns >> NULL. Update the checks accordingly. > Not exactly. It can return NULL when CONFIG_PINCTRL is not defined. We probably should fix that API too. > > include/linux/pinctrl/consumer.h: > static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev) > { > return NULL; > } So, as Leo pointed out it seems devm_pinctrl_get() can in fact return NULL, when CONFIG_PINCTRL is not defined. What do we do about this? Proposals: 1/ make sure all call sites of devm_pinctrl_get() do check for error with IS_ERR *and* check for NULL => therefore using IS_ERR_OR_NULL 2/ change the fallback implementation in include/linux/pinctrl/consumer.h to return ERR_PTR(-Esomething) (which errno?) 3/ another solution? Regards,
On 2023/8/18 7:07, Yann Sionneau wrote: > Hi, > > Le 17/08/2023 à 19:30, Leo Li a écrit : > >>> The devm_pinctrl_get() function returns error pointers and never returns >>> NULL. Update the checks accordingly. >> Not exactly. It can return NULL when CONFIG_PINCTRL is not defined. >> We probably should fix that API too. >> >> include/linux/pinctrl/consumer.h: >> static inline struct pinctrl * __must_check devm_pinctrl_get(struct >> device *dev) >> { >> return NULL; >> } > > So, as Leo pointed out it seems devm_pinctrl_get() can in fact return > NULL, when CONFIG_PINCTRL is not defined. > > What do we do about this? > > Proposals: > > 1/ make sure all call sites of devm_pinctrl_get() do check for error > with IS_ERR *and* check for NULL => therefore using IS_ERR_OR_NULL I think it's the best. > > 2/ change the fallback implementation in > include/linux/pinctrl/consumer.h to return ERR_PTR(-Esomething) (which > errno?) It seems a convention to return NULL if the related macro is not defined. > > 3/ another solution? Make I2C_IMX and I2C_AT91 config depends on PINCTRL config is another option. However it seems that the function call devm_pinctrl_get() has an optional recovery feature from the following notes and dev_info(). So this dependency is not necessary. 1378 /* 1379 * We switch SCL and SDA to their GPIO function and do some bitbanging 1380 * for bus recovery. These alternative pinmux settings can be 1381 * described in the device tree by a separate pinctrl state "gpio". If 1382 * this is missing this is not a big problem, the only implication is 1383 * that we can't do bus recovery. 1384 */ 1385 static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx, 1386 struct platform_device *pdev) 1387 { 1388 struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo; 1389 1390 i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev); 828 static int at91_init_twi_recovery_gpio(struct platform_device *pdev, 829 struct at91_twi_dev *dev) 830 { 831 struct i2c_bus_recovery_info *rinfo = &dev->rinfo; 832 833 rinfo->pinctrl = devm_pinctrl_get(&pdev->dev); 834 if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) { 835 dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n"); 836 return PTR_ERR(rinfo->pinctrl); 837 } > > Regards, >
On 2023/8/18 1:30, Leo Li wrote: > > >> -----Original Message----- >> From: Ruan Jinjie <ruanjinjie@huawei.com> >> Sent: Wednesday, August 16, 2023 9:20 PM >> To: linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Codrin >> Ciubotariu <codrin.ciubotariu@microchip.com>; Andi Shyti >> <andi.shyti@kernel.org>; Nicolas Ferre <nicolas.ferre@microchip.com>; >> Alexandre Belloni <alexandre.belloni@bootlin.com>; Claudiu Beznea >> <claudiu.beznea@tuxon.dev>; Oleksij Rempel <linux@rempel-privat.de>; >> Pengutronix Kernel Team <kernel@pengutronix.de>; Shawn Guo >> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio >> Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; >> Wolfram Sang <wsa@kernel.org>; Linus Walleij <linus.walleij@linaro.org>; >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Leo Li >> <leoyang.li@nxp.com> >> Cc: ruanjinjie@huawei.com >> Subject: [PATCH -next v2] I2C: Fix return value check for devm_pinctrl_get() >> >> The devm_pinctrl_get() function returns error pointers and never returns >> NULL. Update the checks accordingly. > > Not exactly. It can return NULL when CONFIG_PINCTRL is not defined. We probably should fix that API too. > > include/linux/pinctrl/consumer.h: > static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev) > { > return NULL; > } As ARCH_AT91 select PINCTRL and I2C_AT91 depends on ARCH_AT91, it can not be NULL except for I2C_AT91 is selected by COMPILE_TEST. And ARCH_MXC select PINCTRL and I2C_IMX depends on ARCH_MXC it can not be NULL except for I2C_IMX is selected by ARCH_LAYERSCAPE or COLDFIRE or COMPILE_TEST. In general, it is possible to be null. > > Regards, > Leo >> >> 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> >> --- >> 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 >
On Fri, Aug 18, 2023 at 01:07:27AM +0200, Yann Sionneau wrote: > Hi, > > Le 17/08/2023 à 19:30, Leo Li a écrit : > > > > The devm_pinctrl_get() function returns error pointers and never returns > > > NULL. Update the checks accordingly. > > Not exactly. It can return NULL when CONFIG_PINCTRL is not defined. We probably should fix that API too. > > > > include/linux/pinctrl/consumer.h: > > static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev) > > { > > return NULL; > > } > > So, as Leo pointed out it seems devm_pinctrl_get() can in fact return NULL, > when CONFIG_PINCTRL is not defined. > > What do we do about this? > > Proposals: > > 1/ make sure all call sites of devm_pinctrl_get() do check for error with > IS_ERR *and* check for NULL => therefore using IS_ERR_OR_NULL > > 2/ change the fallback implementation in include/linux/pinctrl/consumer.h to > return ERR_PTR(-Esomething) (which errno?) > > 3/ another solution? NULL is returned on purpose. 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(), only the commit message of these patches is a bit inaccurate. Sascha
On Fri, Aug 18, 2023 at 7:33 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > NULL is returned on purpose. 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(), only the commit message of these patches is a bit inaccurate. Sascha is spot on, maybe copyedit some of the above into the commit message and resend? Yours, Linus Walleij
On 2023/8/18 15:18, Linus Walleij wrote: > On Fri, Aug 18, 2023 at 7:33 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > >> NULL is returned on purpose. 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(), only the commit message of these patches is a bit inaccurate. > > Sascha is spot on, maybe copyedit some of the above > into the commit message and resend? OK! I'll resend it. > > Yours, > Linus Walleij
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); }
The devm_pinctrl_get() function returns error pointers and never returns NULL. 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> --- 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(-)