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

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Tang Bin <tangbin@cmss.chinamobile.com>
Cc: hongxing.zhu@nxp.com, l.stach@pengutronix.de,
	lorenzo.pieralisi@arm.com, robh@kernel.org, kw@linux.com,
	shawnguo@kernel.org, bhelgaas@google.com, s.hauer@pengutronix.de,
	kernel@pengutronix.de, festevam@gmail.com, 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()
Date: Tue, 13 Sep 2022 08:31:35 +0100	[thread overview]
Message-ID: <YyAx17VNvDMyvgBV@shell.armlinux.org.uk> (raw)
In-Reply-To: <20220913065910.15348-1-tangbin@cmss.chinamobile.com>

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