Re: [PATCH V5 1/4] PCI: imx6: Move pci_pwrctrl_create_devices() to imx_pcie_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In Endpoint (EP) mode, `imx_add_pcie_ep()` ignores the return value of `imx_pcie_host_init()` and fails to clean up on error, leading to a kernel panic.
- [High] Missing PM domain detach and device link deletion in `imx_pcie_probe()` error paths causes permanent probe failure on `-EPROBE_DEFER`.
--

--- Patch [1]: [PATCH V5 1/4] PCI: imx6: Move pci_pwrctrl_create_devices() to imx_pcie_probe() ---
commit c4ce7635410a21f29d4b7bcc43f48a82e9c31e5b
Author: Sherry Sun <sherry.sun@nxp.com>

PCI: imx6: Move pci_pwrctrl_create_devices() to imx_pcie_probe()

This commit moves pci_pwrctrl_create_devices() from imx_pcie_host_init()
to imx_pcie_probe(). This ensures the power control devices are created
only once during probe, avoiding duplicate creation across suspend and
resume cycles.

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index f55a68f60eea9..92f8e4a299e88 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -1954,11 +1945,15 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = pci_pwrctrl_create_devices(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to create pwrctrl devices\n");
> +
>  	pci->use_parent_dt_ranges = true;
>  	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
>  		ret = imx_add_pcie_ep(imx_pcie, pdev);

[Severity: High]
This is a pre-existing issue, but does imx_add_pcie_ep() need to check the
return value of imx_pcie_host_init()?

If imx_pcie_host_init() fails (for example, returning -EPROBE_DEFER) and
powers down the hardware, imx_add_pcie_ep() completely ignores this and
continues execution:

imx_add_pcie_ep()
    imx_pcie_host_init(pp); /* Return value ignored */
    ep = &pci->ep;
    ...
    ret = dw_pcie_ep_init(ep); /* Writes to hardware registers */

Writing to unclocked or unpowered peripheral registers on i.MX causes an
asynchronous external abort (System Error / Data Abort). Could this result
in an immediate kernel panic during boot if imx_pcie_host_init() fails?

>  		if (ret < 0)
> -			return ret;
> +			goto err_pwrctrl_destroy;

[ ... ]

> @@ -1985,6 +1980,11 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	}
>  
>  	return 0;
> +
> +err_pwrctrl_destroy:
> +	if (ret != -EPROBE_DEFER)
> +		pci_pwrctrl_destroy_devices(dev);
> +	return ret;

[Severity: High]
This is a pre-existing issue, but does this error path also need to detach
the PM domains and delete device links?

Earlier in imx_pcie_probe(), imx_pcie_attach_pd() dynamically allocates
virtual PM domain devices and creates stateless device links. If the probe
fails and jumps here (for instance, if pci_pwrctrl_create_devices() or
imx_add_pcie_ep() returns -EPROBE_DEFER), these virtual devices and device
links are not cleaned up.

On the next probe attempt, dev_pm_domain_attach_by_name() attempts to
register a new virtual device with the exact same name, which causes a
sysfs/kobject naming collision.

Could this missing cleanup cause device_register() to fail with -EEXIST,
permanently preventing the PCIe controller from probing if it defers once
during boot?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709101555.3034853-1-sherry.sun@oss.nxp.com?part=1