On 19/08/25 01:16PM, Ira Weiny wrote:
RE Subject: [PATCH V2 05/20] nvdimm/region_label: Add region label updation
routine
^^^^^^^
update
Sure, I will fix it in next patch-set
Neeraj Kumar wrote:
Added __pmem_region_label_update region label update routine to update
^^^
Add
Sure, I will fix it in next patch-set
region label.
How about:
Add __pmem_region_label_update to update region labels. ???
May be I will re-use __pmem_label_update() for region label also.
But is that really what this patch is doing? And why do we need such a
function?
Why is __pmem_label_update changing?
__pmem_label_update() is changing because modification of mutex locking.
Yes, Its not really related hunk, So I will handle it in separate
patch-set.
Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
mutex_unlock()
Why?
As per Jonathan's comment on V1 have modified it, and added it in commit
message. seems its not required in commit message. I will remove it
I'm not full out naking the patch but I think its purpose needs to be
clear.
More below...
[snip]
static bool slot_valid(struct nvdimm_drvdata *ndd,
struct nd_lsa_label *lsa_label, u32 slot)
{
@@ -960,7 +970,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
return rc;
/* Garbage collect the previous label */
- mutex_lock(&nd_mapping->lock);
+ guard(mutex)(&nd_mapping->lock);
This, and the following hunks, looks like a completely separate change and
should be in it's own pre-patch with a justification of the change.
Yes this hunk is not related, So I will create a separate patch for this change
list_for_each_entry(label_ent, &nd_mapping->labels, list) {
if (!label_ent->label)
continue;
@@ -972,20 +982,20 @@ static int __pmem_label_update(struct nd_region
*nd_region,
/* update index */
rc = nd_label_write_index(ndd, ndd->ns_next,
nd_inc_seq(__le32_to_cpu(nsindex->seq)), 0);
- if (rc == 0) {
- list_for_each_entry(label_ent, &nd_mapping->labels, list)
- if (!label_ent->label) {
- label_ent->label = lsa_label;
- lsa_label = NULL;
- break;
- }
- dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
- "failed to track label: %d\n",
- to_slot(ndd, lsa_label));
- if (lsa_label)
- rc = -ENXIO;
- }
- mutex_unlock(&nd_mapping->lock);
+ if (rc)
+ return rc;
+
+ list_for_each_entry(label_ent, &nd_mapping->labels, list)
+ if (!label_ent->label) {
+ label_ent->label = lsa_label;
+ lsa_label = NULL;
+ break;
+ }
+ dev_WARN_ONCE(&nspm->nsio.common.dev, lsa_label,
+ "failed to track label: %d\n",
+ to_slot(ndd, lsa_label));
+ if (lsa_label)
+ rc = -ENXIO;
return rc;
}
@@ -1127,6 +1137,137 @@ int nd_pmem_namespace_label_update(struct nd_region
*nd_region,
return 0;
}
[snip]
diff --git a/drivers/nvdimm/label.h b/drivers/nvdimm/label.h
index 4883b3a1320f..0f428695017d 100644
--- a/drivers/nvdimm/label.h
+++ b/drivers/nvdimm/label.h
@@ -190,6 +190,7 @@ struct nd_namespace_label {
struct nd_lsa_label {
union {
struct nd_namespace_label ns_label;
+ struct cxl_region_label rg_label;
Why can't struct cxl_region_label be it's own structure? Or just be part
of the nd_namespace_label union?
Thanks Ira for your suggestion. I will revisit this change and try using
region label handling separately instead of using union.
};
};
@@ -233,4 +234,5 @@ struct nd_region;
struct nd_namespace_pmem;
int nd_pmem_namespace_label_update(struct nd_region *nd_region,
struct nd_namespace_pmem *nspm, resource_size_t size);
+int nd_pmem_region_label_update(struct nd_region *nd_region);
#endif /* __LABEL_H__ */
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 5b73119dc8fd..02ae8162566c 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)
Is this called in a later patch?
Ira
Yes its called from patch 20 (cxl/core/pmem_region.c) by
region_label_update_store()
Regards,
Neeraj
