[PATCH 0/3] Rework register enumeration for later reuse

[PATCH 0/3] Rework register enumeration for later reuse

[Ben Widawsky]

In order to plug a cxl_memdev into cxl_core for the purpose of enumerating HDM
decoders, the physical address of the component registers must be known and
retained at the time of cxl_memdev creation. The cxl_mem driver already
enumerates and maps these registers. The main goal of the series is to be able
to reuse that logic for the upcoming cxl_memdev driver.

This patch series is based upon the core-reorg patches I submitted yesterday,
but it could be merged independently. With core-reorg [1] series, and the
expand-decoders [2] patch, everything is in place to implement the region and
memdev drivers.

A few other options were discussed/pursued:
1. Passing around the mapped addresses instead of physical. This caused more
   churn in the existing code. It also causes a somewhat undesirable combination
   of having one entity "own" the mapping and another one using it.
2. Don't map component registers in cxl_pci, and instead map it when needed in
   the port driver. I actually believe this would have been the right thing to
   do except certain register sets (like IDE) may be needed by the cxl_pci
   driver anyway.
3. Export functionality to obtain component register mapping in cxl_core and let
   the cxl_pci driver and memdev driver use that. This solution might be the
   eventual solution, but it's not yet necessary.
3b. Copy the enumeration code from pci.c into the new memdev driver.
4. Obtaining the register mapping from cxl_mem at driver probe since the
   cxl_memdev and cxl_mem are connected. Like #1, this mixes "ownership" of the
   mapping.

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

Ben Widawsky (3):
  cxl/pci: Ignore unknown register block types
  cxl/pci: Simplify register setup
  cxl/pci: Retain map information in cxl_mem_probe

 drivers/cxl/cxl.h |  1 -
 drivers/cxl/pci.c | 50 ++++++++++++++++++++---------------------------
 drivers/cxl/pci.h |  1 +
 3 files changed, 22 insertions(+), 30 deletions(-)


-- 
2.32.0

[PATCH 1/3] cxl/pci: Ignore unknown register block types

[Ben Widawsky]

In an effort to explicit avoid supporting vendor specific register
blocks (which can happily be mapped from userspace), entirely skip
probing unknown types. The secondary benefit of this will be revealed
in the future with code simplification.

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

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index d7da18ebba81..dd0ac89fbdf4 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1106,14 +1106,6 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		u64 offset;
 		u8 bar;
 
-		map = kzalloc(sizeof(*map), GFP_KERNEL);
-		if (!map) {
-			ret = -ENOMEM;
-			goto free_maps;
-		}
-
-		list_add(&map->list, &register_maps);
-
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
@@ -1123,6 +1115,18 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		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;
+
+		map = kzalloc(sizeof(*map), GFP_KERNEL);
+		if (!map) {
+			ret = -ENOMEM;
+			goto free_maps;
+		}
+
+		list_add(&map->list, &register_maps);
+
 		base = cxl_mem_map_regblock(cxlm, bar, offset);
 		if (!base) {
 			ret = -ENOMEM;
-- 
2.32.0

Re: [PATCH 1/3] cxl/pci: Ignore unknown register block types

[Jonathan Cameron]

On Fri, 16 Jul 2021 16:15:46 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> In an effort to explicit avoid supporting vendor specific register
> blocks (which can happily be mapped from userspace), entirely skip
> probing unknown types. The secondary benefit of this will be revealed
> in the future with code simplification.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Seems sensible even on it's own as we don't do anything with unknown
blocks anyway.

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

> ---
>  drivers/cxl/pci.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index d7da18ebba81..dd0ac89fbdf4 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1106,14 +1106,6 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		u64 offset;
>  		u8 bar;
>  
> -		map = kzalloc(sizeof(*map), GFP_KERNEL);
> -		if (!map) {
> -			ret = -ENOMEM;
> -			goto free_maps;
> -		}
> -
> -		list_add(&map->list, &register_maps);
> -
>  		pci_read_config_dword(pdev, regloc, &reg_lo);
>  		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
>  
> @@ -1123,6 +1115,18 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		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;
> +
> +		map = kzalloc(sizeof(*map), GFP_KERNEL);
> +		if (!map) {
> +			ret = -ENOMEM;
> +			goto free_maps;
> +		}
> +
> +		list_add(&map->list, &register_maps);
> +
>  		base = cxl_mem_map_regblock(cxlm, bar, offset);
>  		if (!base) {
>  			ret = -ENOMEM;

[PATCH 2/3] cxl/pci: Simplify register setup

[Ben Widawsky]

It is desirable to retain the mappings from the calling function. By
simplifying this code, it will be much more straightforward to do that.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/cxl.h |  1 -
 drivers/cxl/pci.c | 38 ++++++++++++--------------------------
 drivers/cxl/pci.h |  1 +
 3 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6bda39a59e3..53927f9fa77e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -140,7 +140,6 @@ struct cxl_device_reg_map {
 };
 
 struct cxl_register_map {
-	struct list_head list;
 	u64 block_offset;
 	u8 reg_type;
 	u8 barno;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index dd0ac89fbdf4..8be18daa1420 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1079,9 +1079,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	struct device *dev = &pdev->dev;
 	u32 regloc_size, regblocks;
 	void __iomem *base;
-	int regloc, i;
-	struct cxl_register_map *map, *n;
-	LIST_HEAD(register_maps);
+	int regloc, i, n_maps;
+	struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
 	int ret = 0;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
@@ -1100,7 +1099,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
 	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
 
-	for (i = 0; i < regblocks; i++, regloc += 8) {
+	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
 		u8 reg_type;
 		u64 offset;
@@ -1119,20 +1118,11 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		map = kzalloc(sizeof(*map), GFP_KERNEL);
-		if (!map) {
-			ret = -ENOMEM;
-			goto free_maps;
-		}
-
-		list_add(&map->list, &register_maps);
-
 		base = cxl_mem_map_regblock(cxlm, bar, offset);
-		if (!base) {
-			ret = -ENOMEM;
-			goto free_maps;
-		}
+		if (!base)
+			return -ENOMEM;
 
+		map = &maps[n_maps];
 		map->barno = bar;
 		map->block_offset = offset;
 		map->reg_type = reg_type;
@@ -1143,21 +1133,17 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		cxl_mem_unmap_regblock(cxlm, base);
 
 		if (ret)
-			goto free_maps;
+			return ret;
+
+		n_maps++;
 	}
 
 	pci_release_mem_regions(pdev);
 
-	list_for_each_entry(map, &register_maps, list) {
-		ret = cxl_map_regs(cxlm, map);
+	for (i = 0; i < n_maps; i++) {
+		ret = cxl_map_regs(cxlm, &maps[i]);
 		if (ret)
-			goto free_maps;
-	}
-
-free_maps:
-	list_for_each_entry_safe(map, n, &register_maps, list) {
-		list_del(&map->list);
-		kfree(map);
+			break;
 	}
 
 	return ret;
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index dad7a831f65f..8c1a58813816 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -25,6 +25,7 @@
 #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
 
 #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
 
-- 
2.32.0

Re: [PATCH 2/3] cxl/pci: Simplify register setup

[Jonathan Cameron]

On Fri, 16 Jul 2021 16:15:47 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> It is desirable to retain the mappings from the calling function. By
> simplifying this code, it will be much more straightforward to do that.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Nice.  Moving such a small structure onto the stack makes sense
as the diff stats show.

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

> ---
>  drivers/cxl/cxl.h |  1 -
>  drivers/cxl/pci.c | 38 ++++++++++++--------------------------
>  drivers/cxl/pci.h |  1 +
>  3 files changed, 13 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b6bda39a59e3..53927f9fa77e 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -140,7 +140,6 @@ struct cxl_device_reg_map {
>  };
>  
>  struct cxl_register_map {
> -	struct list_head list;
>  	u64 block_offset;
>  	u8 reg_type;
>  	u8 barno;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index dd0ac89fbdf4..8be18daa1420 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1079,9 +1079,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  	struct device *dev = &pdev->dev;
>  	u32 regloc_size, regblocks;
>  	void __iomem *base;
> -	int regloc, i;
> -	struct cxl_register_map *map, *n;
> -	LIST_HEAD(register_maps);
> +	int regloc, i, n_maps;
> +	struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
>  	int ret = 0;
>  
>  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
> @@ -1100,7 +1099,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
>  	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
>  
> -	for (i = 0; i < regblocks; i++, regloc += 8) {
> +	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
>  		u32 reg_lo, reg_hi;
>  		u8 reg_type;
>  		u64 offset;
> @@ -1119,20 +1118,11 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
>  			continue;
>  
> -		map = kzalloc(sizeof(*map), GFP_KERNEL);
> -		if (!map) {
> -			ret = -ENOMEM;
> -			goto free_maps;
> -		}
> -
> -		list_add(&map->list, &register_maps);
> -
>  		base = cxl_mem_map_regblock(cxlm, bar, offset);
> -		if (!base) {
> -			ret = -ENOMEM;
> -			goto free_maps;
> -		}
> +		if (!base)
> +			return -ENOMEM;
>  
> +		map = &maps[n_maps];
>  		map->barno = bar;
>  		map->block_offset = offset;
>  		map->reg_type = reg_type;
> @@ -1143,21 +1133,17 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		cxl_mem_unmap_regblock(cxlm, base);
>  
>  		if (ret)
> -			goto free_maps;
> +			return ret;
> +
> +		n_maps++;
>  	}
>  
>  	pci_release_mem_regions(pdev);
>  
> -	list_for_each_entry(map, &register_maps, list) {
> -		ret = cxl_map_regs(cxlm, map);
> +	for (i = 0; i < n_maps; i++) {
> +		ret = cxl_map_regs(cxlm, &maps[i]);
>  		if (ret)
> -			goto free_maps;
> -	}
> -
> -free_maps:
> -	list_for_each_entry_safe(map, n, &register_maps, list) {
> -		list_del(&map->list);
> -		kfree(map);
> +			break;
>  	}
>  
>  	return ret;
> diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
> index dad7a831f65f..8c1a58813816 100644
> --- a/drivers/cxl/pci.h
> +++ b/drivers/cxl/pci.h
> @@ -25,6 +25,7 @@
>  #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
>  
>  #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
>  

[PATCH 3/3] cxl/pci: Retain map information in cxl_mem_probe

[Ben Widawsky]

In order for a memdev to participate in cxl_core's port APIs, the
physical address of the memdev's component registers is needed. This is
accomplished by allocating the array of maps in probe so they can be
used after the memdev is created.

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

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 8be18daa1420..f924a8c5a831 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1066,21 +1066,22 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
 /**
  * cxl_mem_setup_regs() - Setup necessary MMIO.
  * @cxlm: The CXL memory device to communicate with.
+ * @maps: Array of maps populated by this function.
  *
- * Return: 0 if all necessary registers mapped.
+ * Return: 0 if all necessary registers mapped. The results are stored in @maps.
  *
  * A memory device is required by spec to implement a certain set of MMIO
  * regions. The purpose of this function is to enumerate and map those
  * registers.
  */
-static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
+static int cxl_mem_setup_regs(struct cxl_mem *cxlm, struct cxl_register_map maps[])
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
 	u32 regloc_size, regblocks;
 	void __iomem *base;
 	int regloc, i, n_maps;
-	struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
+	struct cxl_register_map *map;
 	int ret = 0;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
@@ -1364,6 +1365,7 @@ static void cxl_memdev_shutdown(struct cxl_memdev *cxlmd)
 
 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	struct cxl_register_map maps[CXL_REGLOC_RBI_TYPES];
 	struct cxl_memdev *cxlmd;
 	struct cxl_mem *cxlm;
 	int rc;
@@ -1376,7 +1378,7 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlm))
 		return PTR_ERR(cxlm);
 
-	rc = cxl_mem_setup_regs(cxlm);
+	rc = cxl_mem_setup_regs(cxlm, maps);
 	if (rc)
 		return rc;
 
-- 
2.32.0

Re: [PATCH 3/3] cxl/pci: Retain map information in cxl_mem_probe

[Jonathan Cameron]

On Fri, 16 Jul 2021 16:15:48 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> In order for a memdev to participate in cxl_core's port APIs, the
> physical address of the memdev's component registers is needed. This is
> accomplished by allocating the array of maps in probe so they can be
> used after the memdev is created.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Hmm. I don't entirely like the the passing of an array of 
unknown size into cxl_mem_setup_regs.  It is perhaps paranoid
but I'd separately pass in the size and error out should we
overflow with a suitable message to highlight the bug.

So far this code is also not justified by anything using the
array now it's been moved up a layer. Looks that doesn't happen
until patch 22 of your large WIP series. I think this patch needs
to be in the same series as that one as it doesn't stand on
it's own.

Jonathan


> ---
>  drivers/cxl/pci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8be18daa1420..f924a8c5a831 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1066,21 +1066,22 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
>  /**
>   * cxl_mem_setup_regs() - Setup necessary MMIO.
>   * @cxlm: The CXL memory device to communicate with.
> + * @maps: Array of maps populated by this function.
>   *
> - * Return: 0 if all necessary registers mapped.
> + * Return: 0 if all necessary registers mapped. The results are stored in @maps.
>   *
>   * A memory device is required by spec to implement a certain set of MMIO
>   * regions. The purpose of this function is to enumerate and map those
>   * registers.
>   */
> -static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> +static int cxl_mem_setup_regs(struct cxl_mem *cxlm, struct cxl_register_map maps[])
>  {
>  	struct pci_dev *pdev = cxlm->pdev;
>  	struct device *dev = &pdev->dev;
>  	u32 regloc_size, regblocks;
>  	void __iomem *base;
>  	int regloc, i, n_maps;
> -	struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
> +	struct cxl_register_map *map;
>  	int ret = 0;
>  
>  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
> @@ -1364,6 +1365,7 @@ static void cxl_memdev_shutdown(struct cxl_memdev *cxlmd)
>  
>  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> +	struct cxl_register_map maps[CXL_REGLOC_RBI_TYPES];
>  	struct cxl_memdev *cxlmd;
>  	struct cxl_mem *cxlm;
>  	int rc;
> @@ -1376,7 +1378,7 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(cxlm))
>  		return PTR_ERR(cxlm);
>  
> -	rc = cxl_mem_setup_regs(cxlm);
> +	rc = cxl_mem_setup_regs(cxlm, maps);
>  	if (rc)
>  		return rc;
>  

Re: [PATCH 3/3] cxl/pci: Retain map information in cxl_mem_probe

[Dan Williams]

On Mon, Aug 2, 2021 at 8:57 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 16 Jul 2021 16:15:48 -0700
> Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> > In order for a memdev to participate in cxl_core's port APIs, the
> > physical address of the memdev's component registers is needed. This is
> > accomplished by allocating the array of maps in probe so they can be
> > used after the memdev is created.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>
> Hmm. I don't entirely like the the passing of an array of
> unknown size into cxl_mem_setup_regs.  It is perhaps paranoid
> but I'd separately pass in the size and error out should we
> overflow with a suitable message to highlight the bug.

Agree.

>
> So far this code is also not justified by anything using the
> array now it's been moved up a layer. Looks that doesn't happen
> until patch 22 of your large WIP series. I think this patch needs
> to be in the same series as that one as it doesn't stand on
> it's own.

Agree, I'll reorder this change to closer to where it is used.

Re: [PATCH 3/3] cxl/pci: Retain map information in cxl_mem_probe

[Dan Williams]

On Mon, Aug 2, 2021 at 9:10 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Aug 2, 2021 at 8:57 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Fri, 16 Jul 2021 16:15:48 -0700
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > > In order for a memdev to participate in cxl_core's port APIs, the
> > > physical address of the memdev's component registers is needed. This is
> > > accomplished by allocating the array of maps in probe so they can be
> > > used after the memdev is created.
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> >
> > Hmm. I don't entirely like the the passing of an array of
> > unknown size into cxl_mem_setup_regs.  It is perhaps paranoid
> > but I'd separately pass in the size and error out should we
> > overflow with a suitable message to highlight the bug.
>
> Agree.

Here's the incremental diff I came up with:

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index c370ab2e48bc..ff72286142e7 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1086,7 +1086,8 @@ static void cxl_decode_register_block(u32
reg_lo, u32 reg_hi,
  * regions. The purpose of this function is to enumerate and map those
  * registers.
  */
-static int cxl_mem_setup_regs(struct cxl_mem *cxlm, struct
cxl_register_map maps[])
+static int cxl_mem_setup_regs(struct cxl_mem *cxlm,
+                             struct cxl_register_maps *maps)
 {
        struct pci_dev *pdev = cxlm->pdev;
        struct device *dev = &pdev->dev;
@@ -1135,7 +1136,9 @@ static int cxl_mem_setup_regs(struct cxl_mem
*cxlm, struct cxl_register_map maps
                if (!base)
                        return -ENOMEM;

-               map = &maps[n_maps];
+               if (n_maps > ARRAY_SIZE(maps->map))
+                       return -ENXIO;
+               map = &maps->map[n_maps++];
                map->barno = bar;
                map->block_offset = offset;
                map->reg_type = reg_type;
@@ -1147,14 +1150,12 @@ static int cxl_mem_setup_regs(struct cxl_mem
*cxlm, struct cxl_register_map maps

                if (ret)
                        return ret;
-
-               n_maps++;
        }

        pci_release_mem_regions(pdev);

        for (i = 0; i < n_maps; i++) {
-               ret = cxl_map_regs(cxlm, &maps[i]);
+               ret = cxl_map_regs(cxlm, &maps->map[i]);
                if (ret)
                        break;
        }
@@ -1370,7 +1371,7 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)

 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
-       struct cxl_register_map maps[CXL_REGLOC_RBI_TYPES];
+       struct cxl_register_maps maps;
        struct cxl_memdev *cxlmd;
        struct cxl_mem *cxlm;
        int rc;
@@ -1383,7 +1384,7 @@ static int cxl_mem_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
        if (IS_ERR(cxlm))
                return PTR_ERR(cxlm);

-       rc = cxl_mem_setup_regs(cxlm, maps);
+       rc = cxl_mem_setup_regs(cxlm, &maps);
        if (rc)
                return rc;

diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index 8c1a58813816..5b7828003b76 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -2,6 +2,7 @@
 /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
 #ifndef __CXL_PCI_H__
 #define __CXL_PCI_H__
+#include "cxlmem.h"

 #define CXL_MEMORY_PROGIF      0x10

@@ -29,4 +30,8 @@

 #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)

+struct cxl_register_maps {
+       struct cxl_register_map map[CXL_REGLOC_RBI_TYPES];
+};
+
 #endif /* __CXL_PCI_H__ */

Re: [PATCH 3/3] cxl/pci: Retain map information in cxl_mem_probe

[Jonathan Cameron]

On Mon, 2 Aug 2021 10:09:45 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Mon, Aug 2, 2021 at 9:10 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Mon, Aug 2, 2021 at 8:57 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:  
> > >
> > > On Fri, 16 Jul 2021 16:15:48 -0700
> > > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >  
> > > > In order for a memdev to participate in cxl_core's port APIs, the
> > > > physical address of the memdev's component registers is needed. This is
> > > > accomplished by allocating the array of maps in probe so they can be
> > > > used after the memdev is created.
> > > >
> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> > >
> > > Hmm. I don't entirely like the the passing of an array of
> > > unknown size into cxl_mem_setup_regs.  It is perhaps paranoid
> > > but I'd separately pass in the size and error out should we
> > > overflow with a suitable message to highlight the bug.  
> >
> > Agree.  
> 
> Here's the incremental diff I came up with:
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index c370ab2e48bc..ff72286142e7 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1086,7 +1086,8 @@ static void cxl_decode_register_block(u32
> reg_lo, u32 reg_hi,
>   * regions. The purpose of this function is to enumerate and map those
>   * registers.
>   */
> -static int cxl_mem_setup_regs(struct cxl_mem *cxlm, struct
> cxl_register_map maps[])
> +static int cxl_mem_setup_regs(struct cxl_mem *cxlm,
> +                             struct cxl_register_maps *maps)
>  {
>         struct pci_dev *pdev = cxlm->pdev;
>         struct device *dev = &pdev->dev;
> @@ -1135,7 +1136,9 @@ static int cxl_mem_setup_regs(struct cxl_mem
> *cxlm, struct cxl_register_map maps
>                 if (!base)
>                         return -ENOMEM;
> 
> -               map = &maps[n_maps];
> +               if (n_maps > ARRAY_SIZE(maps->map))
> +                       return -ENXIO;
> +               map = &maps->map[n_maps++];
>                 map->barno = bar;
>                 map->block_offset = offset;
>                 map->reg_type = reg_type;
> @@ -1147,14 +1150,12 @@ static int cxl_mem_setup_regs(struct cxl_mem
> *cxlm, struct cxl_register_map maps
> 
>                 if (ret)
>                         return ret;
> -
> -               n_maps++;

I found original form of this block with the separate n_maps++ a little
bit more readable.  But otherwise this approach looks good to me.

Jonathan


>         }
> 
>         pci_release_mem_regions(pdev);
> 
>         for (i = 0; i < n_maps; i++) {
> -               ret = cxl_map_regs(cxlm, &maps[i]);
> +               ret = cxl_map_regs(cxlm, &maps->map[i]);
>                 if (ret)
>                         break;
>         }
> @@ -1370,7 +1371,7 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
> 
>  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> -       struct cxl_register_map maps[CXL_REGLOC_RBI_TYPES];
> +       struct cxl_register_maps maps;
>         struct cxl_memdev *cxlmd;
>         struct cxl_mem *cxlm;
>         int rc;
> @@ -1383,7 +1384,7 @@ static int cxl_mem_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>         if (IS_ERR(cxlm))
>                 return PTR_ERR(cxlm);
> 
> -       rc = cxl_mem_setup_regs(cxlm, maps);
> +       rc = cxl_mem_setup_regs(cxlm, &maps);
>         if (rc)
>                 return rc;
> 
> diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
> index 8c1a58813816..5b7828003b76 100644
> --- a/drivers/cxl/pci.h
> +++ b/drivers/cxl/pci.h
> @@ -2,6 +2,7 @@
>  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
>  #ifndef __CXL_PCI_H__
>  #define __CXL_PCI_H__
> +#include "cxlmem.h"
> 
>  #define CXL_MEMORY_PROGIF      0x10
> 
> @@ -29,4 +30,8 @@
> 
>  #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
> 
> +struct cxl_register_maps {
> +       struct cxl_register_map map[CXL_REGLOC_RBI_TYPES];
> +};
> +
>  #endif /* __CXL_PCI_H__ */