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

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

[Yann Sionneau]

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,

-- 

Yann

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

[Ruan Jinjie]

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

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

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Linus Walleij]

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

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

[Ruan Jinjie]

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