Re: [PATCH v2 3/4] PCI: qcom: Add link retention support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Checking hardware link capabilities instead of negotiated link status to determine current link speed and width.
- [High] Returning 0 instead of -EPROBE_DEFER defeats the probe deferral mechanism and leaves the controller partially initialized.
- [High] Early returns during link retention bypass critical controller software state initialization and hardware capabilities.
--

commit 6dd7e9bfe9f104689a018777f9dcaa31b6e736f8
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
PCI: qcom: Add link retention support

This patch introduces link retention support for Qualcomm PCIe controllers,
skipping reinitialization and PERST# toggling if the bootloader left the
link active in L0 or L1 idle. It falls back to normal initialization and
link retraining if the current link does not meet device tree constraints.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index bfe873cbf44f6..b061eaa227b39 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -960,6 +962,42 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +/*
> + * Determine whether the link established by the bootloader can be reused.
> + *
> + * Reuse the existing link only if its current speed and lane count match
> + * the max-link-speed and num-lanes specified in Device Tree; otherwise,
> + * retrain the link.
> + */
> +static bool qcom_pcie_check_link_retain(struct qcom_pcie *pcie)
> +{
> +	u32 cap, speed, val, ltssm, width;
> +	struct dw_pcie *pci = pcie->pci;
> +	u8 offset;
> +
> +	val = readl(pcie->parf + PARF_LTSSM);
> +	ltssm = val & 0x1f;
> +	if ((val & LTSSM_EN) &&
> +	    (ltssm == DW_PCIE_LTSSM_L0 || ltssm == DW_PCIE_LTSSM_L1_IDLE)) {
> +		qcom_pcie_configure_dbi_atu_base(pcie);
> +
> +		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +		cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +		speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> +		width = dw_pcie_link_get_max_link_width(pci);

[Severity: High]
Does this check the maximum hardware capabilities instead of the currently
negotiated link status?

Reading PCI_EXP_LNKCAP (and using dw_pcie_link_get_max_link_width) retrieves
the hardware capabilities rather than the current link speed and width from
PCI_EXP_LNKSTA.

If the bootloader brought up the link at a degraded speed (e.g., Gen1x1 on a
port capable of Gen3x2), could this incorrectly pass the constraint checks
and leave the system with severely degraded PCIe performance?

> +
> +		if (pci->max_link_speed > 0 && speed > pci->max_link_speed)
> +			return false;
> +
> +		if (pci->num_lanes > 0 && width > pci->num_lanes)
> +			return false;
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> @@ -978,6 +1016,14 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  	if (ret < 0)
>  		goto err_disable_regulators;
>  
> +	if (pcie->cfg->link_retain) {
> +		pci->pp.link_retain = qcom_pcie_check_link_retain(pcie);
> +		if (pci->pp.link_retain) {
> +			dev_info(dev, "Retaining PCIe link\n");
> +			return 0;
> +		}
> +	}

[Severity: High]
Does this early return bypass critical software state initialization?

Returning early here skips the assignment of pci->l1ss_support = true later
in this function. Because struct dw_pcie is zero-initialized, the downstream
DesignWare core might falsely assume ASPM L1 Substates are unsupported and
fail to configure them correctly.

> +
>  	ret = reset_control_assert(res->rst);
>  	if (ret) {
>  		dev_err(dev, "reset assert failed (%d)\n", ret);
> @@ -1038,6 +1084,9 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  {
>  	const struct qcom_pcie_cfg *pcie_cfg = pcie->cfg;
>  
> +	if (pcie->pci->pp.link_retain)
> +		return 0;
> +

[Severity: High]
Could this early return skip necessary hot-plug configurations?

By returning early, this skips the call to qcom_pcie_set_slot_nccs() located
further down. Missing the No Command Completed Support (NCCS) configuration
could break PCIe hot-plug command completions and lead to timeouts during
hot-plug operations.

>  	if (pcie_cfg->override_no_snoop)
>  		writel(WR_NO_SNOOP_OVERRIDE_EN | RD_NO_SNOOP_OVERRIDE_EN,
>  				pcie->parf + PARF_NO_SNOOP_OVERRIDE);

[ ... ]

> @@ -1337,8 +1388,9 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  err_pwrctrl_power_off:
>  	pci_pwrctrl_power_off_devices(pci->dev);
>  err_pwrctrl_destroy:
> -	if (ret != -EPROBE_DEFER)
> -		pci_pwrctrl_destroy_devices(pci->dev);
> +	if (pp->link_retain && ret == -EPROBE_DEFER)
> +		return 0;

[Severity: High]
Does returning 0 here defeat the probe deferral mechanism?

If pci_pwrctrl_power_on_devices() earlier in this function returns
-EPROBE_DEFER, returning 0 instead of propagating the error code causes
the driver core to treat the probe as successful. This means the driver
will not be re-probed when resources become available.

Additionally, returning early from this error cleanup path leaves the rest
of the host initialization incomplete (such as SMMU setup and removing
MSIX/DPC capabilities), potentially leaving the controller in an unusable
state while the kernel believes the probe succeeded.

> +	pci_pwrctrl_destroy_devices(pci->dev);
>  err_disable_phy:
>  	qcom_pcie_phy_power_off(pcie);
>  err_deinit:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-link_retain-v2-0-08ed448b081c@oss.qualcomm.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy