Re: [PATCH V2 05/20] nvdimm/region_label: Add region label updation routine

[Neeraj Kumar]

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