| Message ID | 20240409160256.94184-2-dave.jiang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Add Secondary Bus Reset (SBR) support for CXL | expand |
On 4/9/24 9:01 AM, Dave Jiang wrote: > Move PCI_DVSEC_VENDOR_ID_CXL in CXL private code to PCI_VENDOR_ID_CXL in > pci_ids.h in order to be utilized in PCI subsystem. > > The response Bjorn received from the PCI SIG was "1E98h is not a VID in our > system, but 1E98 has already been reserved by CXL." He suggested "we should > add '#define PCI_VENDOR_ID_CXL 0x1e98' so that if we ever *do* see such an > assignment, we'll be more likely to flag it as an issue. Nit: Instead of including the comments as-it-is, I think it is better just state the conclusion. > > Link: https://lore.kernel.org/linux-cxl/20240402172323.GA1818777@bhelgaas/ > Suggested-by: Bjorn Helgaas <helgaas@kernel.org> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > drivers/cxl/core/pci.c | 6 +++--- > drivers/cxl/core/regs.c | 2 +- > drivers/cxl/cxlpci.h | 1 - > drivers/cxl/pci.c | 2 +- > drivers/perf/cxl_pmu.c | 2 +- > include/linux/pci_ids.h | 2 ++ > 6 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 0df09bd79408..c496a9710d62 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -525,7 +525,7 @@ static int cxl_cdat_get_length(struct device *dev, > __le32 response[2]; > int rc; > > - rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL, > + rc = pci_doe(doe_mb, PCI_VENDOR_ID_CXL, > CXL_DOE_PROTOCOL_TABLE_ACCESS, > &request, sizeof(request), > &response, sizeof(response)); > @@ -555,7 +555,7 @@ static int cxl_cdat_read_table(struct device *dev, > __le32 request = CDAT_DOE_REQ(entry_handle); > int rc; > > - rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL, > + rc = pci_doe(doe_mb, PCI_VENDOR_ID_CXL, > CXL_DOE_PROTOCOL_TABLE_ACCESS, > &request, sizeof(request), > rsp, sizeof(*rsp) + remaining); > @@ -640,7 +640,7 @@ void read_cdat_data(struct cxl_port *port) > if (!pdev) > return; > > - doe_mb = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL, > + doe_mb = pci_find_doe_mailbox(pdev, PCI_VENDOR_ID_CXL, > CXL_DOE_PROTOCOL_TABLE_ACCESS); > if (!doe_mb) { > dev_dbg(dev, "No CDAT mailbox\n"); > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index 372786f80955..da52fc9e234b 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -313,7 +313,7 @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, > .resource = CXL_RESOURCE_NONE, > }; > > - regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, > + regloc = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, > CXL_DVSEC_REG_LOCATOR); > if (!regloc) > return -ENXIO; > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h > index 93992a1c8eec..4da07727ab9c 100644 > --- a/drivers/cxl/cxlpci.h > +++ b/drivers/cxl/cxlpci.h > @@ -13,7 +13,6 @@ > * "DVSEC" redundancies removed. When obvious, abbreviations may be used. > */ > #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) > -#define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 > > /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ > #define CXL_DVSEC_PCIE_DEVICE 0 > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 2ff361e756d6..110478573296 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -817,7 +817,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > cxlds->rcd = is_cxl_restricted(pdev); > cxlds->serial = pci_get_dsn(pdev); > cxlds->cxl_dvsec = pci_find_dvsec_capability( > - pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); > + pdev, PCI_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); > if (!cxlds->cxl_dvsec) > dev_warn(&pdev->dev, > "Device DVSEC not present, skip CXL.mem init\n"); > diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c > index 308c9969642e..a1b742b1a735 100644 > --- a/drivers/perf/cxl_pmu.c > +++ b/drivers/perf/cxl_pmu.c > @@ -345,7 +345,7 @@ static ssize_t cxl_pmu_event_sysfs_show(struct device *dev, > > /* For CXL spec defined events */ > #define CXL_PMU_EVENT_CXL_ATTR(_name, _gid, _msk) \ > - CXL_PMU_EVENT_ATTR(_name, PCI_DVSEC_VENDOR_ID_CXL, _gid, _msk) > + CXL_PMU_EVENT_ATTR(_name, PCI_VENDOR_ID_CXL, _gid, _msk) > > static struct attribute *cxl_pmu_event_attrs[] = { > CXL_PMU_EVENT_CXL_ATTR(clock_ticks, CXL_PMU_GID_CLOCK_TICKS, BIT(0)), > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index a0c75e467df3..7dfbf6d96b3d 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2607,6 +2607,8 @@ > > #define PCI_VENDOR_ID_ALIBABA 0x1ded > > +#define PCI_VENDOR_ID_CXL 0x1e98 > + > #define PCI_VENDOR_ID_TEHUTI 0x1fc9 > #define PCI_DEVICE_ID_TEHUTI_3009 0x3009 > #define PCI_DEVICE_ID_TEHUTI_3010 0x3010
Dave Jiang wrote: > Move PCI_DVSEC_VENDOR_ID_CXL in CXL private code to PCI_VENDOR_ID_CXL in > pci_ids.h in order to be utilized in PCI subsystem. > > The response Bjorn received from the PCI SIG was "1E98h is not a VID in our > system, but 1E98 has already been reserved by CXL." He suggested "we should > add '#define PCI_VENDOR_ID_CXL 0x1e98' so that if we ever *do* see such an > assignment, we'll be more likely to flag it as an issue. Perhaps indent these as quotes, the second quote is missing a trailing double-quote. ...but maybe even better to justify this change rather than the reaction to the change. Something like: -- When uplevelling PCI_DVSEC_VENDOR_ID_CXL to a common location Bjorn suggested making it a proper PCI_VENDOR_ID_* define in include/linux/pci_ids.h. While it is not in the PCI IDs database it is a reserved value and Linux treats it as a 'vendor id' for all intents and purposes [1]. --- ...where that footnote can link back to the conversation for the background. Other than that, you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com>
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 0df09bd79408..c496a9710d62 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -525,7 +525,7 @@ static int cxl_cdat_get_length(struct device *dev, __le32 response[2]; int rc; - rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL, + rc = pci_doe(doe_mb, PCI_VENDOR_ID_CXL, CXL_DOE_PROTOCOL_TABLE_ACCESS, &request, sizeof(request), &response, sizeof(response)); @@ -555,7 +555,7 @@ static int cxl_cdat_read_table(struct device *dev, __le32 request = CDAT_DOE_REQ(entry_handle); int rc; - rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL, + rc = pci_doe(doe_mb, PCI_VENDOR_ID_CXL, CXL_DOE_PROTOCOL_TABLE_ACCESS, &request, sizeof(request), rsp, sizeof(*rsp) + remaining); @@ -640,7 +640,7 @@ void read_cdat_data(struct cxl_port *port) if (!pdev) return; - doe_mb = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL, + doe_mb = pci_find_doe_mailbox(pdev, PCI_VENDOR_ID_CXL, CXL_DOE_PROTOCOL_TABLE_ACCESS); if (!doe_mb) { dev_dbg(dev, "No CDAT mailbox\n"); diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index 372786f80955..da52fc9e234b 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -313,7 +313,7 @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, .resource = CXL_RESOURCE_NONE, }; - regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, + regloc = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL, CXL_DVSEC_REG_LOCATOR); if (!regloc) return -ENXIO; diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h index 93992a1c8eec..4da07727ab9c 100644 --- a/drivers/cxl/cxlpci.h +++ b/drivers/cxl/cxlpci.h @@ -13,7 +13,6 @@ * "DVSEC" redundancies removed. When obvious, abbreviations may be used. */ #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) -#define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ #define CXL_DVSEC_PCIE_DEVICE 0 diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 2ff361e756d6..110478573296 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -817,7 +817,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) cxlds->rcd = is_cxl_restricted(pdev); cxlds->serial = pci_get_dsn(pdev); cxlds->cxl_dvsec = pci_find_dvsec_capability( - pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); + pdev, PCI_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); if (!cxlds->cxl_dvsec) dev_warn(&pdev->dev, "Device DVSEC not present, skip CXL.mem init\n"); diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c index 308c9969642e..a1b742b1a735 100644 --- a/drivers/perf/cxl_pmu.c +++ b/drivers/perf/cxl_pmu.c @@ -345,7 +345,7 @@ static ssize_t cxl_pmu_event_sysfs_show(struct device *dev, /* For CXL spec defined events */ #define CXL_PMU_EVENT_CXL_ATTR(_name, _gid, _msk) \ - CXL_PMU_EVENT_ATTR(_name, PCI_DVSEC_VENDOR_ID_CXL, _gid, _msk) + CXL_PMU_EVENT_ATTR(_name, PCI_VENDOR_ID_CXL, _gid, _msk) static struct attribute *cxl_pmu_event_attrs[] = { CXL_PMU_EVENT_CXL_ATTR(clock_ticks, CXL_PMU_GID_CLOCK_TICKS, BIT(0)), diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index a0c75e467df3..7dfbf6d96b3d 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -2607,6 +2607,8 @@ #define PCI_VENDOR_ID_ALIBABA 0x1ded +#define PCI_VENDOR_ID_CXL 0x1e98 + #define PCI_VENDOR_ID_TEHUTI 0x1fc9 #define PCI_DEVICE_ID_TEHUTI_3009 0x3009 #define PCI_DEVICE_ID_TEHUTI_3010 0x3010
Move PCI_DVSEC_VENDOR_ID_CXL in CXL private code to PCI_VENDOR_ID_CXL in pci_ids.h in order to be utilized in PCI subsystem. The response Bjorn received from the PCI SIG was "1E98h is not a VID in our system, but 1E98 has already been reserved by CXL." He suggested "we should add '#define PCI_VENDOR_ID_CXL 0x1e98' so that if we ever *do* see such an assignment, we'll be more likely to flag it as an issue. Link: https://lore.kernel.org/linux-cxl/20240402172323.GA1818777@bhelgaas/ Suggested-by: Bjorn Helgaas <helgaas@kernel.org> Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/pci.c | 6 +++--- drivers/cxl/core/regs.c | 2 +- drivers/cxl/cxlpci.h | 1 - drivers/cxl/pci.c | 2 +- drivers/perf/cxl_pmu.c | 2 +- include/linux/pci_ids.h | 2 ++ 6 files changed, 8 insertions(+), 7 deletions(-)