Added __pmem_region_label_update region label update routine to update
region label.

Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
mutex_unlock()

Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
---
 drivers/nvdimm/label.c          | 171 +++++++++++++++++++++++++++++---
 drivers/nvdimm/label.h          |   2 +
 drivers/nvdimm/namespace_devs.c |  12 +++
 drivers/nvdimm/nd.h             |  20 ++++
 include/linux/libnvdimm.h       |   8 ++
 5 files changed, 198 insertions(+), 15 deletions(-)

diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
index 3f8a6bdb77c7..94f2d0ba7aca 100644
--- a/drivers/nvdimm/label.c
+++ b/drivers/nvdimm/label.c
@@ -381,6 +381,16 @@ static void nsl_calculate_checksum(struct nvdimm_drvdata *ndd,
 	nsl_set_checksum(ndd, nd_label, sum);
 }
 
+static void rgl_calculate_checksum(struct nvdimm_drvdata *ndd,
+				   struct cxl_region_label *rg_label)
+{
+	u64 sum;
+
+	rgl_set_checksum(rg_label, 0);
+	sum = nd_fletcher64(rg_label, sizeof_namespace_label(ndd), 1);
+	rgl_set_checksum(rg_label, sum);
+}
+
 static bool slot_valid(struct nvdimm_drvdata *ndd,
 		struct nd_lsa_label *lsa_label, u32 slot)
 {
@@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
 		return rc;
 
 	/* Garbage collect the previous label */
-	mutex_lock(&nd_mapping->lock);
+	guard(mutex)(&nd_mapping->lock);
 	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
 		if (!label_ent->label)
 			continue;
@@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
 	/* update index */
 	rc = nd_label_write_index(ndd, ndd->ns_next,
 			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
-	if (rc == 0) {
-		list_for_each_entry(label_ent, &nd_mapping->labels, list)
-			if (!label_ent->label) {
-				label_ent->label = lsa_label;
-				lsa_label = NULL;
-				break;
-			}
-		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
-				"failed to track label: %d\n",
-				to_slot(ndd, lsa_label));
-		if (lsa_label)
-			rc = -ENXIO;
-	}
-	mutex_unlock(&nd_mapping->lock);
+	if (rc)
+		return rc;
+
+	list_for_each_entry(label_ent, &nd_mapping->labels, list)
+		if (!label_ent->label) {
+			label_ent->label = lsa_label;
+			lsa_label = NULL;
+			break;
+		}
+	dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
+			"failed to track label: %d\n",
+			to_slot(ndd, lsa_label));
+	if (lsa_label)
+		rc = -ENXIO;
 
 	return rc;
 }
@@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
 	return 0;
 }
 
+static int __pmem_region_label_update(struct nd_region *nd_region,
+		struct nd_mapping *nd_mapping, int pos, unsigned long flags)
+{
+	struct nd_interleave_set *nd_set = nd_region->nd_set;
+	struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
+	struct nd_lsa_label *nd_label;
+	struct cxl_region_label *rg_label;
+	struct nd_namespace_index *nsindex;
+	struct nd_label_ent *label_ent;
+	unsigned long *free;
+	u32 nslot, slot;
+	size_t offset;
+	int rc;
+	uuid_t tmp;
+
+	if (!preamble_next(ndd, &nsindex, &free, &nslot))
+		return -ENXIO;
+
+	/* allocate and write the label to the staging (next) index */
+	slot = nd_label_alloc_slot(ndd);
+	if (slot == UINT_MAX)
+		return -ENXIO;
+	dev_dbg(ndd->dev, "allocated: %d\n", slot);
+
+	nd_label = to_label(ndd, slot);
+
+	memset(nd_label, 0, sizeof_namespace_label(ndd));
+	rg_label = &nd_label->rg_label;
+
+	/* Set Region Label Format identification UUID */
+	uuid_parse(CXL_REGION_UUID, &tmp);
+	export_uuid(nd_label->rg_label.type, &tmp);
+
+	/* Set Current Region Label UUID */
+	export_uuid(nd_label->rg_label.uuid, &nd_set->uuid);
+
+	rg_label->flags = __cpu_to_le32(flags);
+	rg_label->nlabel = __cpu_to_le16(nd_region->ndr_mappings);
+	rg_label->position = __cpu_to_le16(pos);
+	rg_label->dpa = __cpu_to_le64(nd_mapping->start);
+	rg_label->rawsize = __cpu_to_le64(nd_mapping->size);
+	rg_label->hpa = __cpu_to_le64(nd_set->res->start);
+	rg_label->slot = __cpu_to_le32(slot);
+	rg_label->ig = __cpu_to_le32(nd_set->interleave_granularity);
+	rg_label->align = __cpu_to_le16(0);
+
+	/* Update fletcher64 Checksum */
+	rgl_calculate_checksum(ndd, rg_label);
+
+	/* update label */
+	offset = nd_label_offset(ndd, nd_label);
+	rc = nvdimm_set_config_data(ndd, offset, nd_label,
+			sizeof_namespace_label(ndd));
+	if (rc < 0) {
+		nd_label_free_slot(ndd, slot);
+		return rc;
+	}
+
+	/* Garbage collect the previous label */
+	guard(mutex)(&nd_mapping->lock);
+	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
+		if (!label_ent->label)
+			continue;
+		if (rgl_uuid_equal(&label_ent->label->rg_label, &nd_set->uuid))
+			reap_victim(nd_mapping, label_ent);
+	}
+
+	/* update index */
+	rc = nd_label_write_index(ndd, ndd->ns_next,
+			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
+	if (rc)
+		return rc;
+
+	list_for_each_entry(label_ent, &nd_mapping->labels, list)
+		if (!label_ent->label) {
+			label_ent->label = nd_label;
+			nd_label = NULL;
+			break;
+		}
+	dev_WARN_ONCE(&nd_region->dev, nd_label,
+			"failed to track label: %d\n",
+			to_slot(ndd, nd_label));
+	if (nd_label)
+		rc = -ENXIO;
+
+	return rc;
+}
+
+int nd_pmem_region_label_update(struct nd_region *nd_region)
+{
+	int i, rc;
+
+	for (i = 0; i < nd_region->ndr_mappings; i++) {
+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
+		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
+
+		/* No need to update region label for non cxl format */
+		if (!ndd->cxl)
+			continue;
+
+		/* Init labels to include region label */
+		rc = init_labels(nd_mapping, 1);
+
+		if (rc < 0)
+			return rc;
+
+		rc = __pmem_region_label_update(nd_region, nd_mapping, i,
+					NSLABEL_FLAG_UPDATING);
+
+		if (rc)
+			return rc;
+	}
+
+	/* Clear the UPDATING flag per UEFI 2.7 expectations */
+	for (i = 0; i < nd_region->ndr_mappings; i++) {
+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
+		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
+
+		/* No need to update region label for non cxl format */
+		if (!ndd->cxl)
+			continue;
+
+		rc = __pmem_region_label_update(nd_region, nd_mapping, i, 0);
+
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
 int __init nd_label_init(void)
 {
 	WARN_ON(guid_parse(NVDIMM_BTT_GUID, &nvdimm_btt_guid));
diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
index 4883b3a1320f..0f428695017d 100644
--- a/drivers/nvdimm/label.h
+++ b/drivers/nvdimm/label.h
@@ -190,6 +190,7 @@ struct nd_namespace_label {
 struct nd_lsa_label {
 	union {
 		struct nd_namespace_label ns_label;
+		struct cxl_region_label rg_label;
 	};
 };
 
@@ -233,4 +234,5 @@ struct nd_region;
 struct nd_namespace_pmem;
 int nd_pmem_namespace_label_update(struct nd_region *nd_region,
 		struct nd_namespace_pmem *nspm, resource_size_t size);
+int nd_pmem_region_label_update(struct nd_region *nd_region);
 #endif /* __LABEL_H__ */
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 5b73119dc8fd..02ae8162566c 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -232,6 +232,18 @@ static ssize_t __alt_name_store(struct device *dev, const char *buf,
 	return rc;
 }
 
+int nd_region_label_update(struct nd_region *nd_region)
+{
+	int rc;
+
+	nvdimm_bus_lock(&nd_region->dev);
+	rc = nd_pmem_region_label_update(nd_region);
+	nvdimm_bus_unlock(&nd_region->dev);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(nd_region_label_update);
+
 static int nd_namespace_label_update(struct nd_region *nd_region,
 		struct device *dev)
 {
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 651847f1bbf9..15d94e3937f0 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -322,6 +322,26 @@ static inline void nsl_set_region_uuid(struct nvdimm_drvdata *ndd,
 		export_uuid(ns_label->cxl.region_uuid, uuid);
 }
 
+static inline bool rgl_uuid_equal(struct cxl_region_label *rg_label,
+				  const uuid_t *uuid)
+{
+	uuid_t tmp;
+
+	import_uuid(&tmp, rg_label->uuid);
+	return uuid_equal(&tmp, uuid);
+}
+
+static inline u64 rgl_get_checksum(struct cxl_region_label *rg_label)
+{
+	return __le64_to_cpu(rg_label->checksum);
+}
+
+static inline void rgl_set_checksum(struct cxl_region_label *rg_label,
+				    u64 checksum)
+{
+	rg_label->checksum = __cpu_to_le64(checksum);
+}
+
 bool nsl_validate_type_guid(struct nvdimm_drvdata *ndd,
 			    struct nd_namespace_label *nd_label, guid_t *guid);
 enum nvdimm_claim_class nsl_get_claim_class(struct nvdimm_drvdata *ndd,
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 0a55900842c8..b06bd45373f4 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -115,6 +115,13 @@ struct nd_interleave_set {
 	u64 altcookie;
 
 	guid_t type_guid;
+
+	/* v2.1 region label info */
+	uuid_t uuid;
+	int interleave_ways;
+	int interleave_granularity;
+	struct resource *res;
+	int nr_targets;
 };
 
 struct nd_mapping_desc {
@@ -302,6 +309,7 @@ int nvdimm_has_flush(struct nd_region *nd_region);
 int nvdimm_has_cache(struct nd_region *nd_region);
 int nvdimm_in_overwrite(struct nvdimm *nvdimm);
 bool is_nvdimm_sync(struct nd_region *nd_region);
+int nd_region_label_update(struct nd_region *nd_region);
 
 static inline int nvdimm_ctl(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
 		unsigned int buf_len, int *cmd_rc)
-- 
2.34.1

RE Subject: [PATCH V2 05/20] nvdimm/region_label: Add region label updation routine
                                                                   ^^^^^^^
								   update

Neeraj Kumar wrote:
> Added __pmem_region_label_update region label update routine to update
  ^^^
  Add

> region label.

How about:

Add __pmem_region_label_update to update region labels.  ???

But is that really what this patch is doing?  And why do we need such a
function?

Why is __pmem_label_update changing?

> 
> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
> mutex_unlock()

Why?

I'm not full out naking the patch but I think its purpose needs to be
clear.

More below...

[snip]

>  static bool slot_valid(struct nvdimm_drvdata *ndd,
>  		struct nd_lsa_label *lsa_label, u32 slot)
>  {
> @@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  		return rc;
>  
>  	/* Garbage collect the previous label */
> -	mutex_lock(&nd_mapping->lock);
> +	guard(mutex)(&nd_mapping->lock);

This, and the following hunks, looks like a completely separate change and
should be in it's own pre-patch with a justification of the change.

>  	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>  		if (!label_ent->label)
>  			continue;
> @@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  	/* update index */
>  	rc = nd_label_write_index(ndd, ndd->ns_next,
>  			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
> -	if (rc == 0) {
> -		list_for_each_entry(label_ent, &nd_mapping->labels, list)
> -			if (!label_ent->label) {
> -				label_ent->label = lsa_label;
> -				lsa_label = NULL;
> -				break;
> -			}
> -		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
> -				"failed to track label: %d\n",
> -				to_slot(ndd, lsa_label));
> -		if (lsa_label)
> -			rc = -ENXIO;
> -	}
> -	mutex_unlock(&nd_mapping->lock);
> +	if (rc)
> +		return rc;
> +
> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
> +		if (!label_ent->label) {
> +			label_ent->label = lsa_label;
> +			lsa_label = NULL;
> +			break;
> +		}
> +	dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
> +			"failed to track label: %d\n",
> +			to_slot(ndd, lsa_label));
> +	if (lsa_label)
> +		rc = -ENXIO;
>  
>  	return rc;
>  }
> @@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  	return 0;
>  }
>  

[snip]

> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
> index 4883b3a1320f..0f428695017d 100644
> --- a/drivers/nvdimm/label.h
> +++ b/drivers/nvdimm/label.h
> @@ -190,6 +190,7 @@ struct nd_namespace_label {
>  struct nd_lsa_label {
>  	union {
>  		struct nd_namespace_label ns_label;
> +		struct cxl_region_label rg_label;

Why can't struct cxl_region_label be it's own structure?  Or just be part
of the nd_namespace_label union?

>  	};
>  };
>  
> @@ -233,4 +234,5 @@ struct nd_region;
>  struct nd_namespace_pmem;
>  int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  		struct nd_namespace_pmem *nspm, resource_size_t size);
> +int nd_pmem_region_label_update(struct nd_region *nd_region);
>  #endif /* __LABEL_H__ */
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 5b73119dc8fd..02ae8162566c 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -232,6 +232,18 @@ static ssize_t __alt_name_store(struct device *dev, const char *buf,
>  	return rc;
>  }
>  
> +int nd_region_label_update(struct nd_region *nd_region)

Is this called in a later patch?

Ira

[snip]

On 19/08/25 01:16PM, Ira Weiny wrote:
>RE Subject: [PATCH V2 05/20] nvdimm/region_label: Add region label updation routine
>                                                                   ^^^^^^^
>								   update

Sure, I will fix it in next patch-set

>
>Neeraj Kumar wrote:
>> Added __pmem_region_label_update region label update routine to update
>  ^^^
>  Add

Sure, I will fix it in next patch-set

>
>> region label.
>
>How about:
>
>Add __pmem_region_label_update to update region labels.  ???

May be I will re-use __pmem_label_update() for region label also.

>
>But is that really what this patch is doing?  And why do we need such a
>function?
>
>Why is __pmem_label_update changing?

__pmem_label_update() is changing because modification of mutex locking.
Yes, Its not really related hunk, So I will handle it in separate
patch-set.

>
>>
>> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
>> mutex_unlock()
>
>Why?

As per Jonathan's comment on V1 have modified it, and added it in commit
message. seems its not required in commit message. I will remove it

>
>I'm not full out naking the patch but I think its purpose needs to be
>clear.
>
>More below...
>
>[snip]
>
>>  static bool slot_valid(struct nvdimm_drvdata *ndd,
>>  		struct nd_lsa_label *lsa_label, u32 slot)
>>  {
>> @@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  		return rc;
>>
>>  	/* Garbage collect the previous label */
>> -	mutex_lock(&nd_mapping->lock);
>> +	guard(mutex)(&nd_mapping->lock);
>
>This, and the following hunks, looks like a completely separate change and
>should be in it's own pre-patch with a justification of the change.

Yes this hunk is not related, So I will create a separate patch for this change

>
>>  	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>>  		if (!label_ent->label)
>>  			continue;
>> @@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	/* update index */
>>  	rc = nd_label_write_index(ndd, ndd->ns_next,
>>  			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
>> -	if (rc == 0) {
>> -		list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> -			if (!label_ent->label) {
>> -				label_ent->label = lsa_label;
>> -				lsa_label = NULL;
>> -				break;
>> -			}
>> -		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
>> -				"failed to track label: %d\n",
>> -				to_slot(ndd, lsa_label));
>> -		if (lsa_label)
>> -			rc = -ENXIO;
>> -	}
>> -	mutex_unlock(&nd_mapping->lock);
>> +	if (rc)
>> +		return rc;
>> +
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> +		if (!label_ent->label) {
>> +			label_ent->label = lsa_label;
>> +			lsa_label = NULL;
>> +			break;
>> +		}
>> +	dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
>> +			"failed to track label: %d\n",
>> +			to_slot(ndd, lsa_label));
>> +	if (lsa_label)
>> +		rc = -ENXIO;
>>
>>  	return rc;
>>  }
>> @@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>>  	return 0;
>>  }
>>
>
>[snip]
>
>> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
>> index 4883b3a1320f..0f428695017d 100644
>> --- a/drivers/nvdimm/label.h
>> +++ b/drivers/nvdimm/label.h
>> @@ -190,6 +190,7 @@ struct nd_namespace_label {
>>  struct nd_lsa_label {
>>  	union {
>>  		struct nd_namespace_label ns_label;
>> +		struct cxl_region_label rg_label;
>
>Why can't struct cxl_region_label be it's own structure?  Or just be part
>of the nd_namespace_label union?

Thanks Ira for your suggestion. I will revisit this change and try using
region label handling separately instead of using union.

>
>>  	};
>>  };
>>
>> @@ -233,4 +234,5 @@ struct nd_region;
>>  struct nd_namespace_pmem;
>>  int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>>  		struct nd_namespace_pmem *nspm, resource_size_t size);
>> +int nd_pmem_region_label_update(struct nd_region *nd_region);
>>  #endif /* __LABEL_H__ */
>> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
>> index 5b73119dc8fd..02ae8162566c 100644
>> --- a/drivers/nvdimm/namespace_devs.c
>> +++ b/drivers/nvdimm/namespace_devs.c
>> @@ -232,6 +232,18 @@ static ssize_t __alt_name_store(struct device *dev, const char *buf,
>>  	return rc;
>>  }
>>
>> +int nd_region_label_update(struct nd_region *nd_region)
>
>Is this called in a later patch?
>
>Ira

Yes its called from patch 20 (cxl/core/pmem_region.c) by region_label_update_store()


Regards,
Neeraj

On 7/30/25 5:11 AM, Neeraj Kumar wrote:
<snip>

> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 651847f1bbf9..15d94e3937f0 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -322,6 +322,26 @@ static inline void nsl_set_region_uuid(struct nvdimm_drvdata *ndd,
>  		export_uuid(ns_label->cxl.region_uuid, uuid);
>  }
>  
> +static inline bool rgl_uuid_equal(struct cxl_region_label *rg_label,
> +				  const uuid_t *uuid)
> +{
> +	uuid_t tmp;
> +
> +	import_uuid(&tmp, rg_label->uuid);
> +	return uuid_equal(&tmp, uuid);

Why the extra copy via import_uuid() rather than directly compare rg_labe->uuid vs the uuid param?

DJ

> +}
> +
> +static inline u64 rgl_get_checksum(struct cxl_region_label *rg_label)
> +{
> +	return __le64_to_cpu(rg_label->checksum);
> +}
> +
> +static inline void rgl_set_checksum(struct cxl_region_label *rg_label,
> +				    u64 checksum)
> +{
> +	rg_label->checksum = __cpu_to_le64(checksum);
> +}
> +
>  bool nsl_validate_type_guid(struct nvdimm_drvdata *ndd,
>  			    struct nd_namespace_label *nd_label, guid_t *guid);
>  enum nvdimm_claim_class nsl_get_claim_class(struct nvdimm_drvdata *ndd,
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 0a55900842c8..b06bd45373f4 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -115,6 +115,13 @@ struct nd_interleave_set {
>  	u64 altcookie;
>  
>  	guid_t type_guid;
> +
> +	/* v2.1 region label info */
> +	uuid_t uuid;
> +	int interleave_ways;
> +	int interleave_granularity;
> +	struct resource *res;
> +	int nr_targets;
>  };
>  
>  struct nd_mapping_desc {
> @@ -302,6 +309,7 @@ int nvdimm_has_flush(struct nd_region *nd_region);
>  int nvdimm_has_cache(struct nd_region *nd_region);
>  int nvdimm_in_overwrite(struct nvdimm *nvdimm);
>  bool is_nvdimm_sync(struct nd_region *nd_region);
> +int nd_region_label_update(struct nd_region *nd_region);
>  
>  static inline int nvdimm_ctl(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>  		unsigned int buf_len, int *cmd_rc)

On 15/08/25 04:12PM, Dave Jiang wrote:
>
>
>On 7/30/25 5:11 AM, Neeraj Kumar wrote:
><snip>
>
>> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
>> index 651847f1bbf9..15d94e3937f0 100644
>> --- a/drivers/nvdimm/nd.h
>> +++ b/drivers/nvdimm/nd.h
>> @@ -322,6 +322,26 @@ static inline void nsl_set_region_uuid(struct nvdimm_drvdata *ndd,
>>  		export_uuid(ns_label->cxl.region_uuid, uuid);
>>  }
>>
>> +static inline bool rgl_uuid_equal(struct cxl_region_label *rg_label,
>> +				  const uuid_t *uuid)
>> +{
>> +	uuid_t tmp;
>> +
>> +	import_uuid(&tmp, rg_label->uuid);
>> +	return uuid_equal(&tmp, uuid);
>
>Why the extra copy via import_uuid() rather than directly compare rg_labe->uuid vs the uuid param?
>
>DJ

Thanks Dave for your suggestion. I have used because of import_uuid()
and uuid_equal() signature difference. Sure I will use uuid_equal()
directly using typecasting and will modify it in next patch-set.

Regards,
Neeraj

On 7/30/25 5:11 AM, Neeraj Kumar wrote:
> Added __pmem_region_label_update region label update routine to update
> region label.
> 
> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
> mutex_unlock()
> 
> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>

Subject, s/updation/update/ ?

> ---
>  drivers/nvdimm/label.c          | 171 +++++++++++++++++++++++++++++---
>  drivers/nvdimm/label.h          |   2 +
>  drivers/nvdimm/namespace_devs.c |  12 +++
>  drivers/nvdimm/nd.h             |  20 ++++
>  include/linux/libnvdimm.h       |   8 ++
>  5 files changed, 198 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
> index 3f8a6bdb77c7..94f2d0ba7aca 100644
> --- a/drivers/nvdimm/label.c
> +++ b/drivers/nvdimm/label.c
> @@ -381,6 +381,16 @@ static void nsl_calculate_checksum(struct nvdimm_drvdata *ndd,
>  	nsl_set_checksum(ndd, nd_label, sum);
>  }
>  
> +static void rgl_calculate_checksum(struct nvdimm_drvdata *ndd,

region_label_checksum()? rgl/rg is just a bit jarring to read even though I get nvdimm already has nd and nsl etc.

> +				   struct cxl_region_label *rg_label)

Prefer just spell out region_label

> +{
> +	u64 sum;
> +
> +	rgl_set_checksum(rg_label, 0);
> +	sum = nd_fletcher64(rg_label, sizeof_namespace_label(ndd), 1);
> +	rgl_set_checksum(rg_label, sum);
> +}
> +
>  static bool slot_valid(struct nvdimm_drvdata *ndd,
>  		struct nd_lsa_label *lsa_label, u32 slot)
>  {
> @@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  		return rc;
>  
>  	/* Garbage collect the previous label */
> -	mutex_lock(&nd_mapping->lock);
> +	guard(mutex)(&nd_mapping->lock);
>  	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>  		if (!label_ent->label)
>  			continue;
> @@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  	/* update index */
>  	rc = nd_label_write_index(ndd, ndd->ns_next,
>  			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
> -	if (rc == 0) {
> -		list_for_each_entry(label_ent, &nd_mapping->labels, list)
> -			if (!label_ent->label) {
> -				label_ent->label = lsa_label;
> -				lsa_label = NULL;
> -				break;
> -			}
> -		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
> -				"failed to track label: %d\n",
> -				to_slot(ndd, lsa_label));
> -		if (lsa_label)
> -			rc = -ENXIO;
> -	}
> -	mutex_unlock(&nd_mapping->lock);
> +	if (rc)
> +		return rc;
> +
> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
> +		if (!label_ent->label) {
> +			label_ent->label = lsa_label;
> +			lsa_label = NULL;
> +			break;
> +		}

Would've preferred the original code to look like:

list_for_each_entry(label_ent, &nd_mapping->labels, list) {
	if (label_ent->label)
		continue;

	label_ent->label = lsa_label;
	lsa_label = NULL;
	break;
}

But ah well....

> +	dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,> +			"failed to track label: %d\n",
> +			to_slot(ndd, lsa_label));
> +	if (lsa_label)
> +		rc = -ENXIO;

Just return here as Jonathan already mentioned. guard() helps with cleaning that up.

>  
>  	return rc;
>  }
> @@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  	return 0;
>  }
>  
> +static int __pmem_region_label_update(struct nd_region *nd_region,
> +		struct nd_mapping *nd_mapping, int pos, unsigned long flags)
> +{
> +	struct nd_interleave_set *nd_set = nd_region->nd_set;
> +	struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> +	struct nd_lsa_label *nd_label;
> +	struct cxl_region_label *rg_label;
> +	struct nd_namespace_index *nsindex;> +	struct nd_label_ent *label_ent;
> +	unsigned long *free;
> +	u32 nslot, slot;
> +	size_t offset;
> +	int rc;
> +	uuid_t tmp;

uuid instead of tmp would make the variable clearer to read

Also please arrange variable ordering in reverse xmas tree. 

> +
> +	if (!preamble_next(ndd, &nsindex, &free, &nslot))
> +		return -ENXIO;
> +
> +	/* allocate and write the label to the staging (next) index */
> +	slot = nd_label_alloc_slot(ndd);
> +	if (slot == UINT_MAX)
> +		return -ENXIO;
> +	dev_dbg(ndd->dev, "allocated: %d\n", slot);
> +
> +	nd_label = to_label(ndd, slot);
> +
> +	memset(nd_label, 0, sizeof_namespace_label(ndd));
> +	rg_label = &nd_label->rg_label;
> +
> +	/* Set Region Label Format identification UUID */
> +	uuid_parse(CXL_REGION_UUID, &tmp);
> +	export_uuid(nd_label->rg_label.type, &tmp);
> +
> +	/* Set Current Region Label UUID */
> +	export_uuid(nd_label->rg_label.uuid, &nd_set->uuid);
> +
> +	rg_label->flags = __cpu_to_le32(flags);
> +	rg_label->nlabel = __cpu_to_le16(nd_region->ndr_mappings);
> +	rg_label->position = __cpu_to_le16(pos);
> +	rg_label->dpa = __cpu_to_le64(nd_mapping->start);
> +	rg_label->rawsize = __cpu_to_le64(nd_mapping->size);
> +	rg_label->hpa = __cpu_to_le64(nd_set->res->start);
> +	rg_label->slot = __cpu_to_le32(slot);
> +	rg_label->ig = __cpu_to_le32(nd_set->interleave_granularity);
> +	rg_label->align = __cpu_to_le16(0);
> +
> +	/* Update fletcher64 Checksum */
> +	rgl_calculate_checksum(ndd, rg_label);
> +
> +	/* update label */
> +	offset = nd_label_offset(ndd, nd_label);
> +	rc = nvdimm_set_config_data(ndd, offset, nd_label,
> +			sizeof_namespace_label(ndd));
> +	if (rc < 0) {
> +		nd_label_free_slot(ndd, slot);
> +		return rc;
> +	}
> +
> +	/* Garbage collect the previous label */
> +	guard(mutex)(&nd_mapping->lock);
> +	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
> +		if (!label_ent->label)
> +			continue;
> +		if (rgl_uuid_equal(&label_ent->label->rg_label, &nd_set->uuid))
> +			reap_victim(nd_mapping, label_ent);
> +	}
> +
> +	/* update index */
> +	rc = nd_label_write_index(ndd, ndd->ns_next,
> +			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
> +	if (rc)
> +		return rc;
> +
> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
> +		if (!label_ent->label) {
> +			label_ent->label = nd_label;
> +			nd_label = NULL;
> +			break;
> +		}
> +	dev_WARN_ONCE(&nd_region->dev, nd_label,
> +			"failed to track label: %d\n",
> +			to_slot(ndd, nd_label));
> +	if (nd_label)
> +		rc = -ENXIO;
just return here

> +
> +	return rc;
> +}
> +
> +int nd_pmem_region_label_update(struct nd_region *nd_region)
> +{
> +	int i, rc;
> +
> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> +
> +		/* No need to update region label for non cxl format */
> +		if (!ndd->cxl)
> +			continue;

Would there be a mix of different nd mappings? I wonder if you can just 'return 0' if you find ndd->cxl on the first one and just skip everything.

> +
> +		/* Init labels to include region label */
> +		rc = init_labels(nd_mapping, 1);
> +
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i,
> +					NSLABEL_FLAG_UPDATING);
> +
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* Clear the UPDATING flag per UEFI 2.7 expectations */
> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> +
> +		/* No need to update region label for non cxl format */
> +		if (!ndd->cxl)
> +			continue;
> +> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i, 0);
> +
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  int __init nd_label_init(void)
>  {
>  	WARN_ON(guid_parse(NVDIMM_BTT_GUID, &nvdimm_btt_guid));
> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
> index 4883b3a1320f..0f428695017d 100644
> --- a/drivers/nvdimm/label.h
> +++ b/drivers/nvdimm/label.h
> @@ -190,6 +190,7 @@ struct nd_namespace_label {
>  struct nd_lsa_label {
>  	union {
>  		struct nd_namespace_label ns_label;
> +		struct cxl_region_label rg_label;
>  	};
>  };
>  
> @@ -233,4 +234,5 @@ struct nd_region;
>  struct nd_namespace_pmem;
>  int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  		struct nd_namespace_pmem *nspm, resource_size_t size);
> +int nd_pmem_region_label_update(struct nd_region *nd_region);
>  #endif /* __LABEL_H__ */
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 5b73119dc8fd..02ae8162566c 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -232,6 +232,18 @@ static ssize_t __alt_name_store(struct device *dev, const char *buf,
>  	return rc;
>  }
>  
> +int nd_region_label_update(struct nd_region *nd_region)
> +{
> +	int rc;
> +
> +	nvdimm_bus_lock(&nd_region->dev);
> +	rc = nd_pmem_region_label_update(nd_region);
> +	nvdimm_bus_unlock(&nd_region->dev);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(nd_region_label_update);
> +
>  static int nd_namespace_label_update(struct nd_region *nd_region,
>  		struct device *dev)
>  {
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 651847f1bbf9..15d94e3937f0 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -322,6 +322,26 @@ static inline void nsl_set_region_uuid(struct nvdimm_drvdata *ndd,
>  		export_uuid(ns_label->cxl.region_uuid, uuid);
>  }
>  
> +static inline bool rgl_uuid_equal(struct cxl_region_label *rg_label,
> +				  const uuid_t *uuid)

region_label_uuid_equal() and region_label

> +{
> +	uuid_t tmp;

tmp_uuid

> +
> +	import_uuid(&tmp, rg_label->uuid);
> +	return uuid_equal(&tmp, uuid);
> +}
> +
> +static inline u64 rgl_get_checksum(struct cxl_region_label *rg_label)

region_label_get_checksum()

> +{
> +	return __le64_to_cpu(rg_label->checksum);
> +}
> +
> +static inline void rgl_set_checksum(struct cxl_region_label *rg_label,
region_label_set_checksum()

> +				    u64 checksum)
> +{
> +	rg_label->checksum = __cpu_to_le64(checksum);
> +}
> +
>  bool nsl_validate_type_guid(struct nvdimm_drvdata *ndd,
>  			    struct nd_namespace_label *nd_label, guid_t *guid);
>  enum nvdimm_claim_class nsl_get_claim_class(struct nvdimm_drvdata *ndd,
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 0a55900842c8..b06bd45373f4 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -115,6 +115,13 @@ struct nd_interleave_set {
>  	u64 altcookie;
>  
>  	guid_t type_guid;
> +
> +	/* v2.1 region label info */
> +	uuid_t uuid;
> +	int interleave_ways;
> +	int interleave_granularity;
> +	struct resource *res;
> +	int nr_targets;
>  };
>  
>  struct nd_mapping_desc {
> @@ -302,6 +309,7 @@ int nvdimm_has_flush(struct nd_region *nd_region);
>  int nvdimm_has_cache(struct nd_region *nd_region);
>  int nvdimm_in_overwrite(struct nvdimm *nvdimm);
>  bool is_nvdimm_sync(struct nd_region *nd_region);
> +int nd_region_label_update(struct nd_region *nd_region);
>  
>  static inline int nvdimm_ctl(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>  		unsigned int buf_len, int *cmd_rc)

On 15/08/25 02:55PM, Dave Jiang wrote:
>
>
>On 7/30/25 5:11 AM, Neeraj Kumar wrote:
>> Added __pmem_region_label_update region label update routine to update
>> region label.
>>
>> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
>> mutex_unlock()
>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>
>Subject, s/updation/update/ ?

Thanks Dave, Sure. Will fix it in next patch-set

>
>> ---
>>  drivers/nvdimm/label.c          | 171 +++++++++++++++++++++++++++++---
>>  drivers/nvdimm/label.h          |   2 +
>>  drivers/nvdimm/namespace_devs.c |  12 +++
>>  drivers/nvdimm/nd.h             |  20 ++++
>>  include/linux/libnvdimm.h       |   8 ++
>>  5 files changed, 198 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
>> index 3f8a6bdb77c7..94f2d0ba7aca 100644
>> --- a/drivers/nvdimm/label.c
>> +++ b/drivers/nvdimm/label.c
>> @@ -381,6 +381,16 @@ static void nsl_calculate_checksum(struct nvdimm_drvdata *ndd,
>>  	nsl_set_checksum(ndd, nd_label, sum);
>>  }
>>
>> +static void rgl_calculate_checksum(struct nvdimm_drvdata *ndd,
>
>region_label_checksum()? rgl/rg is just a bit jarring to read even though I get nvdimm already has nd and nsl etc.
>
>> +				   struct cxl_region_label *rg_label)
>
>Prefer just spell out region_label

Sure Dave, I will fix it wherever its rgl/rg to region_label

>
>> +{
>> +	u64 sum;
>> +
>> +	rgl_set_checksum(rg_label, 0);
>> +	sum = nd_fletcher64(rg_label, sizeof_namespace_label(ndd), 1);
>> +	rgl_set_checksum(rg_label, sum);
>> +}
>> +
>>  static bool slot_valid(struct nvdimm_drvdata *ndd,
>>  		struct nd_lsa_label *lsa_label, u32 slot)
>>  {
>> @@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  		return rc;
>>
>>  	/* Garbage collect the previous label */
>> -	mutex_lock(&nd_mapping->lock);
>> +	guard(mutex)(&nd_mapping->lock);
>>  	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>>  		if (!label_ent->label)
>>  			continue;
>> @@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	/* update index */
>>  	rc = nd_label_write_index(ndd, ndd->ns_next,
>>  			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
>> -	if (rc == 0) {
>> -		list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> -			if (!label_ent->label) {
>> -				label_ent->label = lsa_label;
>> -				lsa_label = NULL;
>> -				break;
>> -			}
>> -		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
>> -				"failed to track label: %d\n",
>> -				to_slot(ndd, lsa_label));
>> -		if (lsa_label)
>> -			rc = -ENXIO;
>> -	}
>> -	mutex_unlock(&nd_mapping->lock);
>> +	if (rc)
>> +		return rc;
>> +
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> +		if (!label_ent->label) {
>> +			label_ent->label = lsa_label;
>> +			lsa_label = NULL;
>> +			break;
>> +		}
>
>Would've preferred the original code to look like:
>
>list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>	if (label_ent->label)
>		continue;
>
>	label_ent->label = lsa_label;
>	lsa_label = NULL;
>	break;
>}
>
>But ah well....

Thanks, I will fix it here

>
>> +	dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,> +			"failed to track label: %d\n",
>> +			to_slot(ndd, lsa_label));
>> +	if (lsa_label)
>> +		rc = -ENXIO;
>
>Just return here as Jonathan already mentioned. guard() helps with cleaning that up.

Sure, Will fix it in next patch-set

>
>>
>>  	return rc;
>>  }
>> @@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>>  	return 0;
>>  }
>>
>> +static int __pmem_region_label_update(struct nd_region *nd_region,
>> +		struct nd_mapping *nd_mapping, int pos, unsigned long flags)
>> +{
>> +	struct nd_interleave_set *nd_set = nd_region->nd_set;
>> +	struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> +	struct nd_lsa_label *nd_label;
>> +	struct cxl_region_label *rg_label;
>> +	struct nd_namespace_index *nsindex;> +	struct nd_label_ent *label_ent;
>> +	unsigned long *free;
>> +	u32 nslot, slot;
>> +	size_t offset;
>> +	int rc;
>> +	uuid_t tmp;
>
>uuid instead of tmp would make the variable clearer to read
>
>Also please arrange variable ordering in reverse xmas tree.

Sure Dave, I will fix it in next patch-set

>
>> +
>> +	if (!preamble_next(ndd, &nsindex, &free, &nslot))
>> +		return -ENXIO;
>> +
>> +	/* allocate and write the label to the staging (next) index */
>> +	slot = nd_label_alloc_slot(ndd);
>> +	if (slot == UINT_MAX)
>> +		return -ENXIO;
>> +	dev_dbg(ndd->dev, "allocated: %d\n", slot);
>> +
>> +	nd_label = to_label(ndd, slot);
>> +
>> +	memset(nd_label, 0, sizeof_namespace_label(ndd));
>> +	rg_label = &nd_label->rg_label;
>> +
>> +	/* Set Region Label Format identification UUID */
>> +	uuid_parse(CXL_REGION_UUID, &tmp);
>> +	export_uuid(nd_label->rg_label.type, &tmp);
>> +
>> +	/* Set Current Region Label UUID */
>> +	export_uuid(nd_label->rg_label.uuid, &nd_set->uuid);
>> +
>> +	rg_label->flags = __cpu_to_le32(flags);
>> +	rg_label->nlabel = __cpu_to_le16(nd_region->ndr_mappings);
>> +	rg_label->position = __cpu_to_le16(pos);
>> +	rg_label->dpa = __cpu_to_le64(nd_mapping->start);
>> +	rg_label->rawsize = __cpu_to_le64(nd_mapping->size);
>> +	rg_label->hpa = __cpu_to_le64(nd_set->res->start);
>> +	rg_label->slot = __cpu_to_le32(slot);
>> +	rg_label->ig = __cpu_to_le32(nd_set->interleave_granularity);
>> +	rg_label->align = __cpu_to_le16(0);
>> +
>> +	/* Update fletcher64 Checksum */
>> +	rgl_calculate_checksum(ndd, rg_label);
>> +
>> +	/* update label */
>> +	offset = nd_label_offset(ndd, nd_label);
>> +	rc = nvdimm_set_config_data(ndd, offset, nd_label,
>> +			sizeof_namespace_label(ndd));
>> +	if (rc < 0) {
>> +		nd_label_free_slot(ndd, slot);
>> +		return rc;
>> +	}
>> +
>> +	/* Garbage collect the previous label */
>> +	guard(mutex)(&nd_mapping->lock);
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>> +		if (!label_ent->label)
>> +			continue;
>> +		if (rgl_uuid_equal(&label_ent->label->rg_label, &nd_set->uuid))
>> +			reap_victim(nd_mapping, label_ent);
>> +	}
>> +
>> +	/* update index */
>> +	rc = nd_label_write_index(ndd, ndd->ns_next,
>> +			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
>> +	if (rc)
>> +		return rc;
>> +
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> +		if (!label_ent->label) {
>> +			label_ent->label = nd_label;
>> +			nd_label = NULL;
>> +			break;
>> +		}
>> +	dev_WARN_ONCE(&nd_region->dev, nd_label,
>> +			"failed to track label: %d\n",
>> +			to_slot(ndd, nd_label));
>> +	if (nd_label)
>> +		rc = -ENXIO;
>just return here

Sure, Will fix it

>
>> +
>> +	return rc;
>> +}
>> +
>> +int nd_pmem_region_label_update(struct nd_region *nd_region)
>> +{
>> +	int i, rc;
>> +
>> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
>> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> +
>> +		/* No need to update region label for non cxl format */
>> +		if (!ndd->cxl)
>> +			continue;
>
>Would there be a mix of different nd mappings? I wonder if you can just 'return 0' if you find ndd->cxl on the first one and just skip everything.

When we create cxl region with two mem device, then we will have two separate
nd_mapping for both mem devices. But Yes, I don't see difference in both device
nd_mapping characters. So instead of "continue", I will just "return 0".

>
>> +
>> +		/* Init labels to include region label */
>> +		rc = init_labels(nd_mapping, 1);
>> +
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i,
>> +					NSLABEL_FLAG_UPDATING);
>> +
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	/* Clear the UPDATING flag per UEFI 2.7 expectations */
>> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
>> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> +
>> +		/* No need to update region label for non cxl format */
>> +		if (!ndd->cxl)
>> +			continue;
>> +> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i, 0);
>> +
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  int __init nd_label_init(void)
>>  {
>>  	WARN_ON(guid_parse(NVDIMM_BTT_GUID, &nvdimm_btt_guid));
>> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
>> index 4883b3a1320f..0f428695017d 100644
>> --- a/drivers/nvdimm/label.h
>> +++ b/drivers/nvdimm/label.h
>> @@ -190,6 +190,7 @@ struct nd_namespace_label {
>>  struct nd_lsa_label {
>>  	union {
>>  		struct nd_namespace_label ns_label;
>> +		struct cxl_region_label rg_label;
>>  	};
>>  };
>>
>> @@ -233,4 +234,5 @@ struct nd_region;
>>  struct nd_namespace_pmem;
>>  int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>>  		struct nd_namespace_pmem *nspm, resource_size_t size);
>> +int nd_pmem_region_label_update(struct nd_region *nd_region);
>>  #endif /* __LABEL_H__ */
>> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
>> index 5b73119dc8fd..02ae8162566c 100644
>> --- a/drivers/nvdimm/namespace_devs.c
>> +++ b/drivers/nvdimm/namespace_devs.c
>> @@ -232,6 +232,18 @@ static ssize_t __alt_name_store(struct device *dev, const char *buf,
>>  	return rc;
>>  }
>>
>> +int nd_region_label_update(struct nd_region *nd_region)
>> +{
>> +	int rc;
>> +
>> +	nvdimm_bus_lock(&nd_region->dev);
>> +	rc = nd_pmem_region_label_update(nd_region);
>> +	nvdimm_bus_unlock(&nd_region->dev);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(nd_region_label_update);
>> +
>>  static int nd_namespace_label_update(struct nd_region *nd_region,
>>  		struct device *dev)
>>  {
>> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
>> index 651847f1bbf9..15d94e3937f0 100644
>> --- a/drivers/nvdimm/nd.h
>> +++ b/drivers/nvdimm/nd.h
>> @@ -322,6 +322,26 @@ static inline void nsl_set_region_uuid(struct nvdimm_drvdata *ndd,
>>  		export_uuid(ns_label->cxl.region_uuid, uuid);
>>  }
>>
>> +static inline bool rgl_uuid_equal(struct cxl_region_label *rg_label,
>> +				  const uuid_t *uuid)
>
>region_label_uuid_equal() and region_label
>
>> +{
>> +	uuid_t tmp;
>
>tmp_uuid
>
>> +
>> +	import_uuid(&tmp, rg_label->uuid);
>> +	return uuid_equal(&tmp, uuid);
>> +}
>> +
>> +static inline u64 rgl_get_checksum(struct cxl_region_label *rg_label)
>
>region_label_get_checksum()
>
>> +{
>> +	return __le64_to_cpu(rg_label->checksum);
>> +}
>> +
>> +static inline void rgl_set_checksum(struct cxl_region_label *rg_label,
>region_label_set_checksum()

Sure, I will fix them in next patch-set


Regards,
Neeraj

On Thu, 4 Sep 2025 19:42:31 +0530
Neeraj Kumar <s.neeraj@samsung.com> wrote:

> On 15/08/25 02:55PM, Dave Jiang wrote:
> >
> >
> >On 7/30/25 5:11 AM, Neeraj Kumar wrote:  
> >> Added __pmem_region_label_update region label update routine to update
> >> region label.
> >>
> >> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
> >> mutex_unlock()
> >>
> >> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>  
> >
> >Subject, s/updation/update/ ?  
> 
> Thanks Dave, Sure. Will fix it in next patch-set
> 

Hi Neeraj,
 
Really small process point.  We all get too many emails, so
when replying crop out anything that doesn't need more discussion.
Where you are just saying you agree, leave that for the change logs of the
next version (and appropriate thanks can go there as well).

I know not replying can feel little rude, but trust me when I say all
or almost all who review a lot appreciate efficiency!

It also makes it a lot harder to miss the more substantial replies.

> >> +int nd_pmem_region_label_update(struct nd_region *nd_region)
> >> +{
> >> +	int i, rc;
> >> +
> >> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> >> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> >> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> >> +
> >> +		/* No need to update region label for non cxl format */
> >> +		if (!ndd->cxl)
> >> +			continue;  
> >
> >Would there be a mix of different nd mappings? I wonder if you can just 'return 0' if you find ndd->cxl on the first one and just skip everything.  
> 
> When we create cxl region with two mem device, then we will have two separate
> nd_mapping for both mem devices. But Yes, I don't see difference in both device
> nd_mapping characters. So instead of "continue", I will just "return 0".
> 

This for instance is good discussion and well worth the reply!

Good discussion is great, but reducing the noise is key to keeping
things manageable.

Jonathan

p.s. I'm having a full day or reviewing code so getting a little grumpier
than I was first thing this morning when I might not have given this feedback!

On Wed, 30 Jul 2025 17:41:54 +0530
Neeraj Kumar <s.neeraj@samsung.com> wrote:

> Added __pmem_region_label_update region label update routine to update
> region label.
> 
> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
> mutex_unlock()
> 
> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>

A few comments inline,

Thanks,

Jonathan


>  static bool slot_valid(struct nvdimm_drvdata *ndd,
>  		struct nd_lsa_label *lsa_label, u32 slot)
>  {
> @@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  		return rc;
>  
>  	/* Garbage collect the previous label */
> -	mutex_lock(&nd_mapping->lock);
> +	guard(mutex)(&nd_mapping->lock);
>  	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>  		if (!label_ent->label)
>  			continue;
> @@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  	/* update index */
>  	rc = nd_label_write_index(ndd, ndd->ns_next,
>  			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
> -	if (rc == 0) {
> -		list_for_each_entry(label_ent, &nd_mapping->labels, list)
> -			if (!label_ent->label) {
> -				label_ent->label = lsa_label;
> -				lsa_label = NULL;
> -				break;
> -			}
> -		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
> -				"failed to track label: %d\n",
> -				to_slot(ndd, lsa_label));
> -		if (lsa_label)
> -			rc = -ENXIO;
> -	}
> -	mutex_unlock(&nd_mapping->lock);
> +	if (rc)
> +		return rc;
> +
> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
> +		if (!label_ent->label) {
> +			label_ent->label = lsa_label;
> +			lsa_label = NULL;
> +			break;
> +		}
> +	dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
> +			"failed to track label: %d\n",
> +			to_slot(ndd, lsa_label));
> +	if (lsa_label)
> +		rc = -ENXIO;
	if (lsa_label)
		return -ENXIO;

	return 0;

is a little clearer.

>  
>  	return rc;
>  }
> @@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  	return 0;
>  }
>  
> +static int __pmem_region_label_update(struct nd_region *nd_region,
> +		struct nd_mapping *nd_mapping, int pos, unsigned long flags)
> +{
> +	struct nd_interleave_set *nd_set = nd_region->nd_set;
> +	struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> +	struct nd_lsa_label *nd_label;
> +	struct cxl_region_label *rg_label;
> +	struct nd_namespace_index *nsindex;
> +	struct nd_label_ent *label_ent;
> +	unsigned long *free;
> +	u32 nslot, slot;
> +	size_t offset;
> +	int rc;
> +	uuid_t tmp;
> +
> +	if (!preamble_next(ndd, &nsindex, &free, &nslot))
> +		return -ENXIO;
> +
> +	/* allocate and write the label to the staging (next) index */
> +	slot = nd_label_alloc_slot(ndd);
> +	if (slot == UINT_MAX)
> +		return -ENXIO;
> +	dev_dbg(ndd->dev, "allocated: %d\n", slot);
> +
> +	nd_label = to_label(ndd, slot);
> +
> +	memset(nd_label, 0, sizeof_namespace_label(ndd));
> +	rg_label = &nd_label->rg_label;
> +
> +	/* Set Region Label Format identification UUID */
> +	uuid_parse(CXL_REGION_UUID, &tmp);
> +	export_uuid(nd_label->rg_label.type, &tmp);

	export_uuid(rg_label->type, &tmp);

> +
> +	/* Set Current Region Label UUID */
> +	export_uuid(nd_label->rg_label.uuid, &nd_set->uuid);

	export_uuid(rg_label->uuid, &nd_set->uuid);


> +
> +	rg_label->flags = __cpu_to_le32(flags);
> +	rg_label->nlabel = __cpu_to_le16(nd_region->ndr_mappings);
> +	rg_label->position = __cpu_to_le16(pos);
> +	rg_label->dpa = __cpu_to_le64(nd_mapping->start);
> +	rg_label->rawsize = __cpu_to_le64(nd_mapping->size);
> +	rg_label->hpa = __cpu_to_le64(nd_set->res->start);
> +	rg_label->slot = __cpu_to_le32(slot);
> +	rg_label->ig = __cpu_to_le32(nd_set->interleave_granularity);
> +	rg_label->align = __cpu_to_le16(0);

As the bot complained... It's le32

> +
> +	/* Update fletcher64 Checksum */
> +	rgl_calculate_checksum(ndd, rg_label);
> +
> +	/* update label */
> +	offset = nd_label_offset(ndd, nd_label);
> +	rc = nvdimm_set_config_data(ndd, offset, nd_label,
> +			sizeof_namespace_label(ndd));
> +	if (rc < 0) {
> +		nd_label_free_slot(ndd, slot);
> +		return rc;
> +	}
> +
> +	/* Garbage collect the previous label */
> +	guard(mutex)(&nd_mapping->lock);
> +	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
> +		if (!label_ent->label)
> +			continue;
> +		if (rgl_uuid_equal(&label_ent->label->rg_label, &nd_set->uuid))
> +			reap_victim(nd_mapping, label_ent);
> +	}
> +
> +	/* update index */
> +	rc = nd_label_write_index(ndd, ndd->ns_next,
> +			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
> +	if (rc)
> +		return rc;
> +
> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
> +		if (!label_ent->label) {
> +			label_ent->label = nd_label;
> +			nd_label = NULL;
> +			break;
> +		}
> +	dev_WARN_ONCE(&nd_region->dev, nd_label,
> +			"failed to track label: %d\n",
> +			to_slot(ndd, nd_label));
> +	if (nd_label)
> +		rc = -ENXIO;

		return -ENXIO;

> +

	return 0;

is clearer.

> +	return rc;
> +}
> +
> +int nd_pmem_region_label_update(struct nd_region *nd_region)
> +{
> +	int i, rc;
> +
> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> +
> +		/* No need to update region label for non cxl format */
> +		if (!ndd->cxl)
> +			continue;
> +
> +		/* Init labels to include region label */
> +		rc = init_labels(nd_mapping, 1);
> +

No blank line here - keep the error check closely associated with the
thing that it is checking.

> +		if (rc < 0)
> +			return rc;
> +
> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i,
> +					NSLABEL_FLAG_UPDATING);
> +

Same here.

> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* Clear the UPDATING flag per UEFI 2.7 expectations */
> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> +
> +		/* No need to update region label for non cxl format */
> +		if (!ndd->cxl)
> +			continue;
> +
> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i, 0);
> +

and here.

> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  int __init nd_label_init(void)
>  {
>  	WARN_ON(guid_parse(NVDIMM_BTT_GUID, &nvdimm_btt_guid));
> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
> index 4883b3a1320f..0f428695017d 100644
> --- a/drivers/nvdimm/label.h
> +++ b/drivers/nvdimm/label.h
> @@ -190,6 +190,7 @@ struct nd_namespace_label {
>  struct nd_lsa_label {

Would be better to have this explicitly as a union
unless later patches add more elements.  That way it'll 
be obvious at all sites where it is used that it can be one
of several things.

>  	union {
>  		struct nd_namespace_label ns_label;
> +		struct cxl_region_label rg_label;
>  	};
>  };
>  
> @@ -233,4 +234,5 @@ struct nd_region;
>  struct nd_namespace_pmem;
>  int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  		struct nd_namespace_pmem *nspm, resource_size_t size);
> +int nd_pmem_region_label_update(struct nd_region *nd_region);
>  #endif /* __LABEL_H__ */

On 13/08/25 03:48PM, Jonathan Cameron wrote:
>On Wed, 30 Jul 2025 17:41:54 +0530
>Neeraj Kumar <s.neeraj@samsung.com> wrote:
>
>> Added __pmem_region_label_update region label update routine to update
>> region label.
>>
>> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
>> mutex_unlock()
>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>
>A few comments inline,
>
>Thanks,
>
>Jonathan
>
>
>>  static bool slot_valid(struct nvdimm_drvdata *ndd,
>>  		struct nd_lsa_label *lsa_label, u32 slot)
>>  {
>> @@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  		return rc;
>>
>>  	/* Garbage collect the previous label */
>> -	mutex_lock(&nd_mapping->lock);
>> +	guard(mutex)(&nd_mapping->lock);
>>  	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>>  		if (!label_ent->label)
>>  			continue;
>> @@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	/* update index */
>>  	rc = nd_label_write_index(ndd, ndd->ns_next,
>>  			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
>> -	if (rc == 0) {
>> -		list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> -			if (!label_ent->label) {
>> -				label_ent->label = lsa_label;
>> -				lsa_label = NULL;
>> -				break;
>> -			}
>> -		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
>> -				"failed to track label: %d\n",
>> -				to_slot(ndd, lsa_label));
>> -		if (lsa_label)
>> -			rc = -ENXIO;
>> -	}
>> -	mutex_unlock(&nd_mapping->lock);
>> +	if (rc)
>> +		return rc;
>> +
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> +		if (!label_ent->label) {
>> +			label_ent->label = lsa_label;
>> +			lsa_label = NULL;
>> +			break;
>> +		}
>> +	dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
>> +			"failed to track label: %d\n",
>> +			to_slot(ndd, lsa_label));
>> +	if (lsa_label)
>> +		rc = -ENXIO;
>	if (lsa_label)
>		return -ENXIO;
>
>	return 0;
>
>is a little clearer.

Sure, I will fix it in next patch-set

>
>>
>>  	return rc;
>>  }
>> @@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>>  	return 0;
>>  }
>>
>> +static int __pmem_region_label_update(struct nd_region *nd_region,
>> +		struct nd_mapping *nd_mapping, int pos, unsigned long flags)
>> +{
>> +	struct nd_interleave_set *nd_set = nd_region->nd_set;
>> +	struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> +	struct nd_lsa_label *nd_label;
>> +	struct cxl_region_label *rg_label;
>> +	struct nd_namespace_index *nsindex;
>> +	struct nd_label_ent *label_ent;
>> +	unsigned long *free;
>> +	u32 nslot, slot;
>> +	size_t offset;
>> +	int rc;
>> +	uuid_t tmp;
>> +
>> +	if (!preamble_next(ndd, &nsindex, &free, &nslot))
>> +		return -ENXIO;
>> +
>> +	/* allocate and write the label to the staging (next) index */
>> +	slot = nd_label_alloc_slot(ndd);
>> +	if (slot == UINT_MAX)
>> +		return -ENXIO;
>> +	dev_dbg(ndd->dev, "allocated: %d\n", slot);
>> +
>> +	nd_label = to_label(ndd, slot);
>> +
>> +	memset(nd_label, 0, sizeof_namespace_label(ndd));
>> +	rg_label = &nd_label->rg_label;
>> +
>> +	/* Set Region Label Format identification UUID */
>> +	uuid_parse(CXL_REGION_UUID, &tmp);
>> +	export_uuid(nd_label->rg_label.type, &tmp);
>
>	export_uuid(rg_label->type, &tmp);

Thanks Jonathan, I will fix it in next patch-set

>
>> +
>> +	/* Set Current Region Label UUID */
>> +	export_uuid(nd_label->rg_label.uuid, &nd_set->uuid);
>
>	export_uuid(rg_label->uuid, &nd_set->uuid);

Sure, I will fix it in next patch-set

>
>
>> +
>> +	rg_label->flags = __cpu_to_le32(flags);
>> +	rg_label->nlabel = __cpu_to_le16(nd_region->ndr_mappings);
>> +	rg_label->position = __cpu_to_le16(pos);
>> +	rg_label->dpa = __cpu_to_le64(nd_mapping->start);
>> +	rg_label->rawsize = __cpu_to_le64(nd_mapping->size);
>> +	rg_label->hpa = __cpu_to_le64(nd_set->res->start);
>> +	rg_label->slot = __cpu_to_le32(slot);
>> +	rg_label->ig = __cpu_to_le32(nd_set->interleave_granularity);
>> +	rg_label->align = __cpu_to_le16(0);
>
>As the bot complained... It's le32

Yes, I will fix it in next patch-set

>
>> +
>> +	/* Update fletcher64 Checksum */
>> +	rgl_calculate_checksum(ndd, rg_label);
>> +
>> +	/* update label */
>> +	offset = nd_label_offset(ndd, nd_label);
>> +	rc = nvdimm_set_config_data(ndd, offset, nd_label,
>> +			sizeof_namespace_label(ndd));
>> +	if (rc < 0) {
>> +		nd_label_free_slot(ndd, slot);
>> +		return rc;
>> +	}
>> +
>> +	/* Garbage collect the previous label */
>> +	guard(mutex)(&nd_mapping->lock);
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>> +		if (!label_ent->label)
>> +			continue;
>> +		if (rgl_uuid_equal(&label_ent->label->rg_label, &nd_set->uuid))
>> +			reap_victim(nd_mapping, label_ent);
>> +	}
>> +
>> +	/* update index */
>> +	rc = nd_label_write_index(ndd, ndd->ns_next,
>> +			nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
>> +	if (rc)
>> +		return rc;
>> +
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> +		if (!label_ent->label) {
>> +			label_ent->label = nd_label;
>> +			nd_label = NULL;
>> +			break;
>> +		}
>> +	dev_WARN_ONCE(&nd_region->dev, nd_label,
>> +			"failed to track label: %d\n",
>> +			to_slot(ndd, nd_label));
>> +	if (nd_label)
>> +		rc = -ENXIO;
>
>		return -ENXIO;
>
>> +
>
>	return 0;
>
>is clearer.

Sure, I will fix it in next patch-set

>
>> +	return rc;
>> +}
>> +
>> +int nd_pmem_region_label_update(struct nd_region *nd_region)
>> +{
>> +	int i, rc;
>> +
>> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
>> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> +
>> +		/* No need to update region label for non cxl format */
>> +		if (!ndd->cxl)
>> +			continue;
>> +
>> +		/* Init labels to include region label */
>> +		rc = init_labels(nd_mapping, 1);
>> +
>
>No blank line here - keep the error check closely associated with the
>thing that it is checking.

Sure, I will fix it in next patch-set

>
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i,
>> +					NSLABEL_FLAG_UPDATING);
>> +
>
>Same here.

Sure, I will fix it in next patch-set

>
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	/* Clear the UPDATING flag per UEFI 2.7 expectations */
>> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
>> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> +
>> +		/* No need to update region label for non cxl format */
>> +		if (!ndd->cxl)
>> +			continue;
>> +
>> +		rc = __pmem_region_label_update(nd_region, nd_mapping, i, 0);
>> +
>
>and here.

Sure, I will fix it in next patch-set

>
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  int __init nd_label_init(void)
>>  {
>>  	WARN_ON(guid_parse(NVDIMM_BTT_GUID, &nvdimm_btt_guid));
>> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
>> index 4883b3a1320f..0f428695017d 100644
>> --- a/drivers/nvdimm/label.h
>> +++ b/drivers/nvdimm/label.h
>> @@ -190,6 +190,7 @@ struct nd_namespace_label {
>>  struct nd_lsa_label {
>
>Would be better to have this explicitly as a union
>unless later patches add more elements.  That way it'll
>be obvious at all sites where it is used that it can be one
>of several things.
>
>>  	union {
>>  		struct nd_namespace_label ns_label;
>> +		struct cxl_region_label rg_label;
>>  	};
>>  };

Thanks Jonathan for your suggestion. I will revisit this change and try
using region label handling separately instead of using union.


Regards,
Neeraj

Hi Neeraj,

kernel test robot noticed the following build warnings:

[auto build test WARNING on f11a5f89910a7ae970fbce4fdc02d86a8ba8570f]

url:    https://github.com/intel-lab-lkp/linux/commits/Neeraj-Kumar/nvdimm-label-Introduce-NDD_CXL_LABEL-flag-to-set-cxl-label-format/20250730-202209
base:   f11a5f89910a7ae970fbce4fdc02d86a8ba8570f
patch link:    https://lore.kernel.org/r/20250730121209.303202-6-s.neeraj%40samsung.com
patch subject: [PATCH V2 05/20] nvdimm/region_label: Add region label updation routine
config: x86_64-randconfig-121-20250731 (https://download.01.org/0day-ci/archive/20250731/202507312211.ovRqDhYY-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250731/202507312211.ovRqDhYY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507312211.ovRqDhYY-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/nvdimm/label.c:1184:25: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] align @@     got restricted __le16 [usertype] @@
   drivers/nvdimm/label.c:1184:25: sparse:     expected restricted __le32 [usertype] align
   drivers/nvdimm/label.c:1184:25: sparse:     got restricted __le16 [usertype]
   drivers/nvdimm/label.c: note: in included file (through drivers/nvdimm/nd-core.h):
   drivers/nvdimm/nd.h:314:37: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] align @@     got restricted __le16 [usertype] @@
   drivers/nvdimm/nd.h:314:37: sparse:     expected restricted __le32 [usertype] align
   drivers/nvdimm/nd.h:314:37: sparse:     got restricted __le16 [usertype]

vim +1184 drivers/nvdimm/label.c

  1139	
  1140	static int __pmem_region_label_update(struct nd_region *nd_region,
  1141			struct nd_mapping *nd_mapping, int pos, unsigned long flags)
  1142	{
  1143		struct nd_interleave_set *nd_set = nd_region->nd_set;
  1144		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
  1145		struct nd_lsa_label *nd_label;
  1146		struct cxl_region_label *rg_label;
  1147		struct nd_namespace_index *nsindex;
  1148		struct nd_label_ent *label_ent;
  1149		unsigned long *free;
  1150		u32 nslot, slot;
  1151		size_t offset;
  1152		int rc;
  1153		uuid_t tmp;
  1154	
  1155		if (!preamble_next(ndd, &nsindex, &free, &nslot))
  1156			return -ENXIO;
  1157	
  1158		/* allocate and write the label to the staging (next) index */
  1159		slot = nd_label_alloc_slot(ndd);
  1160		if (slot == UINT_MAX)
  1161			return -ENXIO;
  1162		dev_dbg(ndd->dev, "allocated: %d\n", slot);
  1163	
  1164		nd_label = to_label(ndd, slot);
  1165	
  1166		memset(nd_label, 0, sizeof_namespace_label(ndd));
  1167		rg_label = &nd_label->rg_label;
  1168	
  1169		/* Set Region Label Format identification UUID */
  1170		uuid_parse(CXL_REGION_UUID, &tmp);
  1171		export_uuid(nd_label->rg_label.type, &tmp);
  1172	
  1173		/* Set Current Region Label UUID */
  1174		export_uuid(nd_label->rg_label.uuid, &nd_set->uuid);
  1175	
  1176		rg_label->flags = __cpu_to_le32(flags);
  1177		rg_label->nlabel = __cpu_to_le16(nd_region->ndr_mappings);
  1178		rg_label->position = __cpu_to_le16(pos);
  1179		rg_label->dpa = __cpu_to_le64(nd_mapping->start);
  1180		rg_label->rawsize = __cpu_to_le64(nd_mapping->size);
  1181		rg_label->hpa = __cpu_to_le64(nd_set->res->start);
  1182		rg_label->slot = __cpu_to_le32(slot);
  1183		rg_label->ig = __cpu_to_le32(nd_set->interleave_granularity);
> 1184		rg_label->align = __cpu_to_le16(0);
  1185	
  1186		/* Update fletcher64 Checksum */
  1187		rgl_calculate_checksum(ndd, rg_label);
  1188	
  1189		/* update label */
  1190		offset = nd_label_offset(ndd, nd_label);
  1191		rc = nvdimm_set_config_data(ndd, offset, nd_label,
  1192				sizeof_namespace_label(ndd));
  1193		if (rc < 0) {
  1194			nd_label_free_slot(ndd, slot);
  1195			return rc;
  1196		}
  1197	
  1198		/* Garbage collect the previous label */
  1199		guard(mutex)(&nd_mapping->lock);
  1200		list_for_each_entry(label_ent, &nd_mapping->labels, list) {
  1201			if (!label_ent->label)
  1202				continue;
  1203			if (rgl_uuid_equal(&label_ent->label->rg_label, &nd_set->uuid))
  1204				reap_victim(nd_mapping, label_ent);
  1205		}
  1206	
  1207		/* update index */
  1208		rc = nd_label_write_index(ndd, ndd->ns_next,
  1209				nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
  1210		if (rc)
  1211			return rc;
  1212	
  1213		list_for_each_entry(label_ent, &nd_mapping->labels, list)
  1214			if (!label_ent->label) {
  1215				label_ent->label = nd_label;
  1216				nd_label = NULL;
  1217				break;
  1218			}
  1219		dev_WARN_ONCE(&nd_region->dev, nd_label,
  1220				"failed to track label: %d\n",
  1221				to_slot(ndd, nd_label));
  1222		if (nd_label)
  1223			rc = -ENXIO;
  1224	
  1225		return rc;
  1226	}
  1227	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki