linux-kernel - Re: [v2] PCI: cadence: Add configuration space capability search API

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] [day] [month] [year] [list]
Message-ID: <20250309100243.ihrxe6vfdugzpzsn@uda0492258>
Date: Sun, 9 Mar 2025 15:32:43 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Hans Zhang <18255117159@....com>
CC: Siddharth Vadapalli <s-vadapalli@...com>, <lpieralisi@...nel.org>,
        <kw@...ux.com>, <manivannan.sadhasivam@...aro.org>, <robh@...nel.org>,
        <bhelgaas@...gle.com>, <bwawrzyn@...co.com>,
        <thomas.richard@...tlin.com>,
        <wojciech.jasko-EXT@...tinental-corporation.com>,
        <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [v2] PCI: cadence: Add configuration space capability search API

On Sun, Mar 09, 2025 at 05:49:09PM +0800, Hans Zhang wrote:
> 
> 
> On 2025/3/9 13:48, Siddharth Vadapalli wrote:
> > On Sun, Mar 09, 2025 at 11:18:21AM +0800, Hans Zhang wrote:
> > > 
> > > 
> > > On 2025/3/9 10:38, Siddharth Vadapalli wrote:
> > > > On Sat, Mar 08, 2025 at 09:39:03PM +0800, Hans Zhang wrote:
> > > > > Add configuration space capability search API using struct cdns_pcie*
> > > > > pointer.
> > > > > 
> > > > > The offset address of capability or extended capability designed by
> > > > > different SOC design companies may not be the same. Therefore, a flexible
> > > > > public API is required to find the offset address of a capability or
> > > > > extended capability in the configuration space.
> > > > > 
> > > > > Signed-off-by: Hans Zhang <18255117159@....com>
> > > > > ---
> > > > > Changes since v1:
> > > > > https://lore.kernel.org/linux-pci/20250123070935.1810110-1-18255117159@163.com
> > > > > 
> > > > > - Added calling the new API in PCI-Cadence ep.c.
> > > > > - Add a commit message reason for adding the API.
> > > > 
> > > > In reply to your v1 patch, you have mentioned the following:
> > > > "Our controller driver currently has no plans for upstream and needs to
> > > > wait for notification from the boss."
> > > > at:
> > > > https://lore.kernel.org/linux-pci/fcfd4827-4d9e-4bcd-b1d0-8f9e349a6be7@163.com/
> > > > 
> > > > Since you have posted this patch, does it mean that you will be
> > > > upstreaming your driver as well? If not, we still end up in the same
> > > > situation as earlier where the Upstream Linux has APIs to support a
> > > > Downstream driver.
> > > > 
> > > > Bjorn indicated the above already at:
> > > > https://lore.kernel.org/linux-pci/20250123170831.GA1226684@bhelgaas/
> > > > and you did agree to do so. But this patch has no reference to the
> > > > upstream driver series which shall be making use of the APIs in this
> > > > patch.
> > > 
> > > Hi Siddharth,
> > > 
> > > 
> > > Bjorn:
> > >    If/when you upstream code that needs this interface, include this
> > >    patch as part of the series.  As Siddharth pointed out, we avoid
> > >    merging code that has no upstream users.
> > > 
> > > 
> > > Hans: This user is: pcie-cadence-ep.c. I think this is an optimization of
> > > Cadence common code. I think this is an optimization of Cadence common code.
> > > Siddharth, what do you think?
> > 
> > This seems to be an extension of the driver rather than an optimization.
> > At first glance, though it seems like this patch is enabling code-reuse,
> > it is actually attempting to walk through the config space registers to
> > identify a capability. Prior to this patch, those offsets were hard-coded,
> > saving the trouble of having to walk through the capability pointers to
> > arrive at the capability.
> 
> Hi Siddharth,
> 
> Prior to this patch, I don't think hard-coded is that reasonable. Because
> the SOC design of each company does not guarantee that the offset of each
> capability is the same. This parameter can be configured when selecting PCIe
> configuration options. The current code that just happens to hit the offset
> address is the same.

1. You are modifying the driver for the Cadence PCIe Controller IP and
not the one for your SoC (a.k.a the application/glue/wrapper driver).
2. The offsets are tied to the Controller IP and not to your SoC. Any
differences that arise due to IP Integration into your SoC should be
handled in the Glue driver (the one which you haven't upstreamed yet).
3. If the offsets in the Controller IP itself have changed, this
indicates a different IP version which has nothing to do with the SoC
that you are using.

Please clarify whether you are facing problems with the offsets due to a
difference in the IP Controller Version, or due to the way in which the IP
was integrated into your SoC.

> 
> You can refer to the pci_find_*capability() or dw_pcie_find_*capability API.
> The meaning of their appearance is the same as what I said, and the design
> of each company may be different.

These APIs exits since there are multiple users with different Controllers
(in the case of pci_find_*capability()) or different versions of the
Controller (in the case of dw_pcie_find_*capability) due to which we cannot
hard-code offsets. In the case of the Cadence PCIe Controller, I see only
one user in Upstream Linux at the moment which happens to be the
"pci-j721e.c" driver. Maybe there are other users, but the offsets seem
to be compatible with their SoCs in that case.

Regards,
Siddharth.