| Message ID | 20210716231548.174778-4-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework register enumeration for later reuse | expand |
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; >
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.
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__ */
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__ */
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;
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(-)