Re: [PATCH v2] PCI: cadence: skip the link polling when endpoint not connected

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Checking PCIe link state during the host controller's `suspend_noirq` incorrectly infers a disconnected link because downstream devices have already been suspended, breaking system resume for all connected endpoints.
--

commit 3a9bc5bfaa54c1ff36dc42ca33244027b719cd03
Author: Aksh Garg <a-garg7@ti.com>

PCI: cadence: skip the link polling when endpoint not connected

This commit introduces a flag to skip PCIe link polling during system resume
on platforms without hotplug support if the link was down before suspend.
By avoiding the polling delay when no endpoint is connected, it speeds up
the resume operation.

> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -686,6 +686,11 @@ static int j721e_pcie_suspend_noirq(struct device *dev)
>  	struct j721e_pcie *pcie = dev_get_drvdata(dev);
>  
>  	if (pcie->mode == PCI_MODE_RC) {
> +		struct cdns_pcie_rc *rc = cdns_pcie_to_rc(pcie->cdns_pcie);
> +
> +		/* If link is down before suspend, skip polling in resume */
> +		rc->link_down_no_hotplug = !j721e_pcie_link_up(pcie->cdns_pcie);

[Severity: High]
Will this correctly identify connected devices during suspend? 

Because the PM core suspends devices in reverse-probe order, child endpoints
will have already been suspended and placed in a low-power state (like D3cold)
before j721e_pcie_suspend_noirq() runs. 

Does this mean the link will likely drop, causing this code to capture a
"down" state even for properly connected, valid endpoints?

> +
>  		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
>  		clk_disable_unprepare(pcie->refclk);
>  	}

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -352,6 +352,9 @@ int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
>  		return ret;
>  	}
>  
> +	if (rc->link_down_no_hotplug)
> +		return 0;
> +

[Severity: High]
Could skipping cdns_pcie_host_start_link() here cause subsequent endpoint
resume operations to fail?

If the flag was incorrectly set to true during suspend, the Root Complex will
skip waiting for the link to train. When the PM core later attempts to resume
the endpoints, couldn't accesses to configuration space over the untrained
link time out or trigger an asynchronous external abort?

>  	ret = cdns_pcie_host_start_link(rc, cdns_pcie_link_up);
>  	if (ret)
>  		dev_dbg(dev, "PCIe link never came up\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623095148.721206-1-a-garg7@ti.com?part=1