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

On 5/7/26 20:31, Aksh Garg wrote:
> 
> 
> On 07/05/26 09:18, Hans Zhang wrote:
>>
>>
>> On 5/7/26 11:31, Hans Zhang wrote:
>>>
>>>
>>> On 5/7/26 01:12, Bjorn Helgaas wrote:
>>>> On Thu, May 07, 2026 at 12:04:34AM +0800, Hans Zhang wrote:
>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> I think cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP) fails on Root
>>>> Ports because it doesn't include the CDNS_PCIE_RP_BASE offset.  Do you
>>>> have hardware where you can test that?
>>
>> Hi Siddharth,
>>
>> Could you please test the functions mentioned above? It would be great 
>> if you could help test it and give us your feedback. (pci-j721e.c)
>>
>> I guess the following changes might need to be made.
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/ 
>> pci/controller/cadence/pcie-cadence.h
>> index 574e9cf4d003..bb01761749f1 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -298,6 +298,9 @@ static inline int cdns_pcie_read_cfg_byte(struct 
>> cdns_pcie *pcie, int where,
>>   {
>>          void __iomem *addr = pcie->reg_base + where;
>>
>> +       if ((pcie->is_rc) && (!pcie->is_hpa))
>> +               addr += CDNS_PCIE_RP_BASE;
>> +
>>          *val = cdns_pcie_read_sz(addr, 0x1);
>>          return PCIBIOS_SUCCESSFUL;
>>   }
>> @@ -307,6 +310,9 @@ static inline int cdns_pcie_read_cfg_word(struct 
>> cdns_pcie *pcie, int where,
>>   {
>>          void __iomem *addr = pcie->reg_base + where;
>>
>> +       if ((pcie->is_rc) && (!pcie->is_hpa))
>> +               addr += CDNS_PCIE_RP_BASE;
>> +
>>          *val = cdns_pcie_read_sz(addr, 0x2);
>>          return PCIBIOS_SUCCESSFUL;
>>   }
>> @@ -314,7 +320,12 @@ static inline int cdns_pcie_read_cfg_word(struct 
>> cdns_pcie *pcie, int where,
>>   static inline int cdns_pcie_read_cfg_dword(struct cdns_pcie *pcie, 
>> int where,
>>                                             u32 *val)
>>   {
>> -       *val = cdns_pcie_readl(pcie, where);
>> +       void __iomem *addr = pcie->reg_base + where;
>> +
>> +       if ((pcie->is_rc) && (!pcie->is_hpa))
>> +               addr += CDNS_PCIE_RP_BASE;
>> +
>> +       *val = cdns_pcie_read_sz(addr, 0x4);
>>          return PCIBIOS_SUCCESSFUL;
>>   }
>>
>>
>> "is_hpa" flag I added it in another series. As mentioned before, the 
>> compatibility of Cadence IP is not very good. It is divided into LGA 
>> and HPA IP. And there are many differences between the Root Port and 
>> the Endpoint.
>>
>> https://patchwork.kernel.org/project/linux-pci/ 
>> patch/[email protected]/
>>
>> Best regards,
>> Hans
>>
> 
> Hi Hans,
> 
> I have tested this on J7200 and J721E SoCs, which uses pci-j721e.c
> driver for their PCIe controllers. The patch alone fails to find the
> capabilities using cdns_pcie_find_capability() for Root Port.
> 
> With the "is_hpa" flag patch, and the patch "PCI: cadence: Use
> cdns_pcie_find_capability() to get PCIe Cap offset in host driver" along
> with the diff suggested above:

Thank you very much for your test. We need to wait for Bjorn's approval.

> 
> Tested-by: Aksh Garg <[email protected]>

When the future patch is sent to the PCI mailing list, if you test it at 
that time, please reply to this tag again.

> 
> 
> I would also like me to be included in the CC list for future
> correspondence regarding Cadence PCIe Controller patches.
> 

Sure. In the future, when I send the patch, I will CC it to you in the 
email.

Best regards,
Hans

> Regards,
> Aksh Garg
> 
>>>
>>> Hi Bjorn,
>>>
>>> The attachment contains the dmesg and lspci -vvv log from the test.
>>>
>>> Here is the test code:
>>>
>>> diff --git a/drivers/pci/controller/cadence/pci-sky1.c b/drivers/pci/ 
>>> controller/cadence/pci-sky1.c
>>> index cd55c64e58a9..0d0c42309127 100644
>>> --- a/drivers/pci/controller/cadence/pci-sky1.c
>>> +++ b/drivers/pci/controller/cadence/pci-sky1.c
>>> @@ -130,6 +130,39 @@ static const struct cdns_pcie_ops sky1_pcie_ops = {
>>>       .link_up = sky1_pcie_link_up,
>>>   };
>>>
>>> +void cix_pcie_test_cap(struct sky1_pcie *pcie)
>>> +{
>>> +    u16 offset;
>>> +
>>> +    printk(KERN_EMERG"[HANS] fun = %s, line = %d ........... \n", 
>>> __func__, __LINE__);
>>> +    /* capability */
>>> +    offset = cdns_pcie_find_capability(pcie->cdns_pcie, PCI_CAP_ID_PM);
>>> +    printk(KERN_EMERG"[HANS]pm offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_capability(pcie->cdns_pcie, 
>>> PCI_CAP_ID_MSI);
>>> +    printk(KERN_EMERG"[HANS]msi offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_capability(pcie->cdns_pcie, 
>>> PCI_CAP_ID_MSIX);
>>> +    printk(KERN_EMERG"[HANS]msix offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_capability(pcie->cdns_pcie, 
>>> PCI_CAP_ID_EXP);
>>> +    printk(KERN_EMERG"[HANS]exp offset = 0x%x\n", offset);
>>> +
>>> +    printk(KERN_EMERG"[HANS] fun = %s, line = %d ........... \n", 
>>> __func__, __LINE__);
>>> +    /* extend capability */
>>> +    offset = cdns_pcie_find_ext_capability(pcie->cdns_pcie, 
>>> PCI_EXT_CAP_ID_ERR);
>>> +    printk(KERN_EMERG"[HANS]aer offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_ext_capability(pcie->cdns_pcie, 
>>> PCI_EXT_CAP_ID_VC);
>>> +    printk(KERN_EMERG"[HANS]Virtual Channel offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_ext_capability(pcie->cdns_pcie, 
>>> PCI_EXT_CAP_ID_DSN);
>>> +    printk(KERN_EMERG"[HANS]Device Serial Number offset = 0x%x\n", 
>>> offset);
>>> +    offset = cdns_pcie_find_ext_capability(pcie->cdns_pcie, 
>>> PCI_EXT_CAP_ID_PWR);
>>> +    printk(KERN_EMERG"[HANS]pwr offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_ext_capability(pcie->cdns_pcie, 
>>> PCI_EXT_CAP_ID_REBAR);
>>> +    printk(KERN_EMERG"[HANS]resize bar offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_ext_capability(pcie->cdns_pcie, 
>>> PCI_EXT_CAP_ID_SECPCI);
>>> +    printk(KERN_EMERG"[HANS]second exp offset = 0x%x\n", offset);
>>> +    offset = cdns_pcie_find_ext_capability(pcie->cdns_pcie, 
>>> PCI_EXT_CAP_ID_L1SS);
>>> +    printk(KERN_EMERG"[HANS]L1ss offset = 0x%x\n", offset);
>>> +}
>>> +
>>>   static int sky1_pcie_probe(struct platform_device *pdev)
>>>   {
>>>       struct cdns_plat_pcie_of_data *reg_off;
>>> @@ -141,6 +174,7 @@ static int sky1_pcie_probe(struct platform_device 
>>> *pdev)
>>>       struct sky1_pcie *pcie;
>>>       int ret;
>>>
>>> +    printk(KERN_EMERG"[HANS] fun = %s, line = %d ........... \n", 
>>> __func__, __LINE__);
>>>       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>>>       if (!pcie)
>>>           return -ENOMEM;
>>> @@ -202,6 +236,8 @@ static int sky1_pcie_probe(struct platform_device 
>>> *pdev)
>>>
>>>       dev_set_drvdata(dev, pcie);
>>>
>>> +    cix_pcie_test_cap(pcie);
>>> +
>>>       ret = cdns_pcie_hpa_host_setup(rc);
>>>       if (ret < 0) {
>>>           pci_ecam_free(pcie->cfg);
>>> @@ -230,7 +266,7 @@ static struct platform_driver sky1_pcie_driver = {
>>>       .driver = {
>>>           .name = "sky1-pcie",
>>>           .of_match_table = of_sky1_pcie_match,
>>> -        .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>> +        // .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>>       },
>>>   };
>>>   module_platform_driver(sky1_pcie_driver);
>>>
>>>
>>> For the LGA IP, an offset address CDNS_PCIE_RP_BASE needs to be 
>>> added. And the HPA IP requires an offset address of 
>>> CDNS_PCIE_HPA_RP_BASE, but it is equal to 0;
>>>
>>> drivers/pci/controller/cadence/pcie-cadence-hpa-regs.h
>>>
>>> /* Root Port register base address */
>>> #define CDNS_PCIE_HPA_RP_BASE            0x0
>>>
>>> Therefore, for "drivers/pci/controller/cadence/pci-sky1.c", it is 
>>> normal.
>>>
>>>
>>>
>>> Using the following code for work is also normal. Later, I will re- 
>>> unify it into the same operation.
>>>
>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/ 
>>> pci/controller/cadence/pcie-cadence.h
>>> index 574e9cf4d003..723ea79fb02e 100644
>>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>>> @@ -314,7 +314,9 @@ static inline int cdns_pcie_read_cfg_word(struct 
>>> cdns_pcie *pcie, int where,
>>>   static inline int cdns_pcie_read_cfg_dword(struct cdns_pcie *pcie, 
>>> int where,
>>>                                             u32 *val)
>>>   {
>>> -       *val = cdns_pcie_readl(pcie, where);
>>> +       void __iomem *addr = pcie->reg_base + where;
>>> +
>>> +       *val = cdns_pcie_read_sz(addr, 0x4);
>>>          return PCIBIOS_SUCCESSFUL;
>>>   }
>>>
>>>
>>> Best regards,
>>> Hans
>>>
>>>
>>