[PATCH v2 0/4] bugfix and optimize for drivers/nvdimm

[PATCH v2 0/4] bugfix and optimize for drivers/nvdimm

[Zhen Lei]

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 (4):
  libnvdimm: fix memmory leaks in of_pmem.c
  libnvdimm: add sanity check for provider_name in
    of_pmem_region_probe()
  libnvdimm/bus: simplify walk_to_nvdimm_bus()
  libnvdimm/region: reduce an unnecessary if branch in
    nd_region_create()

 drivers/nvdimm/bus.c         | 7 +++----
 drivers/nvdimm/of_pmem.c     | 7 +++++++
 drivers/nvdimm/region_devs.c | 5 +----
 3 files changed, 11 insertions(+), 8 deletions(-)

-- 
1.8.3

[PATCH v2 1/4] libnvdimm: fix memmory leaks in of_pmem.c

[Zhen Lei]

The memory priv->bus_desc.provider_name allocated by kstrdup() is not
freed correctly.

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

Re: [PATCH v2 1/4] libnvdimm: Fix memory leaks in of_pmem.c

[Leizhen (ThunderTown)]

On 8/19/2020 8:28 PM, Markus Elfring wrote:
>> The memory priv->bus_desc.provider_name allocated by kstrdup() is not
>> freed correctly.
> 
> How do you think about to choose an imperative wording for
> a corresponding change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=18445bf405cb331117bc98427b1ba6f12418ad17#n151

OK, thanks. I think I known what "imperative wording" means now.
I will rewrite the descriptions.

> 
> Regards,
> Markus
> 
> 

Re: [PATCH v2 1/4] libnvdimm: Fix memory leaks in of_pmem.c

[Oliver O'Halloran]

On Wed, Aug 19, 2020 at 10:28 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > The memory priv->bus_desc.provider_name allocated by kstrdup() is not
> > freed correctly.

Personally I thought his commit message was perfectly fine. A little
unorthodox, but it works.

> How do you think about to choose an imperative wording for
> a corresponding change description?

...but this! This is word salad.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=18445bf405cb331117bc98427b1ba6f12418ad17#n151
>
> Regards,
> Markus

Re: [PATCH v2 1/4] libnvdimm: Fix memory leaks in of_pmem.c

[Leizhen (ThunderTown)]

On 8/19/2020 9:35 PM, Oliver O'Halloran wrote:
> On Wed, Aug 19, 2020 at 10:28 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>>
>>> The memory priv->bus_desc.provider_name allocated by kstrdup() is not
>>> freed correctly.
> 
> Personally I thought his commit message was perfectly fine. A little
> unorthodox, but it works.
> 
>> How do you think about to choose an imperative wording for
>> a corresponding change description?
> 
> ...but this! This is word salad.

Talented students are trained by strict teacher. All of us is trying to make things better.

> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=18445bf405cb331117bc98427b1ba6f12418ad17#n151
>>
>> Regards,
>> Markus
> 
> 

Re: [PATCH v2 1/4] libnvdimm: fix memmory leaks in of_pmem.c

[Oliver O'Halloran]

On Wed, Aug 19, 2020 at 12:05 PM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>
> The memory priv->bus_desc.provider_name allocated by kstrdup() is not
> freed correctly.
>
> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus provider")
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

Yep, that's a bug.

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

Re: [PATCH v2 1/4] libnvdimm: fix memmory leaks in of_pmem.c

[Leizhen (ThunderTown)]

On 8/19/2020 9:37 PM, Oliver O'Halloran wrote:
> On Wed, Aug 19, 2020 at 12:05 PM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>
>> The memory priv->bus_desc.provider_name allocated by kstrdup() is not
>> freed correctly.
>>
>> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus provider")
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Yep, that's a bug.
> 
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>

Thanks for your review.

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

[PATCH v2 2/4] 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

[PATCH v2 3/4] libnvdimm/bus: simplify walk_to_nvdimm_bus()

[Zhen Lei]

I first want to move dev_WARN_ONCE() after "if (dev)" branch, but further
I find the "if (dev)" can only be true when is_nvdimm_bus(dev) successed.

No functional change. In fact, the compiler can optimize it correctly. I
run "size drivers/nvdimm/bus.o" and find nothing has changed. So it's
just source code level optimization, make us can read it faster.

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

Re: [PATCH v2 3/4] libnvdimm/bus: simplify walk_to_nvdimm_bus()

[Leizhen (ThunderTown)]

On 8/19/2020 8:40 PM, Markus Elfring wrote:
>> … when is_nvdimm_bus(dev) successed.
> 
> I imagine that that an other wording will be more appropriate here.

OK, I will rewrite it.

> 
> Regards,
> Markus
> 
> 

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

[Zhen Lei]

The "else" branch can only be entered when ndr_desc->flush is NULL. After
replaced "NULL" with "ndr_desc->flush", we will find that the statements
in "if..else.." are the same.

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

Re: [PATCH v2 0/4] bug fix and optimize for drivers/nvdimm

[Leizhen (ThunderTown)]

On 8/19/2020 8:20 PM, Markus Elfring wrote:
>> v1 --> v2:
>> 1. Add Fixes for Patch 1-2
>> 2. Slightly change the subject and description of Patch 1
> …
>>   libnvdimm: fix memmory leaks in of_pmem.c
> …
> 
> I suggest to avoid a typo in such a patch subject.

OK, Thanks for reminding me.

> 
> Regards,
> Markus
> 
>