| 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: <20250922131220.kgkaxyzopprwdi7t@test-PowerEdge-R740xd>
Date: Mon, 22 Sep 2025 18:42:20 +0530
From: Neeraj Kumar <s.neeraj@...sung.com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: linux-cxl@...r.kernel.org, nvdimm@...ts.linux.dev,
linux-kernel@...r.kernel.org, gost.dev@...sung.com,
a.manzanares@...sung.com, vishak.g@...sung.com, neeraj.kernel@...il.com,
cpgs@...sung.com
Subject: Re: [PATCH V3 06/20] nvdimm/region_label: Add region label update
support
On 17/09/25 04:36PM, Jonathan Cameron wrote:
>On Wed, 17 Sep 2025 19:11:02 +0530
>Neeraj Kumar <s.neeraj@...sung.com> wrote:
>
>> Modified __pmem_label_update() to update region labels into LSA
>>
>I'm struggling to follow the use of the union for the two label types
>in much of this code. To me if feels like that should only be a thing
>at the init_labels() point on the basis I think it's only there that
>we need to handle both in the same storage.
>
>For the rest I'd just pay the small price of duplication that will
>occur if you just split he functions up.
I got your point Jonathan, Let me revisit it again.
>
>> Signed-off-by: Neeraj Kumar <s.neeraj@...sung.com>
>> ---
>> drivers/nvdimm/label.c | 269 ++++++++++++++++++++++++++------
>> drivers/nvdimm/label.h | 15 ++
>> drivers/nvdimm/namespace_devs.c | 12 ++
>> drivers/nvdimm/nd.h | 38 ++++-
>> include/linux/libnvdimm.h | 8 +
>> 5 files changed, 289 insertions(+), 53 deletions(-)
>>
[snip]
>> - nsl_set_position(ndd, nd_label, pos);
>> - nsl_set_isetcookie(ndd, nd_label, cookie);
>> - nsl_set_rawsize(ndd, nd_label, resource_size(res));
>> - 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");
>> + lsa_label = (union nd_lsa_label *) to_label(ndd, slot);
>
>This cast feels rather dubious.
>
>If the union makes sense in general, then this should be changed
>to return the union.
Sure, I will fix it in next patch-set
>
>> + memset(lsa_label, 0, sizeof_namespace_label(ndd));
>> +
>> + switch (ltype) {
>> + case NS_LABEL_TYPE:
>> + dev = &nspm->nsio.common.dev;
>> + rc = namespace_label_update(nd_region, nd_mapping,
>> + nspm, pos, flags, &lsa_label->ns_label,
>> + nsindex, slot);
>> + if (rc)
>> + return rc;
>> +
>> + break;
>> + case RG_LABEL_TYPE:
>> + dev = &nd_region->dev;
>> + region_label_update(nd_region, &lsa_label->region_label,
>> + nd_mapping, pos, flags, slot);
>> +
>> + break;
>> + }
>>
>> /* update label */
>> - offset = nd_label_offset(ndd, nd_label);
>> - rc = nvdimm_set_config_data(ndd, offset, nd_label,
>> + offset = nd_label_offset(ndd, &lsa_label->ns_label);
>
>This doesn't make sense as the type might be either an ns_label or a region_label.
>If there is a generic header (I'm guessing there is) then define that as part of the
>union with just the shared parts and use that to avoid any implication of what the type
>is in calls like this. Also make nd_label_offset() take the union not the specific bit.
Okay, I will update the signature of nd_label_offset() to use union and not the specific bit
>
>> + rc = nvdimm_set_config_data(ndd, offset, lsa_label,
>> sizeof_namespace_label(ndd));
>> if (rc < 0)
>> return rc;
>> @@ -955,8 +1054,10 @@ static int __pmem_label_update(struct nd_region *nd_region,
>> list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>> if (!label_ent->label)
>> continue;
>> - if (test_and_clear_bit(ND_LABEL_REAP, &label_ent->flags) ||
>> - nsl_uuid_equal(ndd, label_ent->label, nspm->uuid))
>> +
>> + if (is_label_reapable(nd_set, nspm, ndd,
>> + (union nd_lsa_label *) label_ent->label,
>
>If we are going with the union that label_ent->label should be a union that
>we don't need to cast.
Sure, I will fix this in next patch-set
>
>> + ltype, &label_ent->flags))
>> reap_victim(nd_mapping, label_ent);
>> }
>>
>> @@ -966,19 +1067,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>> if (rc)
>> return rc;
>>
>> - list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> - if (!label_ent->label) {
>> - label_ent->label = nd_label;
>> - nd_label = NULL;
>> - break;
>> - }
>> - dev_WARN_ONCE(&nspm->nsio.common.dev, nd_label,
>> - "failed to track label: %d\n",
>> - to_slot(ndd, nd_label));
>> - if (nd_label)
>> - rc = -ENXIO;
>> + list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>> + if (label_ent->label)
>> + continue;
>
>This flow change is unrelated to the rest here. I'd push it back
>to the simpler patch that change the locking. Make sure to call it out there
>though. Or just don't do it and keep this patch a little more readable!
Yes, I will fix this in previous patch.
>> @@ -1091,12 +1209,19 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>> count++;
>> WARN_ON_ONCE(!count);
>>
>> - rc = init_labels(nd_mapping, count);
>> + region_label_cnt = find_region_label_count(ndd, nd_mapping);
>> + /*
>> + * init_labels() scan labels and allocate new label based
>> + * on its second parameter (num_labels). Therefore to
>> + * allocate new namespace label also include previously
>> + * added region label
>> + */
>> + rc = init_labels(nd_mapping, count + region_label_cnt);
>> if (rc < 0)
>> return rc;
>>
>> rc = __pmem_label_update(nd_region, nd_mapping, nspm, i,
>> - NSLABEL_FLAG_UPDATING);
>> + NSLABEL_FLAG_UPDATING, NS_LABEL_TYPE);
>
>The amount of shared code in __pmem_label_update() across the two types in
>the union isn't that high. I'd be tempted to just split it for simplicity.
Sure, I will split it out in next patch-set.
>
>> if (rc)
>> return rc;
>> }
>>
[snip]
>> +int nd_region_label_update(struct nd_region *nd_region)
>> +{
>> + int rc;
>> +
>> + nvdimm_bus_lock(&nd_region->dev);
>> + rc = nd_pmem_region_label_update(nd_region);
>> + nvdimm_bus_unlock(&nd_region->dev);
>Looks like it would be worth introducing a
>DEFINE_GUARD() for this lock.
>
>Not necessarily in this series however.
Yes, Fixing it here will add some extra change.
>> +
>> + return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(nd_region_label_update);
>> +
>> static int nd_namespace_label_update(struct nd_region *nd_region,
>> struct device *dev)
>> {
>> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
>> index e362611d82cc..f04c042dcfa9 100644
>> --- a/drivers/nvdimm/nd.h
>> +++ b/drivers/nvdimm/nd.h
>
>> bool nsl_validate_type_guid(struct nvdimm_drvdata *ndd,
>> struct nd_namespace_label *nd_label, guid_t *guid);
>> enum nvdimm_claim_class nsl_get_claim_class(struct nvdimm_drvdata *ndd,
>> @@ -399,7 +432,10 @@ enum nd_label_flags {
>> struct nd_label_ent {
>> struct list_head list;
>> unsigned long flags;
>> - struct nd_namespace_label *label;
>> + union {
>> + struct nd_namespace_label *label;
>> + struct cxl_region_label *region_label;
>
>It is a bit odd to have a union above of the two types in
>here but then union the pointers here.
Yes Jonathan, I will replace this unnamed union with "union nd_lsa_label". I will
also help in avoid extra typecasting in is_region_label() and is_label_reapable()
Regards,
Neeraj
Powered by blists - more mailing lists