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

[Dan Williams <dan.j.williams@intel.com>]

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__ */