Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-929-g09f3e68182-fm-20220908.004-g09f3e681
Mime-Version: 1.0
Message-Id: <91244c88-d909-4e19-a058-b4f110e42ed7@www.fastmail.com>
In-Reply-To: <YyAx17VNvDMyvgBV@shell.armlinux.org.uk>
References: <20220913065910.15348-1-tangbin@cmss.chinamobile.com>
 <YyAx17VNvDMyvgBV@shell.armlinux.org.uk>
Date:   Tue, 13 Sep 2022 10:29:08 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Russell King" <linux@armlinux.org.uk>,
        "Tang Bin" <tangbin@cmss.chinamobile.com>
Cc:     hongxing.zhu@nxp.com, l.stach@pengutronix.de,
        "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, robh@kernel.org,
        kw@linux.com, "Shawn Guo" <shawnguo@kernel.org>,
        bhelgaas@google.com, "Sascha Hauer" <s.hauer@pengutronix.de>,
        "Pengutronix Kernel Team" <kernel@pengutronix.de>,
        "Fabio Estevam" <festevam@gmail.com>,
        "NXP Linux Team" <linux-imx@nxp.com>, linux-pci@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: imx6: Fix wrong check in imx6_pcie_attach_pd()
Content-Type: text/plain
Precedence: bulk

On Tue, Sep 13, 2022, at 9:31 AM, Russell King (Oracle) wrote:
> On Tue, Sep 13, 2022 at 02:59:10PM +0800, Tang Bin wrote:
>> @@ -352,8 +352,8 @@ static int imx6_pcie_attach_pd(struct device *dev)
>>  	}
>>  
>>  	imx6_pcie->pd_pcie_phy = dev_pm_domain_attach_by_name(dev, "pcie_phy");
>> -	if (IS_ERR(imx6_pcie->pd_pcie_phy))
>> -		return PTR_ERR(imx6_pcie->pd_pcie_phy);
>> +	if (IS_ERR_OR_NULL(imx6_pcie->pd_pcie_phy))
>> +		return PTR_ERR(imx6_pcie->pd_pcie_phy) ? : -ENODATA;
>>  
>>  	link = device_link_add(dev, imx6_pcie->pd_pcie_phy,
>>  			DL_FLAG_STATELESS |
>
> This change is unnecessary. If dev_pm_domain_attach_by_name() returns
> Null, then device_link_add() will also return NULL, and the check for
> a NULL link will then succeed. So the NULL case is already cleanly
> handled.
>
> So overall, your patch is unnecessary and introduces a bug rather than
> fixing it. Therefore, you can discard the patch in its entirety.

Agreed. On top of this, I would argue that any use of IS_ERR_OR_NULL()
is an indication of a problem. If an interface requires using this,
then we should generally fix the interface to have sane calling
conventions, typically by ensuring that it consistently uses error
pointers rather than NULL values to indicate an error.

Some interfaces like this one return NULL to indicate that there
is no object to return but there is no error. This is a somewhat
confusing interface design and users tend to get it wrong the same
way. It is probably a good idea for someone to go through
the users of IS_ERR_OR_NULL() and see how many are wrong, and
improve the error handling, or if they can be expressed in
a more readable way that avoids IS_ERR_OR_NULL().

     Arnd