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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250623115351.00005312@huawei.com>
Date: Mon, 23 Jun 2025 11:53:51 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Neeraj Kumar <s.neeraj@...sung.com>
CC: <dan.j.williams@...el.com>, <dave@...olabs.net>, <dave.jiang@...el.com>,
	<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
	<ira.weiny@...el.com>, <a.manzanares@...sung.com>, <nifan.cxl@...il.com>,
	<anisa.su@...sung.com>, <vishak.g@...sung.com>, <krish.reddy@...sung.com>,
	<arun.george@...sung.com>, <alok.rathore@...sung.com>,
	<neeraj.kernel@...il.com>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <nvdimm@...ts.linux.dev>,
	<gost.dev@...sung.com>, <cpgs@...sung.com>
Subject: Re: [RFC PATCH 02/20] nvdimm/label: Prep patch to accommodate cxl
 lsa 2.1 support

On Tue, 17 Jun 2025 18:09:26 +0530
Neeraj Kumar <s.neeraj@...sung.com> 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.

> 
> No functional change introduced.
> 
> Signed-off-by: Neeraj Kumar <s.neeraj@...sung.com>

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.

> 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.


>  				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.



>  		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);
>  	}