Re: [PATCH V2 05/20] nvdimm/region_label: Add region label updation routine

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

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

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

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