Re: [PATCH] PCI: cadence: Use cdns_pcie_find_capability() to get PCIe Cap offset in host driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] cdns_pcie_find_capability() searches the wrong register space in Host mode and its return value is not validated, leading to Root Port configuration space corruption.
--

commit 09b3c64b8f5892eb365acd13030a8067652a15e8
Author: Hans Zhang <18255117159@163.com>

PCI: cadence: Use cdns_pcie_find_capability() to get PCIe Cap offset in host driver

This commit replaces the hardcoded CDNS_PCIE_RP_CAP_OFFSET with a dynamic
lookup using cdns_pcie_find_capability() in Cadence PCIe host-related
functions. The goal is to ensure correct operation across various SoC designs
where the capability might not reside at offset 0xC0.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> index 2b0211870f02..5013d3fe23b5 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> @@ -26,14 +26,14 @@ EXPORT_SYMBOL_GPL(bar_max_size);
>  
>  int cdns_pcie_host_training_complete(struct cdns_pcie *pcie)
>  {
> -	u32 pcie_cap_off = CDNS_PCIE_RP_CAP_OFFSET;
>  	unsigned long end_jiffies;
>  	u16 lnk_stat;
> +	u8 cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP);

Does this capability search scan the correct register space when operating in
host mode?

Looking at the helper function, it uses cdns_pcie_read_cfg() which reads
directly from the base address without the Root Port offset:

drivers/pci/controller/cadence/pcie-cadence.c:cdns_pcie_find_capability() {
    return PCI_FIND_NEXT_CAP(cdns_pcie_read_cfg, PCI_CAPABILITY_LIST,
                             cap, NULL, pcie);
}

However, the Root Port configuration registers are physically located at
pcie->reg_base + CDNS_PCIE_RP_BASE. Could this cause the search to scan the
Local Management register space instead, failing to find PCI_CAP_ID_EXP
and returning 0?

[ ... ]

> @@ -68,27 +68,26 @@ EXPORT_SYMBOL_GPL(cdns_pcie_host_wait_for_link);
>  int cdns_pcie_retrain(struct cdns_pcie *pcie,
>  		      cdns_pcie_linkup_func pcie_link_up)
>  {
> -	u32 lnk_cap_sls, pcie_cap_off = CDNS_PCIE_RP_CAP_OFFSET;
> +	u32 lnk_cap_sls;
>  	u16 lnk_stat, lnk_ctl;
>  	int ret = 0;
> +	u8 cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP);
>  
>  	/*
>  	 * Set retrain bit if current speed is 2.5 GB/s,
>  	 * but the PCIe root port support is > 2.5 GB/s.
>  	 */
>  
> -	lnk_cap_sls = cdns_pcie_readl(pcie, (CDNS_PCIE_RP_BASE + pcie_cap_off +
> +	lnk_cap_sls = cdns_pcie_readl(pcie, (CDNS_PCIE_RP_BASE + cap +
>  					     PCI_EXP_LNKCAP));

If the capability is not found and cap evaluates to 0, is it safe to proceed
without validating the return value?

Since the fallback value would be 0, evaluating cap + PCI_EXP_LNKCAP will
result in an offset of 0x0C.

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 0bc9e6e90e0e..2a3fd41c1cf4 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -147,12 +148,12 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>  	cdns_pcie_rp_writeb(pcie, PCI_CLASS_PROG, 0);
>  	cdns_pcie_rp_writew(pcie, PCI_CLASS_DEVICE, PCI_CLASS_BRIDGE_PCI);
>  
> -	value = cdns_pcie_rp_readl(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_LNKCAP);
> +	value = cdns_pcie_rp_readl(pcie, cap + PCI_EXP_LNKCAP);
>  	if (rc->quirk_broken_aspm_l0s)
>  		value &= ~PCI_EXP_LNKCAP_ASPM_L0S;
>  	if (rc->quirk_broken_aspm_l1)
>  		value &= ~PCI_EXP_LNKCAP_ASPM_L1;
> -	cdns_pcie_rp_writel(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_LNKCAP, value);
> +	cdns_pcie_rp_writel(pcie, cap + PCI_EXP_LNKCAP, value);

Will this overwrite the wrong configuration registers if cap is 0?

The cdns_pcie_rp_writel() function automatically adds CDNS_PCIE_RP_BASE to
the destination offset. If cap is 0, writing to cap + PCI_EXP_LNKCAP will
access offset 0x0C.

Could this regression unintentionally clobber the standard Type 1 PCI header
registers, such as the Cache Line Size and Latency Timer, and corrupt the
Root Port hardware state?

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