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

["Shradha Todi" <shradha.t@samsung.com>]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 15 January 2025 22:00
> To: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Shradha Todi <shradha.t@samsung.com>; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> fan.ni@samsung.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> quic_krichai@quicinc.com; gost.dev@samsung.com
> Subject: Re: [PATCH v4 1/2] PCI: dwc: Add support for vendor specific capability search
> 
> 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@kernel.org>
> > > > > > Sent: 06 December 2024 21:43
> > > > > > To: Shradha Todi <shradha.t@samsung.com>
> > > > > > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org;
> > > > > > manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> > > > > > kw@linux.com; robh@kernel.org; bhelgaas@google.com;
> > > > > > jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> > > > > > fan.ni@samsung.com; a.manzanares@samsung.com;
> > > > > > pankaj.dubey@samsung.com; quic_nitegupt@quicinc.com;
> > > > > > quic_krichai@quicinc.com; gost.dev@samsung.com
> > > > > > 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@samsung.com>
> > > > > > > ---
> > > > > > >  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.

Actually the missing caller is intentional. Jonathan rightly pointed out in the
previous version that the function : dw_pcie_setup() was being called in the
resume path as well and so I thought it would be best to leave it up to the
platform drivers to decide when and how to call the rasdes init. Do you suggest any
other approach?

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

Makes sense to add the vendor ID check. Will include it in the next version.

> 
> 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
> 
> --
> மணிவண்ணன் சதாசிவம்