Re: [PATCH V2 02/20] nvdimm/label: Prep patch to accommodate cxl lsa 2.1 support

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

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

On 15/08/25 03:02PM, Dave Jiang wrote:
>
>
>On 7/30/25 5:11 AM, Neeraj Kumar wrote:
>> LSA 2.1 format introduces region label, which can also reside
>> into LSA along with only namespace label as per v1.1 and v1.2
>>
>> As both namespace and region labels are of same size of 256 bytes.
>> Thus renamed "struct nd_namespace_label" to "struct nd_lsa_label",
>> where both namespace label and region label can stay as union.
>>
>> No functional change introduced.
>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>> ---
>>  drivers/nvdimm/label.c          | 58 +++++++++++++++++++--------------
>>  drivers/nvdimm/label.h          | 12 ++++++-
>>  drivers/nvdimm/namespace_devs.c | 33 +++++++++++++------
>>  drivers/nvdimm/nd.h             |  2 +-
>>  4 files changed, 68 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
>> index 7a011ee02d79..75bc11da4c11 100644
>> --- a/drivers/nvdimm/label.c
>> +++ b/drivers/nvdimm/label.c
>> @@ -271,7 +271,7 @@ static void nd_label_copy(struct nvdimm_drvdata *ndd,
>>  	memcpy(dst, src, sizeof_namespace_index(ndd));
>>  }
>>
>> -static struct nd_namespace_label *nd_label_base(struct nvdimm_drvdata *ndd)
>> +static struct nd_lsa_label *nd_label_base(struct nvdimm_drvdata *ndd)
>>  {
>>  	void *base = to_namespace_index(ndd, 0);
>>
>> @@ -279,7 +279,7 @@ static struct nd_namespace_label *nd_label_base(struct nvdimm_drvdata *ndd)
>>  }
>>
>>  static int to_slot(struct nvdimm_drvdata *ndd,
>> -		struct nd_namespace_label *nd_label)
>> +		struct nd_lsa_label *nd_label)
>>  {
>>  	unsigned long label, base;
>>
>> @@ -289,14 +289,14 @@ static int to_slot(struct nvdimm_drvdata *ndd,
>>  	return (label - base) / sizeof_namespace_label(ndd);
>>  }
>>
>> -static struct nd_namespace_label *to_label(struct nvdimm_drvdata *ndd, int slot)
>> +static struct nd_lsa_label *to_label(struct nvdimm_drvdata *ndd, int slot)
>>  {
>>  	unsigned long label, base;
>>
>>  	base = (unsigned long) nd_label_base(ndd);
>>  	label = base + sizeof_namespace_label(ndd) * slot;
>>
>> -	return (struct nd_namespace_label *) label;
>> +	return (struct nd_lsa_label *) label;
>>  }
>>
>>  #define for_each_clear_bit_le(bit, addr, size) \
>> @@ -382,9 +382,10 @@ static void nsl_calculate_checksum(struct nvdimm_drvdata *ndd,
>>  }
>>
>>  static bool slot_valid(struct nvdimm_drvdata *ndd,
>> -		struct nd_namespace_label *nd_label, u32 slot)
>> +		struct nd_lsa_label *lsa_label, u32 slot)
>>  {
>>  	bool valid;
>> +	struct nd_namespace_label *nd_label = &lsa_label->ns_label;
>>
>>  	/* check that we are written where we expect to be written */
>>  	if (slot != nsl_get_slot(ndd, nd_label))
>> @@ -405,6 +406,7 @@ int nd_label_reserve_dpa(struct nvdimm_drvdata *ndd)
>>  		return 0; /* no label, nothing to reserve */
>>
>>  	for_each_clear_bit_le(slot, free, nslot) {
>> +		struct nd_lsa_label *lsa_label;
>>  		struct nd_namespace_label *nd_label;
>>  		struct nd_region *nd_region = NULL;
>>  		struct nd_label_id label_id;
>> @@ -412,9 +414,10 @@ int nd_label_reserve_dpa(struct nvdimm_drvdata *ndd)
>>  		uuid_t label_uuid;
>>  		u32 flags;
>>
>> -		nd_label = to_label(ndd, slot);
>> +		lsa_label = to_label(ndd, slot);
>> +		nd_label = &lsa_label->ns_label;
>>
>> -		if (!slot_valid(ndd, nd_label, slot))
>> +		if (!slot_valid(ndd, lsa_label, slot))
>>  			continue;
>>
>>  		nsl_get_uuid(ndd, nd_label, &label_uuid);
>> @@ -565,11 +568,13 @@ int nd_label_active_count(struct nvdimm_drvdata *ndd)
>>  		return 0;
>>
>>  	for_each_clear_bit_le(slot, free, nslot) {
>> +		struct nd_lsa_label *lsa_label;
>>  		struct nd_namespace_label *nd_label;
>>
>> -		nd_label = to_label(ndd, slot);
>> +		lsa_label = to_label(ndd, slot);
>> +		nd_label = &lsa_label->ns_label;
>>
>> -		if (!slot_valid(ndd, nd_label, slot)) {
>> +		if (!slot_valid(ndd, lsa_label, slot)) {
>>  			u32 label_slot = nsl_get_slot(ndd, nd_label);
>>  			u64 size = nsl_get_rawsize(ndd, nd_label);
>>  			u64 dpa = nsl_get_dpa(ndd, nd_label);
>> @@ -584,7 +589,7 @@ int nd_label_active_count(struct nvdimm_drvdata *ndd)
>>  	return count;
>>  }
>>
>> -struct nd_namespace_label *nd_label_active(struct nvdimm_drvdata *ndd, int n)
>> +struct nd_lsa_label *nd_label_active(struct nvdimm_drvdata *ndd, int n)
>>  {
>>  	struct nd_namespace_index *nsindex;
>>  	unsigned long *free;
>> @@ -594,10 +599,10 @@ struct nd_namespace_label *nd_label_active(struct nvdimm_drvdata *ndd, int n)
>>  		return NULL;
>>
>>  	for_each_clear_bit_le(slot, free, nslot) {
>> -		struct nd_namespace_label *nd_label;
>> +		struct nd_lsa_label *lsa_label;
>>
>> -		nd_label = to_label(ndd, slot);
>> -		if (!slot_valid(ndd, nd_label, slot))
>> +		lsa_label = to_label(ndd, slot);
>> +		if (!slot_valid(ndd, lsa_label, slot))
>>  			continue;
>>
>>  		if (n-- == 0)
>> @@ -738,7 +743,7 @@ static int nd_label_write_index(struct nvdimm_drvdata *ndd, int index, u32 seq,
>>  }
>>
>>  static unsigned long nd_label_offset(struct nvdimm_drvdata *ndd,
>> -		struct nd_namespace_label *nd_label)
>> +		struct nd_lsa_label *nd_label)
>>  {
>>  	return (unsigned long) nd_label
>>  		- (unsigned long) to_namespace_index(ndd, 0);
>> @@ -892,6 +897,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	struct nd_namespace_common *ndns = &nspm->nsio.common;
>>  	struct nd_interleave_set *nd_set = nd_region->nd_set;
>>  	struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> +	struct nd_lsa_label *lsa_label;
>>  	struct nd_namespace_label *nd_label;
>>  	struct nd_namespace_index *nsindex;
>>  	struct nd_label_ent *label_ent;
>> @@ -923,8 +929,10 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  		return -ENXIO;
>>  	dev_dbg(ndd->dev, "allocated: %d\n", slot);
>>
>> -	nd_label = to_label(ndd, slot);
>> -	memset(nd_label, 0, sizeof_namespace_label(ndd));
>> +	lsa_label = to_label(ndd, slot);
>> +	memset(lsa_label, 0, sizeof_namespace_label(ndd));
>> +
>> +	nd_label = &lsa_label->ns_label;
>>  	nsl_set_uuid(ndd, nd_label, nspm->uuid);
>>  	nsl_set_name(ndd, nd_label, nspm->alt_name);
>>  	nsl_set_flags(ndd, nd_label, flags);
>> @@ -942,7 +950,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	nd_dbg_dpa(nd_region, ndd, res, "\n");
>>
>>  	/* update label */
>> -	offset = nd_label_offset(ndd, nd_label);
>> +	offset = nd_label_offset(ndd, lsa_label);
>>  	rc = nvdimm_set_config_data(ndd, offset, nd_label,
>>  			sizeof_namespace_label(ndd));
>>  	if (rc < 0)
>> @@ -954,7 +962,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  		if (!label_ent->label)
>>  			continue;
>>  		if (test_and_clear_bit(ND_LABEL_REAP, &label_ent->flags) ||
>> -		    nsl_uuid_equal(ndd, label_ent->label, nspm->uuid))
>> +		    nsl_uuid_equal(ndd, &label_ent->label->ns_label, nspm->uuid))
>>  			reap_victim(nd_mapping, label_ent);
>>  	}
>>
>> @@ -964,14 +972,14 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	if (rc == 0) {
>>  		list_for_each_entry(label_ent, &nd_mapping->labels, list)
>>  			if (!label_ent->label) {
>> -				label_ent->label = nd_label;
>> -				nd_label = NULL;
>> +				label_ent->label = lsa_label;
>> +				lsa_label = NULL;
>>  				break;
>>  			}
>> -		dev_WARN_ONCE(&nspm->nsio.common.dev, nd_label,
>> +		dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
>>  				"failed to track label: %d\n",
>> -				to_slot(ndd, nd_label));
>> -		if (nd_label)
>> +				to_slot(ndd, lsa_label));
>> +		if (lsa_label)
>>  			rc = -ENXIO;
>>  	}
>>  	mutex_unlock(&nd_mapping->lock);
>> @@ -1042,12 +1050,12 @@ static int del_labels(struct nd_mapping *nd_mapping, uuid_t *uuid)
>>
>>  	mutex_lock(&nd_mapping->lock);
>>  	list_for_each_entry_safe(label_ent, e, &nd_mapping->labels, list) {
>> -		struct nd_namespace_label *nd_label = label_ent->label;
>> +		struct nd_lsa_label *nd_label = label_ent->label;
>>
>>  		if (!nd_label)
>>  			continue;
>>  		active++;
>> -		if (!nsl_uuid_equal(ndd, nd_label, uuid))
>> +		if (!nsl_uuid_equal(ndd, &nd_label->ns_label, uuid))
>>  			continue;
>>  		active--;
>>  		slot = to_slot(ndd, nd_label);
>> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
>> index 0650fb4b9821..4883b3a1320f 100644
>> --- a/drivers/nvdimm/label.h
>> +++ b/drivers/nvdimm/label.h
>> @@ -183,6 +183,16 @@ struct nd_namespace_label {
>>  	};
>>  };
>>
>> +/*
>> + * LSA 2.1 format introduces region label, which can also reside
>> + * into LSA along with only namespace label as per v1.1 and v1.2
>> + */
>> +struct nd_lsa_label {
>> +	union {
>> +		struct nd_namespace_label ns_label;
>> +	};
>> +};
>
>I think originally 'struct nd_namespace_label' wrapped a union to avoid changing code that already has 'struct nd_namespace_label' in the function. But since you are creating a whole new thing, maybe just create a union directly since you end up touching all the function parameters in this patch anyways.
>
>union nd_lsa_label {
>	struct nd_namespace_label ns_label;
>	struct cxl_region_label region_label;
>};
>
>DJ

Thanks Dave for your suggestion. Yes I have wrapped a union here to
avoid changing existing code which may create more noise.

I think changing "struct nd_lsa_label" to "union nd_lsa_label" will be
kind of same. Only diff will be usage of "union" in place of "struct",
wherever I use nd_lsa_label.

I understand that this renaming has created some extra noise in existing
code. May be I will revisit this change and try using region label handling
separately instead of using union.


Regards,
Neeraj

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