[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