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]
