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

[Manivannan Sadhasivam]

On Wed, Jan 15, 2025 at 10:12:01AM -0600, Bjorn Helgaas wrote:
> 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.

I guess that the caller got missed unintentionally in patch 2/2.

>  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".
> 

It should be called from the DWC code driver (pcie-desginware-host.c).

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

Fair enough. I was trying to avoid updating the vendor id table for enabling the
RAS DES debug feature, but I think it would be worth doing so (perf driver is
also doing the same).

So yeah, I'm OK with the idea of having the vendor_id check in this API.

(Also, I don't see the VSEC_IDs defined in the DWC reference manual that I got
access to).

- Mani

-- 
மணிவண்ணன் சதாசிவம்