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

[Bjorn Helgaas <helgaas@kernel.org>]

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, 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.
> > 
> > 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?