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

[Hans Zhang <18255117159@163.com>]

On 5/4/26 00:52, sashiko-bot@kernel.org wrote:
> 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?

I don't think we need to worry about this issue.

As per PCIe r7.0, sec 7.5.1.1.11, Since all PCI Express Functions

are required to implement the PCI Express Capability structure, which 
must be included somewhere in this linked list;

So, it won't return 0 here.

Best regards,
Hans


> 
> [ ... ]
> 
>> @@ -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?
>