Re: [PATCH V4 15/17] cxl/pmem_region: Add sysfs attribute cxl region label updation/deletion

[Dave Jiang <dave.jiang@intel.com>]

On 11/19/25 12:52 AM, Neeraj Kumar wrote:
> Using these attributes region label is added/deleted into LSA. These
> attributes are called from userspace (ndctl) after region creation.
> 
> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 22 ++++++
>  drivers/cxl/core/pmem_region.c          | 93 ++++++++++++++++++++++++-
>  drivers/cxl/cxl.h                       |  7 ++
>  3 files changed, 121 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index c80a1b5a03db..76d79c03dde4 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -624,3 +624,25 @@ Description:
>  		The count is persistent across power loss and wraps back to 0
>  		upon overflow. If this file is not present, the device does not
>  		have the necessary support for dirty tracking.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/pmem_regionZ/region_label_update
> +Date:		Nov, 2025
> +KernelVersion:	v6.19
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RW) Write a boolean 'true' string value to this attribute to
> +		update cxl region information into LSA as region label. It
> +		uses nvdimm nd_region_label_update() to update cxl region
> +		information saved during cxl region creation into LSA. This
> +		attribute must be called at last during cxl region creation.

Please consider:
attribute must be written last during cxl region creation.

No need to mention kernel specifics like function names. Just give general description of what the attribute does. Same for the next attribute below.

Also, does this attribute needs to be readable? The documentation above does not explain the read attribute if so.

DJ

> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/pmem_regionZ/region_label_delete
> +Date:		Nov, 2025
> +KernelVersion:	v6.19
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(WO) When a boolean 'true' is written to this attribute then
> +		pmem_region driver deletes cxl region label from LSA using
> +		nvdimm nd_region_label_delete()
> diff --git a/drivers/cxl/core/pmem_region.c b/drivers/cxl/core/pmem_region.c
> index b45e60f04ff4..be4feb73aafc 100644
> --- a/drivers/cxl/core/pmem_region.c
> +++ b/drivers/cxl/core/pmem_region.c
> @@ -30,9 +30,100 @@ static void cxl_pmem_region_release(struct device *dev)
>  	kfree(cxlr_pmem);
>  }
>  
> +static ssize_t region_label_update_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t len)
> +{
> +	struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
> +	struct cxl_region *cxlr = cxlr_pmem->cxlr;
> +	ssize_t rc;
> +	bool update;
> +
> +	rc = kstrtobool(buf, &update);
> +	if (rc)
> +		return rc;
> +
> +	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> +	rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem);
> +	if (rc)
> +		return rc;
> +
> +	/* Region not yet committed */
> +	if (update && cxlr && cxlr->params.state != CXL_CONFIG_COMMIT) {
> +		dev_dbg(dev, "region not committed, can't update into LSA\n");
> +		return -ENXIO;
> +	}
> +
> +	if (!cxlr || !cxlr->cxlr_pmem || !cxlr->cxlr_pmem->nd_region)
> +		return 0;
> +
> +	rc = nd_region_label_update(cxlr->cxlr_pmem->nd_region);
> +	if (rc)
> +		return rc;
> +
> +	cxlr->params.state_region_label = CXL_REGION_LABEL_ACTIVE;
> +
> +	return len;
> +}
> +
> +static ssize_t region_label_update_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
> +	struct cxl_region *cxlr = cxlr_pmem->cxlr;
> +	struct cxl_region_params *p = &cxlr->params;
> +	ssize_t rc;
> +
> +	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
> +	rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem);
> +	if (rc)
> +		return rc;
> +
> +	return sysfs_emit(buf, "%d\n", p->state_region_label);
> +}
> +static DEVICE_ATTR_RW(region_label_update);
> +
> +static ssize_t region_label_delete_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t len)
> +{
> +	struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
> +	struct cxl_region *cxlr = cxlr_pmem->cxlr;
> +	ssize_t rc;
> +
> +	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> +	rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem);
> +	if (rc)
> +		return rc;
> +
> +	if (!cxlr && !cxlr->cxlr_pmem && !cxlr->cxlr_pmem->nd_region)
> +		return 0;
> +
> +	rc = nd_region_label_delete(cxlr->cxlr_pmem->nd_region);
> +	if (rc)
> +		return rc;
> +
> +	cxlr->params.state_region_label = CXL_REGION_LABEL_INACTIVE;
> +
> +	return len;
> +}
> +static DEVICE_ATTR_WO(region_label_delete);
> +
> +static struct attribute *cxl_pmem_region_attrs[] = {
> +	&dev_attr_region_label_update.attr,
> +	&dev_attr_region_label_delete.attr,
> +	NULL
> +};
> +
> +static struct attribute_group cxl_pmem_region_group = {
> +	.attrs = cxl_pmem_region_attrs,
> +};
> +
>  static const struct attribute_group *cxl_pmem_region_attribute_groups[] = {
>  	&cxl_base_attribute_group,
> -	NULL,
> +	&cxl_pmem_region_group,
> +	NULL
>  };
>  
>  const struct device_type cxl_pmem_region_type = {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6ac3b40cb5ff..8c76c4a981bf 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -473,9 +473,15 @@ enum cxl_config_state {
>  	CXL_CONFIG_COMMIT,
>  };
>  
> +enum region_label_state {
> +	CXL_REGION_LABEL_INACTIVE,
> +	CXL_REGION_LABEL_ACTIVE,
> +};
> +
>  /**
>   * struct cxl_region_params - region settings
>   * @state: allow the driver to lockdown further parameter changes
> + * @state: region label state
>   * @uuid: unique id for persistent regions
>   * @interleave_ways: number of endpoints in the region
>   * @interleave_granularity: capacity each endpoint contributes to a stripe
> @@ -488,6 +494,7 @@ enum cxl_config_state {
>   */
>  struct cxl_region_params {
>  	enum cxl_config_state state;
> +	enum region_label_state state_region_label;
>  	uuid_t uuid;
>  	int interleave_ways;
>  	int interleave_granularity;