Re: [PATCH v3 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:
- [Medium] Probe deferral causes reference count and resource leaks for clocks, regulators, and PHY components when link retention is active.
- [Medium] Bypassing `post_init` during link retention skips critical controller quirks and DMA coherency settings.
- [Medium] `qcom_pcie_check_link_retain()` uses an incorrect hardcoded mask (`0x1f`) to read the LTSSM state, risking state aliasing.
--

--- Patch [3]: [PATCH v3 3/4] PCI: qcom: Add link retention support ---
commit 48e4b763c5282c1440201e104587c3e6e2342ab6
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
PCI: qcom: Add link retention support

This patch adds link retention support for Qualcomm PCIe controllers. It
skips controller reset and PERST# toggling if the bootloader has already
trained the link and left it in L0 or L1 idle. It checks device tree caps
and configures base addresses in the retention path.

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

[ ... ]

> @@ -996,6 +998,50 @@ 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.
> + *
> + * The max-link-speed and num-lanes specified in Device Tree are meant to cap
> + * the link's maximum capability. Retrain the link if the controller's max
> + * link speed/width exceeds the Device Tree cap, unless the currently
> + * negotiated speed/width is already within that cap, in which case the
> + * existing link can be retained as-is.
> + */
> +static bool qcom_pcie_check_link_retain(struct qcom_pcie *pcie)
> +{
> +	u32 cap, speed, cur_speed, val, ltssm, width, cur_width;
> +	struct dw_pcie *pci = pcie->pci;
> +	u8 offset;
> +
> +	val = readl(pcie->parf + PARF_LTSSM);
> +	ltssm = val & 0x1f;

[Severity: Medium]
Does this hardcoded 0x1f mask truncate the LTSSM state?

The standard LTSSM state mask (PARF_LTSSM_STATE_MASK) is defined as
GENMASK(5, 0), which is 0x3f.

By discarding bit 5, higher-order states like 0x31 could erroneously alias
to DW_PCIE_LTSSM_L0 (0x11).

This might cause the driver to incorrectly assume the link is active and
retain it, skipping initialization and leaving the link broken.

[ ... ]

> @@ -1074,6 +1129,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: Medium]
Does bypassing the rest of qcom_pcie_post_init_2_7_0() skip critical OS quirks?

By returning early, this bypasses the configuration of NO_SNOOP_OVERRIDE and
the NCCS hot-plug quirk.

Missing NO_SNOOP_OVERRIDE can cause DMA cache coherency issues and data
corruption on ARM.

Missing the NCCS quirk can cause the OS pciehp driver to wait for
non-existent command completions, leading to hot-plug timeouts.

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

[ ... ]

> @@ -1414,6 +1474,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (!pp->skip_pwrctrl_off)
>  		pci_pwrctrl_power_off_devices(pci->dev);
>  err_pwrctrl_destroy:
> +	if (pp->link_retain && ret == -EPROBE_DEFER)
> +		return ret;

[Severity: Medium]
Could this early return leak hardware enable counts during probe deferral?

If a downstream function like pci_pwrctrl_power_on_devices() returns
-EPROBE_DEFER, this early return explicitly bypasses the err_disable_phy
and err_deinit cleanup paths.

The driver core's devres cleanup drops the resource references but does not
manually disable them.

On the next probe attempt, qcom_pcie_init_2_7_0() will re-enable the
regulators and clocks:

	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
...
	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);

This compounds the leaked reference counts, preventing the resources from
ever being powered off upon driver unbind or system shutdown.

>  	if (ret != -EPROBE_DEFER && !pci->suspended)
>  		pci_pwrctrl_destroy_devices(pci->dev);
>  err_disable_phy:
>  	qcom_pcie_phy_power_off(pcie);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-link_retain-v3-0-81a9d187bb61@oss.qualcomm.com?part=3