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

On 5/6/26 05:23, Bjorn Helgaas wrote:
> On Mon, May 04, 2026 at 04:22:26PM +0800, Hans Zhang wrote:
>> On 5/4/26 00:52, [email protected] 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 <[email protected]>
>>>
>>> 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.
> 
> It's true that all Root Ports must have a PCIe Capability, but
> that's not related to this issue.
> 
> cdns_pcie_host_init_root_port() accesses PCI_EXP_LNKCAP at the
> address:
> 
>    pcie->reg_base + CDNS_PCIE_RP_BASE + CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_LNKCAP
> 
> but when we search with cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP),
> we start reading at:
> 
>    pcie->reg_base + PCI_CAPABILITY_LIST
> 
> It should be starting at:
> 
>    pcie->reg_base + CDNS_PCIE_RP_BASE + PCI_CAPABILITY_LIST
> 
> Previously, cdns_pcie_find_capability() and
> cdns_pcie_find_ext_capability() were only used for endpoints, and I
> assume they work fine there.  There is pcie->is_rc, so there should be
> a way to make this work for both endpoints and Root Ports.
> 

Hi Bjorn,

The reason for using the "is_rc" tag is that for Cadence IP, it is not 
only applicable to the RC or EP mode, but also there are significant 
differences between the LGA and HPA generations of IP. Including 
register offset values, definitions, etc., it fails to achieve good 
compatibility. It is not like Synopsys IP, where compatibility has been 
handled very well. It was truly out of necessity.

> 
> Separate issue: I think some of the config accessors ended up being
> sub-optimal.  They should all have the same structure, but
> cdns_pcie_read_cfg_dword() is different from the rest, I think because
> some devices don't support 1- and 2-byte accesses.


Yes, it has been changed in the following submission.

Commit: d9cf7154deed (PCI: cadence: Use cdns_pcie_read_sz() for byte or 
word read access)

In my submission 18ac51ae9df9 ("PCI: cadence: Implement capability 
search using PCI core APIs"). It works very well for the Root Port. (For 
drivers/pci/controller/cadence/pci-sky1.c, no other upstream code has 
been implemented yet. It is functioning properly in the product.)

> 
>    cdns_pcie_read_cfg_byte
>      addr = pcie->reg_base + where
>      cdns_pcie_read_sz(addr, 0x1)
>       readl(addr)
> 
>    cdns_pcie_read_cfg_word
>      addr = pcie->reg_base + where
>      cdns_pcie_read_sz(addr, 0x2)
>       readl(addr)
> 
>    cdns_pcie_read_cfg_dword
>      cdns_pcie_readl
>        readl(pcie->reg_base + reg)
> 
> I think it would be better to use "cdns_pcie_read_sz(addr, 0x4)"
> even for cdns_pcie_read_cfg_dword().

Agree. I will make some revisions. I've recently found some time to 
conduct a CIX SOC test.

> 
>>> [ ... ]
>>>
>>>> @@ -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));
> 
> Could reduce the size of the diff by keeping the original
> "pcie_cap_off" name.

I would like to refer to:

struct pci_dev {
     ......
     u8			pcie_cap;	/* PCIe capability offset */
     ......
};

Additionally, I have already created a series of patches here locally to 
simplify the DWC driver. This is because each Root Port driver is 
looking at the PCI_CAP_ID_EXP offset.

Best regards,
Hans