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

[Ira Weiny <ira.weiny@intel.com>]

RE Subject: [PATCH V2 05/20] nvdimm/region_label: Add region label updation routine
                                                                   ^^^^^^^
								   update

Neeraj Kumar wrote:
> Added __pmem_region_label_update region label update routine to update
  ^^^
  Add

> region label.

How about:

Add __pmem_region_label_update to update region labels.  ???

But is that really what this patch is doing?  And why do we need such a
function?

Why is __pmem_label_update changing?

> 
> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
> mutex_unlock()

Why?

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.

>  	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?

>  	};
>  };
>  
> @@ -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

[snip]