| 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
| ||
|
Message-ID: <1296674576.21750943703230.JavaMail.epsvc@epcpadp1new>
Date: Thu, 26 Jun 2025 15:21:58 +0530
From: Neeraj Kumar <s.neeraj@...sung.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.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 23/06/25 11:53AM, Jonathan Cameron wrote:
>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.
>
Thanks Jonathan, I will elaborate commit message with more information.
>>
>> 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.
>
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);
>> }
>
Powered by blists - more mailing lists