[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <439928219.101757055783676.JavaMail.epsvc@epcpadp2new>
Date: Thu, 4 Sep 2025 19:48:21 +0530
From: Neeraj Kumar <s.neeraj@...sung.com>
To: Ira Weiny <ira.weiny@...el.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 V2 05/20] nvdimm/region_label: Add region label updation
routine
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
Powered by blists - more mailing lists