linux-kernel - Re: [PATCH v3 5/6] PCI: cadence: Add callback functions for RP and EP controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <07457d00-82bb-4096-ba07-6cc7b7d118e3@cixtech.com>
Date: Sat, 12 Apr 2025 23:45:03 +0800
From: Hans Zhang <hans.zhang@...tech.com>
To: Rob Herring <robh@...nel.org>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
 manivannan.sadhasivam@...aro.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Manikandan K Pillai <mpillai@...ence.com>
Subject: Re: [PATCH v3 5/6] PCI: cadence: Add callback functions for RP and EP
 controller

On 2025/4/12 04:24, Rob Herring wrote:
>> +void __iomem *cdns_pci_hpa_map_bus(struct pci_bus *bus, unsigned int devfn,
>> +                                int where)
>> +{
>> +     struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
>> +     struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
>> +     struct cdns_pcie *pcie = &rc->pcie;
>> +     unsigned int busn = bus->number;
>> +     u32 addr0, desc0, desc1, ctrl0;
>> +     u32 regval;
>> +
>> +     if (pci_is_root_bus(bus)) {
>> +             /*
>> +              * Only the root port (devfn == 0) is connected to this bus.
>> +              * All other PCI devices are behind some bridge hence on another
>> +              * bus.
>> +              */
>> +             if (devfn)
>> +                     return NULL;
>> +
>> +             return pcie->reg_base + (where & 0xfff);
>> +     }
>> +
>> +     /*
>> +      * Clear AXI link-down status
>> +      */
> 
> That is an odd thing to do in map_bus. Also, it is completely racy
> because...
> 
>> +     regval = cdns_pcie_hpa_readl(pcie, REG_BANK_AXI_SLAVE, CDNS_PCIE_HPA_AT_LINKDOWN);
>> +     cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE, CDNS_PCIE_HPA_AT_LINKDOWN,
>> +                          (regval & GENMASK(0, 0)));
>> +
> 
> What if the link goes down again here.
> 

Hi Rob,

Thanks your for reply. Compared to Synopsys PCIe IP, Cadence PCIe IP has 
one more register - CDNS_PCIE_HPA_AT_LINKDOWN.  When the PCIe link 
appears link down, the register -CDNS_PCIE_HPA_AT_LINKDOWN bit0 is set 
to 1.  Then, ECAM cannot access config space. You need to clear 
CDNS_PCIE_HPA_AT_LINKDOWN bit0 to continue the access.

In my opinion, this is where Cadence PCIe IP doesn't make sense.  As 
Cadence users, we had no choice, and the chip had already been posted to 
silicon.

Therefore, when we design the second-generation SOC, we will design an 
SPI interrupt. When link down occurs, an SPI interrupt is generated. We 
set CDNS_PCIE_HPA_AT_LINKDOWN bit0 to 0 in the interrupt function.

If there are other reasons, please Manikandan add.

In addition, by the way, ECAM is not accessible when the link is down. 
For example, if the RP is set to hot reset, the hot reset cannot be 
unreset. In this case, you need to use the APB to unreset. We mentioned 
an RTL bug to Cadence that we currently can't fix with our first or 
second generation chips. Cadence has not released RTL patch to us so far.

This software workaround approach will also later appear in the Cixtech 
PCIe controller series patch.

Best regards,
Hans