Re: [PATCH v4 1/2] PCI: dwc: Add support for vendor specific capability search

[Bjorn Helgaas]

On Wed, Jan 15, 2025 at 08:57:42PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 11, 2024 at 08:43:30AM -0600, Bjorn Helgaas wrote:
> > On Wed, Dec 11, 2024 at 05:15:50PM +0530, Shradha Todi wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > > Sent: 06 December 2024 21:43
> > > > To: Shradha Todi <shradha.t@xxxxxxxxxxx>
> > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; manivannan.sadhasivam@xxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> > > > kw@xxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; jingoohan1@xxxxxxxxx; Jonathan.Cameron@xxxxxxxxxx;
> > > > fan.ni@xxxxxxxxxxx; a.manzanares@xxxxxxxxxxx; pankaj.dubey@xxxxxxxxxxx; quic_nitegupt@xxxxxxxxxxx;
> > > > quic_krichai@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx
> > > > Subject: Re: [PATCH v4 1/2] PCI: dwc: Add support for vendor specific capability search
> > > > 
> > > > On Fri, Dec 06, 2024 at 01:14:55PM +0530, Shradha Todi wrote:
> > > > > Add vendor specific extended configuration space capability search API
> > > > > using struct dw_pcie pointer for DW controllers.
> > > > >
> > > > > Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++++++++
> > > > > drivers/pci/controller/dwc/pcie-designware.h |  1 +
> > > > >  2 files changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > index 6d6cbc8b5b2c..41230c5e4a53 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > @@ -277,6 +277,22 @@ static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u8 vsec_cap)
> > > > 
> > > > To make sure that we find a VSEC ID that corresponds to the
> > > > expected vendor, I think this interface needs to be the same
> > > > as pci_find_vsec_capability().  In particular, it needs to take a
> > > > "u16 vendor"
> > >
> > > As per my understanding, Synopsys is the vendor here when we talk
> > > about vsec capabilities.  VSEC cap IDs are fixed for each vendor
> > > (eg: For Synopsys Designware controllers, 0x2 is always RAS CAP, 0x4
> > > is always PTM responder and so on).
> > 
> > For VSEC, the vendor that matters is the one identified at 0x0 in
> > config space.  That's why pci_find_vsec_capability() checks the
> > supplied "vendor" against "dev->vendor".
> > 
> > > So no matter if the DWC IP is being integrated by Samsung, NVDIA or
> > > Qcom, the vendor specific CAP IDs will remain constant. Now since
> > > this function is being written as part of designware file, the
> > > control will reach here only when the PCIe IP is DWC. So, we don't
> > > really require a vendor ID to be checked here. EG: If 0x2 VSEC ID is
> > > present in any DWC controller, it means RAS is supported. Please
> > > correct me if I'm wrong.
> > 
> > In this case, the Vendor ID is typically Samsung, NVIDIA, Qcom, etc.,
> > even though it may contain Synopsys DWC IP.  Each vendor assigns VSEC
> > IDs independently, so VSEC ID 0x2 may mean something different to
> > Samsung than it does to NVIDIA or Qcom.
> > 
> > PCIe r6.0, sec 7.9.5 has the details, but the important part is this:
> > 
> >   With a PCI Express Function, the structure and definition of the
> >   vendor-specific Registers area is determined by the vendor indicated
> >   by the Vendor ID field located at byte offset 00h in PCI-compatible
> >   Configuration Space.
> > 
> > There IS a separate DVSEC ("Designated Vendor-Specific") Capability;
> > see sec 7.9.6.  That one does include a DVSEC Vendor ID in the
> > Capability itself, and this would make more sense for this situation.
> > 
> > If Synopsys assigned DVSEC ID 0x2 from the Synopsys namespace for RAS,
> > then devices from Samsung, NVIDIA, Qcom, etc., could advertise a DVSEC
> > Capability that contained a DVSEC Vendor ID of PCI_VENDOR_ID_SYNOPSYS
> > with DVSEC ID 0x2, and all those devices could easily locate it.
> > 
> > Unfortunately Samsung et al used VSEC instead of DVSEC, so we're stuck
> > with having to specify the device vendor and the VSEC ID assigned by
> > that vendor, and those VSEC IDs might be different per vendor.
> 
> Atleast on Qcom platforms, VSEC_ID is 0x2 for RAS. But this is not
> guaranteed to be the same as per the PCIe spec as you mentioned.
> Though, I think it is safe to go with it since we have seen the same
> IDs on 2 platforms (my gut feeling is that it is going to be the
> same on other DWC vendor platforms as well). If we encounter
> different IDs, then we can add vendor id check.

This series uses:

  dw_pcie_find_vsec_capability(pci, DW_PCIE_VSEC_EXT_CAP_RAS_DES)

in dwc_pcie_rasdes_debugfs_init(), but I don't see any calls of that
function yet.  If it is called only from code that already knows the
device vendor has assigned VSEC ID 0x02 for the DWC RAS functionality,
I guess it is "safe".

But I think it would be a bad idea because it perpetuates the
misunderstanding that DesignWare can independently claim ownership of
VSEC ID 0x02 for *all* vendors, and other vendors have already used
VSEC ID 0x02 for different things (examples at [1]).  If any of them
incorporates this DWC IP, they will have to use a different VSEC ID to
avoid a collision with their existing VSEC ID 0x02.

Bjorn

[1] https://lore.kernel.org/r/20241209222938.3219364-1-helgaas@xxxxxxxxxx/