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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
>
>
On Wed, Aug 19, 2020 at 12:05 PM Zhen Lei <[email protected]> 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 <[email protected]>
Yep, that's a bug.
Reviewed-by: Oliver O'Halloran <[email protected]>
> ---
> 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
>
>
On 8/19/2020 9:37 PM, Oliver O'Halloran wrote:
> On Wed, Aug 19, 2020 at 12:05 PM Zhen Lei <[email protected]> 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 <[email protected]>
>
> Yep, that's a bug.
>
> Reviewed-by: Oliver O'Halloran <[email protected]>
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
>>
>>
>
> .
>