Re: [PATCH V2 03/20] nvdimm/namespace_label: Add namespace label changes as per CXL LSA v2.1

[Ira Weiny <ira.weiny@intel.com>]

Neeraj Kumar wrote:
> CXL 3.2 Spec mentions CXL LSA 2.1 Namespace Labels at section 9.13.2.5
> Modified __pmem_label_update function using setter functions to update
> namespace label as per CXL LSA 2.1

But why?  And didn't we just remove nd_namespace_label in patch 2?

Why are we now defining accessor functions for it?

> 
> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
> ---
>  drivers/nvdimm/label.c |  3 +++
>  drivers/nvdimm/nd.h    | 27 +++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
> index 75bc11da4c11..3f8a6bdb77c7 100644
> --- a/drivers/nvdimm/label.c
> +++ b/drivers/nvdimm/label.c
> @@ -933,6 +933,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  	memset(lsa_label, 0, sizeof_namespace_label(ndd));
>  
>  	nd_label = &lsa_label->ns_label;
> +	nsl_set_type(ndd, nd_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);
> @@ -944,7 +945,9 @@ static int __pmem_label_update(struct nd_region *nd_region,
>  	nsl_set_lbasize(ndd, nd_label, nspm->lbasize);
>  	nsl_set_dpa(ndd, nd_label, res->start);
>  	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_claim_class(ndd, nd_label, ndns->claim_class);
>  	nsl_calculate_checksum(ndd, nd_label);
>  	nd_dbg_dpa(nd_region, ndd, res, "\n");
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 61348dee687d..651847f1bbf9 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -295,6 +295,33 @@ static inline const u8 *nsl_uuid_raw(struct nvdimm_drvdata *ndd,
>  	return nd_label->efi.uuid;
>  }
>  
> +static inline void nsl_set_type(struct nvdimm_drvdata *ndd,
> +				struct nd_namespace_label *ns_label)

Set type to what?

Why is driver data passed here?

This reads as an accessor function for some sort of label class but seems
to do some back checking into ndd to set the uuid of the label?

At a minimum this should be *_set_uuid(..., uuid_t uuid)  But I'm not
following this chunk of changes so don't just change it without more
explaination.

> +{
> +	uuid_t tmp;
> +
> +	if (ndd->cxl) {
> +		uuid_parse(CXL_NAMESPACE_UUID, &tmp);
> +		export_uuid(ns_label->cxl.type, &tmp);
> +	}
> +}
> +
> +static inline void nsl_set_alignment(struct nvdimm_drvdata *ndd,
> +				     struct nd_namespace_label *ns_label,
> +				     u32 align)

Why is this needed?

> +{
> +	if (ndd->cxl)
> +		ns_label->cxl.align = __cpu_to_le16(align);
> +}
> +
> +static inline void nsl_set_region_uuid(struct nvdimm_drvdata *ndd,
> +				       struct nd_namespace_label *ns_label,
> +				       const uuid_t *uuid)

Again why?

> +{
> +	if (ndd->cxl)
> +		export_uuid(ns_label->cxl.region_uuid, uuid);

export does a memcpy() and you are passing it NULL.  Is that safe?

Ira

[snip]