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

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

[Tang Bin]

In the function imx6_pcie_attach_pd(),
dev_pm_domain_attach_by_name() may return NULL in some cases,
so IS_ERR() doesn't meet the requirements. Thus fix it.

Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support")
Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6619e3caf..65d6ebbba 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -337,8 +337,8 @@ static int imx6_pcie_attach_pd(struct device *dev)
 		return 0;
 
 	imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie");
-	if (IS_ERR(imx6_pcie->pd_pcie))
-		return PTR_ERR(imx6_pcie->pd_pcie);
+	if (IS_ERR_OR_NULL(imx6_pcie->pd_pcie))
+		return PTR_ERR(imx6_pcie->pd_pcie) ? : -ENODATA;
 	/* Do nothing when power domain missing */
 	if (!imx6_pcie->pd_pcie)
 		return 0;
@@ -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 |
-- 
2.20.1.windows.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

On Tue, Sep 13, 2022 at 02:59:10PM +0800, Tang Bin wrote:
> In the function imx6_pcie_attach_pd(),
> dev_pm_domain_attach_by_name() may return NULL in some cases,
> so IS_ERR() doesn't meet the requirements. Thus fix it.

NAK. You are clearly doing a mechanical search and replace, and then
throwing out patches without a care in the world for other people to
then decide whether the changes are in fact appropriate or not.

Please don't do that. Please read and understand the code before you
waste reviewers and developers time - otherwise you will educate
reviews and developers to ignore your efforts.

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6619e3caf..65d6ebbba 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -337,8 +337,8 @@ static int imx6_pcie_attach_pd(struct device *dev)
>  		return 0;
>  
>  	imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie");
> -	if (IS_ERR(imx6_pcie->pd_pcie))
> -		return PTR_ERR(imx6_pcie->pd_pcie);
> +	if (IS_ERR_OR_NULL(imx6_pcie->pd_pcie))
> +		return PTR_ERR(imx6_pcie->pd_pcie) ? : -ENODATA;
>  	/* Do nothing when power domain missing */
>  	if (!imx6_pcie->pd_pcie)
>  		return 0;

Your change is incorrect, as can be seen by the following if()
statement, which checks for NULL here. Clearly, the explicit
intention is that if dev_pm_domain_attach_by_name() returns NULL,
imx6_pcie_attach_pd() does _not_ fail.

So you are likely creating a regression by your change. You are
likely introducing a bug rather than fixing something. This is
why you must always carefully review any mechanical change.

> @@ -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.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

On Tue, Sep 13, 2022 at 08:31:35AM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 13, 2022 at 02:59:10PM +0800, Tang Bin wrote:
> > In the function imx6_pcie_attach_pd(),
> > dev_pm_domain_attach_by_name() may return NULL in some cases,
> > so IS_ERR() doesn't meet the requirements. Thus fix it.
> 
> NAK. You are clearly doing a mechanical search and replace, and then
> throwing out patches without a care in the world for other people to
> then decide whether the changes are in fact appropriate or not.
> 
> Please don't do that. Please read and understand the code before you
> waste reviewers and developers time - otherwise you will educate
> reviews and developers to ignore your efforts.

It is also highly likely that many of these changes are just plain
broken.

If you read the documentation for this function and the referred
to function:

 * Returns the created virtual device if successfully attached PM domain, NULL
 * when the device don't need a PM domain, else an ERR_PTR() in case of
 * failures. If a power-domain exists for the device, but cannot be found or
 * turned on, then ERR_PTR(-EPROBE_DEFER) is returned to ensure that the device
 * is not probed and to re-try again later.

So, NULL is *not* an error condition. It means that the device does not
need a power domain, which is *not* a failure.

You are probably causing more harm than good by trying to do this
mechanical change all over the kernel.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lucas Stach]

Am Dienstag, dem 13.09.2022 um 14:59 +0800 schrieb Tang Bin:
> In the function imx6_pcie_attach_pd(),
> dev_pm_domain_attach_by_name() may return NULL in some cases,
> so IS_ERR() doesn't meet the requirements. Thus fix it.
> 
I don't like this added complexity in the driver. IHMO if there is a
real issue, dev_pm_domain_attach_by_name() should just return a error
code, instead of NULL. The fact that you need to pull a error code out
of thin air in the driver is a big hint that this should be fixed in
the called function, not in the return handling in the driver.

A bit down the callstack genpd_dev_pm_attach_by_id() is called, which
is documented like this "Returns the created virtual device if
successfully attached PM domain, NULL when the device don't need a PM
domain [...]". NULL is a valid return code, where the driver should
_not_ stop probing, as the device should work without the power domain
attached.

Regards,
Lucas

> Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support")
> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6619e3caf..65d6ebbba 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -337,8 +337,8 @@ static int imx6_pcie_attach_pd(struct device *dev)
>  		return 0;
>  
>  	imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie");
> -	if (IS_ERR(imx6_pcie->pd_pcie))
> -		return PTR_ERR(imx6_pcie->pd_pcie);
> +	if (IS_ERR_OR_NULL(imx6_pcie->pd_pcie))
> +		return PTR_ERR(imx6_pcie->pd_pcie) ? : -ENODATA;
>  	/* Do nothing when power domain missing */
>  	if (!imx6_pcie->pd_pcie)
>  		return 0;
> @@ -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 |



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

On Tue, Sep 13, 2022 at 09:39:03AM +0200, Lucas Stach wrote:
> Am Dienstag, dem 13.09.2022 um 14:59 +0800 schrieb Tang Bin:
> > In the function imx6_pcie_attach_pd(),
> > dev_pm_domain_attach_by_name() may return NULL in some cases,
> > so IS_ERR() doesn't meet the requirements. Thus fix it.
> > 
> I don't like this added complexity in the driver. IHMO if there is a
> real issue, dev_pm_domain_attach_by_name() should just return a error
> code, instead of NULL.

You've fallen into the trap that Tang Bin laid. It returns an error
code for all cases where an error has happened. It returns NULL only
when the device does not require a power domain. So, a NULL return is
*not* an error condition, but merely an indication that "this device
does not require this power domain".

Mechanical changes like this are really quite harmful to the kernel.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel