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

[PATCH v2] 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 releasing the host resources 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 register
imx_pcie_host_exit() with devm_add_action_or_reset() so the host
resources are released through the devres framework, which unregisters
the EPC device before powering the hardware off.  This mirrors the root
port path, where dw_pcie_host_init() releases these resources through
the same 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>
---
v2:
- Use devm_add_action_or_reset() for imx_pcie_host_exit() instead of
  calling it directly on the error paths, so the EPC device is
  unregistered before the hardware is powered off

 drivers/pci/controller/dwc/pci-imx6.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 39790e66b..f0fb4e1b0 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1488,6 +1488,11 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
 		regulator_disable(imx_pcie->vpcie);
 }
 
+static void imx_pcie_host_exit_action(void *data)
+{
+	imx_pcie_host_exit(data);
+}
+
 static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -1634,7 +1639,20 @@ 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;
+
+	/*
+	 * Tear the host resources down via a devm action so that, on probe
+	 * failure, the EPC device created by dw_pcie_ep_init() is
+	 * unregistered (also via devres) before the clocks, regulators and
+	 * PHY are switched off.
+	 */
+	ret = devm_add_action_or_reset(dev, imx_pcie_host_exit_action, pp);
+	if (ret)
+		return ret;
+
 	ep = &pci->ep;
 	ep->ops = &pcie_ep_ops;
 
-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a double-free of eDMA resources in the error path of dw_pcie_ep_init_registers().
--

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

PCI: imx6: fix endpoint init error handling

This commit addresses unhandled errors from imx_pcie_host_init() during
endpoint initialization, preventing the driver from operating on unpowered
hardware. It also registers imx_pcie_host_exit() via the devres framework
to ensure host resources are properly released if subsequent initialization
steps fail.

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..f0fb4e1b0ac6b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -1634,7 +1639,20 @@ 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;
> +
> +	/*
> +	 * Tear the host resources down via a devm action so that, on probe
> +	 * failure, the EPC device created by dw_pcie_ep_init() is
> +	 * unregistered (also via devres) before the clocks, regulators and
> +	 * PHY are switched off.
> +	 */
> +	ret = devm_add_action_or_reset(dev, imx_pcie_host_exit_action, pp);
> +	if (ret)
> +		return ret;
> +
>  	ep = &pci->ep;
>  	ep->ops = &pcie_ep_ops;
>  

[Severity: High]
This isn't a bug introduced by this patch, but could the error handling
further down in imx_add_pcie_ep() cause a double-free of eDMA resources?

drivers/pci/controller/dwc/pci-imx6.c:imx_add_pcie_ep() {
    ...
	ret = dw_pcie_ep_init_registers(ep);
	if (ret) {
		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
		dw_pcie_ep_deinit(ep);
		return ret;
	}
    ...
}

If dw_pcie_ep_init_registers() fails after dw_pcie_edma_detect() succeeds,
it handles the error by jumping to err_remove_edma:

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

	return ret;
}

When this error is returned to imx_add_pcie_ep(), it unconditionally calls
dw_pcie_ep_deinit(), which in turn calls dw_pcie_ep_cleanup() and executes
dw_pcie_edma_remove() a second time.

Since dw_edma_remove() doesn't nullify the chip->dw pointer after freeing
IRQs and unregistering the DMA device, does this second call attempt to free
the same resources again, potentially leading to a use-after-free, kernel
crash, or refcount underflow?

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

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

[Markus Elfring]

…
> Check the return value of imx_pcie_host_init() and …

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

See also:
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n145
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2#n34


Regards,
Markus

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

[Frank Li]

On Tue, Aug 25, 2026 at 02:03:40PM +0800, Zhijian Han wrote:
> 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 releasing the host resources 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 register
> imx_pcie_host_exit() with devm_add_action_or_reset() so the host
> resources are released through the devres framework, which unregisters
> the EPC device before powering the hardware off.  This mirrors the root
> port path, where dw_pcie_host_init() releases these resources through
> the same 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>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> v2:
> - Use devm_add_action_or_reset() for imx_pcie_host_exit() instead of
>   calling it directly on the error paths, so the EPC device is
>   unregistered before the hardware is powered off
>
>  drivers/pci/controller/dwc/pci-imx6.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 39790e66b..f0fb4e1b0 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1488,6 +1488,11 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
>  		regulator_disable(imx_pcie->vpcie);
>  }
>
> +static void imx_pcie_host_exit_action(void *data)
> +{
> +	imx_pcie_host_exit(data);
> +}
> +
>  static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -1634,7 +1639,20 @@ 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;
> +
> +	/*
> +	 * Tear the host resources down via a devm action so that, on probe
> +	 * failure, the EPC device created by dw_pcie_ep_init() is
> +	 * unregistered (also via devres) before the clocks, regulators and
> +	 * PHY are switched off.
> +	 */
> +	ret = devm_add_action_or_reset(dev, imx_pcie_host_exit_action, pp);
> +	if (ret)
> +		return ret;
> +
>  	ep = &pci->ep;
>  	ep->ops = &pcie_ep_ops;
>
> --
> 2.43.0
>
>