| Message ID | 20230817022018.3527570-1-ruanjinjie@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [-next,v2] I2C: Fix return value check for devm_pinctrl_get() | expand |
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 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 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(-)