[PATCH v3 0/7] bugfix and optimize for drivers/nvdimm

[PATCH v3 0/7] bugfix and optimize for drivers/nvdimm

[Zhen Lei]

v2 --> v3:
1. Fix spelling error of patch 1 subject: memmory --> memory
2. Add "Reviewed-by: Oliver O'Halloran <oohall@gmail.com>" into patch 1
3. Rewrite patch descriptions of Patch 1, 3, 4
4. Add 3 new trivial patches 5-7, I just found that yesterday.
5. Unify all "subsystem" names to "libnvdimm:"

v1 --> v2:
1. Add Fixes for Patch 1-2
2. Slightly change the subject and description of Patch 1
3. Add a new trivial Patch 4, I just found that yesterday.

v1:
I found a memleak when I learned the drivers/nvdimm code today. And I also
added a sanity check for priv->bus_desc.provider_name, because strdup()
maybe failed. Patch 3 is a trivial source code optimization.


Zhen Lei (7):
  libnvdimm: fix memory leaks in of_pmem.c
  libnvdimm: add sanity check for provider_name in
    of_pmem_region_probe()
  libnvdimm: simplify walk_to_nvdimm_bus()
  libnvdimm: reduce an unnecessary if branch in nd_region_create()
  libnvdimm: reduce an unnecessary if branch in nd_region_activate()
  libnvdimm: make sure EXPORT_SYMBOL_GPL(nvdimm_flush) close to its
    function
  libnvdimm: slightly simplify available_slots_show()

 drivers/nvdimm/bus.c         |  7 +++----
 drivers/nvdimm/dimm_devs.c   |  5 ++---
 drivers/nvdimm/of_pmem.c     |  7 +++++++
 drivers/nvdimm/region_devs.c | 13 ++++---------
 4 files changed, 16 insertions(+), 16 deletions(-)

-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v3 1/7] libnvdimm: fix memory leaks in of_pmem.c

[Zhen Lei]

Currently, in the last error path of of_pmem_region_probe() and in
of_pmem_region_remove(), free the memory allocated by kstrdup() is
missing. Add kfree(priv->bus_desc.provider_name) to fix it.

Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus provider")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
---
 drivers/nvdimm/of_pmem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
index 10dbdcdfb9ce913..1292ffca7b2ecc0 100644
--- a/drivers/nvdimm/of_pmem.c
+++ b/drivers/nvdimm/of_pmem.c
@@ -36,6 +36,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
 
 	priv->bus = bus = nvdimm_bus_register(&pdev->dev, &priv->bus_desc);
 	if (!bus) {
+		kfree(priv->bus_desc.provider_name);
 		kfree(priv);
 		return -ENODEV;
 	}
@@ -83,6 +84,7 @@ static int of_pmem_region_remove(struct platform_device *pdev)
 	struct of_pmem_private *priv = platform_get_drvdata(pdev);
 
 	nvdimm_bus_unregister(priv->bus);
+	kfree(priv->bus_desc.provider_name);
 	kfree(priv);
 
 	return 0;
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v3 2/7] libnvdimm: add sanity check for provider_name in of_pmem_region_probe()

[Zhen Lei]

kstrdup() may return NULL because of no memory, check it.

Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus provider")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/of_pmem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
index 1292ffca7b2ecc0..13c4c274ca6ea88 100644
--- a/drivers/nvdimm/of_pmem.c
+++ b/drivers/nvdimm/of_pmem.c
@@ -31,6 +31,11 @@ static int of_pmem_region_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
+	if (!priv->bus_desc.provider_name) {
+		kfree(priv);
+		return -ENOMEM;
+	}
+
 	priv->bus_desc.module = THIS_MODULE;
 	priv->bus_desc.of_node = np;
 
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v3 3/7] libnvdimm: simplify walk_to_nvdimm_bus()

[Zhen Lei]

Return as soon as nvdimm_bus device has been found, make us no need to
check "dev" or "!dev" in subsequent code.

No functional change.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/bus.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 955265656b96c73..1d89114cb6ab93e 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -316,10 +316,9 @@ struct nvdimm_bus *walk_to_nvdimm_bus(struct device *nd_dev)
 
 	for (dev = nd_dev; dev; dev = dev->parent)
 		if (is_nvdimm_bus(dev))
-			break;
-	dev_WARN_ONCE(nd_dev, !dev, "invalid dev, not on nd bus\n");
-	if (dev)
-		return to_nvdimm_bus(dev);
+			return to_nvdimm_bus(dev);
+
+	dev_WARN_ONCE(nd_dev, 1, "invalid dev, not on nd bus\n");
 	return NULL;
 }
 
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v3 4/7] libnvdimm: reduce an unnecessary if branch in nd_region_create()

[Zhen Lei]

if (ndr_desc->flush)
1)	nd_region->flush = ndr_desc->flush;
else
2)	nd_region->flush = NULL;

When entering the "else" branch, the value of ndr_desc->flush is NULL.
After replaced "NULL" with "ndr_desc->flush" at 2), we will find that
it becomes the same to 1).

So the above code snippet can be reduced to one statement:
nd_region->flush = ndr_desc->flush;

No functional change.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/region_devs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index ef23119db574663..7cf9c7d857909ce 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1131,10 +1131,7 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
 	nd_region->ndr_size = resource_size(ndr_desc->res);
 	nd_region->ndr_start = ndr_desc->res->start;
 	nd_region->align = default_align(nd_region);
-	if (ndr_desc->flush)
-		nd_region->flush = ndr_desc->flush;
-	else
-		nd_region->flush = NULL;
+	nd_region->flush = ndr_desc->flush;
 
 	nd_device_register(dev);
 
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v3 5/7] libnvdimm: reduce an unnecessary if branch in nd_region_activate()

[Zhen Lei]

According to the original code logic:
if (!nvdimm->num_flush) {
	flush_data_size += sizeof(void *);
	//nvdimm->num_flush is zero now, add 1) have no side effects
} else {
	flush_data_size += sizeof(void *);
1)	flush_data_size += nvdimm->num_flush * sizeof(void *);
}

Obviously, the above code snippet can be reduced to one statement:
flush_data_size += (nvdimm->num_flush + 1) * sizeof(void *);

No functional change.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/region_devs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 7cf9c7d857909ce..49be115c9189eff 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -77,11 +77,8 @@ int nd_region_activate(struct nd_region *nd_region)
 		}
 
 		/* at least one null hint slot per-dimm for the "no-hint" case */
-		flush_data_size += sizeof(void *);
+		flush_data_size += (nvdimm->num_flush + 1) * sizeof(void *);
 		num_flush = min_not_zero(num_flush, nvdimm->num_flush);
-		if (!nvdimm->num_flush)
-			continue;
-		flush_data_size += nvdimm->num_flush * sizeof(void *);
 	}
 	nvdimm_bus_unlock(&nd_region->dev);
 
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v3 6/7] libnvdimm: make sure EXPORT_SYMBOL_GPL(nvdimm_flush) close to its function

[Zhen Lei]

Move EXPORT_SYMBOL_GPL(nvdimm_flush) close to nvdimm_flush(), currently
it's near to generic_nvdimm_flush().

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/region_devs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 49be115c9189eff..909bf03edb132cf 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1183,6 +1183,8 @@ int nvdimm_flush(struct nd_region *nd_region, struct bio *bio)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(nvdimm_flush);
+
 /**
  * nvdimm_flush - flush any posted write queues between the cpu and pmem media
  * @nd_region: blk or interleaved pmem region
@@ -1214,7 +1216,6 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(nvdimm_flush);
 
 /**
  * nvdimm_has_flush - determine write flushing requirements
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v3 7/7] libnvdimm: slightly simplify available_slots_show()

[Zhen Lei]

The type of "nfree" is u32, so "nfree - 1" can only be overflowed when
"nfree" is zero. Replace "if (nfree - 1 > nfree)" with "if (nfree == 0)"
seems more clear. And remove the assignment "nfree = 0", no need for it.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/dimm_devs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 61374def515552f..bf7d0cdc147cb39 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -347,10 +347,9 @@ static ssize_t available_slots_show(struct device *dev,
 
 	nvdimm_bus_lock(dev);
 	nfree = nd_label_nfree(ndd);
-	if (nfree - 1 > nfree) {
+	if (nfree == 0)
 		dev_WARN_ONCE(dev, 1, "we ate our last label?\n");
-		nfree = 0;
-	} else
+	else
 		nfree--;
 	rc = sprintf(buf, "%d\n", nfree);
 	nvdimm_bus_unlock(dev);
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v3 6/7] libnvdimm: make sure EXPORT_SYMBOL_GPL(nvdimm_flush) close to its function

[Pankaj Gupta]

> Move EXPORT_SYMBOL_GPL(nvdimm_flush) close to nvdimm_flush(), currently
> it's near to generic_nvdimm_flush().
>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  drivers/nvdimm/region_devs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 49be115c9189eff..909bf03edb132cf 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -1183,6 +1183,8 @@ int nvdimm_flush(struct nd_region *nd_region, struct bio *bio)
>
>         return rc;
>  }
> +EXPORT_SYMBOL_GPL(nvdimm_flush);
> +
>  /**
>   * nvdimm_flush - flush any posted write queues between the cpu and pmem media
>   * @nd_region: blk or interleaved pmem region
> @@ -1214,7 +1216,6 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(nvdimm_flush);
>
>  /**
>   * nvdimm_has_flush - determine write flushing requirements
> --

Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>

> 1.8.3
>
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v3 7/7] libnvdimm: slightly simplify available_slots_show()

[Salih Agenter]

https://www.agenter.com/how-to-earn
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v3 0/7] bugfix and optimize for drivers/nvdimm

[Leizhen (ThunderTown)]

Hi all:
  Any comment? I want to merge patches 1 and 2 into one, then send
other patches separately.

On 2020/8/20 10:16, Zhen Lei wrote:
> v2 --> v3:
> 1. Fix spelling error of patch 1 subject: memmory --> memory
> 2. Add "Reviewed-by: Oliver O'Halloran <oohall@gmail.com>" into patch 1
> 3. Rewrite patch descriptions of Patch 1, 3, 4
> 4. Add 3 new trivial patches 5-7, I just found that yesterday.
> 5. Unify all "subsystem" names to "libnvdimm:"
> 
> v1 --> v2:
> 1. Add Fixes for Patch 1-2
> 2. Slightly change the subject and description of Patch 1
> 3. Add a new trivial Patch 4, I just found that yesterday.
> 
> v1:
> I found a memleak when I learned the drivers/nvdimm code today. And I also
> added a sanity check for priv->bus_desc.provider_name, because strdup()
> maybe failed. Patch 3 is a trivial source code optimization.
> 
> 
> Zhen Lei (7):
>   libnvdimm: fix memory leaks in of_pmem.c
>   libnvdimm: add sanity check for provider_name in
>     of_pmem_region_probe()
>   libnvdimm: simplify walk_to_nvdimm_bus()
>   libnvdimm: reduce an unnecessary if branch in nd_region_create()
>   libnvdimm: reduce an unnecessary if branch in nd_region_activate()
>   libnvdimm: make sure EXPORT_SYMBOL_GPL(nvdimm_flush) close to its
>     function
>   libnvdimm: slightly simplify available_slots_show()
> 
>  drivers/nvdimm/bus.c         |  7 +++----
>  drivers/nvdimm/dimm_devs.c   |  5 ++---
>  drivers/nvdimm/of_pmem.c     |  7 +++++++
>  drivers/nvdimm/region_devs.c | 13 ++++---------
>  4 files changed, 16 insertions(+), 16 deletions(-)
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v3 5/7] libnvdimm: reduce an unnecessary if branch in nd_region_activate()

[Leizhen (ThunderTown)]

I will drop this patch, because I have a doubt:
Suppose the nd_region->ndr_mappings is 4, and for each nd_region->mapping[],
the value of num_flush is "0, 0, 4, 0", so the flush_data_size is "1 + 1 + 5 + 1", * sizeof(void *).
But in ndrd_get_flush_wpq() or ndrd_set_flush_wpq(), the expression is
"ndrd->flush_wpq[dimm * num + (hint & mask)]", I don't think the memory "ndrd" allocated is enough.
Please refer call chain: nd_region_activate() --> nvdimm_map_flush() --> ndrd_set_flush_wpq()

	for (i = 0; i < nd_region->ndr_mappings; i++) {
                struct nd_mapping *nd_mapping = &nd_region->mapping[i];
                struct nvdimm *nvdimm = nd_mapping->nvdimm;

                /* at least one null hint slot per-dimm for the "no-hint" case */
                flush_data_size += sizeof(void *);
                num_flush = min_not_zero(num_flush, nvdimm->num_flush);
                if (!nvdimm->num_flush)
                        continue;
                flush_data_size += nvdimm->num_flush * sizeof(void *);
        }

        ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);




On 2020/8/20 10:16, Zhen Lei wrote:
> According to the original code logic:
> if (!nvdimm->num_flush) {
> 	flush_data_size += sizeof(void *);
> 	//nvdimm->num_flush is zero now, add 1) have no side effects
> } else {
> 	flush_data_size += sizeof(void *);
> 1)	flush_data_size += nvdimm->num_flush * sizeof(void *);
> }
> 
> Obviously, the above code snippet can be reduced to one statement:
> flush_data_size += (nvdimm->num_flush + 1) * sizeof(void *);
> 
> No functional change.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  drivers/nvdimm/region_devs.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 7cf9c7d857909ce..49be115c9189eff 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -77,11 +77,8 @@ int nd_region_activate(struct nd_region *nd_region)
>  		}
>  
>  		/* at least one null hint slot per-dimm for the "no-hint" case */
> -		flush_data_size += sizeof(void *);
> +		flush_data_size += (nvdimm->num_flush + 1) * sizeof(void *);
>  		num_flush = min_not_zero(num_flush, nvdimm->num_flush);
> -		if (!nvdimm->num_flush)
> -			continue;
> -		flush_data_size += nvdimm->num_flush * sizeof(void *);
>  	}
>  	nvdimm_bus_unlock(&nd_region->dev);
>  
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org