nd_mapping->labels maintains the list of labels present into LSA.
init_labels() prepares this list while adding new label into LSA
and updates nd_mapping->labels accordingly. During cxl region
creation nd_mapping->labels list and LSA was updated with one
region label. Therefore during new namespace label creation
pre-include the previously created region label, so increase
num_labels count by 1.

Also updated nsl_set_region_uuid with region uuid with which
namespace is associated with.

Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
---
 drivers/nvdimm/label.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
index be18278d6cea..fd02b557612e 100644
--- a/drivers/nvdimm/label.c
+++ b/drivers/nvdimm/label.c
@@ -957,7 +957,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
 	nsl_set_slot(ndd, nd_label, slot);
 	nsl_set_alignment(ndd, nd_label, 0);
 	nsl_set_type_guid(ndd, nd_label, &nd_set->type_guid);
-	nsl_set_region_uuid(ndd, nd_label, NULL);
+	nsl_set_region_uuid(ndd, nd_label, &nd_set->uuid);
 	nsl_set_claim_class(ndd, nd_label, ndns->claim_class);
 	nsl_calculate_checksum(ndd, nd_label);
 	nd_dbg_dpa(nd_region, ndd, res, "\n");
@@ -1129,7 +1129,8 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
 				count++;
 		WARN_ON_ONCE(!count);
 
-		rc = init_labels(nd_mapping, count);
+		/* Adding 1 to pre include the already added region label */
+		rc = init_labels(nd_mapping, count + 1);
 		if (rc < 0)
 			return rc;
 
-- 
2.34.1

Neeraj Kumar wrote:
> nd_mapping->labels maintains the list of labels present into LSA.
> init_labels() prepares this list while adding new label into LSA
> and updates nd_mapping->labels accordingly. During cxl region
> creation nd_mapping->labels list and LSA was updated with one
> region label. Therefore during new namespace label creation
> pre-include the previously created region label, so increase
> num_labels count by 1.

Why does the count of the labels in the list not work?

static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
{
        int i, old_num_labels = 0;
...
        mutex_lock(&nd_mapping->lock);
        list_for_each_entry(label_ent, &nd_mapping->labels, list)
                old_num_labels++;
        mutex_unlock(&nd_mapping->lock);
...

> 
> Also updated nsl_set_region_uuid with region uuid with which
> namespace is associated with.

Whenever using a word like 'Also' in the commit message ask if this should be a
separate patch.  I'm thinking this hunk should be somewhere else in the series.

Ira

> 
> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
> ---
>  drivers/nvdimm/label.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
> index be18278d6cea..fd02b557612e 100644
> --- a/drivers/nvdimm/label.c
> +++ b/drivers/nvdimm/label.c
> @@ -957,7 +957,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  	nsl_set_slot(ndd, nd_label, slot);
>  	nsl_set_alignment(ndd, nd_label, 0);
>  	nsl_set_type_guid(ndd, nd_label, &nd_set->type_guid);
> -	nsl_set_region_uuid(ndd, nd_label, NULL);
> +	nsl_set_region_uuid(ndd, nd_label, &nd_set->uuid);
>  	nsl_set_claim_class(ndd, nd_label, ndns->claim_class);
>  	nsl_calculate_checksum(ndd, nd_label);
>  	nd_dbg_dpa(nd_region, ndd, res, "\n");
> @@ -1129,7 +1129,8 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  				count++;
>  		WARN_ON_ONCE(!count);
>  
> -		rc = init_labels(nd_mapping, count);
> +		/* Adding 1 to pre include the already added region label */
> +		rc = init_labels(nd_mapping, count + 1);
>  		if (rc < 0)
>  			return rc;
>  
> -- 
> 2.34.1
> 
>

On 19/08/25 01:56PM, Ira Weiny wrote:
>Neeraj Kumar wrote:
>> nd_mapping->labels maintains the list of labels present into LSA.
>> init_labels() prepares this list while adding new label into LSA
>> and updates nd_mapping->labels accordingly. During cxl region
>> creation nd_mapping->labels list and LSA was updated with one
>> region label. Therefore during new namespace label creation
>> pre-include the previously created region label, so increase
>> num_labels count by 1.
>
>Why does the count of the labels in the list not work?
>
>static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
>{
>        int i, old_num_labels = 0;
>...
>        mutex_lock(&nd_mapping->lock);
>        list_for_each_entry(label_ent, &nd_mapping->labels, list)
>                old_num_labels++;
>        mutex_unlock(&nd_mapping->lock);
>...
>

Hi Ira,

init_labels() allocates new label based on comparison with existing
count of the labels in the list and passed num_labels. If num_labels
is greater than count of the labels in the list then new label is
allocated and stored in list for later usage

...
         mutex_lock(&nd_mapping->lock);
	list_for_each_entry(label_ent, &nd_mapping->labels, list)
		old_num_labels++;
	mutex_unlock(&nd_mapping->lock);

	for (i = old_num_labels; i < num_labels; i++) {
		label_ent = kzalloc(sizeof(*label_ent), GFP_KERNEL);
		if (!label_ent)
			return -ENOMEM;

		mutex_lock(&nd_mapping->lock);
		list_add_tail(&label_ent->list, nd_mapping->labels);
		mutex_unlock(&nd_mapping->lock);
	}
...


>>
>> Also updated nsl_set_region_uuid with region uuid with which
>> namespace is associated with.
>
>Whenever using a word like 'Also' in the commit message ask if this should be a
>separate patch.  I'm thinking this hunk should be somewhere else in the series.
>
>Ira

Sure Ira, Yes both hunks are not associated, I just added both to avoid
extra commit. I will create separate commit to update region_uuid and
will remove 'Also' part from commit message.


Regards,
Neeraj

Neeraj Kumar wrote:
> On 19/08/25 01:56PM, Ira Weiny wrote:
> >Neeraj Kumar wrote:
> >> nd_mapping->labels maintains the list of labels present into LSA.
> >> init_labels() prepares this list while adding new label into LSA
> >> and updates nd_mapping->labels accordingly. During cxl region
> >> creation nd_mapping->labels list and LSA was updated with one
> >> region label. Therefore during new namespace label creation
> >> pre-include the previously created region label, so increase
> >> num_labels count by 1.
> >
> >Why does the count of the labels in the list not work?
> >
> >static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
> >{
> >        int i, old_num_labels = 0;
> >...
> >        mutex_lock(&nd_mapping->lock);
> >        list_for_each_entry(label_ent, &nd_mapping->labels, list)
> >                old_num_labels++;
> >        mutex_unlock(&nd_mapping->lock);
> >...
> >
> 
> Hi Ira,
> 
> init_labels() allocates new label based on comparison with existing
> count of the labels in the list and passed num_labels. If num_labels
> is greater than count of the labels in the list then new label is
> allocated and stored in list for later usage

I think I'm following better but shouldn't this hunk be included in the
code which creates the region label in the list?

I'm concerned that this '+ 1' out of the blue and will be confusing in the
future.  Why can't count be kept up to date when the region label was
created and added?

What code (patch) added this region label?

Ira

[snip]

On 05/09/25 03:08PM, Ira Weiny wrote:
>Neeraj Kumar wrote:
>> On 19/08/25 01:56PM, Ira Weiny wrote:
>> >Neeraj Kumar wrote:
>> >> nd_mapping->labels maintains the list of labels present into LSA.
>> >> init_labels() prepares this list while adding new label into LSA
>> >> and updates nd_mapping->labels accordingly. During cxl region
>> >> creation nd_mapping->labels list and LSA was updated with one
>> >> region label. Therefore during new namespace label creation
>> >> pre-include the previously created region label, so increase
>> >> num_labels count by 1.
>> >
>> >Why does the count of the labels in the list not work?
>> >
>> >static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
>> >{
>> >        int i, old_num_labels = 0;
>> >...
>> >        mutex_lock(&nd_mapping->lock);
>> >        list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> >                old_num_labels++;
>> >        mutex_unlock(&nd_mapping->lock);
>> >...
>> >
>>
>> Hi Ira,
>>
>> init_labels() allocates new label based on comparison with existing
>> count of the labels in the list and passed num_labels. If num_labels
>> is greater than count of the labels in the list then new label is
>> allocated and stored in list for later usage
>
>I think I'm following better but shouldn't this hunk be included in the
>code which creates the region label in the list?
>

Yes we can include this hunk in patch 5 where we are updating region
label. I will drop this commit and include it in patch 5.

>I'm concerned that this '+ 1' out of the blue and will be confusing in the
>future.  Why can't count be kept up to date when the region label was
>created and added?

Yes this '+1' is hardcoded and it creating confusion. I will fix this
with available region labels in the list.

>
>What code (patch) added this region label?
>
>Ira

Patch 5 add this region label using nd_pmem_region_label_update()


Regards,
Neeraj

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

> nd_mapping->labels maintains the list of labels present into LSA.
> init_labels() prepares this list while adding new label into LSA
> and updates nd_mapping->labels accordingly. During cxl region
> creation nd_mapping->labels list and LSA was updated with one
> region label. Therefore during new namespace label creation
> pre-include the previously created region label, so increase
> num_labels count by 1.
> 
> Also updated nsl_set_region_uuid with region uuid with which
> namespace is associated with.

Any reason these are in the same patch?  I'd like to have
seen a bit more on why this 'Also' change is here and a separate
patch might make that easier to see.

> 
> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
> ---
>  drivers/nvdimm/label.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
> index be18278d6cea..fd02b557612e 100644
> --- a/drivers/nvdimm/label.c
> +++ b/drivers/nvdimm/label.c
> @@ -957,7 +957,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  	nsl_set_slot(ndd, nd_label, slot);
>  	nsl_set_alignment(ndd, nd_label, 0);
>  	nsl_set_type_guid(ndd, nd_label, &nd_set->type_guid);
> -	nsl_set_region_uuid(ndd, nd_label, NULL);
> +	nsl_set_region_uuid(ndd, nd_label, &nd_set->uuid);
>  	nsl_set_claim_class(ndd, nd_label, ndns->claim_class);
>  	nsl_calculate_checksum(ndd, nd_label);
>  	nd_dbg_dpa(nd_region, ndd, res, "\n");
> @@ -1129,7 +1129,8 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>  				count++;
>  		WARN_ON_ONCE(!count);
>  
> -		rc = init_labels(nd_mapping, count);
> +		/* Adding 1 to pre include the already added region label */
> +		rc = init_labels(nd_mapping, count + 1);
>  		if (rc < 0)
>  			return rc;
>

On 13/08/25 03:58PM, Jonathan Cameron wrote:
>On Wed, 30 Jul 2025 17:41:56 +0530
>Neeraj Kumar <s.neeraj@samsung.com> wrote:
>
>> nd_mapping->labels maintains the list of labels present into LSA.
>> init_labels() prepares this list while adding new label into LSA
>> and updates nd_mapping->labels accordingly. During cxl region
>> creation nd_mapping->labels list and LSA was updated with one
>> region label. Therefore during new namespace label creation
>> pre-include the previously created region label, so increase
>> num_labels count by 1.
>>
>> Also updated nsl_set_region_uuid with region uuid with which
>> namespace is associated with.
>
>Any reason these are in the same patch?  I'd like to have
>seen a bit more on why this 'Also' change is here and a separate
>patch might make that easier to see.

I will create separate commit to update region_uuid and will remove
'Also' part from commit message. Yes both hunks are not associated,
I just added both to avoid extra commit.


Regards,
Neeraj