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

[Neeraj Kumar <s.neeraj@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]

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


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]