Re: [PATCH v3 11/16] PCI: dwc: qcom: Use cached PCIe capability offset

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `pci->pcie_cap` field remains uninitialized (0) when used in `.post_init` callbacks, leading to corruption of the standard PCI configuration header.
--

--- Patch [11]: [PATCH v3 11/16] PCI: dwc: qcom: Use cached PCIe capability offset ---
commit ddf8caa0039918520481c1cff7b9e80cbf8ee212
Author: Hans Zhang <[email protected]>

PCI: dwc: qcom: Use cached PCIe capability offset
    
This commit updates the Qualcomm PCIe driver to use the cached PCIe capability
offset (pci->pcie_cap) instead of repeatedly calling dw_pcie_find_capability().
It aims to optimize capability lookups during initialization and link updates.

> dw_pcie_host_init() caches the offset after .init, so .post_init callbacks
> and later functions (.link_up, icc_opp_update) can use pci->pcie_cap
> directly. For .init itself, we must call dw_pcie_get_pcie_cap() inside
> qcom_pcie_host_init() to obtain the offset (hardware is already enabled).

Is there a confusion here between dw_pcie_host_ops->post_init and
qcom_pcie_ops->post_init?

The commit message correctly notes that dw_pcie_host_init() caches the offset
after dw_pcie_host_ops->init returns. However, the .post_init callbacks
modified in this patch belong to struct qcom_pcie_ops.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d8eb52857f69c..bd383d7cb02b1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -360,7 +360,6 @@ static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
>  
>  static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
>  {
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>  	u32 val;
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
> @@ -370,9 +369,9 @@ static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
>  	 * notifications for the Hot-Plug commands. So set the NCCS field to
>  	 * avoid waiting for the completions.
>  	 */
> -	val = readl(pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +	val = readl(pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);

[Severity: High]
Is pci->pcie_cap initialized when this is called?

Looking at qcom_pcie_host_init(), it calls pcie->cfg->ops->post_init(pcie)
before it calls qcom_pcie_clear_aspm_l0s() where dw_pcie_get_pcie_cap() is
finally executed:

qcom_pcie_host_init()
  pcie->cfg->ops->post_init(pcie) 
    qcom_pcie_set_slot_nccs(pcie->pci)
      val = readl(pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);
  ...
  qcom_pcie_clear_aspm_l0s(pcie->pci)
    offset = dw_pcie_get_pcie_cap(pci);

Since pci->pcie_cap is 0 during qcom_pcie_ops->post_init, won't this access
standard PCI configuration space (like offset 0x14 for PCI_EXP_SLTCAP, which
is BAR1) instead of the PCIe capability registers?

>  	val |= PCI_EXP_SLTCAP_NCCS;
> -	writel(val, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +	writel(val, pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);
>  
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  }

[ ... ]

> @@ -935,7 +934,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;
>  	u32 val;

[Severity: High]
Does this also suffer from the same initialization ordering issue?

Since qcom_pcie_post_init_2_3_3() is called by qcom_pcie_host_init() before
pci->pcie_cap is populated, offset will be 0 here, causing the subsequent
writes to PCIE_CAP_SLOT_VAL and PCI_EXP_LNKCAP to overwrite standard PCI
header fields.

>  
>  	/* Force PHY out of lowest power state */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic