Re: [PATCH V6 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The return value of `imx_pcie_host_init()` is ignored in `imx_add_pcie_ep()`, leading to potential kernel crashes on unpowered hardware access.
--

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

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

Previously, pci_pwrctrl_create_devices() was placed in imx_pcie_host_init(),
which caused it to be called multiple times across suspend/resume cycles.
This patch moves pci_pwrctrl_create_devices() to imx_pcie_probe() so that it
is only called once during probe.

> 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);
>  		if (ret < 0)
> -			return ret;
> +			goto err_pwrctrl_destroy;

[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() here?

Looking at imx_add_pcie_ep(), it calls imx_pcie_host_init(pp) but ignores
the integer error code it returns:

drivers/pci/controller/dwc/pci-imx6.c:imx_add_pcie_ep() {
    ...
    imx_pcie_host_init(pp);
    ep = &pci->ep;
    ep->ops = &pcie_ep_ops;
    ...
    ret = dw_pcie_ep_init(ep);
    if (ret) {
        dev_err(dev, "failed to initialize endpoint\n");
        return ret;
    }
    ...
}

If imx_pcie_host_init() fails to enable a clock or regulator, the hardware
is left unpowered and unclocked.

Since the function proceeds to call dw_pcie_ep_init() which reads and writes
to the controller's DBI registers, could this lead to a synchronous external
abort (bus error) and crash the entire system when it accesses the unclocked
peripheral registers?

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