On 17/12/25 02:31PM, Jonathan Cameron wrote: >On Wed, 19 Nov 2025 13:22:41 +0530 >Neeraj Kumar wrote: > >> Modify __pmem_label_update() to update region labels into LSA >> >> CXL 3.2 Spec mentions CXL LSA 2.1 Namespace Labels at section 9.13.2.5 >> Modified __pmem_label_update() using setter functions to update >> namespace label as per CXL LSA 2.1 >> >> Create export routine nd_region_label_update() used for creating >> region label into LSA. It will be used later from CXL subsystem >> >> Signed-off-by: Neeraj Kumar >Hi Neeraj, > >A few things inline from a fresh read. > >Thanks, > >Jonathan > >> --- >> drivers/nvdimm/label.c | 360 ++++++++++++++++++++++++++------ >> drivers/nvdimm/label.h | 17 +- >> drivers/nvdimm/namespace_devs.c | 25 ++- >> drivers/nvdimm/nd.h | 66 ++++++ >> include/linux/libnvdimm.h | 8 + >> 5 files changed, 406 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c >> index 0a9b6c5cb2c3..0d587a5b9f7e 100644 >> --- a/drivers/nvdimm/label.c >> +++ b/drivers/nvdimm/label.c > > > > >> @@ -978,7 +1132,8 @@ static int __pmem_label_update(struct nd_region *nd_region, >> return rc; >> } > >> int nd_pmem_namespace_label_update(struct nd_region *nd_region, >> struct nd_namespace_pmem *nspm, resource_size_t size) >> { >> @@ -1075,6 +1253,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; > >Always initialized anyway before use I think. So no need to do it here. Fixed it in V5 > >> struct resource *res; >> int count = 0; >> >> @@ -1090,12 +1269,20 @@ 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(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, >> + NS_LABEL_TYPE); >> if (rc < 0) >> return rc; >> >> rc = __pmem_label_update(nd_region, nd_mapping, nspm, i, >> - NSLABEL_FLAG_UPDATING); >> + NSLABEL_FLAG_UPDATING, NS_LABEL_TYPE); >> if (rc) >> return rc; >> } > >> +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; > >Seems to always be initialized before use anyway so no need to do it here. Fixed it in V5 > >> + >> + /* No need to update region label for non cxl format */ >> + if (!ndd->cxl) >> + return 0; >> + >> + region_label_cnt = find_region_label_count(nd_mapping); >> + rc = init_labels(nd_mapping, region_label_cnt + 1, >> + RG_LABEL_TYPE); >> + 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/namespace_devs.c b/drivers/nvdimm/namespace_devs.c >> index 43fdb806532e..b1abbe602a5e 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); > >In line with much of the nvdimm stuff I'd use guard and save a couple of lines. > > guard(nvdimm_bus)(&nd_region->dev); > return nd_pmem_region_label_update(nd_region); > Fixed it in V5 >> + >> + return rc; >> +} >> +EXPORT_SYMBOL_GPL(nd_region_label_update); > >> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h >> index f631bd84d6f0..5fd69c28ffe7 100644 >> --- a/drivers/nvdimm/nd.h >> +++ b/drivers/nvdimm/nd.h >> @@ -14,6 +14,9 @@ >> #include >> #include "label.h" >> >> +extern uuid_t cxl_namespace_uuid; >> +extern uuid_t cxl_region_uuid; >> + >> enum { >> /* >> * Limits the maximum number of block apertures a dimm can >> @@ -295,6 +298,67 @@ static inline const u8 *nsl_uuid_raw(struct nvdimm_drvdata *ndd, >> return nd_label->efi.uuid; >> } >> >> +static inline void nsl_set_type(struct nvdimm_drvdata *ndd, >> + struct nd_namespace_label *ns_label) >> +{ >> + if (!(ndd->cxl && ns_label)) >> + return; >> + >> + uuid_copy((uuid_t *)ns_label->cxl.type, &cxl_namespace_uuid); > >uuid_import() perhaps more appropriate given it is coming(I think) >from a __u8 &. > Yes we can avoid extra typecasting in uuid_copy using uuid_export(). Actually cxl_namespace_uuid is of type uuid_t and ns_label->cxl.type is of type __u8 So export_uuid() is appropriate here than uuid_inport() I have fixed it in V5 >> +} >> + > > >> + >> +static inline bool is_region_label(struct nvdimm_drvdata *ndd, >> + union nd_lsa_label *lsa_label) >> +{ >> + uuid_t *region_type; >> + >> + if (!ndd->cxl) >> + return false; >> + >> + region_type = (uuid_t *) lsa_label->region_label.type; >> + return uuid_equal(&cxl_region_uuid, region_type) > >I'd match style of next function and not have the local variable. > > return uuid_equal(&cxl_region_uuid, > (uuid_t *)lsa_label->region_label.type); Fixed it in V5 >> +} >> + >> +static inline bool >> +region_label_uuid_equal(struct cxl_region_label *region_label, >> + const uuid_t *uuid) >> +{ >> + return uuid_equal((uuid_t *) region_label->uuid, uuid); > >Dave pointed out that there shouldn't be a space after the cast. >Make sure you catch all of these. > Fixed it in V5 >> +} >> + >> +static inline u64 >> +region_label_get_checksum(struct cxl_region_label *region_label) >> +{ >> + return __le64_to_cpu(region_label->checksum); >> +} >> + >> +static inline void >> +region_label_set_checksum(struct cxl_region_label *region_label, >> + u64 checksum) >> +{ >> + region_label->checksum = __cpu_to_le64(checksum); >> +} > >Perhaps add a little justification to the patch description on why these >get/set are helpful? Seems like just setting them directly would perhaps >be fine as all call sites can see the structure definition anyway? > Thanks Jonathan, I have used them directly instead of extra setter function in V5 Regards, Neeraj