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

[Neeraj Kumar]

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