Re: [PATCH 4/6] PCI: qcom-ep: Set initial interconnect bandwidth before enabling clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: asymmetric resource teardown in qcom_pcie_disable_resources()
--

commit e8d1ed8b0977eb318a03b77e20d6636bb30f02d4
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

PCI: qcom-ep: Set initial interconnect bandwidth before enabling clocks

Some Qualcomm platforms require the interconnect bandwidth constraints
to be set before enabling the interconnect clocks, otherwise some
clocks get stuck in the off state. Move the icc_set_bw() call before
clk_bulk_prepare_enable() so the bandwidth vote is in place first.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 56184e6ca6e69..0b23d7181539d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -331,9 +331,23 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>  	struct dw_pcie *pci = &pcie_ep->pci;
>  	int ret;
>  
> +	/*
> +	 * Some Qualcomm platforms require interconnect bandwidth constraints
> +	 * to be set before enabling interconnect clocks.
> +	 *
> +	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
> +	 * for the pcie-mem path.
> +	 */
> +	ret = icc_set_bw(pcie_ep->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> +	if (ret) {
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
>  	ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
>  	if (ret)
> -		return ret;
> +		goto err_disable_icc;

[ ... ]

> @@ -351,28 +365,14 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>  	if (ret)
>  		goto err_phy_exit;
>  
>  	return 0;
>  
> -err_phy_off:
> -	phy_power_off(pcie_ep->phy);
>  err_phy_exit:
>  	phy_exit(pcie_ep->phy);
>  err_disable_clk:
>  	clk_bulk_disable_unprepare(pcie_ep->num_clks, pcie_ep->clks);
> +err_disable_icc:
> +	icc_set_bw(pcie_ep->icc_mem, 0, 0);
>  
>  	return ret;
>  }

[Severity: High]
Does qcom_pcie_disable_resources() also need to be updated to match this new
LIFO teardown ordering?

Since this patch moves the interconnect bandwidth enablement to be the first
step in qcom_pcie_enable_resources(), LIFO ordering requires it to be the last
step disabled. The error path here handles it correctly, but
qcom_pcie_disable_resources() still drops the interconnect bandwidth constraint
before disabling the PHY and clocks:

drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_disable_resources() {
	...
	icc_set_bw(pcie_ep->icc_mem, 0, 0);
	phy_power_off(pcie_ep->phy);
	phy_exit(pcie_ep->phy);
	clk_bulk_disable_unprepare(pcie_ep->num_clks, pcie_ep->clks);
}

When the host asserts PERST, qcom_pcie_disable_resources() is called. Could
this order of teardown lead to bus hangs, NOC errors, or clocks getting stuck
in an invalid state if the interconnect powers down while dependent clocks are
still active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-monaco_ep-v1-0-63c5282fb0db@oss.qualcomm.com?part=4

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