Re: [PATCH v3] 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:
- [High] Deferring imx_pcie_host_exit() via devm_add_action_or_reset() creates a teardown ordering conflict with manual cleanup in probe, causing pwrctrl devices to leak on endpoint initialization failure.
--

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

PCI: imx6: fix endpoint init error handling

This commit updates imx_add_pcie_ep() to check the return value of
imx_pcie_host_init(). It also registers a devres action to release host
resources on error, aiming to unregister the EPC device before powering
off hardware.

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

> This mirrors the root
> port path, where dw_pcie_host_init() releases these resources through
> the same framework.

[Severity: Low]
Is this description accurate? The root port path handles deinitialization
synchronously without devres, rather than through the same framework.

> 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);

[Severity: High]
Does deferring imx_pcie_host_exit() via devres create a teardown ordering
conflict with the manual cleanup in imx_pcie_probe()?

If imx_add_pcie_ep() returns an error during endpoint initialization,
imx_pcie_probe() will execute its manual cleanup before any devres actions
run:

drivers/pci/controller/dwc/pci-imx6.c:imx_pcie_probe() {
    ...
    if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
        ret = imx_add_pcie_ep(imx_pcie, pdev);
        if (ret < 0)
            goto err_pwrctrl_destroy;
    ...
err_pwrctrl_destroy:
    pci_pwrctrl_destroy_devices(dev);
    ...
}

Since pci_pwrctrl_destroy_devices() unregisters the child pwrctrl devices,
when the deferred imx_pcie_host_exit_action() finally executes and calls
pci_pwrctrl_power_off_devices(), the devices are already unregistered.

Could this cause the power-off to be skipped, leaving the hardware resources
permanently powered on?

> +	if (ret)
> +		return ret;
> +
>  	ep = &pci->ep;
>  	ep->ops = &pcie_ep_ops;

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