Re: [PATCH] PCI: imx6: fix power domain leak on probe failure

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] LIFO order violations during error cleanup cause premature power domain detachment.

Pre-existing issues:
- [High] Resource leak of power domain and device link on partial attach failure.
- [High] Probe error path bypasses cleanup logic, leaking power domains and pwrctrl devices.
--

--- Patch [1]: [PATCH] PCI: imx6: fix power domain leak on probe failure ---
commit 887e76ee860dcc87d2150e367092c569b216fc14
Author: hanzhijian <[email protected]>

PCI: imx6: fix power domain leak on probe failure

This commit fixes a power domain leak in the i.MX6 PCIe driver by
introducing imx_pcie_detach_pd() and calling it in probe error paths.
It also saves the device links to properly release them during cleanup.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 39790e66b98dc..4df548679464c 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -662,6 +665,7 @@ static int imx_pcie_attach_pd(struct device *dev)
>  		dev_err(dev, "Failed to add device_link to pcie pd\n");
>  		return -EINVAL;
>  	}
> +	imx_pcie->pd_link = link;
>  
>  	imx_pcie->pd_pcie_phy = dev_pm_domain_attach_by_name(dev, "pcie_phy");
>  	if (IS_ERR(imx_pcie->pd_pcie_phy))
>  		return PTR_ERR(imx_pcie->pd_pcie_phy);

[Severity: High]
This is a pre-existing issue, but if dev_pm_domain_attach_by_name() fails
here, does it leak the previously attached "pcie" power domain and its
device link?

Because the function returns directly without cleaning up the earlier
attach, these resources might be permanently leaked on every probe deferral.

Furthermore, the function leaves an ERR_PTR in imx_pcie->pd_pcie_phy,
which would cause a crash if a cleanup path ever attempted to use
imx_pcie_detach_pd() to handle this partial failure.

> @@ -675,10 +679,24 @@ static int imx_pcie_attach_pd(struct device *dev)
>  		dev_err(dev, "Failed to add device_link to pcie_phy pd\n");
>  		return -EINVAL;
>  	}
> +	imx_pcie->pd_phy_link = link;
>  
>  	return 0;
>  }
>  
> +static void imx_pcie_detach_pd(struct imx_pcie *imx_pcie)
> +{
> +	if (imx_pcie->pd_link)
> +		device_link_del(imx_pcie->pd_link);
> +	if (imx_pcie->pd_pcie)
> +		dev_pm_domain_detach(imx_pcie->pd_pcie, true);
> +
> +	if (imx_pcie->pd_phy_link)
> +		device_link_del(imx_pcie->pd_phy_link);
> +	if (imx_pcie->pd_pcie_phy)
> +		dev_pm_domain_detach(imx_pcie->pd_pcie_phy, true);
> +}

[Severity: High]
Does imx_pcie_detach_pd() detach the power domains in the wrong order?

The power domains are attached in FIFO order ("pcie", then "pcie_phy") in
imx_pcie_attach_pd(). However, they are also detached in FIFO order here.

Should the "pcie_phy" domain and link be detached before the "pcie" domain
to properly follow reverse initialization (LIFO) order?

[ ... ]

> @@ -1999,6 +2019,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_pwrctrl_destroy:

[Severity: High]
This is a pre-existing issue, but does the probe error path for PM runtime
enablement bypass this cleanup logic?

Looking at imx_pcie_probe() earlier in the function:

drivers/pci/controller/dwc/pci-imx6.c:imx_pcie_probe() {
    ...
	if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_PM_RUNTIME) {
		pm_runtime_no_callbacks(dev);
		ret = devm_pm_runtime_set_active_enabled(dev);
		if (ret < 0)
			return ret;
	}
    ...
}

If devm_pm_runtime_set_active_enabled() fails, it returns directly. Because
it completely bypasses the err_pwrctrl_destroy label, doesn't it permanently
leak the power domains and pwrctrl devices initialized earlier in the probe?

> +	imx_pcie_detach_pd(imx_pcie);
>  	if (ret != -EPROBE_DEFER)
>  		pci_pwrctrl_destroy_devices(dev);
>  	return ret;

[Severity: High]
Does detaching the power domains before destroying the pwrctrl devices
violate LIFO cleanup order?

If a failure occurs after pci_pwrctrl_create_devices() succeeds, this calls
imx_pcie_detach_pd() to cut power before pci_pwrctrl_destroy_devices() has
a chance to unregister the devices. 

Could this cause the driver remove() callbacks to attempt to access
unpowered PCI registers and trigger a hardware hang or kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1