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

Neeraj Kumar
2025-10-06T04:55:08Z

[-- 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 --]