Re: [RFC PATCH 01/20] nvdimm/label: Introduce NDD_CXL_LABEL flag to set cxl label format

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

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

On 09/07/25 03:57PM, Dave Jiang wrote:
>
>
>On 6/17/25 5:39 AM, Neeraj Kumar wrote:
>> NDD_CXL_LABEL is introduced to set cxl LSA 2.1 label format
>> Accordingly updated label index version
>
>Maybe add the spec reference that defines label 2.1 format

Sure Dave, I will update commit message accordingly.

>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>> ---
>>  drivers/nvdimm/dimm.c      |  1 +
>>  drivers/nvdimm/dimm_devs.c | 10 ++++++++++
>>  drivers/nvdimm/label.c     | 16 ++++++++++++----
>>  drivers/nvdimm/nd.h        |  1 +
>>  include/linux/libnvdimm.h  |  3 +++
>>  5 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
>> index 91d9163ee303..8753b5cd91cc 100644
>> --- a/drivers/nvdimm/dimm.c
>> +++ b/drivers/nvdimm/dimm.c
>> @@ -62,6 +62,7 @@ static int nvdimm_probe(struct device *dev)
>>  	if (rc < 0)
>>  		dev_dbg(dev, "failed to unlock dimm: %d\n", rc);
>>
>> +	ndd->cxl = nvdimm_check_cxl_label_format(ndd->dev);
>>
>>  	/*
>>  	 * EACCES failures reading the namespace label-area-properties
>> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
>> index 21498d461fde..e8f545f889fd 100644
>> --- a/drivers/nvdimm/dimm_devs.c
>> +++ b/drivers/nvdimm/dimm_devs.c
>> @@ -18,6 +18,16 @@
>>
>>  static DEFINE_IDA(dimm_ida);
>>
>> +bool nvdimm_check_cxl_label_format(struct device *dev)
>> +{
>> +	struct nvdimm *nvdimm = to_nvdimm(dev);
>> +
>> +	if (test_bit(NDD_CXL_LABEL, &nvdimm->flags))
>> +		return true;
>> +
>> +	return false;
>> +}
>
>I think we may want to move the checking of the flag to where the patch also set the flag in order to provide a more coherent review experience. Given that I haven't read the rest of the patchset and don't know how NDD_CXL_LABEL is set, I really can't comment on whether there's a better way to detect LSA 2.1 labels. Is there a generic way to determine label versions without the implication of this is CXL device?
>

Its been set in cxl driver in later patch, May be I will update the
commit message with this information.

>> +
>>  /*
>>   * Retrieve bus and dimm handle and return if this bus supports
>>   * get_config_data commands
>> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
>> index 082253a3a956..48b5ba90216d 100644
>> --- a/drivers/nvdimm/label.c
>> +++ b/drivers/nvdimm/label.c
>> @@ -687,11 +687,19 @@ static int nd_label_write_index(struct nvdimm_drvdata *ndd, int index, u32 seq,
>>  		- (unsigned long) to_namespace_index(ndd, 0);
>>  	nsindex->labeloff = __cpu_to_le64(offset);
>>  	nsindex->nslot = __cpu_to_le32(nslot);
>> -	nsindex->major = __cpu_to_le16(1);
>> -	if (sizeof_namespace_label(ndd) < 256)
>> +
>> +	/* Support CXL LSA 2.1 label format */
>> +	if (ndd->cxl) {
>> +		nsindex->major = __cpu_to_le16(2);
>>  		nsindex->minor = __cpu_to_le16(1);
>> -	else
>> -		nsindex->minor = __cpu_to_le16(2);
>> +	} else {
>> +		nsindex->major = __cpu_to_le16(1);
>> +		if (sizeof_namespace_label(ndd) < 256)
>> +			nsindex->minor = __cpu_to_le16(1);
>> +		else
>> +			nsindex->minor = __cpu_to_le16(2);
>> +	}
>
>Would like to see a more coherent way of detecting label versioning. What happens when there are newer versions introduced later on? This currently feels very disjointed.
>

Thanks Dave for your suggestion. This current patch-set is extension of
commit 5af96835e4daf. We may have to do some more change to address this
coherent way of detecting label versioning. May be we can take this later.

>> +
>>  	nsindex->checksum = __cpu_to_le64(0);
>>  	if (flags & ND_NSINDEX_INIT) {
>>  		unsigned long *free = (unsigned long *) nsindex->free;
>> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
>> index 5ca06e9a2d29..304f0e9904f1 100644
>> --- a/drivers/nvdimm/nd.h
>> +++ b/drivers/nvdimm/nd.h
>> @@ -522,6 +522,7 @@ void nvdimm_set_labeling(struct device *dev);
>>  void nvdimm_set_locked(struct device *dev);
>>  void nvdimm_clear_locked(struct device *dev);
>>  int nvdimm_security_setup_events(struct device *dev);
>> +bool nvdimm_check_cxl_label_format(struct device *dev);
>>  #if IS_ENABLED(CONFIG_NVDIMM_KEYS)
>>  int nvdimm_security_unlock(struct device *dev);
>>  #else
>> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
>> index e772aae71843..0a55900842c8 100644
>> --- a/include/linux/libnvdimm.h
>> +++ b/include/linux/libnvdimm.h
>> @@ -44,6 +44,9 @@ enum {
>>  	/* dimm provider wants synchronous registration by __nvdimm_create() */
>>  	NDD_REGISTER_SYNC = 8,
>>
>> +	/* dimm supports region labels (LSA Format 2.1) */
>> +	NDD_CXL_LABEL = 9,
>
>While 2.1 is defined by the CXL spec, is there anything declared by the CXL spec that makes 2.1 exclusive to CXL? Maybe the focus should be to support LSA 2.1 and avoid dragging naming CXL into the conversation at this point. It can be made more generic right? I'm concerned about dragging CXL into nvdimm when it isn't necessary. Maybe introduce a label cap field where when an nvdimm device is registered, the caller can pass in that it's capable of supporting up to a certain version of labeling? Just throwing ideas out to see if it's feasible.
>

Hi Dave,

I have taken this naming reference from "drivers/nvdimm/label.h"
where namespace label is defined as "struct nvdimm_efi_label" (LSA 1.1
is defined in nvdimm namespace spec and LSA 1.2 is defined in efi spec)
where (from commit 540ccaa2e4dd6) region label is defined as "struct
cxl_region_label"

Please let me know if I should use some other name in place of this


Regards,
Neeraj

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