Re: [RFC PATCH 12/15] cxl/region: Add region creation ABI

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

On Wed, 2022-04-13 at 11:37 -0700, Ben Widawsky wrote:
> Regions are created as a child of the decoder that encompasses an
> address space with constraints. Regions have a number of attributes that
> must be configured before the region can be activated.
> 
> Multiple processes which are trying not to race with each other
> shouldn't need special userspace synchronization to do so.
> 
> // Allocate a new region name
> region=$(cat /sys/bus/cxl/devices/decoder0.0/create_pmem_region)
> 
> // Create a new region by name
> while
> region=$(cat /sys/bus/cxl/devices/decoder0.0/create_pmem_region)
> ! echo $region > /sys/bus/cxl/devices/decoder0.0/create_pmem_region
> do true; done
> 
> // Region now exists in sysfs
> stat -t /sys/bus/cxl/devices/decoder0.0/$region
> 
> // Delete the region, and name
> echo $region > /sys/bus/cxl/devices/decoder0.0/delete_region

I noticed a slight ABI inconsistency here while working on the libcxl
side of this - see below.

> +
> +static ssize_t create_pmem_region_show(struct device *dev,
> +                                      struct device_attribute *attr, char *buf)
> +{
> +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +       struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxld);
> +       size_t rc;
> +
> +       /*
> +        * There's no point in returning known bad answers when the lock is held
> +        * on the store side, even though the answer given here may be
> +        * immediately invalidated as soon as the lock is dropped it's still
> +        * useful to throttle readers in the presence of writers.
> +        */
> +       rc = mutex_lock_interruptible(&cxlrd->id_lock);
> +       if (rc)
> +               return rc;
> +       rc = sysfs_emit(buf, "%d\n", cxlrd->next_region_id);

This emits a numeric region ID, e.g. "0", where as

> +       mutex_unlock(&cxlrd->id_lock);
> +
> +       return rc;
> +}
> +

<snip>

> +static ssize_t delete_region_store(struct device *dev,
> +                                  struct device_attribute *attr,
> +                                  const char *buf, size_t len)
> +{
> +       struct cxl_port *port = to_cxl_port(dev->parent);
> +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +       struct cxl_region *cxlr;
> +
> +       cxlr = cxl_find_region_by_name(cxld, buf);

This expects a full region name string e.g. "region0"

Was this intentional? I don't think it's a huge deal, the library can
certainly deal with it if needed - but would it be better to have the
ABI symmetrical between create and delete?