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
>
> Signed-off-by: Neeraj Kumar <s.nee...@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?
> +
> /*
> * 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.
> +
> 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.
> +
> /* need to set a limit somewhere, but yes, this is likely overkill */
> ND_IOCTL_MAX_BUFLEN = SZ_4M,
> ND_CMD_MAX_ELEM = 5,
