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

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

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