[PATCH] PCI: imx6: fix endpoint init error handling

[PATCH] PCI: imx6: fix endpoint init error handling

[Zhijian Han]

imx_add_pcie_ep() ignores the return value of imx_pcie_host_init(), so
a failure to enable the clocks, regulators, or PHY during endpoint
initialization goes unnoticed and the driver proceeds to initialize the
endpoint against unpowered hardware.

It also returns directly without calling imx_pcie_host_exit() when
dw_pcie_ep_init() or dw_pcie_ep_init_registers() fails, leaking the
clocks, regulators, and PHY that imx_pcie_host_init() acquired.

Check the return value of imx_pcie_host_init() and jump to a new
err_host_exit label that calls imx_pcie_host_exit() from the endpoint
initialization error paths, mirroring the root port path which releases
these resources through the dw_pcie_host_init() framework.

Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/all/20260824152857.DB9771F00A3D@smtp.kernel.org/
Signed-off-by: Zhijian Han <hanzhijian1991@gmail.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 39790e66b..1dad93e9d 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1634,7 +1634,10 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
 	struct dw_pcie_rp *pp = &pci->pp;
 	struct device *dev = pci->dev;
 
-	imx_pcie_host_init(pp);
+	ret = imx_pcie_host_init(pp);
+	if (ret)
+		return ret;
+
 	ep = &pci->ep;
 	ep->ops = &pcie_ep_ops;
 
@@ -1646,7 +1649,7 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
 	ret = dw_pcie_ep_init(ep);
 	if (ret) {
 		dev_err(dev, "failed to initialize endpoint\n");
-		return ret;
+		goto err_host_exit;
 	}
 	imx_pcie_host_post_init(pp);
 
@@ -1654,12 +1657,16 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
 	if (ret) {
 		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
 		dw_pcie_ep_deinit(ep);
-		return ret;
+		goto err_host_exit;
 	}
 
 	pci_epc_init_notify(ep->epc);
 
 	return 0;
+
+err_host_exit:
+	imx_pcie_host_exit(pp);
+	return ret;
 }
 
 static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save)
-- 
2.43.0

Re: [PATCH] PCI: imx6: fix endpoint init error handling

[sashiko-bot]

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

New issues:
- [High] Manual hardware power-off races with devres unregistration, risking a synchronous external abort.

Pre-existing issues:
- [High] A preexisting error path bug causes dw_pcie_edma_remove() to be called twice when dw_pcie_ep_init_registers() fails.
--

--- Patch [1]: [PATCH] PCI: imx6: fix endpoint init error handling ---
commit 4bb2b4d6db6396f6e99b53eb9afa966615f05042
Author: Zhijian Han <hanzhijian1991@gmail.com>

PCI: imx6: fix endpoint init error handling

imx_add_pcie_ep() ignores the return value of imx_pcie_host_init(), so
a failure to enable the clocks, regulators, or PHY during endpoint
initialization goes unnoticed and the driver proceeds to initialize the
endpoint against unpowered hardware.

Link: https://lore.kernel.org/all/20260824152857.DB9771F00A3D@smtp.kernel.org/

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 39790e66b98dc..1dad93e9dd97c 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
[ ... ]
> @@ -1654,12 +1657,16 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
>  	if (ret) {
>  		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
>  		dw_pcie_ep_deinit(ep);

[Severity: High]
This is a pre-existing issue, but won't this trigger a double-free on the
error path?

If dw_pcie_ep_init_registers() fails, it cleans up EDMA internally:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_init_registers() {
    ...
err_remove_edma:
    dw_pcie_edma_remove(pci);
    return ret;
}

Then this caller in imx_add_pcie_ep() unconditionally calls
dw_pcie_ep_deinit(), which eventually calls dw_pcie_ep_cleanup() and tries to
remove EDMA again:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_cleanup() {
    ...
    dw_pcie_edma_remove(pci);
}

Since dw_edma_remove() in drivers/dma/dw-edma/dw-edma-core.c does not nullify
internal pointers after freeing resources, will executing it twice lead to list
corruption and kernel panics?

> -		return ret;
> +		goto err_host_exit;
>  	}
>  
>  	pci_epc_init_notify(ep->epc);
>  
>  	return 0;
> +
> +err_host_exit:
> +	imx_pcie_host_exit(pp);

[Severity: High]
Can calling imx_pcie_host_exit() directly here cause a synchronous external
abort?

By invoking this function on the error path, clocks and power are severed
while the EPC device might still be registered in sysfs via devres. If
userspace accesses the device while the hardware is powered off, it could
crash.

Should this cleanup be deferred via devm_add_action_or_reset() instead, to
ensure devres handles unregistration before the power and clocks are removed?

> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824230427.3649829-1-hanzhijian1991@gmail.com?part=1