Re: [PATCH v2 3/4] PCI: qcom: Indicate broken L1ss exit during resume from system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Initialization order bug: `broken_l1ss_resume` is set after child PCI devices have already been enumerated and probed.
- [High] Commit message directly contradicts the actual driver implementation regarding PHY power state during suspend.
--

commit 3ebc1e1ccc09940f1b1046dd596cb5ba9e52cf1e
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI: qcom: Indicate broken L1ss exit during resume from system suspend

This commit sets the broken_l1ss_resume flag on the PCI host bridge for
Qualcomm platforms to signal to client drivers that the RC driver will
turn off the PHY during suspend. This aims to prevent fatal link down
conditions by avoiding L1ss during system suspend.

>     Qcom PCIe RCs can successfully exit from L1ss during OS runtime. However,
>     during system suspend, the Qcom PCIe RC driver may remove all resource
>     votes and turns off the PHY to maximize power savings.

Does the commit message accurately describe the driver's suspend behavior?

Looking at qcom_pcie_suspend_noirq(), the PHY is only turned off if no
devices are active:

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_suspend_noirq() {
	/*
	 * Turn OFF the resources only for controllers without active PCIe
	 * devices. For controllers with active devices, the resources are kept
	 * ON and the link is expected to be in L0/L1 (sub)states.
	...
	 */
	if (!dw_pcie_link_up(pcie->pci)) {
		qcom_pcie_host_deinit(&pcie->pci->pp);
		pcie->suspended = true;
	}
...
}

Because the resources are kept active when an endpoint device is attached, it
seems the stated condition for link down would not occur. Could the commit
message be clarified to match the implementation, or is there a missing
patch that changes this suspend behavior?

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index af6bf5cce65be..75bb6cb5e35e5 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1368,6 +1368,18 @@ static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>  
> +	/*
> +	 * During system suspend, the Qcom RC driver may turn off the analog
> +	 * circuitry of PHY and remove controller votes to save power. If the
> +	 * link is in L1ss and the endpoint asserts CLKREQ# to exit L1ss, the
> +	 * time required to wake the system and restore the PHY/refclk will
> +	 * exceed the strict L1ss exit timing (L10_REFCLK_ON + T_COMMONMODE),
> +	 * resulting in Link Down (LDn) condition. Set this flag to indicate
> +	 * this limitation to client drivers so that they can avoid relying on
> +	 * L1ss during system suspend.
> +	 */
> +	pp->bridge->broken_l1ss_resume = true;
> +
>  	if (pcie->cfg->ops->host_post_init)
>  		pcie->cfg->ops->host_post_init(pcie);
>  }

Does this initialization order introduce a regression?

Since qcom_pcie_host_post_init() acts as the post_init callback for the
DesignWare host operations, it is called after pci_host_probe(bridge) in
dw_pcie_host_init():

drivers/pci/controller/dwc/pcie-designware-host.c:dw_pcie_host_init() {
    ...
	ret = pci_host_probe(bridge);
	if (ret)
		goto err_stop_link;

	if (pp->ops->post_init)
		pp->ops->post_init(pp);
    ...
}

Because pci_host_probe() handles device enumeration and triggers endpoint
drivers' probe() routines, any driver that checks the broken_l1ss_resume
flag during its probe sequence might read an uninitialized false value.

Would it be safer to set broken_l1ss_resume inside the init callback
instead, before enumeration begins?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-l1ss-fix-v2-0-b2c3a4bdeb15@oss.qualcomm.com?part=3