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

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.


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.

  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().

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