Re: [PATCH] PCI: imx6: Fix wrong check in imx6_pcie_attach_pd()

[Arnd Bergmann]

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