From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 88132EE49A6 for ; Mon, 21 Aug 2023 02:57:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232593AbjHUC5A (ORCPT ); Sun, 20 Aug 2023 22:57:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44708 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229908AbjHUC47 (ORCPT ); Sun, 20 Aug 2023 22:56:59 -0400 Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6821F9C for ; Sun, 20 Aug 2023 19:56:57 -0700 (PDT) Received: from kwepemi500008.china.huawei.com (unknown [172.30.72.55]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4RTcW41BlMzLp5k; Mon, 21 Aug 2023 10:53:52 +0800 (CST) Received: from [10.67.109.254] (10.67.109.254) by kwepemi500008.china.huawei.com (7.221.188.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Mon, 21 Aug 2023 10:56:53 +0800 Message-ID: <31cc1b22-188d-d01b-c370-9678c188d191@huawei.com> Date: Mon, 21 Aug 2023 10:56:53 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH -next v2 RESEND] I2C: Fix return value check for devm_pinctrl_get() Content-Language: en-US To: Andi Shyti , "Russell King (Oracle)" CC: Linus Walleij , , , , , , , Codrin Ciubotariu , Alexandre Belloni , Claudiu Beznea , Oleksij Rempel , Pengutronix Kernel Team , Shawn Guo , Fabio Estevam , NXP Linux Team , Wolfram Sang , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <20230818074509.295220-1-ruanjinjie@huawei.com> <20230818192034.bgjmurn2rp7ngyel@intel.intel> <20230819144533.dc3t777ggcnfq3rh@intel.intel> From: Ruan Jinjie In-Reply-To: <20230819144533.dc3t777ggcnfq3rh@intel.intel> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.109.254] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To kwepemi500008.china.huawei.com (7.221.188.139) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org 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