Re: [PATCH 05/18] cxl/region: Add volatile region creation support

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Sun, 2023-02-05 at 17:02 -0800, Dan Williams wrote:
> Expand the region creation infrastructure to enable 'ram'
> (volatile-memory) regions. The internals of create_pmem_region_store()
> and create_pmem_region_show() are factored out into helpers
> __create_region() and __create_region_show() for the 'ram' case to
> reuse.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   22 +++++-----
>  drivers/cxl/core/core.h                 |    1 
>  drivers/cxl/core/port.c                 |   14 ++++++
>  drivers/cxl/core/region.c               |   71 +++++++++++++++++++++++++------
>  4 files changed, 82 insertions(+), 26 deletions(-)

Looks good,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 4c4e1cbb1169..3acf2f17a73f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -285,20 +285,20 @@ Description:
>                 interleave_granularity).
>  
>  
> -What:          /sys/bus/cxl/devices/decoderX.Y/create_pmem_region
> -Date:          May, 2022
> -KernelVersion: v6.0
> +What:          /sys/bus/cxl/devices/decoderX.Y/create_{pmem,ram}_region
> +Date:          May, 2022, January, 2023
> +KernelVersion: v6.0 (pmem), v6.3 (ram)
>  Contact:       linux-cxl@vger.kernel.org
>  Description:
>                 (RW) Write a string in the form 'regionZ' to start the process
> -               of defining a new persistent memory region (interleave-set)
> -               within the decode range bounded by root decoder 'decoderX.Y'.
> -               The value written must match the current value returned from
> -               reading this attribute. An atomic compare exchange operation is
> -               done on write to assign the requested id to a region and
> -               allocate the region-id for the next creation attempt. EBUSY is
> -               returned if the region name written does not match the current
> -               cached value.
> +               of defining a new persistent, or volatile memory region
> +               (interleave-set) within the decode range bounded by root decoder
> +               'decoderX.Y'. The value written must match the current value
> +               returned from reading this attribute. An atomic compare exchange
> +               operation is done on write to assign the requested id to a
> +               region and allocate the region-id for the next creation attempt.
> +               EBUSY is returned if the region name written does not match the
> +               current cached value.
>  
>  
>  What:          /sys/bus/cxl/devices/decoderX.Y/delete_region
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 8c04672dca56..5eb873da5a30 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -11,6 +11,7 @@ extern struct attribute_group cxl_base_attribute_group;
>  
>  #ifdef CONFIG_CXL_REGION
>  extern struct device_attribute dev_attr_create_pmem_region;
> +extern struct device_attribute dev_attr_create_ram_region;
>  extern struct device_attribute dev_attr_delete_region;
>  extern struct device_attribute dev_attr_region;
>  extern const struct device_type cxl_pmem_region_type;
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 8566451cb22f..47e450c3a5a9 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -294,6 +294,7 @@ static struct attribute *cxl_decoder_root_attrs[] = {
>         &dev_attr_cap_type3.attr,
>         &dev_attr_target_list.attr,
>         SET_CXL_REGION_ATTR(create_pmem_region)
> +       SET_CXL_REGION_ATTR(create_ram_region)
>         SET_CXL_REGION_ATTR(delete_region)
>         NULL,
>  };
> @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd)
>         return (cxlrd->cxlsd.cxld.flags & flags) == flags;
>  }
>  
> +static bool can_create_ram(struct cxl_root_decoder *cxlrd)
> +{
> +       unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM;
> +
> +       return (cxlrd->cxlsd.cxld.flags & flags) == flags;
> +}
> +
>  static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute *a, int n)
>  {
>         struct device *dev = kobj_to_dev(kobj);
> @@ -313,7 +321,11 @@ static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute *
>         if (a == CXL_REGION_ATTR(create_pmem_region) && !can_create_pmem(cxlrd))
>                 return 0;
>  
> -       if (a == CXL_REGION_ATTR(delete_region) && !can_create_pmem(cxlrd))
> +       if (a == CXL_REGION_ATTR(create_ram_region) && !can_create_ram(cxlrd))
> +               return 0;
> +
> +       if (a == CXL_REGION_ATTR(delete_region) &&
> +           !(can_create_pmem(cxlrd) || can_create_ram(cxlrd)))
>                 return 0;
>  
>         return a->mode;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 53d6dbe4de6d..8dea49c021b8 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1685,6 +1685,15 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>         struct device *dev;
>         int rc;
>  
> +       switch (mode) {
> +       case CXL_DECODER_RAM:
> +       case CXL_DECODER_PMEM:
> +               break;
> +       default:
> +               dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
>         cxlr = cxl_region_alloc(cxlrd, id);
>         if (IS_ERR(cxlr))
>                 return cxlr;
> @@ -1713,12 +1722,38 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>         return ERR_PTR(rc);
>  }
>  
> +static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf)
> +{
> +       return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id));
> +}
> +
>  static ssize_t create_pmem_region_show(struct device *dev,
>                                        struct device_attribute *attr, char *buf)
>  {
> -       struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
> +       return __create_region_show(to_cxl_root_decoder(dev), buf);
> +}
>  
> -       return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id));
> +static ssize_t create_ram_region_show(struct device *dev,
> +                                     struct device_attribute *attr, char *buf)
> +{
> +       return __create_region_show(to_cxl_root_decoder(dev), buf);
> +}
> +
> +static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
> +                                         enum cxl_decoder_mode mode, int id)
> +{
> +       int rc;
> +
> +       rc = memregion_alloc(GFP_KERNEL);
> +       if (rc < 0)
> +               return ERR_PTR(rc);
> +
> +       if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
> +               memregion_free(rc);
> +               return ERR_PTR(-EBUSY);
> +       }
> +
> +       return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER);
>  }
>  
>  static ssize_t create_pmem_region_store(struct device *dev,
> @@ -1727,29 +1762,37 @@ static ssize_t create_pmem_region_store(struct device *dev,
>  {
>         struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
>         struct cxl_region *cxlr;
> -       int id, rc;
> +       int rc, id;
>  
>         rc = sscanf(buf, "region%d\n", &id);
>         if (rc != 1)
>                 return -EINVAL;
>  
> -       rc = memregion_alloc(GFP_KERNEL);
> -       if (rc < 0)
> -               return rc;
> +       cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id);
> +       if (IS_ERR(cxlr))
> +               return PTR_ERR(cxlr);
> +       return len;
> +}
> +DEVICE_ATTR_RW(create_pmem_region);
>  
> -       if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
> -               memregion_free(rc);
> -               return -EBUSY;
> -       }
> +static ssize_t create_ram_region_store(struct device *dev,
> +                                      struct device_attribute *attr,
> +                                      const char *buf, size_t len)
> +{
> +       struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
> +       struct cxl_region *cxlr;
> +       int rc, id;
>  
> -       cxlr = devm_cxl_add_region(cxlrd, id, CXL_DECODER_PMEM,
> -                                  CXL_DECODER_EXPANDER);
> +       rc = sscanf(buf, "region%d\n", &id);
> +       if (rc != 1)
> +               return -EINVAL;
> +
> +       cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id);
>         if (IS_ERR(cxlr))
>                 return PTR_ERR(cxlr);
> -
>         return len;
>  }
> -DEVICE_ATTR_RW(create_pmem_region);
> +DEVICE_ATTR_RW(create_ram_region);
>  
>  static ssize_t region_show(struct device *dev, struct device_attribute *attr,
>                            char *buf)
> 
>