linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/10] cxl_pci refactor for reusability
@ 2021-10-09 16:43 Dan Williams
  2021-10-09 16:44 ` [PATCH v3 01/10] cxl/pci: Convert register block identifiers to an enum Dan Williams
                   ` (9 more replies)
  0 siblings, 10 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:43 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ira Weiny, Ben Widawsky, Frederic Barrat, Frederic Barrat,
	Jonathan Cameron, linux-pci, stable, David E. Box, Bjorn Helgaas,
	Lu Baolu, Kan Liang, linuxppc-dev, Andrew Donnellan, linux-pci,
	linux-kernel, hch

Changes since v2 [1]:
- Rework some of the changelogs per feedback (Bjorn, and I)
- Move the cxl_register_map refactor earlier in the series to make the
  cxl_setup_pci_regs() refactor easier to read.
- Fix a bug added in v5.14 for handling the error return case
  cxl_pci_map_regblock()
- Split the addition of @base to cxl_register_map to its own patch
- Drop the cxl_pci_dvsec() wrapper (Christoph)
- Drop the SIOV conversion patch given Baolu's feedback about it being
  dead code

[1]: https://lore.kernel.org/r/20210923172647.72738-1-ben.widawsky@intel.com

---
I am helping out with the review feedback on this set while Ben is
focusing on region provisioning. It appears this rework will be suitable
to just carry in cxl/next, no need to make a cross-tree dependency for
"PCI: Add pci_find_dvsec_capability to find designated VSEC" at this
time.

Ben's original cover:

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-pci/20210913190131.xiiszmno46qie7v5@intel.com/
[2]: https://lore.kernel.org/linux-cxl/20210920225638.1729482-1-ben.widawsky@intel.com/
[3]: https://lore.kernel.org/linux-cxl/20210921220459.2437386-1-ben.widawsky@intel.com/

---

Ben Widawsky (8):
      cxl/pci: Convert register block identifiers to an enum
      cxl/pci: Remove dev_dbg for unknown register blocks
      cxl/pci: Remove pci request/release regions
      cxl/pci: Make more use of cxl_register_map
      cxl/pci: Split cxl_pci_setup_regs()
      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

Dan Williams (2):
      cxl/pci: Fix NULL vs ERR_PTR confusion
      cxl/pci: Add @base to cxl_register_map


 arch/powerpc/platforms/powernv/ocxl.c |    3 -
 drivers/cxl/cxl.h                     |    1 
 drivers/cxl/pci.c                     |  157 +++++++++++++--------------------
 drivers/cxl/pci.h                     |   14 ++-
 drivers/misc/ocxl/config.c            |   13 ---
 drivers/pci/pci.c                     |   32 +++++++
 include/linux/pci.h                   |    1 
 7 files changed, 105 insertions(+), 116 deletions(-)

base-commit: ed97afb53365cd03dde266c9644334a558fe5a16

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

* [PATCH v3 01/10] cxl/pci: Convert register block identifiers to an enum
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-09 16:44 ` [PATCH v3 02/10] cxl/pci: Remove dev_dbg for unknown register blocks Dan Williams
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ben Widawsky, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

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

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
[djbw: changelog fixups ]
Signed-off-by: Dan Williams <dan.j.williams@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__ */


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

* [PATCH v3 02/10] cxl/pci: Remove dev_dbg for unknown register blocks
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
  2021-10-09 16:44 ` [PATCH v3 01/10] cxl/pci: Convert register block identifiers to an enum Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-09 16:48   ` Joe Perches
  2021-10-09 16:44 ` [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion Dan Williams
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ben Widawsky, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

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>
Signed-off-by: Dan Williams <dan.j.williams@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;


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

* [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
  2021-10-09 16:44 ` [PATCH v3 01/10] cxl/pci: Convert register block identifiers to an enum Dan Williams
  2021-10-09 16:44 ` [PATCH v3 02/10] cxl/pci: Remove dev_dbg for unknown register blocks Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-10  3:44   ` Ira Weiny
                     ` (2 more replies)
  2021-10-09 16:44 ` [PATCH v3 04/10] cxl/pci: Remove pci request/release regions Dan Williams
                   ` (6 subsequent siblings)
  9 siblings, 3 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl
  Cc: stable, Jonathan Cameron, Ira Weiny, linux-pci, linux-kernel, hch

cxl_pci_map_regblock() may return an ERR_PTR(), but cxl_pci_setup_regs()
is only prepared for NULL as the error case.

Fixes: f8a7e8c29be8 ("cxl/pci: Reserve all device regions at once")
Cc: <stable@vger.kernel.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index ccc7c2573ddc..9c178002d49e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -317,7 +317,7 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
 			&pdev->resource[bar], (unsigned long long)offset);
-		return IOMEM_ERR_PTR(-ENXIO);
+		return NULL;
 	}
 
 	addr = pci_iomap(pdev, bar, 0);


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

* [PATCH v3 04/10] cxl/pci: Remove pci request/release regions
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
                   ` (2 preceding siblings ...)
  2021-10-09 16:44 ` [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-09 16:44 ` [PATCH v3 05/10] cxl/pci: Make more use of cxl_register_map Dan Williams
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ben Widawsky, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

Quoting Dan, "... 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."

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 9c178002d49e..21dd10a77eb3 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -453,9 +453,6 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 		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);
@@ -499,8 +496,6 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 		n_maps++;
 	}
 
-	pci_release_mem_regions(pdev);
-
 	for (i = 0; i < n_maps; i++) {
 		ret = cxl_map_regs(cxlm, &maps[i]);
 		if (ret)


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

* [PATCH v3 05/10] cxl/pci: Make more use of cxl_register_map
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
                   ` (3 preceding siblings ...)
  2021-10-09 16:44 ` [PATCH v3 04/10] cxl/pci: Remove pci request/release regions Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-09 19:04   ` kernel test robot
  2021-10-09 20:51   ` [PATCH v4 " Dan Williams
  2021-10-09 16:44 ` [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map Dan Williams
                   ` (4 subsequent siblings)
  9 siblings, 2 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ben Widawsky, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

The structure exists to pass around information about register mapping.
Use it for passing @barno and @block_offset, and eliminate duplicate
local variables.

The helpers that use @map do not care about @cxlm, so just pass them a
pdev instead.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
[djbw: separate @base conversion]
[djbw: reorder before cxl_pci_setup_regs() refactor to improver readability]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |   51 +++++++++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 21dd10a77eb3..9f006299a0e3 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 pci_dev *pdev,
+					  struct cxl_register_map *map)
 {
 	void __iomem *addr;
-	struct device *dev = cxlm->dev;
-	struct pci_dev *pdev = to_pci_dev(dev);
+	int bar = map->barno;
+	struct device *dev = &pdev->dev;
+	resource_size_t offset = map->block_offset;
 
 	/* Basic sanity check that BAR is big enough */
 	if (pci_resource_len(pdev, bar) < offset) {
@@ -332,9 +333,9 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
 	return addr;
 }
 
-static void cxl_pci_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
+static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
 {
-	pci_iounmap(to_pci_dev(cxlm->dev), base);
+	pci_iounmap(pdev, base);
 }
 
 static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
@@ -360,12 +361,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
-static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
+static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
 			  struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
-	struct device *dev = cxlm->dev;
+	struct device *dev = &pdev->dev;
 
 	switch (map->reg_type) {
 	case CXL_REGLOC_RBI_COMPONENT:
@@ -420,12 +421,13 @@ static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
 	return 0;
 }
 
-static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
-				      u8 *bar, u64 *offset, u8 *reg_type)
+static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
+				struct cxl_register_map *map)
 {
-	*offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
-	*bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
-	*reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
+	map->block_offset =
+		((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
+	map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
+	map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
 }
 
 /**
@@ -462,34 +464,23 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 
 	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
-		u8 reg_type;
-		u64 offset;
-		u8 bar;
 
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
-		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
-					  &reg_type);
+		map = &maps[n_maps];
+		cxl_decode_regblock(reg_lo, reg_hi, map);
 
 		/* Ignore unknown register block types */
-		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
+		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		base = cxl_pci_map_regblock(cxlm, bar, offset);
+		base = cxl_pci_map_regblock(pdev, map);
 		if (!base)
 			return -ENOMEM;
 
-		map = &maps[n_maps];
-		map->barno = bar;
-		map->block_offset = offset;
-		map->reg_type = reg_type;
-
-		ret = cxl_probe_regs(cxlm, base + offset, map);
-
-		/* Always unmap the regblock regardless of probe success */
-		cxl_pci_unmap_regblock(cxlm, base);
-
+		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
+		cxl_pci_unmap_regblock(pdev, base);
 		if (ret)
 			return ret;
 


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

* [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
                   ` (4 preceding siblings ...)
  2021-10-09 16:44 ` [PATCH v3 05/10] cxl/pci: Make more use of cxl_register_map Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-10  4:20   ` Ira Weiny
                     ` (2 more replies)
  2021-10-09 16:44 ` [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs() Dan Williams
                   ` (3 subsequent siblings)
  9 siblings, 3 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl; +Cc: linux-pci, linux-kernel, hch

In addition to carrying @barno, @block_offset, and @reg_type, add @base
to keep all map/unmap parameters in one object. The helpers
cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
at map and unmap time.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/cxl.h |    1 +
 drivers/cxl/pci.c |   31 ++++++++++++++++---------------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index a6687e7fd598..7cd16ef144dd 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -140,6 +140,7 @@ struct cxl_device_reg_map {
 };
 
 struct cxl_register_map {
+	void __iomem *base;
 	u64 block_offset;
 	u8 reg_type;
 	u8 barno;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 9f006299a0e3..b42407d067ac 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
 	return 0;
 }
 
-static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
-					  struct cxl_register_map *map)
+static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
 {
 	void __iomem *addr;
 	int bar = map->barno;
@@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
 			&pdev->resource[bar], (unsigned long long)offset);
-		return NULL;
+		return -ENXIO;
 	}
 
 	addr = pci_iomap(pdev, bar, 0);
 	if (!addr) {
 		dev_err(dev, "failed to map registers\n");
-		return addr;
+		return -ENOMEM;
 	}
 
 	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
 		bar, offset);
 
-	return addr;
+	map->base = addr + map->block_offset;
+	return 0;
 }
 
-static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
+static void cxl_unmap_regblock(struct pci_dev *pdev,
+			       struct cxl_register_map *map)
 {
-	pci_iounmap(pdev, base);
+	pci_iounmap(pdev, map->base - map->block_offset);
+	map->base = NULL;
 }
 
 static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
@@ -361,12 +363,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
-static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
-			  struct cxl_register_map *map)
+static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
 	struct device *dev = &pdev->dev;
+	void __iomem *base = map->base;
 
 	switch (map->reg_type) {
 	case CXL_REGLOC_RBI_COMPONENT:
@@ -442,7 +444,6 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
  */
 static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 {
-	void __iomem *base;
 	u32 regloc_size, regblocks;
 	int regloc, i, n_maps, ret = 0;
 	struct device *dev = cxlm->dev;
@@ -475,12 +476,12 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		base = cxl_pci_map_regblock(pdev, map);
-		if (!base)
-			return -ENOMEM;
+		ret = cxl_map_regblock(pdev, map);
+		if (ret)
+			return ret;
 
-		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
-		cxl_pci_unmap_regblock(pdev, base);
+		ret = cxl_probe_regs(pdev, map);
+		cxl_unmap_regblock(pdev, map);
 		if (ret)
 			return ret;
 


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

* [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
                   ` (5 preceding siblings ...)
  2021-10-09 16:44 ` [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-10  4:44   ` Ira Weiny
                     ` (3 more replies)
  2021-10-09 16:44 ` [PATCH v3 08/10] PCI: Add pci_find_dvsec_capability to find designated VSEC Dan Williams
                   ` (2 subsequent siblings)
  9 siblings, 4 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ben Widawsky, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

In preparation for moving parts of register mapping to cxl_core, split
cxl_pci_setup_regs() into a helper that finds register blocks,
(cxl_find_regblock()), and a generic wrapper that probes the precise
register sets within a block (cxl_setup_regs()).

Move the actual mapping (cxl_map_regs()) of the only register-set that
cxl_pci cares about (memory device registers) up a level from the former
cxl_pci_setup_regs() into cxl_pci_probe().

With this change the unused component registers are no longer mapped,
but the helpers are primed to move into the core.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
[djbw: rebase on the cxl_register_map refactor]
[djbw: drop cxl_map_regs() for component registers]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |   73 +++++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b42407d067ac..b6bc8e5ca028 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -433,72 +433,69 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
 }
 
 /**
- * cxl_pci_setup_regs() - Setup necessary MMIO.
- * @cxlm: The CXL memory device to communicate with.
+ * cxl_find_regblock() - Locate register blocks by type
+ * @pdev: The CXL PCI device to enumerate.
+ * @type: Register Block Indicator id
+ * @map: Enumeration output, clobbered on error
  *
- * Return: 0 if all necessary registers mapped.
+ * Return: 0 if register block enumerated, negative error code otherwise
  *
- * 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.
+ * A CXL DVSEC may additional point one or more register blocks, search
+ * for them by @type.
  */
-static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
+static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
+			     struct cxl_register_map *map)
 {
 	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];
+	int regloc, i;
 
 	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;
-	}
 
-	/* 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;
 
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
-		map = &maps[n_maps];
 		cxl_decode_regblock(reg_lo, reg_hi, map);
 
-		/* Ignore unknown register block types */
-		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
-			continue;
+		if (map->reg_type == type)
+			return 0;
+	}
 
-		ret = cxl_map_regblock(pdev, map);
-		if (ret)
-			return ret;
+	return -ENODEV;
+}
 
-		ret = cxl_probe_regs(pdev, map);
-		cxl_unmap_regblock(pdev, map);
-		if (ret)
-			return ret;
+static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
+			  struct cxl_register_map *map)
+{
+	int rc;
 
-		n_maps++;
-	}
+	rc = cxl_find_regblock(pdev, type, map);
+	if (rc)
+		return rc;
 
-	for (i = 0; i < n_maps; i++) {
-		ret = cxl_map_regs(cxlm, &maps[i]);
-		if (ret)
-			break;
-	}
+	rc = cxl_map_regblock(pdev, map);
+	if (rc)
+		return rc;
+
+	rc = cxl_probe_regs(pdev, map);
+	cxl_unmap_regblock(pdev, map);
 
-	return ret;
+	return rc;
 }
 
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	struct cxl_register_map map;
 	struct cxl_memdev *cxlmd;
 	struct cxl_mem *cxlm;
 	int rc;
@@ -518,7 +515,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlm))
 		return PTR_ERR(cxlm);
 
-	rc = cxl_pci_setup_regs(cxlm);
+	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
+	if (rc)
+		return rc;
+
+	rc = cxl_map_regs(cxlm, &map);
 	if (rc)
 		return rc;
 


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

* [PATCH v3 08/10] PCI: Add pci_find_dvsec_capability to find designated VSEC
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
                   ` (6 preceding siblings ...)
  2021-10-09 16:44 ` [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs() Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-09 16:44 ` [PATCH v3 09/10] cxl/pci: Use pci core's DVSEC functionality Dan Williams
  2021-10-09 16:44 ` [PATCH v3 10/10] ocxl: " Dan Williams
  9 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl
  Cc: David E. Box, Jonathan Cameron, Bjorn Helgaas, linux-pci,
	linuxppc-dev, Andrew Donnellan, Lu Baolu, Frederic Barrat,
	Ben Widawsky, Andrew Donnellan, Bjorn Helgaas, Kan Liang,
	Kan Liang, Jonathan Cameron, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

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

The Designated Vendor-Specific Extended Capability (DVSEC) allows one or
more "vendor" specific capabilities that are not tied to the Vendor ID
of the PCI component. Where the DVSEC Vendor may be a standards body
like CXL.

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: Andrew Donnellan <ajd@linux.ibm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Tested-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@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);
 


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

* [PATCH v3 09/10] cxl/pci: Use pci core's DVSEC functionality
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
                   ` (7 preceding siblings ...)
  2021-10-09 16:44 ` [PATCH v3 08/10] PCI: Add pci_find_dvsec_capability to find designated VSEC Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  2021-10-11 13:35   ` Jonathan Cameron
  2021-10-09 16:44 ` [PATCH v3 10/10] ocxl: " Dan Williams
  9 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ben Widawsky, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

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

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
[djbw: kill cxl_pci_dvsec()]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |   26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b6bc8e5ca028..f2e2a02d1fe6 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -340,29 +340,6 @@ static void cxl_unmap_regblock(struct pci_dev *pdev,
 	map->base = NULL;
 }
 
-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;
-}
-
 static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
@@ -449,7 +426,8 @@ static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
 	u32 regloc_size, regblocks;
 	int regloc, i;
 
-	regloc = cxl_pci_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
+	regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
+					   PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
 	if (!regloc)
 		return -ENXIO;
 


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

* [PATCH v3 10/10] ocxl: Use pci core's DVSEC functionality
  2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
                   ` (8 preceding siblings ...)
  2021-10-09 16:44 ` [PATCH v3 09/10] cxl/pci: Use pci core's DVSEC functionality Dan Williams
@ 2021-10-09 16:44 ` Dan Williams
  9 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 16:44 UTC (permalink / raw)
  To: linux-cxl
  Cc: linuxppc-dev, Andrew Donnellan, Frederic Barrat, Ben Widawsky,
	Andrew Donnellan, linux-pci, linux-kernel, hch

From: Ben Widawsky <ben.widawsky@intel.com>

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

There are two obvious places to simply drop in the new core
implementation. There remains find_dvsec_from_pos() which would benefit
from using a core implementation. As that change is less trivial it is
reserved for later.

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Andrew Donnellan <ajd@linux.ibm.com>
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com> (v1)
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/powerpc/platforms/powernv/ocxl.c |    3 ++-
 drivers/misc/ocxl/config.c            |   13 +------------
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
index 9105efcf242a..28b009b46464 100644
--- a/arch/powerpc/platforms/powernv/ocxl.c
+++ b/arch/powerpc/platforms/powernv/ocxl.c
@@ -107,7 +107,8 @@ static int get_max_afu_index(struct pci_dev *dev, int *afu_idx)
 	int pos;
 	u32 val;
 
-	pos = find_dvsec_from_pos(dev, OCXL_DVSEC_FUNC_ID, 0);
+	pos = pci_find_dvsec_capability(dev, PCI_VENDOR_ID_IBM,
+					OCXL_DVSEC_FUNC_ID);
 	if (!pos)
 		return -ESRCH;
 
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)


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

* Re: [PATCH v3 02/10] cxl/pci: Remove dev_dbg for unknown register blocks
  2021-10-09 16:44 ` [PATCH v3 02/10] cxl/pci: Remove dev_dbg for unknown register blocks Dan Williams
@ 2021-10-09 16:48   ` Joe Perches
  2021-10-09 18:04     ` Ben Widawsky
  0 siblings, 1 reply; 43+ messages in thread
From: Joe Perches @ 2021-10-09 16:48 UTC (permalink / raw)
  To: Dan Williams, linux-cxl; +Cc: Ben Widawsky, linux-pci, linux-kernel, hch

On Sat, 2021-10-09 at 09:44 -0700, Dan Williams wrote:
> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> While interesting to driver developers, the dev_dbg message doesn't do
> much except clutter up logs.

So?  This isn't enabled by default.  How does it 'clutter' 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>
> Signed-off-by: Dan Williams <dan.j.williams@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;
> 



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

* Re: [PATCH v3 02/10] cxl/pci: Remove dev_dbg for unknown register blocks
  2021-10-09 16:48   ` Joe Perches
@ 2021-10-09 18:04     ` Ben Widawsky
  0 siblings, 0 replies; 43+ messages in thread
From: Ben Widawsky @ 2021-10-09 18:04 UTC (permalink / raw)
  To: Joe Perches; +Cc: Dan Williams, linux-cxl, linux-pci, linux-kernel, hch

On 21-10-09 09:48:10, Joe Perches wrote:
> On Sat, 2021-10-09 at 09:44 -0700, Dan Williams wrote:
> > From: Ben Widawsky <ben.widawsky@intel.com>
> > 
> > While interesting to driver developers, the dev_dbg message doesn't do
> > much except clutter up logs.
> 
> So?  This isn't enabled by default.  How does it 'clutter' logs?
> 

Clutter logs for driver developers working on this subsystem. It's fine to drop
this and just use the next sentence as the explanation as well.

> > 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>
> > Signed-off-by: Dan Williams <dan.j.williams@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;
> > 
> 
> 

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

* Re: [PATCH v3 05/10] cxl/pci: Make more use of cxl_register_map
  2021-10-09 16:44 ` [PATCH v3 05/10] cxl/pci: Make more use of cxl_register_map Dan Williams
@ 2021-10-09 19:04   ` kernel test robot
  2021-10-09 20:51   ` [PATCH v4 " Dan Williams
  1 sibling, 0 replies; 43+ messages in thread
From: kernel test robot @ 2021-10-09 19:04 UTC (permalink / raw)
  To: Dan Williams, linux-cxl
  Cc: kbuild-all, Ben Widawsky, linux-pci, linux-kernel, hch

[-- Attachment #1: Type: text/plain, Size: 6011 bytes --]

Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ed97afb53365cd03dde266c9644334a558fe5a16]

url:    https://github.com/0day-ci/linux/commits/Dan-Williams/cxl_pci-refactor-for-reusability/20211010-004521
base:   ed97afb53365cd03dde266c9644334a558fe5a16
config: parisc-randconfig-r012-20211010 (attached as .config)
compiler: hppa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/049a2765e60ef3807f8e4b8d04f2b70d90b38c94
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dan-Williams/cxl_pci-refactor-for-reusability/20211010-004521
        git checkout 049a2765e60ef3807f8e4b8d04f2b70d90b38c94
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:555,
                    from include/linux/kernel.h:19,
                    from arch/parisc/include/asm/bug.h:5,
                    from include/linux/bug.h:5,
                    from include/linux/io.h:11,
                    from include/linux/io-64-nonatomic-lo-hi.h:5,
                    from drivers/cxl/pci.c:3:
   drivers/cxl/pci.c: In function 'cxl_pci_map_regblock':
>> drivers/cxl/pci.c:330:22: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 5 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
     330 |         dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
         |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:166:9: note: in expansion of macro '_dynamic_func_call'
     166 |         _dynamic_func_call(fmt,__dynamic_dev_dbg,               \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:9: note: in expansion of macro 'dynamic_dev_dbg'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:30: note: in expansion of macro 'dev_fmt'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                              ^~~~~~~
   drivers/cxl/pci.c:330:9: note: in expansion of macro 'dev_dbg'
     330 |         dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
         |         ^~~~~~~
   drivers/cxl/pci.c:330:70: note: format string is defined here
     330 |         dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
         |                                                                  ~~~~^
         |                                                                      |
         |                                                                      long long unsigned int
         |                                                                  %#x


vim +330 drivers/cxl/pci.c

8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  308  
049a2765e60ef3 drivers/cxl/pci.c Ben Widawsky 2021-10-09  309  static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
049a2765e60ef3 drivers/cxl/pci.c Ben Widawsky 2021-10-09  310  					  struct cxl_register_map *map)
1b0a1a2a193400 drivers/cxl/pci.c Ben Widawsky 2021-04-07  311  {
f8a7e8c29be873 drivers/cxl/pci.c Ira Weiny    2021-05-27  312  	void __iomem *addr;
049a2765e60ef3 drivers/cxl/pci.c Ben Widawsky 2021-10-09  313  	int bar = map->barno;
049a2765e60ef3 drivers/cxl/pci.c Ben Widawsky 2021-10-09  314  	struct device *dev = &pdev->dev;
049a2765e60ef3 drivers/cxl/pci.c Ben Widawsky 2021-10-09  315  	resource_size_t offset = map->block_offset;
1b0a1a2a193400 drivers/cxl/pci.c Ben Widawsky 2021-04-07  316  
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  317  	/* Basic sanity check that BAR is big enough */
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  318  	if (pci_resource_len(pdev, bar) < offset) {
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  319  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  320  			&pdev->resource[bar], (unsigned long long)offset);
a0270407a9b3b5 drivers/cxl/pci.c Dan Williams 2021-10-09  321  		return NULL;
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  322  	}
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  323  
30af97296f48d8 drivers/cxl/pci.c Ira Weiny    2021-06-03  324  	addr = pci_iomap(pdev, bar, 0);
f8a7e8c29be873 drivers/cxl/pci.c Ira Weiny    2021-05-27  325  	if (!addr) {
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  326  		dev_err(dev, "failed to map registers\n");
f8a7e8c29be873 drivers/cxl/pci.c Ira Weiny    2021-05-27  327  		return addr;
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  328  	}
8adaf747c9f0b4 drivers/cxl/mem.c Ben Widawsky 2021-02-16  329  
f8a7e8c29be873 drivers/cxl/pci.c Ira Weiny    2021-05-27 @330  	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
f8a7e8c29be873 drivers/cxl/pci.c Ira Weiny    2021-05-27  331  		bar, offset);
6630d31c912ed2 drivers/cxl/pci.c Ben Widawsky 2021-05-20  332  
30af97296f48d8 drivers/cxl/pci.c Ira Weiny    2021-06-03  333  	return addr;
30af97296f48d8 drivers/cxl/pci.c Ira Weiny    2021-06-03  334  }
30af97296f48d8 drivers/cxl/pci.c Ira Weiny    2021-06-03  335  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37105 bytes --]

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

* [PATCH v4 05/10] cxl/pci: Make more use of cxl_register_map
  2021-10-09 16:44 ` [PATCH v3 05/10] cxl/pci: Make more use of cxl_register_map Dan Williams
  2021-10-09 19:04   ` kernel test robot
@ 2021-10-09 20:51   ` Dan Williams
  2021-10-10  4:03     ` Ira Weiny
  2021-10-13 23:53     ` [PATCH v5 " Dan Williams
  1 sibling, 2 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-09 20:51 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ben Widawsky, kernel test robot, linux-pci, linux-kernel

From: Ben Widawsky <ben.widawsky@intel.com>

The structure exists to pass around information about register mapping.
Use it for passing @barno and @block_offset, and eliminate duplicate
local variables.

The helpers that use @map do not care about @cxlm, so just pass them a
pdev instead.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
[djbw: separate @base conversion]
[djbw: reorder before cxl_pci_setup_regs() refactor to improver readability]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v3:
- Fix a 0day report about printing a resource_size_t

 drivers/cxl/pci.c |   55 ++++++++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 21dd10a77eb3..f1de236ccd13 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 pci_dev *pdev,
+					  struct cxl_register_map *map)
 {
 	void __iomem *addr;
-	struct device *dev = cxlm->dev;
-	struct pci_dev *pdev = to_pci_dev(dev);
+	int bar = map->barno;
+	struct device *dev = &pdev->dev;
+	resource_size_t offset = map->block_offset;
 
 	/* Basic sanity check that BAR is big enough */
 	if (pci_resource_len(pdev, bar) < offset) {
@@ -326,15 +327,15 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
 		return addr;
 	}
 
-	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
-		bar, offset);
+	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
+		bar, &offset);
 
 	return addr;
 }
 
-static void cxl_pci_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
+static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
 {
-	pci_iounmap(to_pci_dev(cxlm->dev), base);
+	pci_iounmap(pdev, base);
 }
 
 static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
@@ -360,12 +361,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
-static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
+static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
 			  struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
-	struct device *dev = cxlm->dev;
+	struct device *dev = &pdev->dev;
 
 	switch (map->reg_type) {
 	case CXL_REGLOC_RBI_COMPONENT:
@@ -420,12 +421,13 @@ static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
 	return 0;
 }
 
-static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
-				      u8 *bar, u64 *offset, u8 *reg_type)
+static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
+				struct cxl_register_map *map)
 {
-	*offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
-	*bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
-	*reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
+	map->block_offset =
+		((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
+	map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
+	map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
 }
 
 /**
@@ -462,34 +464,23 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 
 	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
-		u8 reg_type;
-		u64 offset;
-		u8 bar;
 
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
-		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
-					  &reg_type);
+		map = &maps[n_maps];
+		cxl_decode_regblock(reg_lo, reg_hi, map);
 
 		/* Ignore unknown register block types */
-		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
+		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		base = cxl_pci_map_regblock(cxlm, bar, offset);
+		base = cxl_pci_map_regblock(pdev, map);
 		if (!base)
 			return -ENOMEM;
 
-		map = &maps[n_maps];
-		map->barno = bar;
-		map->block_offset = offset;
-		map->reg_type = reg_type;
-
-		ret = cxl_probe_regs(cxlm, base + offset, map);
-
-		/* Always unmap the regblock regardless of probe success */
-		cxl_pci_unmap_regblock(cxlm, base);
-
+		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
+		cxl_pci_unmap_regblock(pdev, base);
 		if (ret)
 			return ret;
 


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

* Re: [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion
  2021-10-09 16:44 ` [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion Dan Williams
@ 2021-10-10  3:44   ` Ira Weiny
  2021-10-15 16:15   ` Jonathan Cameron
  2021-10-15 21:29   ` [PATCH v6 " Dan Williams
  2 siblings, 0 replies; 43+ messages in thread
From: Ira Weiny @ 2021-10-10  3:44 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-cxl, stable, Jonathan Cameron, linux-pci, linux-kernel, hch

On Sat, Oct 09, 2021 at 09:44:13AM -0700, Dan Williams wrote:
> cxl_pci_map_regblock() may return an ERR_PTR(), but cxl_pci_setup_regs()
> is only prepared for NULL as the error case.
> 
> Fixes: f8a7e8c29be8 ("cxl/pci: Reserve all device regions at once")
> Cc: <stable@vger.kernel.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index ccc7c2573ddc..9c178002d49e 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -317,7 +317,7 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
>  			&pdev->resource[bar], (unsigned long long)offset);
> -		return IOMEM_ERR_PTR(-ENXIO);
> +		return NULL;
>  	}
>  
>  	addr = pci_iomap(pdev, bar, 0);
> 

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

* Re: [PATCH v4 05/10] cxl/pci: Make more use of cxl_register_map
  2021-10-09 20:51   ` [PATCH v4 " Dan Williams
@ 2021-10-10  4:03     ` Ira Weiny
  2021-10-13 23:53     ` [PATCH v5 " Dan Williams
  1 sibling, 0 replies; 43+ messages in thread
From: Ira Weiny @ 2021-10-10  4:03 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-cxl, Ben Widawsky, kernel test robot, linux-pci, linux-kernel

On Sat, Oct 09, 2021 at 01:51:13PM -0700, Dan Williams wrote:
> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> The structure exists to pass around information about register mapping.
> Use it for passing @barno and @block_offset, and eliminate duplicate
> local variables.
> 
> The helpers that use @map do not care about @cxlm, so just pass them a
> pdev instead.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Reported-by: kernel test robot <lkp@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> [djbw: separate @base conversion]
> [djbw: reorder before cxl_pci_setup_regs() refactor to improver readability]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Changes since v3:
> - Fix a 0day report about printing a resource_size_t
> 
>  drivers/cxl/pci.c |   55 ++++++++++++++++++++++-------------------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 21dd10a77eb3..f1de236ccd13 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 pci_dev *pdev,
> +					  struct cxl_register_map *map)
>  {
>  	void __iomem *addr;
> -	struct device *dev = cxlm->dev;
> -	struct pci_dev *pdev = to_pci_dev(dev);
> +	int bar = map->barno;
> +	struct device *dev = &pdev->dev;
> +	resource_size_t offset = map->block_offset;
>  
>  	/* Basic sanity check that BAR is big enough */
>  	if (pci_resource_len(pdev, bar) < offset) {
> @@ -326,15 +327,15 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
>  		return addr;
>  	}
>  
> -	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
> -		bar, offset);
> +	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
> +		bar, &offset);
>  
>  	return addr;
>  }
>  
> -static void cxl_pci_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
> +static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
>  {
> -	pci_iounmap(to_pci_dev(cxlm->dev), base);
> +	pci_iounmap(pdev, base);
>  }
>  
>  static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
> @@ -360,12 +361,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> -static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
> +static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
>  			  struct cxl_register_map *map)
>  {
>  	struct cxl_component_reg_map *comp_map;
>  	struct cxl_device_reg_map *dev_map;
> -	struct device *dev = cxlm->dev;
> +	struct device *dev = &pdev->dev;
>  
>  	switch (map->reg_type) {
>  	case CXL_REGLOC_RBI_COMPONENT:
> @@ -420,12 +421,13 @@ static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
>  	return 0;
>  }
>  
> -static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
> -				      u8 *bar, u64 *offset, u8 *reg_type)
> +static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
> +				struct cxl_register_map *map)
>  {
> -	*offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
> -	*bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
> -	*reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
> +	map->block_offset =
> +		((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
> +	map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
> +	map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
>  }
>  
>  /**
> @@ -462,34 +464,23 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>  
>  	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
>  		u32 reg_lo, reg_hi;
> -		u8 reg_type;
> -		u64 offset;
> -		u8 bar;
>  
>  		pci_read_config_dword(pdev, regloc, &reg_lo);
>  		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
>  
> -		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
> -					  &reg_type);
> +		map = &maps[n_maps];
> +		cxl_decode_regblock(reg_lo, reg_hi, map);
>  
>  		/* Ignore unknown register block types */
> -		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
> +		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
>  			continue;
>  
> -		base = cxl_pci_map_regblock(cxlm, bar, offset);
> +		base = cxl_pci_map_regblock(pdev, map);
>  		if (!base)
>  			return -ENOMEM;
>  
> -		map = &maps[n_maps];
> -		map->barno = bar;
> -		map->block_offset = offset;
> -		map->reg_type = reg_type;
> -
> -		ret = cxl_probe_regs(cxlm, base + offset, map);
> -
> -		/* Always unmap the regblock regardless of probe success */
> -		cxl_pci_unmap_regblock(cxlm, base);
> -
> +		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
> +		cxl_pci_unmap_regblock(pdev, base);
>  		if (ret)
>  			return ret;
>  
> 

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

* Re: [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-09 16:44 ` [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map Dan Williams
@ 2021-10-10  4:20   ` Ira Weiny
  2021-10-13 22:53     ` Dan Williams
  2021-10-13 23:57   ` [PATCH v5 " Dan Williams
  2021-10-15 16:27   ` [PATCH v3 " Jonathan Cameron
  2 siblings, 1 reply; 43+ messages in thread
From: Ira Weiny @ 2021-10-10  4:20 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, linux-pci, linux-kernel, hch

On Sat, Oct 09, 2021 at 09:44:29AM -0700, Dan Williams wrote:
> In addition to carrying @barno, @block_offset, and @reg_type, add @base
> to keep all map/unmap parameters in one object. The helpers
> cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> at map and unmap time.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/cxl.h |    1 +
>  drivers/cxl/pci.c |   31 ++++++++++++++++---------------
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a6687e7fd598..7cd16ef144dd 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -140,6 +140,7 @@ struct cxl_device_reg_map {
>  };
>  
>  struct cxl_register_map {
> +	void __iomem *base;
>  	u64 block_offset;
>  	u8 reg_type;
>  	u8 barno;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 9f006299a0e3..b42407d067ac 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
>  	return 0;
>  }
>  
> -static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> -					  struct cxl_register_map *map)
> +static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
>  	void __iomem *addr;
>  	int bar = map->barno;
> @@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
>  			&pdev->resource[bar], (unsigned long long)offset);
> -		return NULL;
> +		return -ENXIO;
>  	}
>  
>  	addr = pci_iomap(pdev, bar, 0);
>  	if (!addr) {
>  		dev_err(dev, "failed to map registers\n");
> -		return addr;
> +		return -ENOMEM;
>  	}
>  
>  	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
>  		bar, offset);
>  
> -	return addr;
> +	map->base = addr + map->block_offset;
> +	return 0;
>  }
>  
> -static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
> +static void cxl_unmap_regblock(struct pci_dev *pdev,
> +			       struct cxl_register_map *map)
>  {
> -	pci_iounmap(pdev, base);
> +	pci_iounmap(pdev, map->base - map->block_offset);

I know we need to get these in soon.  But I think map->base should be 'base'
and map->block_offset should be handled in cxl_probe_regs() rather than
subtract it here..

Either way this is cleaner than what it was.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> +	map->base = NULL;
>  }
>  
>  static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
> @@ -361,12 +363,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> -static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
> -			  struct cxl_register_map *map)
> +static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
>  	struct cxl_component_reg_map *comp_map;
>  	struct cxl_device_reg_map *dev_map;
>  	struct device *dev = &pdev->dev;
> +	void __iomem *base = map->base;
>  
>  	switch (map->reg_type) {
>  	case CXL_REGLOC_RBI_COMPONENT:
> @@ -442,7 +444,6 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
>   */
>  static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>  {
> -	void __iomem *base;
>  	u32 regloc_size, regblocks;
>  	int regloc, i, n_maps, ret = 0;
>  	struct device *dev = cxlm->dev;
> @@ -475,12 +476,12 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>  		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
>  			continue;
>  
> -		base = cxl_pci_map_regblock(pdev, map);
> -		if (!base)
> -			return -ENOMEM;
> +		ret = cxl_map_regblock(pdev, map);
> +		if (ret)
> +			return ret;
>  
> -		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
> -		cxl_pci_unmap_regblock(pdev, base);
> +		ret = cxl_probe_regs(pdev, map);
> +		cxl_unmap_regblock(pdev, map);
>  		if (ret)
>  			return ret;
>  
> 

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

* Re: [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-09 16:44 ` [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs() Dan Williams
@ 2021-10-10  4:44   ` Ira Weiny
  2021-10-13 22:45   ` Ben Widawsky
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 43+ messages in thread
From: Ira Weiny @ 2021-10-10  4:44 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, Ben Widawsky, linux-pci, linux-kernel, hch

On Sat, Oct 09, 2021 at 09:44:34AM -0700, Dan Williams wrote:
> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> In preparation for moving parts of register mapping to cxl_core, split
> cxl_pci_setup_regs() into a helper that finds register blocks,
> (cxl_find_regblock()), and a generic wrapper that probes the precise
> register sets within a block (cxl_setup_regs()).
> 
> Move the actual mapping (cxl_map_regs()) of the only register-set that
> cxl_pci cares about (memory device registers) up a level from the former
> cxl_pci_setup_regs() into cxl_pci_probe().
> 
> With this change the unused component registers are no longer mapped,
> but the helpers are primed to move into the core.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> [djbw: rebase on the cxl_register_map refactor]
> [djbw: drop cxl_map_regs() for component registers]

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/pci.c |   73 +++++++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b42407d067ac..b6bc8e5ca028 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -433,72 +433,69 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
>  }
>  
>  /**
> - * cxl_pci_setup_regs() - Setup necessary MMIO.
> - * @cxlm: The CXL memory device to communicate with.
> + * cxl_find_regblock() - Locate register blocks by type
> + * @pdev: The CXL PCI device to enumerate.
> + * @type: Register Block Indicator id
> + * @map: Enumeration output, clobbered on error
>   *
> - * Return: 0 if all necessary registers mapped.
> + * Return: 0 if register block enumerated, negative error code otherwise
>   *
> - * 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.
> + * A CXL DVSEC may additional point one or more register blocks, search
> + * for them by @type.
>   */
> -static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
> +static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> +			     struct cxl_register_map *map)
>  {
>  	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];
> +	int regloc, i;
>  
>  	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;
> -	}
>  
> -	/* 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;
>  
>  		pci_read_config_dword(pdev, regloc, &reg_lo);
>  		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
>  
> -		map = &maps[n_maps];
>  		cxl_decode_regblock(reg_lo, reg_hi, map);
>  
> -		/* Ignore unknown register block types */
> -		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
> -			continue;
> +		if (map->reg_type == type)
> +			return 0;
> +	}
>  
> -		ret = cxl_map_regblock(pdev, map);
> -		if (ret)
> -			return ret;
> +	return -ENODEV;
> +}
>  
> -		ret = cxl_probe_regs(pdev, map);
> -		cxl_unmap_regblock(pdev, map);
> -		if (ret)
> -			return ret;
> +static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> +			  struct cxl_register_map *map)
> +{
> +	int rc;
>  
> -		n_maps++;
> -	}
> +	rc = cxl_find_regblock(pdev, type, map);
> +	if (rc)
> +		return rc;
>  
> -	for (i = 0; i < n_maps; i++) {
> -		ret = cxl_map_regs(cxlm, &maps[i]);
> -		if (ret)
> -			break;
> -	}
> +	rc = cxl_map_regblock(pdev, map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_probe_regs(pdev, map);
> +	cxl_unmap_regblock(pdev, map);
>  
> -	return ret;
> +	return rc;
>  }
>  
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> +	struct cxl_register_map map;
>  	struct cxl_memdev *cxlmd;
>  	struct cxl_mem *cxlm;
>  	int rc;
> @@ -518,7 +515,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(cxlm))
>  		return PTR_ERR(cxlm);
>  
> -	rc = cxl_pci_setup_regs(cxlm);
> +	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_map_regs(cxlm, &map);
>  	if (rc)
>  		return rc;
>  
> 

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

* Re: [PATCH v3 09/10] cxl/pci: Use pci core's DVSEC functionality
  2021-10-09 16:44 ` [PATCH v3 09/10] cxl/pci: Use pci core's DVSEC functionality Dan Williams
@ 2021-10-11 13:35   ` Jonathan Cameron
  0 siblings, 0 replies; 43+ messages in thread
From: Jonathan Cameron @ 2021-10-11 13:35 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, Ben Widawsky, linux-pci, linux-kernel, hch

On Sat, 9 Oct 2021 09:44:45 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> Reduce maintenance burden of DVSEC query implementation by using the
> centralized PCI core implementation.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> [djbw: kill cxl_pci_dvsec()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Very pleased to see this being cleaned up.  Thanks,
fwiw
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c |   26 ++------------------------
>  1 file changed, 2 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b6bc8e5ca028..f2e2a02d1fe6 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -340,29 +340,6 @@ static void cxl_unmap_regblock(struct pci_dev *pdev,
>  	map->base = NULL;
>  }
>  
> -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;
> -}
> -
>  static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
>  	struct cxl_component_reg_map *comp_map;
> @@ -449,7 +426,8 @@ static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>  	u32 regloc_size, regblocks;
>  	int regloc, i;
>  
> -	regloc = cxl_pci_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
> +	regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> +					   PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
>  	if (!regloc)
>  		return -ENXIO;
>  
> 


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

* Re: [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-09 16:44 ` [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs() Dan Williams
  2021-10-10  4:44   ` Ira Weiny
@ 2021-10-13 22:45   ` Ben Widawsky
  2021-10-13 22:49     ` Dan Williams
  2021-10-15 16:44   ` Jonathan Cameron
  2021-10-15 23:30   ` [PATCH v6 " Dan Williams
  3 siblings, 1 reply; 43+ messages in thread
From: Ben Widawsky @ 2021-10-13 22:45 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, linux-pci, linux-kernel, hch

On 21-10-09 09:44:34, Dan Williams wrote:
> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> In preparation for moving parts of register mapping to cxl_core, split
> cxl_pci_setup_regs() into a helper that finds register blocks,
> (cxl_find_regblock()), and a generic wrapper that probes the precise
> register sets within a block (cxl_setup_regs()).
> 
> Move the actual mapping (cxl_map_regs()) of the only register-set that
> cxl_pci cares about (memory device registers) up a level from the former
> cxl_pci_setup_regs() into cxl_pci_probe().
> 
> With this change the unused component registers are no longer mapped,
> but the helpers are primed to move into the core.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> [djbw: rebase on the cxl_register_map refactor]
> [djbw: drop cxl_map_regs() for component registers]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

[snip]

Did you mean to also drop the component register handling in cxl_probe_regs()
and cxl_map_regs()?


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

* Re: [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-13 22:45   ` Ben Widawsky
@ 2021-10-13 22:49     ` Dan Williams
  2021-10-14  0:12       ` Ben Widawsky
  0 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-13 22:49 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Linux PCI, Linux Kernel Mailing List, Christoph Hellwig

On Wed, Oct 13, 2021 at 3:45 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-10-09 09:44:34, Dan Williams wrote:
> > From: Ben Widawsky <ben.widawsky@intel.com>
> >
> > In preparation for moving parts of register mapping to cxl_core, split
> > cxl_pci_setup_regs() into a helper that finds register blocks,
> > (cxl_find_regblock()), and a generic wrapper that probes the precise
> > register sets within a block (cxl_setup_regs()).
> >
> > Move the actual mapping (cxl_map_regs()) of the only register-set that
> > cxl_pci cares about (memory device registers) up a level from the former
> > cxl_pci_setup_regs() into cxl_pci_probe().
> >
> > With this change the unused component registers are no longer mapped,
> > but the helpers are primed to move into the core.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > [djbw: rebase on the cxl_register_map refactor]
> > [djbw: drop cxl_map_regs() for component registers]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> [snip]
>
> Did you mean to also drop the component register handling in cxl_probe_regs()
> and cxl_map_regs()?

No, because that has a soon to be added user, right?

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

* Re: [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-10  4:20   ` Ira Weiny
@ 2021-10-13 22:53     ` Dan Williams
  2021-10-15 16:29       ` Jonathan Cameron
  0 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-13 22:53 UTC (permalink / raw)
  To: Ira Weiny
  Cc: linux-cxl, Linux PCI, Linux Kernel Mailing List, Christoph Hellwig

On Sat, Oct 9, 2021 at 9:21 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Sat, Oct 09, 2021 at 09:44:29AM -0700, Dan Williams wrote:
> > In addition to carrying @barno, @block_offset, and @reg_type, add @base
> > to keep all map/unmap parameters in one object. The helpers
> > cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> > at map and unmap time.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/cxl.h |    1 +
> >  drivers/cxl/pci.c |   31 ++++++++++++++++---------------
> >  2 files changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index a6687e7fd598..7cd16ef144dd 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -140,6 +140,7 @@ struct cxl_device_reg_map {
> >  };
> >
> >  struct cxl_register_map {
> > +     void __iomem *base;
> >       u64 block_offset;
> >       u8 reg_type;
> >       u8 barno;
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 9f006299a0e3..b42407d067ac 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
> >       return 0;
> >  }
> >
> > -static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> > -                                       struct cxl_register_map *map)
> > +static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
> >  {
> >       void __iomem *addr;
> >       int bar = map->barno;
> > @@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> >       if (pci_resource_len(pdev, bar) < offset) {
> >               dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> >                       &pdev->resource[bar], (unsigned long long)offset);
> > -             return NULL;
> > +             return -ENXIO;
> >       }
> >
> >       addr = pci_iomap(pdev, bar, 0);
> >       if (!addr) {
> >               dev_err(dev, "failed to map registers\n");
> > -             return addr;
> > +             return -ENOMEM;
> >       }
> >
> >       dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
> >               bar, offset);
> >
> > -     return addr;
> > +     map->base = addr + map->block_offset;
> > +     return 0;
> >  }
> >
> > -static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
> > +static void cxl_unmap_regblock(struct pci_dev *pdev,
> > +                            struct cxl_register_map *map)
> >  {
> > -     pci_iounmap(pdev, base);
> > +     pci_iounmap(pdev, map->base - map->block_offset);
>
> I know we need to get these in soon.  But I think map->base should be 'base'
> and map->block_offset should be handled in cxl_probe_regs() rather than
> subtract it here..

But why? The goal of the cxl_register_map cleanups is to reduce the
open-coding for details that can just be passed around in a @map
instance. Once cxl_map_regblock() sets up @base there's little reason
to consider the hardware regblock details.

> Either way this is cleaner than what it was.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks!

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

* [PATCH v5 05/10] cxl/pci: Make more use of cxl_register_map
  2021-10-09 20:51   ` [PATCH v4 " Dan Williams
  2021-10-10  4:03     ` Ira Weiny
@ 2021-10-13 23:53     ` Dan Williams
  1 sibling, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-13 23:53 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, kernel test robot, Ira Weiny, linux-pci, linux-kernel

From: Ben Widawsky <ben.widawsky@intel.com>

The structure exists to pass around information about register mapping.
Use it for passing @barno and @block_offset, and eliminate duplicate
local variables.

The helpers that use @map do not care about @cxlm, so just pass them a
pdev instead.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
[djbw: separate @base conversion]
[djbw: reorder before cxl_pci_setup_regs() refactor to improver readability]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v4:
- Collect Ira's reviewed-by, thanks Ira!
- Fixup the dev_err() in cxl_pci_map_regblock() to use %pa just like the
  same fixup in v3 of the warning introduced by changing @offset from u64 to
  resource_size_t. 

 drivers/cxl/pci.c |   59 ++++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 21dd10a77eb3..eb0c2f1b9e65 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -306,17 +306,18 @@ 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 pci_dev *pdev,
+					  struct cxl_register_map *map)
 {
 	void __iomem *addr;
-	struct device *dev = cxlm->dev;
-	struct pci_dev *pdev = to_pci_dev(dev);
+	int bar = map->barno;
+	struct device *dev = &pdev->dev;
+	resource_size_t offset = map->block_offset;
 
 	/* Basic sanity check that BAR is big enough */
 	if (pci_resource_len(pdev, bar) < offset) {
-		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
-			&pdev->resource[bar], (unsigned long long)offset);
+		dev_err(dev, "BAR%d: %pr: too small (offset: %pa)\n", bar,
+			&pdev->resource[bar], &offset);
 		return NULL;
 	}
 
@@ -326,15 +327,15 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
 		return addr;
 	}
 
-	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
-		bar, offset);
+	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
+		bar, &offset);
 
 	return addr;
 }
 
-static void cxl_pci_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
+static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
 {
-	pci_iounmap(to_pci_dev(cxlm->dev), base);
+	pci_iounmap(pdev, base);
 }
 
 static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
@@ -360,12 +361,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
-static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
+static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
 			  struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
-	struct device *dev = cxlm->dev;
+	struct device *dev = &pdev->dev;
 
 	switch (map->reg_type) {
 	case CXL_REGLOC_RBI_COMPONENT:
@@ -420,12 +421,13 @@ static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
 	return 0;
 }
 
-static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
-				      u8 *bar, u64 *offset, u8 *reg_type)
+static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
+				struct cxl_register_map *map)
 {
-	*offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
-	*bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
-	*reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
+	map->block_offset =
+		((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
+	map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
+	map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
 }
 
 /**
@@ -462,34 +464,23 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 
 	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
-		u8 reg_type;
-		u64 offset;
-		u8 bar;
 
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
-		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
-					  &reg_type);
+		map = &maps[n_maps];
+		cxl_decode_regblock(reg_lo, reg_hi, map);
 
 		/* Ignore unknown register block types */
-		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
+		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		base = cxl_pci_map_regblock(cxlm, bar, offset);
+		base = cxl_pci_map_regblock(pdev, map);
 		if (!base)
 			return -ENOMEM;
 
-		map = &maps[n_maps];
-		map->barno = bar;
-		map->block_offset = offset;
-		map->reg_type = reg_type;
-
-		ret = cxl_probe_regs(cxlm, base + offset, map);
-
-		/* Always unmap the regblock regardless of probe success */
-		cxl_pci_unmap_regblock(cxlm, base);
-
+		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
+		cxl_pci_unmap_regblock(pdev, base);
 		if (ret)
 			return ret;
 


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

* [PATCH v5 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-09 16:44 ` [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map Dan Williams
  2021-10-10  4:20   ` Ira Weiny
@ 2021-10-13 23:57   ` Dan Williams
  2021-10-15 21:57     ` [PATCH v6 " Dan Williams
  2021-10-15 16:27   ` [PATCH v3 " Jonathan Cameron
  2 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-13 23:57 UTC (permalink / raw)
  To: linux-cxl; +Cc: Ira Weiny, linux-pci, linux-kernel

In addition to carrying @barno, @block_offset, and @reg_type, add @base
to keep all map/unmap parameters in one object. The helpers
cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
at map and unmap time.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v3:
- Collect Ira's reviewed-by, thanks!
- rebase on the %pa fixups in [PATCH v5 05/10] in this series

 drivers/cxl/cxl.h |    1 +
 drivers/cxl/pci.c |   31 ++++++++++++++++---------------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index a6687e7fd598..7cd16ef144dd 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -140,6 +140,7 @@ struct cxl_device_reg_map {
 };
 
 struct cxl_register_map {
+	void __iomem *base;
 	u64 block_offset;
 	u8 reg_type;
 	u8 barno;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index eb0c2f1b9e65..7d5e5548b316 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
 	return 0;
 }
 
-static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
-					  struct cxl_register_map *map)
+static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
 {
 	void __iomem *addr;
 	int bar = map->barno;
@@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %pa)\n", bar,
 			&pdev->resource[bar], &offset);
-		return NULL;
+		return -ENXIO;
 	}
 
 	addr = pci_iomap(pdev, bar, 0);
 	if (!addr) {
 		dev_err(dev, "failed to map registers\n");
-		return addr;
+		return -ENOMEM;
 	}
 
 	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
 		bar, &offset);
 
-	return addr;
+	map->base = addr + map->block_offset;
+	return 0;
 }
 
-static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
+static void cxl_unmap_regblock(struct pci_dev *pdev,
+			       struct cxl_register_map *map)
 {
-	pci_iounmap(pdev, base);
+	pci_iounmap(pdev, map->base - map->block_offset);
+	map->base = NULL;
 }
 
 static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
@@ -361,12 +363,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
-static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
-			  struct cxl_register_map *map)
+static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
 	struct device *dev = &pdev->dev;
+	void __iomem *base = map->base;
 
 	switch (map->reg_type) {
 	case CXL_REGLOC_RBI_COMPONENT:
@@ -442,7 +444,6 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
  */
 static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 {
-	void __iomem *base;
 	u32 regloc_size, regblocks;
 	int regloc, i, n_maps, ret = 0;
 	struct device *dev = cxlm->dev;
@@ -475,12 +476,12 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		base = cxl_pci_map_regblock(pdev, map);
-		if (!base)
-			return -ENOMEM;
+		ret = cxl_map_regblock(pdev, map);
+		if (ret)
+			return ret;
 
-		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
-		cxl_pci_unmap_regblock(pdev, base);
+		ret = cxl_probe_regs(pdev, map);
+		cxl_unmap_regblock(pdev, map);
 		if (ret)
 			return ret;
 


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

* Re: [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-13 22:49     ` Dan Williams
@ 2021-10-14  0:12       ` Ben Widawsky
  2021-10-14  0:48         ` Dan Williams
  0 siblings, 1 reply; 43+ messages in thread
From: Ben Widawsky @ 2021-10-14  0:12 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-cxl, Linux PCI, Linux Kernel Mailing List, Christoph Hellwig

On 21-10-13 15:49:30, Dan Williams wrote:
> On Wed, Oct 13, 2021 at 3:45 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 21-10-09 09:44:34, Dan Williams wrote:
> > > From: Ben Widawsky <ben.widawsky@intel.com>
> > >
> > > In preparation for moving parts of register mapping to cxl_core, split
> > > cxl_pci_setup_regs() into a helper that finds register blocks,
> > > (cxl_find_regblock()), and a generic wrapper that probes the precise
> > > register sets within a block (cxl_setup_regs()).
> > >
> > > Move the actual mapping (cxl_map_regs()) of the only register-set that
> > > cxl_pci cares about (memory device registers) up a level from the former
> > > cxl_pci_setup_regs() into cxl_pci_probe().
> > >
> > > With this change the unused component registers are no longer mapped,
> > > but the helpers are primed to move into the core.
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > [djbw: rebase on the cxl_register_map refactor]
> > > [djbw: drop cxl_map_regs() for component registers]
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >
> > [snip]
> >
> > Did you mean to also drop the component register handling in cxl_probe_regs()
> > and cxl_map_regs()?
> 
> No, because that has a soon to be added user, right?

In the current codebase, the port driver gets the offset from cxl_core, not
through the pci driver. I know you wanted this to be passed from cxl_pci (and
indeed it was before). Currently however, the functionality is subsumed by
cxl_find_regblock and is used by cxl_pci (for device registers), cxl_acpi (to
get the CHBCR) and cxl_core (to get the component register block for switches).

I have no user in cxl_pci for the component registers, and as we discussed, we
have no good way to share them across modules.

We can ignore this for now though and discuss it on the list when I post. If
there is a better way to handle this, I'm open to it.

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

* Re: [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-14  0:12       ` Ben Widawsky
@ 2021-10-14  0:48         ` Dan Williams
  0 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-14  0:48 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Linux PCI, Linux Kernel Mailing List, Christoph Hellwig

On Wed, Oct 13, 2021 at 5:12 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-10-13 15:49:30, Dan Williams wrote:
> > On Wed, Oct 13, 2021 at 3:45 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > On 21-10-09 09:44:34, Dan Williams wrote:
> > > > From: Ben Widawsky <ben.widawsky@intel.com>
> > > >
> > > > In preparation for moving parts of register mapping to cxl_core, split
> > > > cxl_pci_setup_regs() into a helper that finds register blocks,
> > > > (cxl_find_regblock()), and a generic wrapper that probes the precise
> > > > register sets within a block (cxl_setup_regs()).
> > > >
> > > > Move the actual mapping (cxl_map_regs()) of the only register-set that
> > > > cxl_pci cares about (memory device registers) up a level from the former
> > > > cxl_pci_setup_regs() into cxl_pci_probe().
> > > >
> > > > With this change the unused component registers are no longer mapped,
> > > > but the helpers are primed to move into the core.
> > > >
> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > > [djbw: rebase on the cxl_register_map refactor]
> > > > [djbw: drop cxl_map_regs() for component registers]
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > >
> > > [snip]
> > >
> > > Did you mean to also drop the component register handling in cxl_probe_regs()
> > > and cxl_map_regs()?
> >
> > No, because that has a soon to be added user, right?
>
> In the current codebase, the port driver gets the offset from cxl_core, not
> through the pci driver. I know you wanted this to be passed from cxl_pci (and
> indeed it was before). Currently however, the functionality is subsumed by
> cxl_find_regblock and is used by cxl_pci (for device registers), cxl_acpi (to
> get the CHBCR) and cxl_core (to get the component register block for switches).
>
> I have no user in cxl_pci for the component registers, and as we discussed, we
> have no good way to share them across modules.

Are you saying that cxl_probe_regs() will not move to the core in your
upcoming series? I was expecting that cxl_find_regblock() and
cxl_probe_regs() go hand in hand.

>
> We can ignore this for now though and discuss it on the list when I post. If
> there is a better way to handle this, I'm open to it.

It's hard to have discussions about API uses without the patches, but
I'm ok to leave further cxl_probe_regs() refactoring to your series.

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

* Re: [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion
  2021-10-09 16:44 ` [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion Dan Williams
  2021-10-10  3:44   ` Ira Weiny
@ 2021-10-15 16:15   ` Jonathan Cameron
  2021-10-15 20:16     ` Dan Williams
  2021-10-15 21:29   ` [PATCH v6 " Dan Williams
  2 siblings, 1 reply; 43+ messages in thread
From: Jonathan Cameron @ 2021-10-15 16:15 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, stable, Ira Weiny, linux-pci, linux-kernel, hch

On Sat, 9 Oct 2021 09:44:13 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> cxl_pci_map_regblock() may return an ERR_PTR(), but cxl_pci_setup_regs()
> is only prepared for NULL as the error case.
> 

What's the logic behind doing this rather than adjusting the call site to
check for an error pointer?

Either approach is fine as far as I'm concerned though so this is really
just a request for a bit more info in this patch description.

FWIW

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> Fixes: f8a7e8c29be8 ("cxl/pci: Reserve all device regions at once")
> Cc: <stable@vger.kernel.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index ccc7c2573ddc..9c178002d49e 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -317,7 +317,7 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
>  			&pdev->resource[bar], (unsigned long long)offset);
> -		return IOMEM_ERR_PTR(-ENXIO);
> +		return NULL;
>  	}
>  
>  	addr = pci_iomap(pdev, bar, 0);
> 


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

* Re: [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-09 16:44 ` [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map Dan Williams
  2021-10-10  4:20   ` Ira Weiny
  2021-10-13 23:57   ` [PATCH v5 " Dan Williams
@ 2021-10-15 16:27   ` Jonathan Cameron
  2021-10-15 16:55     ` Dan Williams
  2 siblings, 1 reply; 43+ messages in thread
From: Jonathan Cameron @ 2021-10-15 16:27 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, linux-pci, linux-kernel, hch

On Sat, 9 Oct 2021 09:44:29 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In addition to carrying @barno, @block_offset, and @reg_type, add @base
> to keep all map/unmap parameters in one object. The helpers
> cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> at map and unmap time.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I don't really mind them, but why the renames
from cxl_pci_* to cxl_* ?

Jonathan
> ---
>  drivers/cxl/cxl.h |    1 +
>  drivers/cxl/pci.c |   31 ++++++++++++++++---------------
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a6687e7fd598..7cd16ef144dd 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -140,6 +140,7 @@ struct cxl_device_reg_map {
>  };
>  
>  struct cxl_register_map {
> +	void __iomem *base;
>  	u64 block_offset;
>  	u8 reg_type;
>  	u8 barno;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 9f006299a0e3..b42407d067ac 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
>  	return 0;
>  }
>  
> -static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> -					  struct cxl_register_map *map)
> +static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
>  	void __iomem *addr;
>  	int bar = map->barno;
> @@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
>  			&pdev->resource[bar], (unsigned long long)offset);
> -		return NULL;
> +		return -ENXIO;
>  	}
>  
>  	addr = pci_iomap(pdev, bar, 0);
>  	if (!addr) {
>  		dev_err(dev, "failed to map registers\n");
> -		return addr;
> +		return -ENOMEM;
>  	}
>  
>  	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
>  		bar, offset);
>  
> -	return addr;
> +	map->base = addr + map->block_offset;
> +	return 0;
>  }
>  
> -static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
> +static void cxl_unmap_regblock(struct pci_dev *pdev,
> +			       struct cxl_register_map *map)
>  {
> -	pci_iounmap(pdev, base);
> +	pci_iounmap(pdev, map->base - map->block_offset);
> +	map->base = NULL;
>  }
>  
>  static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
> @@ -361,12 +363,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> -static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
> -			  struct cxl_register_map *map)
> +static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
>  	struct cxl_component_reg_map *comp_map;
>  	struct cxl_device_reg_map *dev_map;
>  	struct device *dev = &pdev->dev;
> +	void __iomem *base = map->base;
>  
>  	switch (map->reg_type) {
>  	case CXL_REGLOC_RBI_COMPONENT:
> @@ -442,7 +444,6 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
>   */
>  static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>  {
> -	void __iomem *base;
>  	u32 regloc_size, regblocks;
>  	int regloc, i, n_maps, ret = 0;
>  	struct device *dev = cxlm->dev;
> @@ -475,12 +476,12 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>  		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
>  			continue;
>  
> -		base = cxl_pci_map_regblock(pdev, map);
> -		if (!base)
> -			return -ENOMEM;
> +		ret = cxl_map_regblock(pdev, map);
> +		if (ret)
> +			return ret;
>  
> -		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
> -		cxl_pci_unmap_regblock(pdev, base);
> +		ret = cxl_probe_regs(pdev, map);
> +		cxl_unmap_regblock(pdev, map);
>  		if (ret)
>  			return ret;
>  
> 


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

* Re: [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-13 22:53     ` Dan Williams
@ 2021-10-15 16:29       ` Jonathan Cameron
  2021-10-15 16:56         ` Dan Williams
  0 siblings, 1 reply; 43+ messages in thread
From: Jonathan Cameron @ 2021-10-15 16:29 UTC (permalink / raw)
  To: Dan Williams
  Cc: Ira Weiny, linux-cxl, Linux PCI, Linux Kernel Mailing List,
	Christoph Hellwig

On Wed, 13 Oct 2021 15:53:20 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Sat, Oct 9, 2021 at 9:21 PM Ira Weiny <ira.weiny@intel.com> wrote:
> >
> > On Sat, Oct 09, 2021 at 09:44:29AM -0700, Dan Williams wrote:  
> > > In addition to carrying @barno, @block_offset, and @reg_type, add @base
> > > to keep all map/unmap parameters in one object. The helpers
> > > cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> > > at map and unmap time.
> > >
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  drivers/cxl/cxl.h |    1 +
> > >  drivers/cxl/pci.c |   31 ++++++++++++++++---------------
> > >  2 files changed, 17 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > index a6687e7fd598..7cd16ef144dd 100644
> > > --- a/drivers/cxl/cxl.h
> > > +++ b/drivers/cxl/cxl.h
> > > @@ -140,6 +140,7 @@ struct cxl_device_reg_map {
> > >  };
> > >
> > >  struct cxl_register_map {
> > > +     void __iomem *base;
> > >       u64 block_offset;
> > >       u8 reg_type;
> > >       u8 barno;
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index 9f006299a0e3..b42407d067ac 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> > > @@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
> > >       return 0;
> > >  }
> > >
> > > -static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> > > -                                       struct cxl_register_map *map)
> > > +static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
> > >  {
> > >       void __iomem *addr;
> > >       int bar = map->barno;
> > > @@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> > >       if (pci_resource_len(pdev, bar) < offset) {
> > >               dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> > >                       &pdev->resource[bar], (unsigned long long)offset);
> > > -             return NULL;
> > > +             return -ENXIO;
> > >       }
> > >
> > >       addr = pci_iomap(pdev, bar, 0);
> > >       if (!addr) {
> > >               dev_err(dev, "failed to map registers\n");
> > > -             return addr;
> > > +             return -ENOMEM;
> > >       }
> > >
> > >       dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
> > >               bar, offset);
> > >
> > > -     return addr;
> > > +     map->base = addr + map->block_offset;
> > > +     return 0;
> > >  }
> > >
> > > -static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
> > > +static void cxl_unmap_regblock(struct pci_dev *pdev,
> > > +                            struct cxl_register_map *map)
> > >  {
> > > -     pci_iounmap(pdev, base);
> > > +     pci_iounmap(pdev, map->base - map->block_offset);  
> >
> > I know we need to get these in soon.  But I think map->base should be 'base'
> > and map->block_offset should be handled in cxl_probe_regs() rather than
> > subtract it here..  
> 
> But why? The goal of the cxl_register_map cleanups is to reduce the
> open-coding for details that can just be passed around in a @map
> instance. Once cxl_map_regblock() sets up @base there's little reason
> to consider the hardware regblock details.

I agree with Ira to the extent that this was a little confusing.   Perhaps it is worth
a comment at the structure definition to make the relationship of block_offset
and base clear?

Jonathan

> 
> > Either way this is cleaner than what it was.
> >
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>  
> 
> Thanks!


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

* Re: [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-09 16:44 ` [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs() Dan Williams
  2021-10-10  4:44   ` Ira Weiny
  2021-10-13 22:45   ` Ben Widawsky
@ 2021-10-15 16:44   ` Jonathan Cameron
  2021-10-15 17:00     ` Dan Williams
  2021-10-15 23:30   ` [PATCH v6 " Dan Williams
  3 siblings, 1 reply; 43+ messages in thread
From: Jonathan Cameron @ 2021-10-15 16:44 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, Ben Widawsky, linux-pci, linux-kernel, hch

On Sat, 9 Oct 2021 09:44:34 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> In preparation for moving parts of register mapping to cxl_core, split

Ah. Guess this planned move is why the naming change in the earlier patch.
Fair enough, but perhaps call it out there as well as here.

No comments to add to this one.

> cxl_pci_setup_regs() into a helper that finds register blocks,
> (cxl_find_regblock()), and a generic wrapper that probes the precise
> register sets within a block (cxl_setup_regs()).
> 
> Move the actual mapping (cxl_map_regs()) of the only register-set that
> cxl_pci cares about (memory device registers) up a level from the former
> cxl_pci_setup_regs() into cxl_pci_probe().
> 
> With this change the unused component registers are no longer mapped,
> but the helpers are primed to move into the core.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> [djbw: rebase on the cxl_register_map refactor]
> [djbw: drop cxl_map_regs() for component registers]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c |   73 +++++++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b42407d067ac..b6bc8e5ca028 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -433,72 +433,69 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
>  }
>  
>  /**
> - * cxl_pci_setup_regs() - Setup necessary MMIO.
> - * @cxlm: The CXL memory device to communicate with.
> + * cxl_find_regblock() - Locate register blocks by type
> + * @pdev: The CXL PCI device to enumerate.
> + * @type: Register Block Indicator id
> + * @map: Enumeration output, clobbered on error
>   *
> - * Return: 0 if all necessary registers mapped.
> + * Return: 0 if register block enumerated, negative error code otherwise
>   *
> - * 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.
> + * A CXL DVSEC may additional point one or more register blocks, search

may point to one or more...
(perhaps - I'm not quite sure of the intended meaning)

> + * for them by @type.
>   */
> -static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
> +static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> +			     struct cxl_register_map *map)
>  {
>  	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];
> +	int regloc, i;
>  
>  	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;
> -	}
>  
> -	/* 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;
>  
>  		pci_read_config_dword(pdev, regloc, &reg_lo);
>  		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
>  
> -		map = &maps[n_maps];
>  		cxl_decode_regblock(reg_lo, reg_hi, map);
>  
> -		/* Ignore unknown register block types */
> -		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
> -			continue;
> +		if (map->reg_type == type)
> +			return 0;
> +	}
>  
> -		ret = cxl_map_regblock(pdev, map);
> -		if (ret)
> -			return ret;
> +	return -ENODEV;
> +}
>  
> -		ret = cxl_probe_regs(pdev, map);
> -		cxl_unmap_regblock(pdev, map);
> -		if (ret)
> -			return ret;
> +static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> +			  struct cxl_register_map *map)
> +{
> +	int rc;
>  
> -		n_maps++;
> -	}
> +	rc = cxl_find_regblock(pdev, type, map);
> +	if (rc)
> +		return rc;
>  
> -	for (i = 0; i < n_maps; i++) {
> -		ret = cxl_map_regs(cxlm, &maps[i]);
> -		if (ret)
> -			break;
> -	}
> +	rc = cxl_map_regblock(pdev, map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_probe_regs(pdev, map);
> +	cxl_unmap_regblock(pdev, map);
>  
> -	return ret;
> +	return rc;
>  }
>  
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> +	struct cxl_register_map map;
>  	struct cxl_memdev *cxlmd;
>  	struct cxl_mem *cxlm;
>  	int rc;
> @@ -518,7 +515,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(cxlm))
>  		return PTR_ERR(cxlm);
>  
> -	rc = cxl_pci_setup_regs(cxlm);
> +	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_map_regs(cxlm, &map);
>  	if (rc)
>  		return rc;
>  
> 


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

* Re: [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-15 16:27   ` [PATCH v3 " Jonathan Cameron
@ 2021-10-15 16:55     ` Dan Williams
  2021-10-18  9:30       ` Jonathan Cameron
  0 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-15 16:55 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-cxl, Linux PCI, Linux Kernel Mailing List, Christoph Hellwig

On Fri, Oct 15, 2021 at 9:27 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sat, 9 Oct 2021 09:44:29 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > In addition to carrying @barno, @block_offset, and @reg_type, add @base
> > to keep all map/unmap parameters in one object. The helpers
> > cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> > at map and unmap time.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> I don't really mind them, but why the renames
> from cxl_pci_* to cxl_* ?

Primarily because we had a mix of some functions including the _pci
and some not, and I steered towards just dropping it. I think the
"PCI" aspect of the function is clear by its function signature, and
that was being muddied by passing @cxlm unnecessarily. So instead of:

cxl_pci_$foo(struct cxl_mem *cxlm...)

...I went with:

cxl_$foo(struct pci_dev *pdev...)

...concerns?

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

* Re: [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-15 16:29       ` Jonathan Cameron
@ 2021-10-15 16:56         ` Dan Williams
  0 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-15 16:56 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Ira Weiny, linux-cxl, Linux PCI, Linux Kernel Mailing List,
	Christoph Hellwig

On Fri, Oct 15, 2021 at 9:29 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 13 Oct 2021 15:53:20 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > On Sat, Oct 9, 2021 at 9:21 PM Ira Weiny <ira.weiny@intel.com> wrote:
> > >
> > > On Sat, Oct 09, 2021 at 09:44:29AM -0700, Dan Williams wrote:
> > > > In addition to carrying @barno, @block_offset, and @reg_type, add @base
> > > > to keep all map/unmap parameters in one object. The helpers
> > > > cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> > > > at map and unmap time.
> > > >
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > ---
> > > >  drivers/cxl/cxl.h |    1 +
> > > >  drivers/cxl/pci.c |   31 ++++++++++++++++---------------
> > > >  2 files changed, 17 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > > index a6687e7fd598..7cd16ef144dd 100644
> > > > --- a/drivers/cxl/cxl.h
> > > > +++ b/drivers/cxl/cxl.h
> > > > @@ -140,6 +140,7 @@ struct cxl_device_reg_map {
> > > >  };
> > > >
> > > >  struct cxl_register_map {
> > > > +     void __iomem *base;
> > > >       u64 block_offset;
> > > >       u8 reg_type;
> > > >       u8 barno;
> > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > > index 9f006299a0e3..b42407d067ac 100644
> > > > --- a/drivers/cxl/pci.c
> > > > +++ b/drivers/cxl/pci.c
> > > > @@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
> > > >       return 0;
> > > >  }
> > > >
> > > > -static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> > > > -                                       struct cxl_register_map *map)
> > > > +static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
> > > >  {
> > > >       void __iomem *addr;
> > > >       int bar = map->barno;
> > > > @@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> > > >       if (pci_resource_len(pdev, bar) < offset) {
> > > >               dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> > > >                       &pdev->resource[bar], (unsigned long long)offset);
> > > > -             return NULL;
> > > > +             return -ENXIO;
> > > >       }
> > > >
> > > >       addr = pci_iomap(pdev, bar, 0);
> > > >       if (!addr) {
> > > >               dev_err(dev, "failed to map registers\n");
> > > > -             return addr;
> > > > +             return -ENOMEM;
> > > >       }
> > > >
> > > >       dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
> > > >               bar, offset);
> > > >
> > > > -     return addr;
> > > > +     map->base = addr + map->block_offset;
> > > > +     return 0;
> > > >  }
> > > >
> > > > -static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
> > > > +static void cxl_unmap_regblock(struct pci_dev *pdev,
> > > > +                            struct cxl_register_map *map)
> > > >  {
> > > > -     pci_iounmap(pdev, base);
> > > > +     pci_iounmap(pdev, map->base - map->block_offset);
> > >
> > > I know we need to get these in soon.  But I think map->base should be 'base'
> > > and map->block_offset should be handled in cxl_probe_regs() rather than
> > > subtract it here..
> >
> > But why? The goal of the cxl_register_map cleanups is to reduce the
> > open-coding for details that can just be passed around in a @map
> > instance. Once cxl_map_regblock() sets up @base there's little reason
> > to consider the hardware regblock details.
>
> I agree with Ira to the extent that this was a little confusing.   Perhaps it is worth
> a comment at the structure definition to make the relationship of block_offset
> and base clear?
>

I can add that, sure.

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

* Re: [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-15 16:44   ` Jonathan Cameron
@ 2021-10-15 17:00     ` Dan Williams
  0 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-15 17:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-cxl, Ben Widawsky, Linux PCI, Linux Kernel Mailing List,
	Christoph Hellwig

On Fri, Oct 15, 2021 at 9:44 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sat, 9 Oct 2021 09:44:34 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > From: Ben Widawsky <ben.widawsky@intel.com>
> >
> > In preparation for moving parts of register mapping to cxl_core, split
>
> Ah. Guess this planned move is why the naming change in the earlier patch.
> Fair enough, but perhaps call it out there as well as here.
>
> No comments to add to this one.
>
> > cxl_pci_setup_regs() into a helper that finds register blocks,
> > (cxl_find_regblock()), and a generic wrapper that probes the precise
> > register sets within a block (cxl_setup_regs()).
> >
> > Move the actual mapping (cxl_map_regs()) of the only register-set that
> > cxl_pci cares about (memory device registers) up a level from the former
> > cxl_pci_setup_regs() into cxl_pci_probe().
> >
> > With this change the unused component registers are no longer mapped,
> > but the helpers are primed to move into the core.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > [djbw: rebase on the cxl_register_map refactor]
> > [djbw: drop cxl_map_regs() for component registers]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > ---
> >  drivers/cxl/pci.c |   73 +++++++++++++++++++++++++++--------------------------
> >  1 file changed, 37 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index b42407d067ac..b6bc8e5ca028 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -433,72 +433,69 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
> >  }
> >
> >  /**
> > - * cxl_pci_setup_regs() - Setup necessary MMIO.
> > - * @cxlm: The CXL memory device to communicate with.
> > + * cxl_find_regblock() - Locate register blocks by type
> > + * @pdev: The CXL PCI device to enumerate.
> > + * @type: Register Block Indicator id
> > + * @map: Enumeration output, clobbered on error
> >   *
> > - * Return: 0 if all necessary registers mapped.
> > + * Return: 0 if register block enumerated, negative error code otherwise
> >   *
> > - * 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.
> > + * A CXL DVSEC may additional point one or more register blocks, search
>
> may point to one or more...
> (perhaps - I'm not quite sure of the intended meaning)

Yeah, that looks like it should be:

s/may additional point one/may point to one/

I'll clean that up.

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

* Re: [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion
  2021-10-15 16:15   ` Jonathan Cameron
@ 2021-10-15 20:16     ` Dan Williams
  0 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-15 20:16 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-cxl, stable, Ira Weiny, Linux PCI,
	Linux Kernel Mailing List, Christoph Hellwig

On Fri, Oct 15, 2021 at 9:16 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sat, 9 Oct 2021 09:44:13 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > cxl_pci_map_regblock() may return an ERR_PTR(), but cxl_pci_setup_regs()
> > is only prepared for NULL as the error case.
> >
>
> What's the logic behind doing this rather than adjusting the call site to
> check for an error pointer?

Minimize the fix for the stable backport. In the later patches the
cxl_pci_map_regblock() => cxl_map_regblock() conversion goes from
returning a pointer to an error code.

> Either approach is fine as far as I'm concerned though so this is really
> just a request for a bit more info in this patch description.

I can include that note above to clarify.

>
> FWIW
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks.

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

* [PATCH v6 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion
  2021-10-09 16:44 ` [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion Dan Williams
  2021-10-10  3:44   ` Ira Weiny
  2021-10-15 16:15   ` Jonathan Cameron
@ 2021-10-15 21:29   ` Dan Williams
  2 siblings, 0 replies; 43+ messages in thread
From: Dan Williams @ 2021-10-15 21:29 UTC (permalink / raw)
  To: linux-cxl; +Cc: stable, Ira Weiny, Jonathan Cameron, linux-pci, linux-kernel

cxl_pci_map_regblock() may return an ERR_PTR(), but cxl_pci_setup_regs()
is only prepared for NULL as the error case. Pick the minimal fix for
-stable backport purposes and just have cxl_pci_map_regblock() return
NULL for errors.

Fixes: f8a7e8c29be8 ("cxl/pci: Reserve all device regions at once")
Cc: <stable@vger.kernel.org>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v3:
- clarify in the changelog why cxl_pci_map_regblock() was changed to
  return NULL rather than fix the caller to expect an ERR_PTR().
  (Jonathan)

 drivers/cxl/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index ccc7c2573ddc..9c178002d49e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -317,7 +317,7 @@ static void __iomem *cxl_pci_map_regblock(struct cxl_mem *cxlm,
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
 			&pdev->resource[bar], (unsigned long long)offset);
-		return IOMEM_ERR_PTR(-ENXIO);
+		return NULL;
 	}
 
 	addr = pci_iomap(pdev, bar, 0);


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

* [PATCH v6 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-13 23:57   ` [PATCH v5 " Dan Williams
@ 2021-10-15 21:57     ` Dan Williams
  2021-10-18  9:30       ` Jonathan Cameron
  0 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-15 21:57 UTC (permalink / raw)
  To: linux-cxl; +Cc: Jonathan Cameron, Ira Weiny, linux-pci, linux-kernel

In addition to carrying @barno, @block_offset, and @reg_type, add @base
to keep all map/unmap parameters in one object. The helpers
cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
at map and unmap time.

Document that @base incorporates @block_offset so that downstream
consumers of a mapped cxl_register_map instance do not need perform any
fixups / can use @base directly.

Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v5:
- add kernel-doc for cxl_register_map to explain the interaction between
  @base and @block_offset. (Ira and Jonathan)

 drivers/cxl/cxl.h |   10 ++++++++++
 drivers/cxl/pci.c |   31 ++++++++++++++++---------------
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index a6687e7fd598..5e2e93451928 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -139,7 +139,17 @@ struct cxl_device_reg_map {
 	struct cxl_reg_map memdev;
 };
 
+/**
+ * struct cxl_register_map - DVSEC harvested register block mapping parameters
+ * @base: virtual base of the register-block-BAR + @block_offset
+ * @block_offset: offset to start of register block in @barno
+ * @reg_type: see enum cxl_regloc_type
+ * @barno: PCI BAR number containing the register block
+ * @component_map: cxl_reg_map for component registers
+ * @device_map: cxl_reg_maps for device registers
+ */
 struct cxl_register_map {
+	void __iomem *base;
 	u64 block_offset;
 	u8 reg_type;
 	u8 barno;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index eb0c2f1b9e65..7d5e5548b316 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
 	return 0;
 }
 
-static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
-					  struct cxl_register_map *map)
+static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
 {
 	void __iomem *addr;
 	int bar = map->barno;
@@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %pa)\n", bar,
 			&pdev->resource[bar], &offset);
-		return NULL;
+		return -ENXIO;
 	}
 
 	addr = pci_iomap(pdev, bar, 0);
 	if (!addr) {
 		dev_err(dev, "failed to map registers\n");
-		return addr;
+		return -ENOMEM;
 	}
 
 	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
 		bar, &offset);
 
-	return addr;
+	map->base = addr + map->block_offset;
+	return 0;
 }
 
-static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
+static void cxl_unmap_regblock(struct pci_dev *pdev,
+			       struct cxl_register_map *map)
 {
-	pci_iounmap(pdev, base);
+	pci_iounmap(pdev, map->base - map->block_offset);
+	map->base = NULL;
 }
 
 static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
@@ -361,12 +363,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
-static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
-			  struct cxl_register_map *map)
+static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
 	struct device *dev = &pdev->dev;
+	void __iomem *base = map->base;
 
 	switch (map->reg_type) {
 	case CXL_REGLOC_RBI_COMPONENT:
@@ -442,7 +444,6 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
  */
 static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 {
-	void __iomem *base;
 	u32 regloc_size, regblocks;
 	int regloc, i, n_maps, ret = 0;
 	struct device *dev = cxlm->dev;
@@ -475,12 +476,12 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
 		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		base = cxl_pci_map_regblock(pdev, map);
-		if (!base)
-			return -ENOMEM;
+		ret = cxl_map_regblock(pdev, map);
+		if (ret)
+			return ret;
 
-		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
-		cxl_pci_unmap_regblock(pdev, base);
+		ret = cxl_probe_regs(pdev, map);
+		cxl_unmap_regblock(pdev, map);
 		if (ret)
 			return ret;
 


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

* [PATCH v6 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-09 16:44 ` [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs() Dan Williams
                     ` (2 preceding siblings ...)
  2021-10-15 16:44   ` Jonathan Cameron
@ 2021-10-15 23:30   ` Dan Williams
  2021-11-10 17:14     ` Jonathan Cameron
  3 siblings, 1 reply; 43+ messages in thread
From: Dan Williams @ 2021-10-15 23:30 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Ira Weiny, Jonathan Cameron, linux-pci, linux-kernel

From: Ben Widawsky <ben.widawsky@intel.com>

In preparation for moving parts of register mapping to cxl_core, split
cxl_pci_setup_regs() into a helper that finds register blocks,
(cxl_find_regblock()), and a generic wrapper that probes the precise
register sets within a block (cxl_setup_regs()).

Move the actual mapping (cxl_map_regs()) of the only register-set that
cxl_pci cares about (memory device registers) up a level from the former
cxl_pci_setup_regs() into cxl_pci_probe().

With this change the unused component registers are no longer mapped,
but the helpers are primed to move into the core.

[djbw: drop cxl_map_regs() for component registers]

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
[djbw: rebase on the cxl_register_map refactor]
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v3:
- fixup grammar in kernel-doc for cxl_find_regblock() (Jonathan)

 drivers/cxl/pci.c |   73 +++++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 7d5e5548b316..691a4e59ad8b 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -433,72 +433,69 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
 }
 
 /**
- * cxl_pci_setup_regs() - Setup necessary MMIO.
- * @cxlm: The CXL memory device to communicate with.
+ * cxl_find_regblock() - Locate register blocks by type
+ * @pdev: The CXL PCI device to enumerate.
+ * @type: Register Block Indicator id
+ * @map: Enumeration output, clobbered on error
  *
- * Return: 0 if all necessary registers mapped.
+ * Return: 0 if register block enumerated, negative error code otherwise
  *
- * 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.
+ * A CXL DVSEC may point to one or more register blocks, search for them
+ * by @type.
  */
-static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
+static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
+			     struct cxl_register_map *map)
 {
 	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];
+	int regloc, i;
 
 	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;
-	}
 
-	/* 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;
 
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
-		map = &maps[n_maps];
 		cxl_decode_regblock(reg_lo, reg_hi, map);
 
-		/* Ignore unknown register block types */
-		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
-			continue;
+		if (map->reg_type == type)
+			return 0;
+	}
 
-		ret = cxl_map_regblock(pdev, map);
-		if (ret)
-			return ret;
+	return -ENODEV;
+}
 
-		ret = cxl_probe_regs(pdev, map);
-		cxl_unmap_regblock(pdev, map);
-		if (ret)
-			return ret;
+static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
+			  struct cxl_register_map *map)
+{
+	int rc;
 
-		n_maps++;
-	}
+	rc = cxl_find_regblock(pdev, type, map);
+	if (rc)
+		return rc;
 
-	for (i = 0; i < n_maps; i++) {
-		ret = cxl_map_regs(cxlm, &maps[i]);
-		if (ret)
-			break;
-	}
+	rc = cxl_map_regblock(pdev, map);
+	if (rc)
+		return rc;
+
+	rc = cxl_probe_regs(pdev, map);
+	cxl_unmap_regblock(pdev, map);
 
-	return ret;
+	return rc;
 }
 
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	struct cxl_register_map map;
 	struct cxl_memdev *cxlmd;
 	struct cxl_mem *cxlm;
 	int rc;
@@ -518,7 +515,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlm))
 		return PTR_ERR(cxlm);
 
-	rc = cxl_pci_setup_regs(cxlm);
+	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
+	if (rc)
+		return rc;
+
+	rc = cxl_map_regs(cxlm, &map);
 	if (rc)
 		return rc;
 


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

* Re: [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-15 16:55     ` Dan Williams
@ 2021-10-18  9:30       ` Jonathan Cameron
  0 siblings, 0 replies; 43+ messages in thread
From: Jonathan Cameron @ 2021-10-18  9:30 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-cxl, Linux PCI, Linux Kernel Mailing List, Christoph Hellwig

On Fri, 15 Oct 2021 09:55:57 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Fri, Oct 15, 2021 at 9:27 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Sat, 9 Oct 2021 09:44:29 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > In addition to carrying @barno, @block_offset, and @reg_type, add @base
> > > to keep all map/unmap parameters in one object. The helpers
> > > cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> > > at map and unmap time.
> > >
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> >
> > I don't really mind them, but why the renames
> > from cxl_pci_* to cxl_* ?  
> 
> Primarily because we had a mix of some functions including the _pci
> and some not, and I steered towards just dropping it. I think the
> "PCI" aspect of the function is clear by its function signature, and
> that was being muddied by passing @cxlm unnecessarily. So instead of:
> 
> cxl_pci_$foo(struct cxl_mem *cxlm...)
> 
> ...I went with:
> 
> cxl_$foo(struct pci_dev *pdev...)
> 
> ...concerns?

That's fine,

J


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

* Re: [PATCH v6 06/10] cxl/pci: Add @base to cxl_register_map
  2021-10-15 21:57     ` [PATCH v6 " Dan Williams
@ 2021-10-18  9:30       ` Jonathan Cameron
  0 siblings, 0 replies; 43+ messages in thread
From: Jonathan Cameron @ 2021-10-18  9:30 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, Ira Weiny, linux-pci, linux-kernel

On Fri, 15 Oct 2021 14:57:27 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In addition to carrying @barno, @block_offset, and @reg_type, add @base
> to keep all map/unmap parameters in one object. The helpers
> cxl_{map,unmap}_regblock() handle adjusting @base to the @block_offset
> at map and unmap time.
> 
> Document that @base incorporates @block_offset so that downstream
> consumers of a mapped cxl_register_map instance do not need perform any
> fixups / can use @base directly.
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Changes since v5:
> - add kernel-doc for cxl_register_map to explain the interaction between
>   @base and @block_offset. (Ira and Jonathan)
> 
>  drivers/cxl/cxl.h |   10 ++++++++++
>  drivers/cxl/pci.c |   31 ++++++++++++++++---------------
>  2 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a6687e7fd598..5e2e93451928 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -139,7 +139,17 @@ struct cxl_device_reg_map {
>  	struct cxl_reg_map memdev;
>  };
>  
> +/**
> + * struct cxl_register_map - DVSEC harvested register block mapping parameters
> + * @base: virtual base of the register-block-BAR + @block_offset
> + * @block_offset: offset to start of register block in @barno
> + * @reg_type: see enum cxl_regloc_type
> + * @barno: PCI BAR number containing the register block
> + * @component_map: cxl_reg_map for component registers
> + * @device_map: cxl_reg_maps for device registers
> + */
>  struct cxl_register_map {
> +	void __iomem *base;
>  	u64 block_offset;
>  	u8 reg_type;
>  	u8 barno;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index eb0c2f1b9e65..7d5e5548b316 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -306,8 +306,7 @@ static int cxl_pci_setup_mailbox(struct cxl_mem *cxlm)
>  	return 0;
>  }
>  
> -static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
> -					  struct cxl_register_map *map)
> +static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
>  	void __iomem *addr;
>  	int bar = map->barno;
> @@ -318,24 +317,27 @@ static void __iomem *cxl_pci_map_regblock(struct pci_dev *pdev,
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %pa)\n", bar,
>  			&pdev->resource[bar], &offset);
> -		return NULL;
> +		return -ENXIO;
>  	}
>  
>  	addr = pci_iomap(pdev, bar, 0);
>  	if (!addr) {
>  		dev_err(dev, "failed to map registers\n");
> -		return addr;
> +		return -ENOMEM;
>  	}
>  
>  	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
>  		bar, &offset);
>  
> -	return addr;
> +	map->base = addr + map->block_offset;
> +	return 0;
>  }
>  
> -static void cxl_pci_unmap_regblock(struct pci_dev *pdev, void __iomem *base)
> +static void cxl_unmap_regblock(struct pci_dev *pdev,
> +			       struct cxl_register_map *map)
>  {
> -	pci_iounmap(pdev, base);
> +	pci_iounmap(pdev, map->base - map->block_offset);
> +	map->base = NULL;
>  }
>  
>  static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
> @@ -361,12 +363,12 @@ static int cxl_pci_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> -static int cxl_probe_regs(struct pci_dev *pdev, void __iomem *base,
> -			  struct cxl_register_map *map)
> +static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
>  	struct cxl_component_reg_map *comp_map;
>  	struct cxl_device_reg_map *dev_map;
>  	struct device *dev = &pdev->dev;
> +	void __iomem *base = map->base;
>  
>  	switch (map->reg_type) {
>  	case CXL_REGLOC_RBI_COMPONENT:
> @@ -442,7 +444,6 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
>   */
>  static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>  {
> -	void __iomem *base;
>  	u32 regloc_size, regblocks;
>  	int regloc, i, n_maps, ret = 0;
>  	struct device *dev = cxlm->dev;
> @@ -475,12 +476,12 @@ static int cxl_pci_setup_regs(struct cxl_mem *cxlm)
>  		if (map->reg_type > CXL_REGLOC_RBI_MEMDEV)
>  			continue;
>  
> -		base = cxl_pci_map_regblock(pdev, map);
> -		if (!base)
> -			return -ENOMEM;
> +		ret = cxl_map_regblock(pdev, map);
> +		if (ret)
> +			return ret;
>  
> -		ret = cxl_probe_regs(pdev, base + map->block_offset, map);
> -		cxl_pci_unmap_regblock(pdev, base);
> +		ret = cxl_probe_regs(pdev, map);
> +		cxl_unmap_regblock(pdev, map);
>  		if (ret)
>  			return ret;
>  
> 


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

* Re: [PATCH v6 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-10-15 23:30   ` [PATCH v6 " Dan Williams
@ 2021-11-10 17:14     ` Jonathan Cameron
  2021-11-10 17:30       ` Ben Widawsky
  0 siblings, 1 reply; 43+ messages in thread
From: Jonathan Cameron @ 2021-11-10 17:14 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-cxl, Ben Widawsky, Ira Weiny, linux-pci, linux-kernel

On Fri, 15 Oct 2021 16:30:42 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> In preparation for moving parts of register mapping to cxl_core, split
> cxl_pci_setup_regs() into a helper that finds register blocks,
> (cxl_find_regblock()), and a generic wrapper that probes the precise
> register sets within a block (cxl_setup_regs()).
> 
> Move the actual mapping (cxl_map_regs()) of the only register-set that
> cxl_pci cares about (memory device registers) up a level from the former
> cxl_pci_setup_regs() into cxl_pci_probe().
> 
> With this change the unused component registers are no longer mapped,
> but the helpers are primed to move into the core.
> 
> [djbw: drop cxl_map_regs() for component registers]
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> [djbw: rebase on the cxl_register_map refactor]
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Hi Ben / all,

This is probably the best patch to comment on for this
(note it is not a comment about this patch, but more the state we end up
in after it).

cxl_map_regs() is a generic function, but with the new split approach
as a result of this patch, we now always know at the caller which of
the types of map we are doing.

I think it would be clearer to embrace that situation and drop cxl_map_regs()
in favor of directly calling the relevant specific versions such as
cxl_map_device_regs().  I can't immediately see how the generic cxl_map_regs()
will be useful to us going forwards.

Jonathan

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

* Re: [PATCH v6 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-11-10 17:14     ` Jonathan Cameron
@ 2021-11-10 17:30       ` Ben Widawsky
  2021-11-10 17:43         ` Jonathan Cameron
  0 siblings, 1 reply; 43+ messages in thread
From: Ben Widawsky @ 2021-11-10 17:30 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Dan Williams, linux-cxl, Ira Weiny, linux-pci, linux-kernel

On 21-11-10 17:14:37, Jonathan Cameron wrote:
> On Fri, 15 Oct 2021 16:30:42 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > From: Ben Widawsky <ben.widawsky@intel.com>
> > 
> > In preparation for moving parts of register mapping to cxl_core, split
> > cxl_pci_setup_regs() into a helper that finds register blocks,
> > (cxl_find_regblock()), and a generic wrapper that probes the precise
> > register sets within a block (cxl_setup_regs()).
> > 
> > Move the actual mapping (cxl_map_regs()) of the only register-set that
> > cxl_pci cares about (memory device registers) up a level from the former
> > cxl_pci_setup_regs() into cxl_pci_probe().
> > 
> > With this change the unused component registers are no longer mapped,
> > but the helpers are primed to move into the core.
> > 
> > [djbw: drop cxl_map_regs() for component registers]
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > [djbw: rebase on the cxl_register_map refactor]
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Hi Ben / all,
> 
> This is probably the best patch to comment on for this
> (note it is not a comment about this patch, but more the state we end up
> in after it).
> 
> cxl_map_regs() is a generic function, but with the new split approach
> as a result of this patch, we now always know at the caller which of
> the types of map we are doing.
> 
> I think it would be clearer to embrace that situation and drop cxl_map_regs()
> in favor of directly calling the relevant specific versions such as
> cxl_map_device_regs().  I can't immediately see how the generic cxl_map_regs()
> will be useful to us going forwards.
> 
> Jonathan

I completely agree. Long term, something like cxl_map_regs() might be desirable
for a Type2 device, but we have no such user today. Patches welcome?

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

* Re: [PATCH v6 07/10] cxl/pci: Split cxl_pci_setup_regs()
  2021-11-10 17:30       ` Ben Widawsky
@ 2021-11-10 17:43         ` Jonathan Cameron
  0 siblings, 0 replies; 43+ messages in thread
From: Jonathan Cameron @ 2021-11-10 17:43 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: Dan Williams, linux-cxl, Ira Weiny, linux-pci, linux-kernel

On Wed, 10 Nov 2021 09:30:40 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 21-11-10 17:14:37, Jonathan Cameron wrote:
> > On Fri, 15 Oct 2021 16:30:42 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > From: Ben Widawsky <ben.widawsky@intel.com>
> > > 
> > > In preparation for moving parts of register mapping to cxl_core, split
> > > cxl_pci_setup_regs() into a helper that finds register blocks,
> > > (cxl_find_regblock()), and a generic wrapper that probes the precise
> > > register sets within a block (cxl_setup_regs()).
> > > 
> > > Move the actual mapping (cxl_map_regs()) of the only register-set that
> > > cxl_pci cares about (memory device registers) up a level from the former
> > > cxl_pci_setup_regs() into cxl_pci_probe().
> > > 
> > > With this change the unused component registers are no longer mapped,
> > > but the helpers are primed to move into the core.
> > > 
> > > [djbw: drop cxl_map_regs() for component registers]
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > [djbw: rebase on the cxl_register_map refactor]
> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > 
> > Hi Ben / all,
> > 
> > This is probably the best patch to comment on for this
> > (note it is not a comment about this patch, but more the state we end up
> > in after it).
> > 
> > cxl_map_regs() is a generic function, but with the new split approach
> > as a result of this patch, we now always know at the caller which of
> > the types of map we are doing.
> > 
> > I think it would be clearer to embrace that situation and drop cxl_map_regs()
> > in favor of directly calling the relevant specific versions such as
> > cxl_map_device_regs().  I can't immediately see how the generic cxl_map_regs()
> > will be useful to us going forwards.
> > 
> > Jonathan  
> 
> I completely agree. Long term, something like cxl_map_regs() might be desirable
> for a Type2 device, but we have no such user today. Patches welcome?

Sure, will do.

J

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

end of thread, other threads:[~2021-11-10 17:44 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-09 16:43 [PATCH v3 00/10] cxl_pci refactor for reusability Dan Williams
2021-10-09 16:44 ` [PATCH v3 01/10] cxl/pci: Convert register block identifiers to an enum Dan Williams
2021-10-09 16:44 ` [PATCH v3 02/10] cxl/pci: Remove dev_dbg for unknown register blocks Dan Williams
2021-10-09 16:48   ` Joe Perches
2021-10-09 18:04     ` Ben Widawsky
2021-10-09 16:44 ` [PATCH v3 03/10] cxl/pci: Fix NULL vs ERR_PTR confusion Dan Williams
2021-10-10  3:44   ` Ira Weiny
2021-10-15 16:15   ` Jonathan Cameron
2021-10-15 20:16     ` Dan Williams
2021-10-15 21:29   ` [PATCH v6 " Dan Williams
2021-10-09 16:44 ` [PATCH v3 04/10] cxl/pci: Remove pci request/release regions Dan Williams
2021-10-09 16:44 ` [PATCH v3 05/10] cxl/pci: Make more use of cxl_register_map Dan Williams
2021-10-09 19:04   ` kernel test robot
2021-10-09 20:51   ` [PATCH v4 " Dan Williams
2021-10-10  4:03     ` Ira Weiny
2021-10-13 23:53     ` [PATCH v5 " Dan Williams
2021-10-09 16:44 ` [PATCH v3 06/10] cxl/pci: Add @base to cxl_register_map Dan Williams
2021-10-10  4:20   ` Ira Weiny
2021-10-13 22:53     ` Dan Williams
2021-10-15 16:29       ` Jonathan Cameron
2021-10-15 16:56         ` Dan Williams
2021-10-13 23:57   ` [PATCH v5 " Dan Williams
2021-10-15 21:57     ` [PATCH v6 " Dan Williams
2021-10-18  9:30       ` Jonathan Cameron
2021-10-15 16:27   ` [PATCH v3 " Jonathan Cameron
2021-10-15 16:55     ` Dan Williams
2021-10-18  9:30       ` Jonathan Cameron
2021-10-09 16:44 ` [PATCH v3 07/10] cxl/pci: Split cxl_pci_setup_regs() Dan Williams
2021-10-10  4:44   ` Ira Weiny
2021-10-13 22:45   ` Ben Widawsky
2021-10-13 22:49     ` Dan Williams
2021-10-14  0:12       ` Ben Widawsky
2021-10-14  0:48         ` Dan Williams
2021-10-15 16:44   ` Jonathan Cameron
2021-10-15 17:00     ` Dan Williams
2021-10-15 23:30   ` [PATCH v6 " Dan Williams
2021-11-10 17:14     ` Jonathan Cameron
2021-11-10 17:30       ` Ben Widawsky
2021-11-10 17:43         ` Jonathan Cameron
2021-10-09 16:44 ` [PATCH v3 08/10] PCI: Add pci_find_dvsec_capability to find designated VSEC Dan Williams
2021-10-09 16:44 ` [PATCH v3 09/10] cxl/pci: Use pci core's DVSEC functionality Dan Williams
2021-10-11 13:35   ` Jonathan Cameron
2021-10-09 16:44 ` [PATCH v3 10/10] ocxl: " Dan Williams

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