lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6734182c-579e-439c-906c-f3c053025ef0@intel.com>
Date: Wed, 24 Sep 2025 13:25:43 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Neeraj Kumar <s.neeraj@...sung.com>, linux-cxl@...r.kernel.org,
 nvdimm@...ts.linux.dev, linux-kernel@...r.kernel.org, gost.dev@...sung.com
Cc: a.manzanares@...sung.com, vishak.g@...sung.com, neeraj.kernel@...il.com,
 cpgs@...sung.com
Subject: Re: [PATCH V3 19/20] cxl/pmem_region: Add sysfs attribute cxl region
 label updation/deletion



On 9/17/25 6:41 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@...sung.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 22 ++++++
>  drivers/cxl/core/pmem_region.c          | 91 ++++++++++++++++++++++++-
>  drivers/cxl/cxl.h                       |  1 +
>  3 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 6b4e8c7a963d..d6080fcf843a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -615,3 +615,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:		Sept, 2025
> +KernelVersion:	v6.17
> +Contact:	linux-cxl@...r.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.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/pmem_regionZ/region_label_delete
> +Date:		Sept, 2025
> +KernelVersion:	v6.17
> +Contact:	linux-cxl@...r.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 55b80d587403..665b603c907b 100644
> --- a/drivers/cxl/core/pmem_region.c
> +++ b/drivers/cxl/core/pmem_region.c
> @@ -45,9 +45,98 @@ 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) {
> +		rc = nd_region_label_update(cxlr->cxlr_pmem->nd_region);
> +		if (!rc)
> +			cxlr->params.region_label_state = 1;
> +	}
> +
> +	if (rc)
> +		return rc;

Feels like this segment should look like

if (!cxlr || !cxlr->cxlr_pmem || ! cxlr->cxlr_pmem->nd_region)
	return 0;

rc = nd_region_label_update(..);
if (rc)
	return rc;

cxlr->params.region_label_state = 1;

> +
> +	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->region_label_state);
> +}
> +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) {
> +		rc = nd_region_label_delete(cxlr->cxlr_pmem->nd_region);
> +		if (rc)
> +			return rc;
> +		cxlr->params.region_label_state = 0;
> +	}

Similar to above. You can exit early and not have to indent.

> +
> +	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 0d576b359de6..f01f8c942fdf 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -484,6 +484,7 @@ enum cxl_config_state {
>   */
>  struct cxl_region_params {
>  	enum cxl_config_state state;
> +	int region_label_state;

Maybe a enum?

>  	uuid_t uuid;
>  	int interleave_ways;
>  	int interleave_granularity;


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ