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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 50CADEE49A3 for ; Sat, 19 Aug 2023 14:45:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=vgAa4v/Om/vReIE1J3p/jUTXdVmPxjzOXOXYDYRv+Xw=; b=1Glt+z8DvR5wn4 9gzMeP5CrDXseDHWRR5YpjD3ApClJghXMJ4LDOk0kET8iFmA0WmZLUiDtF5mMsm25tNgb2/RaOO6V KfEW8uf/6OkVW249XiOOkAebxak1K/a+AzuNplc0DaiO6/CaO5tKIfPkqmt3ifJAIfr30/18fTLbx KAelah2uawbkqtkZovsaYj0Eyw8jurI2YrkELj9I9gav7zUYLftMDq5fZoMpySUg0hChHWLl2pBUE sr/1jMq2SOSNJwuqa/uw0gFlO0pD3Y230IQ649D1HpHF0MHNI5S1h1vHzW5JTW6q9WJv8rxPwUpon VFzJvVAVEBpWE2dDKPQA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qXNCu-00Ayus-1q; Sat, 19 Aug 2023 14:45:40 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qXNCs-00AyuZ-1j for linux-arm-kernel@lists.infradead.org; Sat, 19 Aug 2023 14:45:39 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id DB1CC60FD3; Sat, 19 Aug 2023 14:45:37 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 986DBC433C7; Sat, 19 Aug 2023 14:45:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1692456337; bh=rovHH8cqXWceDGBoXOGHUjf5ibZD9QoBrvgIUJLlbgs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hKqXh36537x/f4OTuAYnWAYSbAyRIXTICjrqzgMT5tgyE4lZGXVeBvkj6KIzfH4NM iaomUXVQaU3eoe7KBl5cfNQD52E7x/jZN+qo4GuNKkC42x5+yKE9HapR15nVBMhbRc RY7dvI8JUqgC8gKGXRodJbM1CemvbWzSiNI9ImFKnKrEN6O/uKTAvbtbQLfPu3Fd3c OzjSjwf1175s7yuvPqZ2ljUk6UToxgUcghgwipMEb+2QP3U0l7tjQNVOKb98pu0Gug HEPRbRpe8umd8u623QDb1H+qot4a7oeBIBSJVeEELR3OHLB8iod/mE01nbEBJahVvx L7EjPC3Gcctvw== Date: Sat, 19 Aug 2023 16:45:33 +0200 From: Andi Shyti To: "Russell King (Oracle)" Subject: Re: [PATCH -next v2 RESEND] I2C: Fix return value check for devm_pinctrl_get() Message-ID: <20230819144533.dc3t777ggcnfq3rh@intel.intel> References: <20230818074509.295220-1-ruanjinjie@huawei.com> <20230818192034.bgjmurn2rp7ngyel@intel.intel> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230819_074538_685612_F754408B X-CRM114-Status: GOOD ( 31.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , s.hauer@pengutronix.de, Fabio Estevam , Linus Walleij , Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= , leoyang.li@nxp.com, Wolfram Sang , o.rempel@pengutronix.de, linux-i2c@vger.kernel.org, Pengutronix Kernel Team , Claudiu Beznea , Codrin Ciubotariu , Oleksij Rempel , Shawn Guo , Ruan Jinjie , linux-arm-kernel@lists.infradead.org, NXP Linux Team Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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. Andi _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel