On 9/17/25 8:36 AM, Jonathan Cameron wrote:
> On Wed, 17 Sep 2025 19:11:02 +0530
> Neeraj Kumar <[email protected]> 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.
>
>> Signed-off-by: Neeraj Kumar <[email protected]>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
>> index 182f8c9a01bf..209c73f6b7e7 100644
>> --- a/drivers/nvdimm/label.c
>> +++ b/drivers/nvdimm/label.c
>> @@ -381,6 +381,16 @@ static void nsl_calculate_checksum(struct
>> nvdimm_drvdata *ndd,
>> nsl_set_checksum(ndd, nd_label, sum);
>> }
>
>> +static bool is_label_reapable(struct nd_interleave_set *nd_set,
>> + struct nd_namespace_pmem *nspm,
>> + struct nvdimm_drvdata *ndd,
>> + union nd_lsa_label *label,
>> + enum label_type ltype,
>> + unsigned long *flags)
>> +{
>> + switch (ltype) {
>> + case NS_LABEL_TYPE:
>> + if (test_and_clear_bit(ND_LABEL_REAP, flags) ||
>> + nsl_uuid_equal(ndd, &label->ns_label, nspm->uuid))
>> + return true;
>> +
>> + break;
>> + case RG_LABEL_TYPE:
>> + if (region_label_uuid_equal(&label->region_label,
>> + &nd_set->uuid))
>> + return true;
>> +
>> + break;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int __pmem_label_update(struct nd_region *nd_region,
>> + struct nd_mapping *nd_mapping,
>> + struct nd_namespace_pmem *nspm,
>> + int pos, unsigned long flags,
>> + enum label_type ltype)
>> +{
>> + struct nd_interleave_set *nd_set = nd_region->nd_set;
>> + struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
>> + struct nd_namespace_index *nsindex;
>> + struct nd_label_ent *label_ent;
>> + union nd_lsa_label *lsa_label;
>> + unsigned long *free;
>> + struct device *dev;
>> + u32 nslot, slot;
>> + size_t offset;
>> + int rc;
>> +
>> + if (!preamble_next(ndd, &nsindex, &free, &nslot))
>> + return -ENXIO;
>> +
>> /* allocate and write the label to the staging (next) index */
>> slot = nd_label_alloc_slot(ndd);
>> if (slot == UINT_MAX)
>> return -ENXIO;
>> dev_dbg(ndd->dev, "allocated: %d\n", slot);
>>
>> - nd_label = to_label(ndd, slot);
>> - memset(nd_label, 0, sizeof_namespace_label(ndd));
>> - 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);
>> - nsl_set_nlabel(ndd, nd_label, nd_region->ndr_mappings);
>> - nsl_set_nrange(ndd, nd_label, 1);
>> - 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.
>
>> + 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.
>
>> + 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.
>
>> + 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!
>
>>
>> - return rc;
>> + label_ent->label = &lsa_label->ns_label;
>> + lsa_label = NULL;
>> + break;
>> + }
>> + dev_WARN_ONCE(dev, lsa_label, "failed to track label: %d\n",
>> + to_slot(ndd, &lsa_label->ns_label));
>> + if (lsa_label)
>> + return -ENXIO;
>> +
>> + return 0;
>> }
>>
>> static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
>> @@ -1068,6 +1170,21 @@ static int del_labels(struct nd_mapping *nd_mapping,
>> uuid_t *uuid)
>> nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
>> }
>>
>> +static int find_region_label_count(struct nvdimm_drvdata *ndd,
>> + struct nd_mapping *nd_mapping)
>> +{
>> + struct nd_label_ent *label_ent;
>> + int region_label_cnt = 0;
>> +
>> + guard(mutex)(&nd_mapping->lock);
>> + list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> + if (is_region_label(ndd,
>> + (union nd_lsa_label *) label_ent->label))
>
> As above. If it's a union make label_ent->label that union type.
>
>> + region_label_cnt++;
>> +
>> + return region_label_cnt;
>> +}
>> +
>> int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>> struct nd_namespace_pmem *nspm, resource_size_t size)
>> {
>> @@ -1076,6 +1193,7 @@ int nd_pmem_namespace_label_update(struct nd_region
>> *nd_region,
>> 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);
>> + int region_label_cnt = 0;
>> struct resource *res;
>> int count = 0;
>>
>> @@ -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.
>
>> if (rc)
>> return rc;
>> }
>> @@ -1108,7 +1233,47 @@ int nd_pmem_namespace_label_update(struct nd_region
>> *nd_region,
>> for (i = 0; i < nd_region->ndr_mappings; i++) {
>> struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>>
>> - rc = __pmem_label_update(nd_region, nd_mapping, nspm, i, 0);
>> + rc = __pmem_label_update(nd_region, nd_mapping, nspm, i, 0,
>> + NS_LABEL_TYPE);
>> + if (rc)
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int nd_pmem_region_label_update(struct nd_region *nd_region)
>> +{
>> + int i, rc;
>> +
>> + 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);
>> + int region_label_cnt = 0;
>> +
>> + /* No need to update region label for non cxl format */
>> + if (!ndd->cxl)
>> + return 0;
>> +
>> + region_label_cnt = find_region_label_count(ndd, nd_mapping);
>> + rc = init_labels(nd_mapping, region_label_cnt + 1);
>> + if (rc < 0)
>> + return rc;
>> +
>> + rc = __pmem_label_update(nd_region, nd_mapping, NULL, i,
>> + NSLABEL_FLAG_UPDATING, RG_LABEL_TYPE);
>> + if (rc)
>> + return rc;
>> + }
>> +
>> + /* Clear the UPDATING flag per UEFI 2.7 expectations */
>> + 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);
>> +
>> + WARN_ON_ONCE(!ndd->cxl);
>> + rc = __pmem_label_update(nd_region, nd_mapping, NULL, i, 0,
>> + RG_LABEL_TYPE);
>> if (rc)
>> return rc;
>> }
>> diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
>> index 0650fb4b9821..284e2a763b49 100644
>> --- a/drivers/nvdimm/label.h
>> +++ b/drivers/nvdimm/label.h
>> @@ -30,6 +30,11 @@ enum {
>> ND_NSINDEX_INIT = 0x1,
>> };
>>
>> +enum label_type {
>> + RG_LABEL_TYPE,
>> + NS_LABEL_TYPE,
>> +};
>> +
>> /**
>> * struct nd_namespace_index - label set superblock
>> * @sig: NAMESPACE_INDEX\0
>> @@ -183,6 +188,15 @@ 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
>> + */
>> +union nd_lsa_label {
>> + struct nd_namespace_label ns_label;
>> + struct cxl_region_label region_label;
>> +};
>
>> diff --git a/drivers/nvdimm/namespace_devs.c
>> b/drivers/nvdimm/namespace_devs.c
>> index 3271b1c8569a..559f822ef24f 100644
>> --- a/drivers/nvdimm/namespace_devs.c
>> +++ b/drivers/nvdimm/namespace_devs.c
>> @@ -232,6 +232,18 @@ static ssize_t __alt_name_store(struct device *dev,
>> const char *buf,
>> return rc;
>> }
>>
>> +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.
https://lore.kernel.org/nvdimm/[email protected]/T/#t
Merged in nvdimm/next for 6.18.
DJ
>> +
>> + 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.
>
> I'm also failing to find where region_label is used in this patch.
>
>
>> + };
>> };
>>
>> enum nd_mapping_lock_class {
> _rc)
>
>
