linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] cxl_pci refactor for reusability
@ 2021-09-21 22:04 Ben Widawsky
  2021-09-21 22:04 ` [PATCH 1/7] cxl: Convert "RBI" to enum Ben Widawsky
                   ` (8 more replies)
  0 siblings, 9 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Provide the ability to obtain CXL register blocks as discrete functionality.
This functionality will become useful for other CXL drivers that need access to
CXL register blocks. It is also in line with other additions to core which moves
register mapping functionality.

At the introduction of the CXL driver the only user of CXL MMIO was cxl_pci
(then known as cxl_mem). As the driver has evolved it is clear that cxl_pci will
not be the only entity that needs access to CXL MMIO. This series stops short of
moving the generalized functionality into cxl_core for the sake of getting eyes
on the important foundational bits sooner rather than later. The ultimate plan
is to move much of the code into cxl_core.

Via review of two previous patches [1] & [2] it has been suggested that the bits
which are being used for DVSEC enumeration move into PCI core. As CXL core is
soon going to require these, let's try to get the ball rolling now on making
that happen.

[1]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/
[2]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/

Ben Widawsky (7):
  cxl: Convert "RBI" to enum
  cxl/pci: Remove dev_dbg for unknown register blocks
  cxl/pci: Refactor cxl_pci_setup_regs
  cxl/pci: Make more use of cxl_register_map
  PCI: Add pci_find_dvsec_capability to find designated VSEC
  cxl/pci: Use pci core's DVSEC functionality
  ocxl: Use pci core's DVSEC functionality

 drivers/cxl/pci.c          | 144 ++++++++++++++++++-------------------
 drivers/cxl/pci.h          |  14 ++--
 drivers/misc/ocxl/config.c |  13 +---
 drivers/pci/pci.c          |  32 +++++++++
 include/linux/pci.h        |   1 +
 5 files changed, 113 insertions(+), 91 deletions(-)

-- 
2.33.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/7] cxl: Convert "RBI" to enum
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
@ 2021-09-21 22:04 ` Ben Widawsky
  2021-09-21 22:04 ` [PATCH 2/7] cxl/pci: Remove dev_dbg for unknown register blocks Ben Widawsky
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

In preparation for passing around the Register Block Indicator (RBI) as
a parameter, it is desirable to convert the type to an enum so that the
interface can use a well defined type checked parameter.

As a result of this change, should future versions of the spec add
sparsely defined identifiers, it could become a problem if checking for
invalid identifiers since the code currently checks for the max
identifier. This is not an issue with current spec, and the algorithm to
obtain the register blocks will change before those possible additions
are made.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index 8c1a58813816..7d3e4bf06b45 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -20,13 +20,15 @@
 #define CXL_REGLOC_BIR_MASK GENMASK(2, 0)
 
 /* Register Block Identifier (RBI) */
-#define CXL_REGLOC_RBI_MASK GENMASK(15, 8)
-#define CXL_REGLOC_RBI_EMPTY 0
-#define CXL_REGLOC_RBI_COMPONENT 1
-#define CXL_REGLOC_RBI_VIRT 2
-#define CXL_REGLOC_RBI_MEMDEV 3
-#define CXL_REGLOC_RBI_TYPES CXL_REGLOC_RBI_MEMDEV + 1
+enum cxl_regloc_type {
+	CXL_REGLOC_RBI_EMPTY = 0,
+	CXL_REGLOC_RBI_COMPONENT,
+	CXL_REGLOC_RBI_VIRT,
+	CXL_REGLOC_RBI_MEMDEV,
+	CXL_REGLOC_RBI_TYPES
+};
 
+#define CXL_REGLOC_RBI_MASK GENMASK(15, 8)
 #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
 
 #endif /* __CXL_PCI_H__ */
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/7] cxl/pci: Remove dev_dbg for unknown register blocks
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
  2021-09-21 22:04 ` [PATCH 1/7] cxl: Convert "RBI" to enum Ben Widawsky
@ 2021-09-21 22:04 ` Ben Widawsky
  2021-09-21 22:04 ` [PATCH 3/7] cxl/pci: Refactor cxl_pci_setup_regs Ben Widawsky
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

While interesting to driver developers, the dev_dbg message doesn't do
much except clutter up logs. This information should be attainable
through sysfs, and someday lspci like utilities. This change
additionally helps reduce the LOC in a subsequent patch to refactor some
of cxl_pci register mapping.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 64180f46c895..ccc7c2573ddc 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -475,9 +475,6 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
 					  &reg_type);
 
-		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
-			bar, offset, reg_type);
-
 		/* Ignore unknown register block types */
 		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/7] cxl/pci: Refactor cxl_pci_setup_regs
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
  2021-09-21 22:04 ` [PATCH 1/7] cxl: Convert "RBI" to enum Ben Widawsky
  2021-09-21 22:04 ` [PATCH 2/7] cxl/pci: Remove dev_dbg for unknown register blocks Ben Widawsky
@ 2021-09-21 22:04 ` Ben Widawsky
  2021-09-21 23:39   ` Dan Williams
  2021-09-21 22:04 ` [PATCH 4/7] cxl/pci: Make more use of cxl_register_map Ben Widawsky
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

In preparation for moving parts of register mapping to cxl_core, the
cxl_pci driver is refactored to utilize a new helper to find register
blocks by type.

cxl_pci scanned through all register blocks and mapping the ones that
the driver will use. This logic is inverted so that the driver
specifically requests the register blocks from a new helper. Under the
hood, the same implementation of scanning through all register locator
DVSEC entries exists.

There are 2 behavioral changes (#2 is arguable):
1. A dev_err is introduced if cxl_map_regs fails.
2. The previous logic would try to map component registers and device
   registers multiple times if there were present and keep the mapping
   of the last one found (furthest offset in the register locator).
   While this is disallowed in the spec, CXL 2.0 8.1.9: "Each register
   block identifier shall only occur once in the Register Locator DVSEC
   structure" it was how the driver would respond to the spec violation.
   The new logic will take the first found register block by type and
   move on.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.c | 113 ++++++++++++++++++++++++++--------------------
 1 file changed, 65 insertions(+), 48 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index ccc7c2573ddc..6e5c026f5262 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -428,46 +428,28 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
 	*reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
 }
 
-/**
- * cxl_pci_setup_regs() - Setup necessary MMIO.
- * @cxlm: The CXL memory device to communicate with.
- *
- * Return: 0 if all necessary registers mapped.
- *
- * A memory device is required by spec to implement a certain set of MMIO
- * regions. The purpose of this function is to enumerate and map those
- * registers.
- */
-static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
+static int find_register_block(struct pci_dev *pdev, enum cxl_regloc_type type,
+			       struct cxl_register_map *map)
 {
-	void __iomem *base;
+	int regloc, i, rc = -ENODEV;
 	u32 regloc_size, regblocks;
-	int regloc, i, n_maps, ret = 0;
-	struct device *dev = cxlm->dev;
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
+
+	memset(map, 0, sizeof(*map));
 
 	regloc = cxl_pci_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
-	if (!regloc) {
-		dev_err(dev, "register location dvsec not found\n");
+	if (!regloc)
 		return -ENXIO;
-	}
-
-	if (pci_request_mem_regions(pdev, pci_name(pdev)))
-		return -ENODEV;
 
-	/* Get the size of the Register Locator DVSEC */
 	pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
 	regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
 
 	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
 	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
 
-	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
+	for (i = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
-		u8 reg_type;
+		u8 reg_type, bar;
 		u64 offset;
-		u8 bar;
 
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
@@ -475,39 +457,74 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
 					  &reg_type);
 
-		/* Ignore unknown register block types */
-		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
-			continue;
+		if (reg_type == type) {
+			map->barno = bar;
+			map->block_offset = offset;
+			map->reg_type = reg_type;
+			rc = 0;
+			break;
+		}
+	}
 
-		base = cxl_pci_map_regblock(cxlm, bar, offset);
-		if (!base)
-			return -ENOMEM;
+	pci_release_mem_regions(pdev);
 
-		map = &maps[n_maps];
-		map->barno = bar;
-		map->block_offset = offset;
-		map->reg_type = reg_type;
+	return rc;
+}
 
-		ret = cxl_probe_regs(cxlm, base + offset, map);
+/**
+ * cxl_pci_setup_regs() - Setup necessary MMIO.
+ * @cxlm: The CXL memory device to communicate with.
+ *
+ * Return: 0 if all necessary registers mapped.
+ *
+ * A memory device is required by spec to implement a certain set of MMIO
+ * regions. The purpose of this function is to enumerate and map those
+ * registers.
+ */
+static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
+{
+	int rc, i;
+	struct device *dev = cxlm->dev;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	const enum cxl_regloc_type types[] = { CXL_REGLOC_RBI_MEMDEV,
+					       CXL_REGLOC_RBI_COMPONENT };
 
-		/* Always unmap the regblock regardless of probe success */
-		cxl_pci_unmap_regblock(cxlm, base);
+	if (pci_request_mem_regions(pdev, pci_name(pdev)))
+		return -ENODEV;
 
-		if (ret)
-			return ret;
+	for (i = 0; i < ARRAY_SIZE(types); i++) {
+		struct cxl_register_map map;
+		void __iomem *base;
 
-		n_maps++;
-	}
+		rc = find_register_block(pdev, types[i], &map);
+		if (rc) {
+			dev_err(dev, "Couldn't find %s register block\n",
+				types[i] == CXL_REGLOC_RBI_MEMDEV ?
+					      "device" :
+					      "component");
+			break;
+		}
 
-	pci_release_mem_regions(pdev);
+		base = cxl_pci_map_regblock(cxlm, map.barno, map.block_offset);
+		if (!base) {
+			rc = -ENOMEM;
+			break;
+		}
 
-	for (i = 0; i < n_maps; i++) {
-		ret = cxl_map_regs(cxlm, &maps[i]);
-		if (ret)
+		rc = cxl_probe_regs(cxlm, base + map.block_offset, &map);
+		cxl_pci_unmap_regblock(cxlm, base);
+		if (rc)
+			break;
+
+		rc = cxl_map_regs(cxlm, &map);
+		if (rc) {
+			dev_err(dev, "Failed to map CXL registers\n");
 			break;
+		}
 	}
 
-	return ret;
+	pci_release_mem_regions(pdev);
+	return rc;
 }
 
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/7] cxl/pci: Make more use of cxl_register_map
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
                   ` (2 preceding siblings ...)
  2021-09-21 22:04 ` [PATCH 3/7] cxl/pci: Refactor cxl_pci_setup_regs Ben Widawsky
@ 2021-09-21 22:04 ` Ben Widawsky
  2021-09-21 22:04 ` [PATCH 5/7] PCI: Add pci_find_dvsec_capability to find designated VSEC Ben Widawsky
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

The structure exists to pass around information about register mapping.
As a minor cleanup, use it more extensively.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 6e5c026f5262..7c1d5d5aef6e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -306,12 +306,13 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
 	return 0;
 }
 
-static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
-					  u8 bar, u64 offset)
+static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm, struct cxl_register_map *map)
 {
 	void __iomem *addr;
+	int bar = map->barno;
 	struct device *dev = cxlm->dev;
 	struct pci_dev *pdev = to_pci_dev(dev);
+	resource_size_t offset = map->block_offset;
 
 	/* Basic sanity check that BAR is big enough */
 	if (pci_resource_len(pdev, bar) < offset) {
@@ -363,6 +364,7 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
 			  struct cxl_register_map *map)
 {
+	void __iomem *offset = base + map->block_offset;
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
 	struct device *dev = cxlm->dev;
@@ -370,7 +372,7 @@ static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
 	switch (map->reg_type) {
 	case CXL_REGLOC_RBI_COMPONENT:
 		comp_map = &map->component_map;
-		cxl_probe_component_regs(dev, base, comp_map);
+		cxl_probe_component_regs(dev, offset, comp_map);
 		if (!comp_map->hdm_decoder.valid) {
 			dev_err(dev, "HDM decoder registers not found\n");
 			return -ENXIO;
@@ -380,7 +382,7 @@ static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
 		break;
 	case CXL_REGLOC_RBI_MEMDEV:
 		dev_map = &map->device_map;
-		cxl_probe_device_regs(dev, base, dev_map);
+		cxl_probe_device_regs(dev, offset, dev_map);
 		if (!dev_map->status.valid || !dev_map->mbox.valid ||
 		    !dev_map->memdev.valid) {
 			dev_err(dev, "registers not found: %s%s%s\n",
@@ -505,13 +507,13 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 			break;
 		}
 
-		base = cxl_pci_map_regblock(cxlm, map.barno, map.block_offset);
+		base = cxl_pci_map_regblock(cxlm, &map);
 		if (!base) {
 			rc = -ENOMEM;
 			break;
 		}
 
-		rc = cxl_probe_regs(cxlm, base + map.block_offset, &map);
+		rc = cxl_probe_regs(cxlm, base, &map);
 		cxl_pci_unmap_regblock(cxlm, base);
 		if (rc)
 			break;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/7] PCI: Add pci_find_dvsec_capability to find designated VSEC
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
                   ` (3 preceding siblings ...)
  2021-09-21 22:04 ` [PATCH 4/7] cxl/pci: Make more use of cxl_register_map Ben Widawsky
@ 2021-09-21 22:04 ` Ben Widawsky
  2021-09-22  9:33   ` Frederic Barrat
  2021-09-21 22:04 ` [PATCH 6/7] cxl/pci: Use pci core's DVSEC functionality Ben Widawsky
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, David E . Box, Jonathan Cameron, Bjorn Helgaas,
	Dan Williams, linuxppc-dev, Frederic Barrat, Andrew Donnellan,
	Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma

Add pci_find_dvsec_capability to locate a Designated Vendor-Specific
Extended Capability with the specified DVSEC ID.

The Designated Vendor-Specific Extended Capability (DVSEC) allows one or
more vendor specific capabilities that aren't tied to the vendor ID of
the PCI component.

DVSEC is critical for both the Compute Express Link (CXL) driver as well
as the driver for OpenCAPI coherent accelerator (OCXL).

Cc: David E. Box <david.e.box@linux.intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Frederic Barrat <fbarrat@linux.ibm.com>
Cc: Andrew Donnellan <ajd@linux.ibm.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/pci/pci.c   | 32 ++++++++++++++++++++++++++++++++
 include/linux/pci.h |  1 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce2ab62b64cf..94ac86ff28b0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -732,6 +732,38 @@ u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap)
 }
 EXPORT_SYMBOL_GPL(pci_find_vsec_capability);
 
+/**
+ * pci_find_dvsec_capability - Find DVSEC for vendor
+ * @dev: PCI device to query
+ * @vendor: Vendor ID to match for the DVSEC
+ * @dvsec: Designated Vendor-specific capability ID
+ *
+ * If DVSEC has Vendor ID @vendor and DVSEC ID @dvsec return the capability
+ * offset in config space; otherwise return 0.
+ */
+u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec)
+{
+	int pos;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DVSEC);
+	if (!pos)
+		return 0;
+
+	while (pos) {
+		u16 v, id;
+
+		pci_read_config_word(dev, pos + PCI_DVSEC_HEADER1, &v);
+		pci_read_config_word(dev, pos + PCI_DVSEC_HEADER2, &id);
+		if (vendor == v && dvsec == id)
+			return pos;
+
+		pos = pci_find_next_ext_capability(dev, pos, PCI_EXT_CAP_ID_DVSEC);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_find_dvsec_capability);
+
 /**
  * pci_find_parent_resource - return resource region of parent bus of given
  *			      region
diff --git a/include/linux/pci.h b/include/linux/pci.h
index cd8aa6fce204..c93ccfa4571b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1130,6 +1130,7 @@ u16 pci_find_ext_capability(struct pci_dev *dev, int cap);
 u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 pos, int cap);
 struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
 u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap);
+u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec);
 
 u64 pci_get_dsn(struct pci_dev *dev);
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 6/7] cxl/pci: Use pci core's DVSEC functionality
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
                   ` (4 preceding siblings ...)
  2021-09-21 22:04 ` [PATCH 5/7] PCI: Add pci_find_dvsec_capability to find designated VSEC Ben Widawsky
@ 2021-09-21 22:04 ` Ben Widawsky
  2021-09-21 22:04 ` [PATCH 7/7] ocxl: " Ben Widawsky
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Reduce maintenance burden of DVSEC query implementation by using the
centralized PCI core implementation.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 7c1d5d5aef6e..040379f727ad 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -340,25 +340,7 @@ static void cxl_pci_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
 
 static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 {
-	int pos;
-
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC);
-	if (!pos)
-		return 0;
-
-	while (pos) {
-		u16 vendor, id;
-
-		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vendor);
-		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER2, &id);
-		if (vendor == PCI_DVSEC_VENDOR_ID_CXL && dvsec == id)
-			return pos;
-
-		pos = pci_find_next_ext_capability(pdev, pos,
-						   PCI_EXT_CAP_ID_DVSEC);
-	}
-
-	return 0;
+	return pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, dvsec);
 }
 
 static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 7/7] ocxl: Use pci core's DVSEC functionality
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
                   ` (5 preceding siblings ...)
  2021-09-21 22:04 ` [PATCH 6/7] cxl/pci: Use pci core's DVSEC functionality Ben Widawsky
@ 2021-09-21 22:04 ` Ben Widawsky
  2021-09-22  0:44   ` Dan Williams
  2021-09-21 22:14 ` [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
  2021-09-21 22:28 ` Dan Williams
  8 siblings, 1 reply; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:04 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Ben Widawsky, linuxppc-dev, Frederic Barrat, Andrew Donnellan,
	Alison Schofield, Dan Williams, Ira Weiny, Jonathan Cameron,
	Vishal Verma

Reduce maintenance burden of DVSEC query implementation by using the
centralized PCI core implementation.

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Frederic Barrat <fbarrat@linux.ibm.com>
Cc: Andrew Donnellan <ajd@linux.ibm.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/misc/ocxl/config.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
index a68738f38252..e401a51596b9 100644
--- a/drivers/misc/ocxl/config.c
+++ b/drivers/misc/ocxl/config.c
@@ -33,18 +33,7 @@
 
 static int find_dvsec(struct pci_dev *dev, int dvsec_id)
 {
-	int vsec = 0;
-	u16 vendor, id;
-
-	while ((vsec = pci_find_next_ext_capability(dev, vsec,
-						    OCXL_EXT_CAP_ID_DVSEC))) {
-		pci_read_config_word(dev, vsec + OCXL_DVSEC_VENDOR_OFFSET,
-				&vendor);
-		pci_read_config_word(dev, vsec + OCXL_DVSEC_ID_OFFSET, &id);
-		if (vendor == PCI_VENDOR_ID_IBM && id == dvsec_id)
-			return vsec;
-	}
-	return 0;
+	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_IBM, dvsec_id);
 }
 
 static int find_dvsec_afu_ctrl(struct pci_dev *dev, u8 afu_idx)
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/7] cxl_pci refactor for reusability
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
                   ` (6 preceding siblings ...)
  2021-09-21 22:04 ` [PATCH 7/7] ocxl: " Ben Widawsky
@ 2021-09-21 22:14 ` Ben Widawsky
  2021-09-21 22:28 ` Dan Williams
  8 siblings, 0 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 22:14 UTC (permalink / raw)
  To: linux-cxl, linux-pci
  Cc: Alison Schofield, Dan Williams, Ira Weiny, Jonathan Cameron,
	Vishal Verma

On 21-09-21 15:04:52, Ben Widawsky wrote:
> Provide the ability to obtain CXL register blocks as discrete functionality.
> This functionality will become useful for other CXL drivers that need access to
> CXL register blocks. It is also in line with other additions to core which moves
> register mapping functionality.
> 
> At the introduction of the CXL driver the only user of CXL MMIO was cxl_pci
> (then known as cxl_mem). As the driver has evolved it is clear that cxl_pci will
> not be the only entity that needs access to CXL MMIO. This series stops short of
> moving the generalized functionality into cxl_core for the sake of getting eyes
> on the important foundational bits sooner rather than later. The ultimate plan
> is to move much of the code into cxl_core.
> 
> Via review of two previous patches [1] & [2] it has been suggested that the bits
> which are being used for DVSEC enumeration move into PCI core. As CXL core is
> soon going to require these, let's try to get the ball rolling now on making
> that happen.
> 
> [1]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/

Dangit.
https://lore.kernel.org/linux-pci/20210913190131.xiiszmno46qie7v5@intel.com/

> [2]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/


> 
> Ben Widawsky (7):
>   cxl: Convert "RBI" to enum
>   cxl/pci: Remove dev_dbg for unknown register blocks
>   cxl/pci: Refactor cxl_pci_setup_regs
>   cxl/pci: Make more use of cxl_register_map
>   PCI: Add pci_find_dvsec_capability to find designated VSEC
>   cxl/pci: Use pci core's DVSEC functionality
>   ocxl: Use pci core's DVSEC functionality
> 
>  drivers/cxl/pci.c          | 144 ++++++++++++++++++-------------------
>  drivers/cxl/pci.h          |  14 ++--
>  drivers/misc/ocxl/config.c |  13 +---
>  drivers/pci/pci.c          |  32 +++++++++
>  include/linux/pci.h        |   1 +
>  5 files changed, 113 insertions(+), 91 deletions(-)
> 
> -- 
> 2.33.0
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/7] cxl_pci refactor for reusability
  2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
                   ` (7 preceding siblings ...)
  2021-09-21 22:14 ` [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
@ 2021-09-21 22:28 ` Dan Williams
  2021-09-21 23:03   ` Ben Widawsky
  8 siblings, 1 reply; 16+ messages in thread
From: Dan Williams @ 2021-09-21 22:28 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Linux PCI, Alison Schofield, Ira Weiny,
	Jonathan Cameron, Vishal Verma

On Tue, Sep 21, 2021 at 3:05 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Provide the ability to obtain CXL register blocks as discrete functionality.
> This functionality will become useful for other CXL drivers that need access to
> CXL register blocks. It is also in line with other additions to core which moves
> register mapping functionality.
>
> At the introduction of the CXL driver the only user of CXL MMIO was cxl_pci
> (then known as cxl_mem). As the driver has evolved it is clear that cxl_pci will
> not be the only entity that needs access to CXL MMIO. This series stops short of
> moving the generalized functionality into cxl_core for the sake of getting eyes
> on the important foundational bits sooner rather than later. The ultimate plan
> is to move much of the code into cxl_core.
>
> Via review of two previous patches [1] & [2] it has been suggested that the bits
> which are being used for DVSEC enumeration move into PCI core. As CXL core is
> soon going to require these, let's try to get the ball rolling now on making
> that happen.
>
> [1]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/
> [2]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/
>
> Ben Widawsky (7):
>   cxl: Convert "RBI" to enum
>   cxl/pci: Remove dev_dbg for unknown register blocks
>   cxl/pci: Refactor cxl_pci_setup_regs
>   cxl/pci: Make more use of cxl_register_map
>   PCI: Add pci_find_dvsec_capability to find designated VSEC
>   cxl/pci: Use pci core's DVSEC functionality
>   ocxl: Use pci core's DVSEC functionality

I also found:

siov_find_pci_dvsec()

...and an open coded one in:

drivers/mfd/intel_pmt.c::pmt_pci_probe()

This one looks too weird to replace:

arch/x86/events/intel/uncore_discovery.c::intel_uncore_has_discovery_tables()

In any event I'd expect this cover to also be cc'd to those folks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/7] cxl_pci refactor for reusability
  2021-09-21 22:28 ` Dan Williams
@ 2021-09-21 23:03   ` Ben Widawsky
  0 siblings, 0 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-21 23:03 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-cxl, Linux PCI, Alison Schofield, Ira Weiny,
	Jonathan Cameron, Vishal Verma

On 21-09-21 15:28:21, Dan Williams wrote:
> On Tue, Sep 21, 2021 at 3:05 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > Provide the ability to obtain CXL register blocks as discrete functionality.
> > This functionality will become useful for other CXL drivers that need access to
> > CXL register blocks. It is also in line with other additions to core which moves
> > register mapping functionality.
> >
> > At the introduction of the CXL driver the only user of CXL MMIO was cxl_pci
> > (then known as cxl_mem). As the driver has evolved it is clear that cxl_pci will
> > not be the only entity that needs access to CXL MMIO. This series stops short of
> > moving the generalized functionality into cxl_core for the sake of getting eyes
> > on the important foundational bits sooner rather than later. The ultimate plan
> > is to move much of the code into cxl_core.
> >
> > Via review of two previous patches [1] & [2] it has been suggested that the bits
> > which are being used for DVSEC enumeration move into PCI core. As CXL core is
> > soon going to require these, let's try to get the ball rolling now on making
> > that happen.
> >
> > [1]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/
> > [2]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/
> >
> > Ben Widawsky (7):
> >   cxl: Convert "RBI" to enum
> >   cxl/pci: Remove dev_dbg for unknown register blocks
> >   cxl/pci: Refactor cxl_pci_setup_regs
> >   cxl/pci: Make more use of cxl_register_map
> >   PCI: Add pci_find_dvsec_capability to find designated VSEC
> >   cxl/pci: Use pci core's DVSEC functionality
> >   ocxl: Use pci core's DVSEC functionality
> 
> I also found:
> 
> siov_find_pci_dvsec()

Hadn't seen this one... Thanks.

> 
> ...and an open coded one in:
> 
> drivers/mfd/intel_pmt.c::pmt_pci_probe()

I had spotted this one previously

> 
> This one looks too weird to replace:
> 
> arch/x86/events/intel/uncore_discovery.c::intel_uncore_has_discovery_tables()
> 
> In any event I'd expect this cover to also be cc'd to those folks.

I did Cc OCXL in the relevant patch, I don't think they need most of the
background in the cover letter (I also did Cc David Box who maintains
intel_pmt). I'll add them to the cover letter here shortly...


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/7] cxl/pci: Refactor cxl_pci_setup_regs
  2021-09-21 22:04 ` [PATCH 3/7] cxl/pci: Refactor cxl_pci_setup_regs Ben Widawsky
@ 2021-09-21 23:39   ` Dan Williams
  2021-09-22  4:31     ` Ben Widawsky
  0 siblings, 1 reply; 16+ messages in thread
From: Dan Williams @ 2021-09-21 23:39 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Linux PCI, Alison Schofield, Ira Weiny,
	Jonathan Cameron, Vishal Verma

On Tue, Sep 21, 2021 at 3:05 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> In preparation for moving parts of register mapping to cxl_core, the
> cxl_pci driver is refactored to utilize a new helper to find register
> blocks by type.
>
> cxl_pci scanned through all register blocks and mapping the ones that
> the driver will use. This logic is inverted so that the driver
> specifically requests the register blocks from a new helper. Under the
> hood, the same implementation of scanning through all register locator
> DVSEC entries exists.
>
> There are 2 behavioral changes (#2 is arguable):
> 1. A dev_err is introduced if cxl_map_regs fails.
> 2. The previous logic would try to map component registers and device
>    registers multiple times if there were present and keep the mapping
>    of the last one found (furthest offset in the register locator).
>    While this is disallowed in the spec, CXL 2.0 8.1.9: "Each register
>    block identifier shall only occur once in the Register Locator DVSEC
>    structure" it was how the driver would respond to the spec violation.
>    The new logic will take the first found register block by type and
>    move on.

Yeah, I think it's silly to try to predict how hardware might violate
the specification. Just wait until there is a known shipping device
with a problem and then add a quirk to the driver.

>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/pci.c | 113 ++++++++++++++++++++++++++--------------------
>  1 file changed, 65 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index ccc7c2573ddc..6e5c026f5262 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -428,46 +428,28 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
>         *reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
>  }
>
> -/**
> - * cxl_pci_setup_regs() - Setup necessary MMIO.
> - * @cxlm: The CXL memory device to communicate with.
> - *
> - * Return: 0 if all necessary registers mapped.
> - *
> - * A memory device is required by spec to implement a certain set of MMIO
> - * regions. The purpose of this function is to enumerate and map those
> - * registers.
> - */
> -static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
> +static int find_register_block(struct pci_dev *pdev, enum cxl_regloc_type type,
> +                              struct cxl_register_map *map)
>  {
> -       void __iomem *base;
> +       int regloc, i, rc = -ENODEV;
>         u32 regloc_size, regblocks;
> -       int regloc, i, n_maps, ret = 0;
> -       struct device *dev = cxlm->dev;
> -       struct pci_dev *pdev = to_pci_dev(dev);
> -       struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
> +
> +       memset(map, 0, sizeof(*map));

Is this necessary? It seems this fills in all fields on success, why
does it need to zero-init?

>
>         regloc = cxl_pci_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
> -       if (!regloc) {
> -               dev_err(dev, "register location dvsec not found\n");
> +       if (!regloc)
>                 return -ENXIO;
> -       }
> -
> -       if (pci_request_mem_regions(pdev, pci_name(pdev)))
> -               return -ENODEV;
>
> -       /* Get the size of the Register Locator DVSEC */
>         pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
>         regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
>
>         regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
>         regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
>
> -       for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
> +       for (i = 0; i < regblocks; i++, regloc += 8) {
>                 u32 reg_lo, reg_hi;
> -               u8 reg_type;
> +               u8 reg_type, bar;
>                 u64 offset;
> -               u8 bar;
>
>                 pci_read_config_dword(pdev, regloc, &reg_lo);
>                 pci_read_config_dword(pdev, regloc + 4, &reg_hi);
> @@ -475,39 +457,74 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>                 cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
>                                           &reg_type);
>
> -               /* Ignore unknown register block types */
> -               if (reg_type > CXL_REGLOC_RBI_MEMDEV)
> -                       continue;
> +               if (reg_type == type) {
> +                       map->barno = bar;
> +                       map->block_offset = offset;
> +                       map->reg_type = reg_type;
> +                       rc = 0;
> +                       break;

As this patch is already adding helpers, perhaps rather than a loop
break, make the loop a helper so it can just "return 0;" directly:

Something like:

pci_request_mem_regions(...);
rc = __find_register_block(...);
pci_release_mem_regions(...);

...although, now that I see it written that way I think the request +
release regions should probably just be dropped. It's not like any of
the register enumeration would collide with someone else who already
has the registers mapped. The collision only comes when the registers
are mapped for their final usage, and that will have more precision in
the request.

> +               }
> +       }
>
> -               base = cxl_pci_map_regblock(cxlm, bar, offset);
> -               if (!base)
> -                       return -ENOMEM;
> +       pci_release_mem_regions(pdev);
>
> -               map = &maps[n_maps];
> -               map->barno = bar;
> -               map->block_offset = offset;
> -               map->reg_type = reg_type;
> +       return rc;
> +}
>
> -               ret = cxl_probe_regs(cxlm, base + offset, map);
> +/**
> + * cxl_pci_setup_regs() - Setup necessary MMIO.
> + * @cxlm: The CXL memory device to communicate with.
> + *
> + * Return: 0 if all necessary registers mapped.
> + *
> + * A memory device is required by spec to implement a certain set of MMIO
> + * regions. The purpose of this function is to enumerate and map those
> + * registers.
> + */
> +static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
> +{
> +       int rc, i;
> +       struct device *dev = cxlm->dev;
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +       const enum cxl_regloc_type types[] = { CXL_REGLOC_RBI_MEMDEV,
> +                                              CXL_REGLOC_RBI_COMPONENT };
>
> -               /* Always unmap the regblock regardless of probe success */
> -               cxl_pci_unmap_regblock(cxlm, base);
> +       if (pci_request_mem_regions(pdev, pci_name(pdev)))
> +               return -ENODEV;
>
> -               if (ret)
> -                       return ret;
> +       for (i = 0; i < ARRAY_SIZE(types); i++) {
> +               struct cxl_register_map map;
> +               void __iomem *base;
>
> -               n_maps++;
> -       }
> +               rc = find_register_block(pdev, types[i], &map);
> +               if (rc) {
> +                       dev_err(dev, "Couldn't find %s register block\n",
> +                               types[i] == CXL_REGLOC_RBI_MEMDEV ?
> +                                             "device" :
> +                                             "component");
> +                       break;
> +               }
>
> -       pci_release_mem_regions(pdev);
> +               base = cxl_pci_map_regblock(cxlm, map.barno, map.block_offset);
> +               if (!base) {
> +                       rc = -ENOMEM;
> +                       break;
> +               }
>
> -       for (i = 0; i < n_maps; i++) {
> -               ret = cxl_map_regs(cxlm, &maps[i]);
> -               if (ret)
> +               rc = cxl_probe_regs(cxlm, base + map.block_offset, &map);

It strikes me as odd @map has everything except a copy of @base. I
wonder if this patch becomes easier to read if patch4 comes before
this one and all the map_offset usage is dropped because @map can
carry the required information directly. I'm not sure this suggestion
is a win. I'm struggling to make sense of diff in isolation so will
need to circle back when I can apply this and look at the result, for
now it's just these edge comments.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 7/7] ocxl: Use pci core's DVSEC functionality
  2021-09-21 22:04 ` [PATCH 7/7] ocxl: " Ben Widawsky
@ 2021-09-22  0:44   ` Dan Williams
  2021-09-22  9:38     ` Frederic Barrat
  0 siblings, 1 reply; 16+ messages in thread
From: Dan Williams @ 2021-09-22  0:44 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Linux PCI, linuxppc-dev, Frederic Barrat,
	Andrew Donnellan, Alison Schofield, Ira Weiny, Jonathan Cameron,
	Vishal Verma

On Tue, Sep 21, 2021 at 3:05 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Reduce maintenance burden of DVSEC query implementation by using the
> centralized PCI core implementation.
>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Frederic Barrat <fbarrat@linux.ibm.com>
> Cc: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/misc/ocxl/config.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index a68738f38252..e401a51596b9 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -33,18 +33,7 @@
>
>  static int find_dvsec(struct pci_dev *dev, int dvsec_id)
>  {
> -       int vsec = 0;
> -       u16 vendor, id;
> -
> -       while ((vsec = pci_find_next_ext_capability(dev, vsec,
> -                                                   OCXL_EXT_CAP_ID_DVSEC))) {
> -               pci_read_config_word(dev, vsec + OCXL_DVSEC_VENDOR_OFFSET,
> -                               &vendor);
> -               pci_read_config_word(dev, vsec + OCXL_DVSEC_ID_OFFSET, &id);
> -               if (vendor == PCI_VENDOR_ID_IBM && id == dvsec_id)
> -                       return vsec;
> -       }
> -       return 0;
> +       return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_IBM, dvsec_id);
>  }

What about:

arch/powerpc/platforms/powernv/ocxl.c::find_dvsec_from_pos()

...?  With that converted the redundant definitions below:

OCXL_EXT_CAP_ID_DVSEC
OCXL_DVSEC_VENDOR_OFFSET
OCXL_DVSEC_ID_OFFSET

...can be cleaned up in favor of the core definitions.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/7] cxl/pci: Refactor cxl_pci_setup_regs
  2021-09-21 23:39   ` Dan Williams
@ 2021-09-22  4:31     ` Ben Widawsky
  0 siblings, 0 replies; 16+ messages in thread
From: Ben Widawsky @ 2021-09-22  4:31 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-cxl, Linux PCI, Alison Schofield, Ira Weiny,
	Jonathan Cameron, Vishal Verma

On 21-09-21 16:39:30, Dan Williams wrote:
> On Tue, Sep 21, 2021 at 3:05 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > In preparation for moving parts of register mapping to cxl_core, the
> > cxl_pci driver is refactored to utilize a new helper to find register
> > blocks by type.
> >
> > cxl_pci scanned through all register blocks and mapping the ones that
> > the driver will use. This logic is inverted so that the driver
> > specifically requests the register blocks from a new helper. Under the
> > hood, the same implementation of scanning through all register locator
> > DVSEC entries exists.
> >
> > There are 2 behavioral changes (#2 is arguable):
> > 1. A dev_err is introduced if cxl_map_regs fails.
> > 2. The previous logic would try to map component registers and device
> >    registers multiple times if there were present and keep the mapping
> >    of the last one found (furthest offset in the register locator).
> >    While this is disallowed in the spec, CXL 2.0 8.1.9: "Each register
> >    block identifier shall only occur once in the Register Locator DVSEC
> >    structure" it was how the driver would respond to the spec violation.
> >    The new logic will take the first found register block by type and
> >    move on.
> 
> Yeah, I think it's silly to try to predict how hardware might violate
> the specification. Just wait until there is a known shipping device
> with a problem and then add a quirk to the driver.
> 
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/pci.c | 113 ++++++++++++++++++++++++++--------------------
> >  1 file changed, 65 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index ccc7c2573ddc..6e5c026f5262 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -428,46 +428,28 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
> >         *reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
> >  }
> >
> > -/**
> > - * cxl_pci_setup_regs() - Setup necessary MMIO.
> > - * @cxlm: The CXL memory device to communicate with.
> > - *
> > - * Return: 0 if all necessary registers mapped.
> > - *
> > - * A memory device is required by spec to implement a certain set of MMIO
> > - * regions. The purpose of this function is to enumerate and map those
> > - * registers.
> > - */
> > -static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
> > +static int find_register_block(struct pci_dev *pdev, enum cxl_regloc_type type,
> > +                              struct cxl_register_map *map)
> >  {
> > -       void __iomem *base;
> > +       int regloc, i, rc = -ENODEV;
> >         u32 regloc_size, regblocks;
> > -       int regloc, i, n_maps, ret = 0;
> > -       struct device *dev = cxlm->dev;
> > -       struct pci_dev *pdev = to_pci_dev(dev);
> > -       struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
> > +
> > +       memset(map, 0, sizeof(*map));
> 
> Is this necessary? It seems this fills in all fields on success, why
> does it need to zero-init?
> 

Am earlier version of this patch attempted to determine success based on the
values in @map. It is no longer necessary.

> >
> >         regloc = cxl_pci_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
> > -       if (!regloc) {
> > -               dev_err(dev, "register location dvsec not found\n");
> > +       if (!regloc)
> >                 return -ENXIO;
> > -       }
> > -
> > -       if (pci_request_mem_regions(pdev, pci_name(pdev)))
> > -               return -ENODEV;
> >
> > -       /* Get the size of the Register Locator DVSEC */
> >         pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
> >         regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
> >
> >         regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
> >         regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
> >
> > -       for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
> > +       for (i = 0; i < regblocks; i++, regloc += 8) {
> >                 u32 reg_lo, reg_hi;
> > -               u8 reg_type;
> > +               u8 reg_type, bar;
> >                 u64 offset;
> > -               u8 bar;
> >
> >                 pci_read_config_dword(pdev, regloc, &reg_lo);
> >                 pci_read_config_dword(pdev, regloc + 4, &reg_hi);
> > @@ -475,39 +457,74 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
> >                 cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
> >                                           &reg_type);
> >
> > -               /* Ignore unknown register block types */
> > -               if (reg_type > CXL_REGLOC_RBI_MEMDEV)
> > -                       continue;
> > +               if (reg_type == type) {
> > +                       map->barno = bar;
> > +                       map->block_offset = offset;
> > +                       map->reg_type = reg_type;
> > +                       rc = 0;
> > +                       break;
> 
> As this patch is already adding helpers, perhaps rather than a loop
> break, make the loop a helper so it can just "return 0;" directly:
> 
> Something like:
> 
> pci_request_mem_regions(...);
> rc = __find_register_block(...);
> pci_release_mem_regions(...);
> 
> ...although, now that I see it written that way I think the request +
> release regions should probably just be dropped. It's not like any of
> the register enumeration would collide with someone else who already
> has the registers mapped. The collision only comes when the registers
> are mapped for their final usage, and that will have more precision in
> the request.
> 

I think the origin of this was copy-pasta on my part. However, what's the
idiomatic way to do this? I guess I don't fully understand what kind of
collisions exist today and how to prevent them. Removing this entirely would
make easier code and if you assert we can do that, I'm all for it.

> > +               }
> > +       }
> >
> > -               base = cxl_pci_map_regblock(cxlm, bar, offset);
> > -               if (!base)
> > -                       return -ENOMEM;
> > +       pci_release_mem_regions(pdev);
> >
> > -               map = &maps[n_maps];
> > -               map->barno = bar;
> > -               map->block_offset = offset;
> > -               map->reg_type = reg_type;
> > +       return rc;
> > +}
> >
> > -               ret = cxl_probe_regs(cxlm, base + offset, map);
> > +/**
> > + * cxl_pci_setup_regs() - Setup necessary MMIO.
> > + * @cxlm: The CXL memory device to communicate with.
> > + *
> > + * Return: 0 if all necessary registers mapped.
> > + *
> > + * A memory device is required by spec to implement a certain set of MMIO
> > + * regions. The purpose of this function is to enumerate and map those
> > + * registers.
> > + */
> > +static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
> > +{
> > +       int rc, i;
> > +       struct device *dev = cxlm->dev;
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> > +       const enum cxl_regloc_type types[] = { CXL_REGLOC_RBI_MEMDEV,
> > +                                              CXL_REGLOC_RBI_COMPONENT };
> >
> > -               /* Always unmap the regblock regardless of probe success */
> > -               cxl_pci_unmap_regblock(cxlm, base);
> > +       if (pci_request_mem_regions(pdev, pci_name(pdev)))
> > +               return -ENODEV;
> >
> > -               if (ret)
> > -                       return ret;
> > +       for (i = 0; i < ARRAY_SIZE(types); i++) {
> > +               struct cxl_register_map map;
> > +               void __iomem *base;
> >
> > -               n_maps++;
> > -       }
> > +               rc = find_register_block(pdev, types[i], &map);
> > +               if (rc) {
> > +                       dev_err(dev, "Couldn't find %s register block\n",
> > +                               types[i] == CXL_REGLOC_RBI_MEMDEV ?
> > +                                             "device" :
> > +                                             "component");
> > +                       break;
> > +               }
> >
> > -       pci_release_mem_regions(pdev);
> > +               base = cxl_pci_map_regblock(cxlm, map.barno, map.block_offset);
> > +               if (!base) {
> > +                       rc = -ENOMEM;
> > +                       break;
> > +               }
> >
> > -       for (i = 0; i < n_maps; i++) {
> > -               ret = cxl_map_regs(cxlm, &maps[i]);
> > -               if (ret)
> > +               rc = cxl_probe_regs(cxlm, base + map.block_offset, &map);
> 
> It strikes me as odd @map has everything except a copy of @base. I
> wonder if this patch becomes easier to read if patch4 comes before
> this one and all the map_offset usage is dropped because @map can
> carry the required information directly. I'm not sure this suggestion
> is a win. I'm struggling to make sense of diff in isolation so will
> need to circle back when I can apply this and look at the result, for
> now it's just these edge comments.

It's a good point that I've also thought about. I think Ira's original goal was
to try to keep the iomem base in the API, but if we consider this series a win,
I think everything might benefit moving into @map. I'm certainly willing to try
it.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/7] PCI: Add pci_find_dvsec_capability to find designated VSEC
  2021-09-21 22:04 ` [PATCH 5/7] PCI: Add pci_find_dvsec_capability to find designated VSEC Ben Widawsky
@ 2021-09-22  9:33   ` Frederic Barrat
  0 siblings, 0 replies; 16+ messages in thread
From: Frederic Barrat @ 2021-09-22  9:33 UTC (permalink / raw)
  To: Ben Widawsky, linux-cxl, linux-pci
  Cc: David E . Box, Jonathan Cameron, Bjorn Helgaas, Dan Williams,
	linuxppc-dev, Andrew Donnellan, Alison Schofield, Ira Weiny,
	Vishal Verma



On 22/09/2021 00:04, Ben Widawsky wrote:
> Add pci_find_dvsec_capability to locate a Designated Vendor-Specific
> Extended Capability with the specified DVSEC ID.
> 
> The Designated Vendor-Specific Extended Capability (DVSEC) allows one or
> more vendor specific capabilities that aren't tied to the vendor ID of
> the PCI component.
> 
> DVSEC is critical for both the Compute Express Link (CXL) driver as well
> as the driver for OpenCAPI coherent accelerator (OCXL).
> 
> Cc: David E. Box <david.e.box@linux.intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Frederic Barrat <fbarrat@linux.ibm.com>
> Cc: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---


LGTM
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   drivers/pci/pci.c   | 32 ++++++++++++++++++++++++++++++++
>   include/linux/pci.h |  1 +
>   2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ce2ab62b64cf..94ac86ff28b0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -732,6 +732,38 @@ u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap)
>   }
>   EXPORT_SYMBOL_GPL(pci_find_vsec_capability);
>   
> +/**
> + * pci_find_dvsec_capability - Find DVSEC for vendor
> + * @dev: PCI device to query
> + * @vendor: Vendor ID to match for the DVSEC
> + * @dvsec: Designated Vendor-specific capability ID
> + *
> + * If DVSEC has Vendor ID @vendor and DVSEC ID @dvsec return the capability
> + * offset in config space; otherwise return 0.
> + */
> +u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec)
> +{
> +	int pos;
> +
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DVSEC);
> +	if (!pos)
> +		return 0;
> +
> +	while (pos) {
> +		u16 v, id;
> +
> +		pci_read_config_word(dev, pos + PCI_DVSEC_HEADER1, &v);
> +		pci_read_config_word(dev, pos + PCI_DVSEC_HEADER2, &id);
> +		if (vendor == v && dvsec == id)
> +			return pos;
> +
> +		pos = pci_find_next_ext_capability(dev, pos, PCI_EXT_CAP_ID_DVSEC);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_find_dvsec_capability);
> +
>   /**
>    * pci_find_parent_resource - return resource region of parent bus of given
>    *			      region
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cd8aa6fce204..c93ccfa4571b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1130,6 +1130,7 @@ u16 pci_find_ext_capability(struct pci_dev *dev, int cap);
>   u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 pos, int cap);
>   struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
>   u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap);
> +u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec);
>   
>   u64 pci_get_dsn(struct pci_dev *dev);
>   
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 7/7] ocxl: Use pci core's DVSEC functionality
  2021-09-22  0:44   ` Dan Williams
@ 2021-09-22  9:38     ` Frederic Barrat
  0 siblings, 0 replies; 16+ messages in thread
From: Frederic Barrat @ 2021-09-22  9:38 UTC (permalink / raw)
  To: Dan Williams, Ben Widawsky
  Cc: linux-cxl, Linux PCI, linuxppc-dev, Andrew Donnellan,
	Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma



On 22/09/2021 02:44, Dan Williams wrote:
> On Tue, Sep 21, 2021 at 3:05 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>>
>> Reduce maintenance burden of DVSEC query implementation by using the
>> centralized PCI core implementation.
>>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: Frederic Barrat <fbarrat@linux.ibm.com>
>> Cc: Andrew Donnellan <ajd@linux.ibm.com>
>> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>> ---
>>   drivers/misc/ocxl/config.c | 13 +------------
>>   1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
>> index a68738f38252..e401a51596b9 100644
>> --- a/drivers/misc/ocxl/config.c
>> +++ b/drivers/misc/ocxl/config.c
>> @@ -33,18 +33,7 @@
>>
>>   static int find_dvsec(struct pci_dev *dev, int dvsec_id)
>>   {
>> -       int vsec = 0;
>> -       u16 vendor, id;
>> -
>> -       while ((vsec = pci_find_next_ext_capability(dev, vsec,
>> -                                                   OCXL_EXT_CAP_ID_DVSEC))) {
>> -               pci_read_config_word(dev, vsec + OCXL_DVSEC_VENDOR_OFFSET,
>> -                               &vendor);
>> -               pci_read_config_word(dev, vsec + OCXL_DVSEC_ID_OFFSET, &id);
>> -               if (vendor == PCI_VENDOR_ID_IBM && id == dvsec_id)
>> -                       return vsec;
>> -       }
>> -       return 0;
>> +       return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_IBM, dvsec_id);
>>   }


That looks fine, thanks for spotting it. You can add this for the next 
revision:
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>



> 
> What about:
> 
> arch/powerpc/platforms/powernv/ocxl.c::find_dvsec_from_pos()
> 
> ...?  With that converted the redundant definitions below:
> 
> OCXL_EXT_CAP_ID_DVSEC
> OCXL_DVSEC_VENDOR_OFFSET
> OCXL_DVSEC_ID_OFFSET
> 
> ...can be cleaned up in favor of the core definitions.


That would be great. Are you guys willing to do it? If not, I could have 
a follow-on patch, if I don't forget :-)

Thanks,

   Fred


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-09-22  9:38 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 22:04 [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
2021-09-21 22:04 ` [PATCH 1/7] cxl: Convert "RBI" to enum Ben Widawsky
2021-09-21 22:04 ` [PATCH 2/7] cxl/pci: Remove dev_dbg for unknown register blocks Ben Widawsky
2021-09-21 22:04 ` [PATCH 3/7] cxl/pci: Refactor cxl_pci_setup_regs Ben Widawsky
2021-09-21 23:39   ` Dan Williams
2021-09-22  4:31     ` Ben Widawsky
2021-09-21 22:04 ` [PATCH 4/7] cxl/pci: Make more use of cxl_register_map Ben Widawsky
2021-09-21 22:04 ` [PATCH 5/7] PCI: Add pci_find_dvsec_capability to find designated VSEC Ben Widawsky
2021-09-22  9:33   ` Frederic Barrat
2021-09-21 22:04 ` [PATCH 6/7] cxl/pci: Use pci core's DVSEC functionality Ben Widawsky
2021-09-21 22:04 ` [PATCH 7/7] ocxl: " Ben Widawsky
2021-09-22  0:44   ` Dan Williams
2021-09-22  9:38     ` Frederic Barrat
2021-09-21 22:14 ` [PATCH 0/7] cxl_pci refactor for reusability Ben Widawsky
2021-09-21 22:28 ` Dan Williams
2021-09-21 23:03   ` Ben Widawsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).