Re: [PATCH v2 02/16] PCI: dwc: Use cached PCIe capability offset in core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Direct usage of cached `pci->pcie_cap` leads to incorrect offset calculation and DBI corruption in endpoint drivers with deferred hardware enablement.
--

commit 02cd0d194f8ffdc28d0564602c5f35da3c6e8582
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: Use cached PCIe capability offset in core

This patch modifies the DWC core functions to use the cached pcie_cap offset
instead of calling dw_pcie_find_capability() each time. The cached offset is
initialized after hardware is supposedly ready in host and endpoint modes.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index a396b58d67462..11b4fa6d129c7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c

[ ... ]

> @@ -1524,6 +1524,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	if (ep->ops->pre_init)
>  		ep->ops->pre_init(ep);
>  
> +	dw_pcie_get_pcie_cap(pci);
> +

[Severity: High]
Does this code incorrectly attempt to read the capability offset before the
hardware is powered on in endpoint drivers with deferred enablement, like
qcom-pcie-ep?

In these drivers, hardware resources like clocks and PHY are not enabled
during initial probe, but are deferred until the host de-asserts PERST#.
Because the DBI space is unclocked and inaccessible at this time, reading it
can either trigger a synchronous external abort or return 0xFFFFFFFF.

This would cause the capability search to fail, leaving pci->pcie_cap as 0.

>  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>  			       ep->page_size);
>  	if (ret < 0) {

[ ... ]

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 37d2715e38eb4..810e920c1d8b5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c

[ ... ]

> @@ -848,7 +847,7 @@ EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
>  static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  {
>  	u32 cap, ctrl2, link_speed;
> -	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;
>  
>  	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);

[Severity: High]
If pci->pcie_cap is 0 due to the earlier failed capability search in
dw_pcie_ep_init(), will this read and write to offset 0 (the standard PCI
configuration space header) instead of the actual PCIe capability offset?

Since dw_pcie_setup() calls this function after the link is initialized,
operating on offset 0 could corrupt the Vendor ID and Device ID, completely
breaking link initialization.

[ ... ]

> @@ -940,10 +937,9 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
>  
> -	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);
>  	FIELD_MODIFY(PCI_EXP_LNKCAP_MLW, &lnkcap, num_lanes);
> -	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap);
> +	dw_pcie_writel_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP, lnkcap);
>  }

[Severity: High]
Could this similarly corrupt the standard PCI configuration space header at
offset 0 if pci->pcie_cap was not properly initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530153101.695580-1-18255117159@163.com?part=2