linux-kernel - Re: [PATCH V3 19/20] cxl/pmem_region: Add sysfs attribute cxl region label updation/deletion

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: <1690859824.141759726504518.JavaMail.epsvc@epcpadp1new>
Date: Mon, 29 Sep 2025 19:30:24 +0530
From: Neeraj Kumar <s.neeraj@...sung.com>
To: Dave Jiang <dave.jiang@...el.com>
Cc: linux-cxl@...r.kernel.org, nvdimm@...ts.linux.dev,
	linux-kernel@...r.kernel.org, gost.dev@...sung.com,
	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 24/09/25 01:25PM, Dave Jiang wrote:
>
>

<snip>

>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.
>> +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;
>> +}
>> +

<snip>

>> +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;
>> +}

<snip>

>> --- 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?
>

Thanks for you suggestion Dave, I will fix it accrodingly.


Regards,
Neeraj