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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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;
>