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

[sashiko-bot@kernel.org]

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