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

[Neeraj Kumar]

On 23/06/25 11:53AM, Jonathan Cameron wrote:
> On Tue, 17 Jun 2025 18:09:26 +0530
> Neeraj Kumar <s.neeraj@xxxxxxxxxxx> wrote:
>
> > In order to accommodate cxl lsa 2.1 format region label, renamed
> > nd_namespace_label to nd_lsa_label.
>
> I would add some more information on why.  I've no idea from this
> description whether the issue is a naming clash or a need for a broader
> name or something entirely different.
>
> Most readers aren't going to be particular familiar with the lsa spec
> version changes, so help them out with a little more detail.
>
> The comment you have with the union is probably the right info
> to duplicate here.

Thanks Jonathan, I will elaborate commit message with more information.

> > No functional change introduced.
> >
> > Signed-off-by: Neeraj Kumar <s.neeraj@xxxxxxxxxxx>
>
> This is quite a lot of churn which, just looking at this patch, could
> be avoided by just setting local variables to point at a particular
> member of the union rather than the containing struct.
> You already do this in a few places like nd_label_reserve_dpa().
>
> Perhaps it will be come clearer why that doesn't make sense as
> I ready later patches.

Sure, I will try to minimize wherever its possible.

> > 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;
> > +	};
> > +};
>
> > diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> > index 55cfbf1e0a95..f180f0068c15 100644
> > --- a/drivers/nvdimm/namespace_devs.c
> > +++ b/drivers/nvdimm/namespace_devs.c
>
> > @@ -1595,7 +1596,8 @@ static bool has_uuid_at_pos(struct nd_region *nd_region, const uuid_t *uuid,
> >  				return false;
> >  			}
> >  			found_uuid = true;
> > -			if (!nsl_validate_nlabel(nd_region, ndd, nd_label))
> > +			if (!nsl_validate_nlabel(nd_region,
> > +						 ndd, &nd_label->ns_label))
>
> Strange wrap.  I'd move ndd up a line.

Sure, I will fix it in V1

> >  				continue;
> >  			if (position != pos)
> >  				continue;
> > @@ -1615,7 +1617,7 @@ static int select_pmem_id(struct nd_region *nd_region, const uuid_t *pmem_id)
> >  	for (i = 0; i < nd_region->ndr_mappings; i++) {
> >  		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> >  		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> > -		struct nd_namespace_label *nd_label = NULL;
> > +		struct nd_lsa_label *nd_label = NULL;
> >  		u64 hw_start, hw_end, pmem_start, pmem_end;
> >  		struct nd_label_ent *label_ent;
>
>
> > @@ -1739,14 +1741,14 @@ static struct device *create_namespace_pmem(struct nd_region *nd_region,
> >  	 * that position at labels[0], and NULL at labels[1].  In the process,
> >  	 * check that the namespace aligns with interleave-set.
> >  	 */
> > -	nsl_get_uuid(ndd, nd_label, &uuid);
> > +	nsl_get_uuid(ndd, &nd_label->ns_label, &uuid);
> >  	rc = select_pmem_id(nd_region, &uuid);
> >  	if (rc)
> >  		goto err;
> >
> >  	/* Calculate total size and populate namespace properties from label0 */
> >  	for (i = 0; i < nd_region->ndr_mappings; i++) {
> > -		struct nd_namespace_label *label0;
> > +		struct nd_lsa_label *label0;
>
> If you are only ever going to use one part of the union in a given
> bit of code, why not use a struct of that type so you only need to change
> the point where it is assigned rather that lots of places.
>
> So keep this as struct nd_namespace_label *label0;
>
> If that makes sense, check for other places where doing that will reduce the churn
> and complexity of the code.

Sure, Let me revisit these. Thanks

> >  		struct nvdimm_drvdata *ndd;
> >
> >  		nd_mapping = &nd_region->mapping[i];
> > @@ -1760,17 +1762,17 @@ static struct device *create_namespace_pmem(struct nd_region *nd_region,
> >  		}
> >
> >  		ndd = to_ndd(nd_mapping);
> > -		size += nsl_get_rawsize(ndd, label0);
> > -		if (nsl_get_position(ndd, label0) != 0)
> > +		size += nsl_get_rawsize(ndd, &label0->ns_label);
> > +		if (nsl_get_position(ndd, &label0->ns_label) != 0)
> >  			continue;
> >  		WARN_ON(nspm->alt_name || nspm->uuid);
> > -		nspm->alt_name = kmemdup(nsl_ref_name(ndd, label0),
> > +		nspm->alt_name = kmemdup(nsl_ref_name(ndd, &label0->ns_label),
> >  					 NSLABEL_NAME_LEN, GFP_KERNEL);
> > -		nsl_get_uuid(ndd, label0, &uuid);
> > +		nsl_get_uuid(ndd, &label0->ns_label, &uuid);
> >  		nspm->uuid = kmemdup(&uuid, sizeof(uuid_t), GFP_KERNEL);
> > -		nspm->lbasize = nsl_get_lbasize(ndd, label0);
> > +		nspm->lbasize = nsl_get_lbasize(ndd, &label0->ns_label);
> >  		nspm->nsio.common.claim_class =
> > -			nsl_get_claim_class(ndd, label0);
> > +			nsl_get_claim_class(ndd, &label0->ns_label);
> >  	}