* [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-14 15:45 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2015-01-14 15:45 UTC (permalink / raw)
To: Grant Likely, Rob Herring
Cc: Gaurav Minocha, devicetree, linux-kernel, Geert Uytterhoeven
When traversing all nodes and moving to a new path component, the old
one must be released by calling of_node_put(). Else the refcounts of the
parent node(s) will not be decremented.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Background.
While investigating a reference count imbalance issue with
CONFIG_OF_DYNAMIC=y, I wrote the debug code below to validate the
reference counts of the nodes I was interested in.
During the first call of check_refcnts(), it gathers all reference
counts. During a subsequent call, it verifies that they are still the
same.
Surprisingly, lots of reference counts were wrong, and kept incrementing
every time check_refcnts() was called.
I was just wondering whether it would be useful to have a reference
count test in OF_UNITTEST, but now I see the "select OF_DYNAMIC" will go
away?
Feel free to (ab)use the code below and derive a unittest from it...
static struct to_check {
const char *path;
int refcnt;
} to_check[] = {
{ "/" },
{ "/cpus/cpu@0" },
{ "/cpus/cpu@1" },
/* ... other paths I was interested in ... */
};
static void check_refcnts(void)
{
static bool called;
unsigned int i;
const char *path;
struct device_node *np;
int refcnt;
unsigned int errors = 0;
pr_info("----- %s reference counts -----\n",
called ? "Checking" : "Saving");
for (i = 0; i < ARRAY_SIZE(to_check); i++) {
path = to_check[i].path;
np = of_find_node_by_path(path);
if (!np)
continue;
refcnt = atomic_read(&np->kobj.kref.refcount);
if (!called) {
pr_info("%s %d\n", path, refcnt);
to_check[i].refcnt = refcnt;
} else if (refcnt == to_check[i].refcnt) {
pr_info("%s %d (OK)\n", path, refcnt);
} else {
pr_info("%s %d (should be %d)\n", path, refcnt,
to_check[i].refcnt);
errors++;
}
of_node_put(np);
}
if (called)
pr_info("----- Checking done (%u errors) -----\n", errors);
else
pr_info("----- Saving done -----\n");
called = true;
}
---
drivers/of/base.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 36536b6a8834acd2..f3e346e19c69d1f2 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -791,8 +791,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
if (!np)
np = of_node_get(of_root);
while (np && *path == '/') {
+ struct device_node *parent = np;
path++; /* Increment past '/' delimiter */
- np = __of_find_node_by_path(np, path);
+ np = __of_find_node_by_path(parent, path);
+ of_node_put(parent);
path = strchrnul(path, '/');
}
raw_spin_unlock_irqrestore(&devtree_lock, flags);
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-14 15:45 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2015-01-14 15:45 UTC (permalink / raw)
To: Grant Likely, Rob Herring
Cc: Gaurav Minocha, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven
When traversing all nodes and moving to a new path component, the old
one must be released by calling of_node_put(). Else the refcounts of the
parent node(s) will not be decremented.
Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
Background.
While investigating a reference count imbalance issue with
CONFIG_OF_DYNAMIC=y, I wrote the debug code below to validate the
reference counts of the nodes I was interested in.
During the first call of check_refcnts(), it gathers all reference
counts. During a subsequent call, it verifies that they are still the
same.
Surprisingly, lots of reference counts were wrong, and kept incrementing
every time check_refcnts() was called.
I was just wondering whether it would be useful to have a reference
count test in OF_UNITTEST, but now I see the "select OF_DYNAMIC" will go
away?
Feel free to (ab)use the code below and derive a unittest from it...
static struct to_check {
const char *path;
int refcnt;
} to_check[] = {
{ "/" },
{ "/cpus/cpu@0" },
{ "/cpus/cpu@1" },
/* ... other paths I was interested in ... */
};
static void check_refcnts(void)
{
static bool called;
unsigned int i;
const char *path;
struct device_node *np;
int refcnt;
unsigned int errors = 0;
pr_info("----- %s reference counts -----\n",
called ? "Checking" : "Saving");
for (i = 0; i < ARRAY_SIZE(to_check); i++) {
path = to_check[i].path;
np = of_find_node_by_path(path);
if (!np)
continue;
refcnt = atomic_read(&np->kobj.kref.refcount);
if (!called) {
pr_info("%s %d\n", path, refcnt);
to_check[i].refcnt = refcnt;
} else if (refcnt == to_check[i].refcnt) {
pr_info("%s %d (OK)\n", path, refcnt);
} else {
pr_info("%s %d (should be %d)\n", path, refcnt,
to_check[i].refcnt);
errors++;
}
of_node_put(np);
}
if (called)
pr_info("----- Checking done (%u errors) -----\n", errors);
else
pr_info("----- Saving done -----\n");
called = true;
}
---
drivers/of/base.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 36536b6a8834acd2..f3e346e19c69d1f2 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -791,8 +791,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
if (!np)
np = of_node_get(of_root);
while (np && *path == '/') {
+ struct device_node *parent = np;
path++; /* Increment past '/' delimiter */
- np = __of_find_node_by_path(np, path);
+ np = __of_find_node_by_path(parent, path);
+ of_node_put(parent);
path = strchrnul(path, '/');
}
raw_spin_unlock_irqrestore(&devtree_lock, flags);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-22 16:14 ` Grant Likely
0 siblings, 0 replies; 9+ messages in thread
From: Grant Likely @ 2015-01-22 16:14 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring
Cc: Gaurav Minocha, devicetree, linux-kernel, Geert Uytterhoeven
On Wed, 14 Jan 2015 16:45:56 +0100
, Geert Uytterhoeven <geert+renesas@glider.be>
wrote:
> When traversing all nodes and moving to a new path component, the old
> one must be released by calling of_node_put(). Else the refcounts of the
> parent node(s) will not be decremented.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Background.
>
> While investigating a reference count imbalance issue with
> CONFIG_OF_DYNAMIC=y, I wrote the debug code below to validate the
> reference counts of the nodes I was interested in.
> During the first call of check_refcnts(), it gathers all reference
> counts. During a subsequent call, it verifies that they are still the
> same.
>
> Surprisingly, lots of reference counts were wrong, and kept incrementing
> every time check_refcnts() was called.
Yup, reference counting is a mess. We definitely need tests to make sure
the core code does the right things with them.
> I was just wondering whether it would be useful to have a reference
> count test in OF_UNITTEST, but now I see the "select OF_DYNAMIC" will go
> away?
select OF_DYNAMIC is going away, but that only so that unittests work
with both OF_DYNAMIC and !OF_DYANMIC. Instead there will be some
unittests that are only run when OF_DYNAMIC is selected.
>
> Feel free to (ab)use the code below and derive a unittest from it...
>
> static struct to_check {
> const char *path;
> int refcnt;
> } to_check[] = {
> { "/" },
> { "/cpus/cpu@0" },
> { "/cpus/cpu@1" },
> /* ... other paths I was interested in ... */
> };
>
> static void check_refcnts(void)
> {
> static bool called;
> unsigned int i;
> const char *path;
> struct device_node *np;
> int refcnt;
> unsigned int errors = 0;
>
> pr_info("----- %s reference counts -----\n",
> called ? "Checking" : "Saving");
> for (i = 0; i < ARRAY_SIZE(to_check); i++) {
> path = to_check[i].path;
> np = of_find_node_by_path(path);
> if (!np)
> continue;
>
> refcnt = atomic_read(&np->kobj.kref.refcount);
> if (!called) {
> pr_info("%s %d\n", path, refcnt);
> to_check[i].refcnt = refcnt;
> } else if (refcnt == to_check[i].refcnt) {
> pr_info("%s %d (OK)\n", path, refcnt);
> } else {
> pr_info("%s %d (should be %d)\n", path, refcnt,
> to_check[i].refcnt);
> errors++;
> }
>
> of_node_put(np);
> }
>
> if (called)
> pr_info("----- Checking done (%u errors) -----\n", errors);
> else
> pr_info("----- Saving done -----\n");
>
> called = true;
> }
> ---
> drivers/of/base.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 36536b6a8834acd2..f3e346e19c69d1f2 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -791,8 +791,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
> if (!np)
> np = of_node_get(of_root);
> while (np && *path == '/') {
> + struct device_node *parent = np;
> path++; /* Increment past '/' delimiter */
> - np = __of_find_node_by_path(np, path);
> + np = __of_find_node_by_path(parent, path);
> + of_node_put(parent);
> path = strchrnul(path, '/');
> }
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
This doesn't look /quite/ the best. __for_each_child_of_node() is
fiddling with refcounts, but the '__' of functions shouldn't need to do
that since they are called under the spinlock (nothing is going to
change while they are called). __of_find_all_nodes() for instance
doesn't do refcounting, but of_find_all_nodes() does.
Does the following also solve the problem?
g.
---
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 36536b6a8834..0357b51a7440 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -626,9 +626,8 @@ static struct device_node *__of_get_next_child(const struct device_node *node,
next = prev ? prev->sibling : node->child;
for (; next; next = next->sibling)
- if (of_node_get(next))
+ if (next)
break;
- of_node_put(prev);
return next;
}
#define __for_each_child_of_node(parent, child) \
@@ -650,7 +649,8 @@ struct device_node *of_get_next_child(const struct device_node *node,
unsigned long flags;
raw_spin_lock_irqsave(&devtree_lock, flags);
- next = __of_get_next_child(node, prev);
+ next = of_node_get(__of_get_next_child(node, prev));
+ of_node_put(prev);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return next;
}
@@ -789,12 +789,13 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
/* Step down the tree matching path components */
raw_spin_lock_irqsave(&devtree_lock, flags);
if (!np)
- np = of_node_get(of_root);
+ np = of_root;
while (np && *path == '/') {
path++; /* Increment past '/' delimiter */
np = __of_find_node_by_path(np, path);
path = strchrnul(path, '/');
}
+ of_node_get(np);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-22 16:14 ` Grant Likely
0 siblings, 0 replies; 9+ messages in thread
From: Grant Likely @ 2015-01-22 16:14 UTC (permalink / raw)
To: Rob Herring
Cc: Gaurav Minocha, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven
On Wed, 14 Jan 2015 16:45:56 +0100
, Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
wrote:
> When traversing all nodes and moving to a new path component, the old
> one must be released by calling of_node_put(). Else the refcounts of the
> parent node(s) will not be decremented.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> ---
> Background.
>
> While investigating a reference count imbalance issue with
> CONFIG_OF_DYNAMIC=y, I wrote the debug code below to validate the
> reference counts of the nodes I was interested in.
> During the first call of check_refcnts(), it gathers all reference
> counts. During a subsequent call, it verifies that they are still the
> same.
>
> Surprisingly, lots of reference counts were wrong, and kept incrementing
> every time check_refcnts() was called.
Yup, reference counting is a mess. We definitely need tests to make sure
the core code does the right things with them.
> I was just wondering whether it would be useful to have a reference
> count test in OF_UNITTEST, but now I see the "select OF_DYNAMIC" will go
> away?
select OF_DYNAMIC is going away, but that only so that unittests work
with both OF_DYNAMIC and !OF_DYANMIC. Instead there will be some
unittests that are only run when OF_DYNAMIC is selected.
>
> Feel free to (ab)use the code below and derive a unittest from it...
>
> static struct to_check {
> const char *path;
> int refcnt;
> } to_check[] = {
> { "/" },
> { "/cpus/cpu@0" },
> { "/cpus/cpu@1" },
> /* ... other paths I was interested in ... */
> };
>
> static void check_refcnts(void)
> {
> static bool called;
> unsigned int i;
> const char *path;
> struct device_node *np;
> int refcnt;
> unsigned int errors = 0;
>
> pr_info("----- %s reference counts -----\n",
> called ? "Checking" : "Saving");
> for (i = 0; i < ARRAY_SIZE(to_check); i++) {
> path = to_check[i].path;
> np = of_find_node_by_path(path);
> if (!np)
> continue;
>
> refcnt = atomic_read(&np->kobj.kref.refcount);
> if (!called) {
> pr_info("%s %d\n", path, refcnt);
> to_check[i].refcnt = refcnt;
> } else if (refcnt == to_check[i].refcnt) {
> pr_info("%s %d (OK)\n", path, refcnt);
> } else {
> pr_info("%s %d (should be %d)\n", path, refcnt,
> to_check[i].refcnt);
> errors++;
> }
>
> of_node_put(np);
> }
>
> if (called)
> pr_info("----- Checking done (%u errors) -----\n", errors);
> else
> pr_info("----- Saving done -----\n");
>
> called = true;
> }
> ---
> drivers/of/base.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 36536b6a8834acd2..f3e346e19c69d1f2 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -791,8 +791,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
> if (!np)
> np = of_node_get(of_root);
> while (np && *path == '/') {
> + struct device_node *parent = np;
> path++; /* Increment past '/' delimiter */
> - np = __of_find_node_by_path(np, path);
> + np = __of_find_node_by_path(parent, path);
> + of_node_put(parent);
> path = strchrnul(path, '/');
> }
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
This doesn't look /quite/ the best. __for_each_child_of_node() is
fiddling with refcounts, but the '__' of functions shouldn't need to do
that since they are called under the spinlock (nothing is going to
change while they are called). __of_find_all_nodes() for instance
doesn't do refcounting, but of_find_all_nodes() does.
Does the following also solve the problem?
g.
---
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 36536b6a8834..0357b51a7440 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -626,9 +626,8 @@ static struct device_node *__of_get_next_child(const struct device_node *node,
next = prev ? prev->sibling : node->child;
for (; next; next = next->sibling)
- if (of_node_get(next))
+ if (next)
break;
- of_node_put(prev);
return next;
}
#define __for_each_child_of_node(parent, child) \
@@ -650,7 +649,8 @@ struct device_node *of_get_next_child(const struct device_node *node,
unsigned long flags;
raw_spin_lock_irqsave(&devtree_lock, flags);
- next = __of_get_next_child(node, prev);
+ next = of_node_get(__of_get_next_child(node, prev));
+ of_node_put(prev);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return next;
}
@@ -789,12 +789,13 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
/* Step down the tree matching path components */
raw_spin_lock_irqsave(&devtree_lock, flags);
if (!np)
- np = of_node_get(of_root);
+ np = of_root;
while (np && *path == '/') {
path++; /* Increment past '/' delimiter */
np = __of_find_node_by_path(np, path);
path = strchrnul(path, '/');
}
+ of_node_get(np);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
2015-01-22 16:14 ` Grant Likely
(?)
@ 2015-01-22 16:18 ` Grant Likely
2015-01-22 20:35 ` Geert Uytterhoeven
-1 siblings, 1 reply; 9+ messages in thread
From: Grant Likely @ 2015-01-22 16:18 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring
Cc: Gaurav Minocha, devicetree, Linux Kernel Mailing List
On Thu, Jan 22, 2015 at 4:14 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 14 Jan 2015 16:45:56 +0100
> , Geert Uytterhoeven <geert+renesas@glider.be>
> wrote:
>> When traversing all nodes and moving to a new path component, the old
>> one must be released by calling of_node_put(). Else the refcounts of the
>> parent node(s) will not be decremented.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
[...]
>> Feel free to (ab)use the code below and derive a unittest from it...
BTW, can you do this please? They are pretty important for core
changes now, and I'm stuck with doing them if the person supplying a
patch does not (and I'm already too much of a bottleneck on the DT
code).
g.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-22 20:35 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2015-01-22 20:35 UTC (permalink / raw)
To: Grant Likely
Cc: Geert Uytterhoeven, Rob Herring, Gaurav Minocha, devicetree,
linux-kernel
Hi Grant,
On Thu, Jan 22, 2015 at 5:14 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 14 Jan 2015 16:45:56 +0100
> , Geert Uytterhoeven <geert+renesas@glider.be>
> wrote:
>> When traversing all nodes and moving to a new path component, the old
>> one must be released by calling of_node_put(). Else the refcounts of the
>> parent node(s) will not be decremented.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Background.
>>
>> While investigating a reference count imbalance issue with
>> CONFIG_OF_DYNAMIC=y, I wrote the debug code below to validate the
>> reference counts of the nodes I was interested in.
>> During the first call of check_refcnts(), it gathers all reference
>> counts. During a subsequent call, it verifies that they are still the
>> same.
>>
>> Surprisingly, lots of reference counts were wrong, and kept incrementing
>> every time check_refcnts() was called.
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 36536b6a8834acd2..f3e346e19c69d1f2 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -791,8 +791,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>> if (!np)
>> np = of_node_get(of_root);
>> while (np && *path == '/') {
>> + struct device_node *parent = np;
>> path++; /* Increment past '/' delimiter */
>> - np = __of_find_node_by_path(np, path);
>> + np = __of_find_node_by_path(parent, path);
>> + of_node_put(parent);
>> path = strchrnul(path, '/');
>> }
>> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> This doesn't look /quite/ the best. __for_each_child_of_node() is
> fiddling with refcounts, but the '__' of functions shouldn't need to do
> that since they are called under the spinlock (nothing is going to
> change while they are called). __of_find_all_nodes() for instance
> doesn't do refcounting, but of_find_all_nodes() does.
>
> Does the following also solve the problem?
Yes, it does.
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 36536b6a8834..0357b51a7440 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -626,9 +626,8 @@ static struct device_node *__of_get_next_child(const struct device_node *node,
>
> next = prev ? prev->sibling : node->child;
> for (; next; next = next->sibling)
> - if (of_node_get(next))
> + if (next)
> break;
> - of_node_put(prev);
> return next;
> }
> #define __for_each_child_of_node(parent, child) \
> @@ -650,7 +649,8 @@ struct device_node *of_get_next_child(const struct device_node *node,
> unsigned long flags;
>
> raw_spin_lock_irqsave(&devtree_lock, flags);
> - next = __of_get_next_child(node, prev);
> + next = of_node_get(__of_get_next_child(node, prev));
> + of_node_put(prev);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
> return next;
> }
> @@ -789,12 +789,13 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
> /* Step down the tree matching path components */
> raw_spin_lock_irqsave(&devtree_lock, flags);
> if (!np)
> - np = of_node_get(of_root);
> + np = of_root;
> while (np && *path == '/') {
> path++; /* Increment past '/' delimiter */
> np = __of_find_node_by_path(np, path);
> path = strchrnul(path, '/');
> }
> + of_node_get(np);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
> return np;
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-22 20:35 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2015-01-22 20:35 UTC (permalink / raw)
To: Grant Likely
Cc: Geert Uytterhoeven, Rob Herring, Gaurav Minocha,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi Grant,
On Thu, Jan 22, 2015 at 5:14 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, 14 Jan 2015 16:45:56 +0100
> , Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> wrote:
>> When traversing all nodes and moving to a new path component, the old
>> one must be released by calling of_node_put(). Else the refcounts of the
>> parent node(s) will not be decremented.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>> ---
>> Background.
>>
>> While investigating a reference count imbalance issue with
>> CONFIG_OF_DYNAMIC=y, I wrote the debug code below to validate the
>> reference counts of the nodes I was interested in.
>> During the first call of check_refcnts(), it gathers all reference
>> counts. During a subsequent call, it verifies that they are still the
>> same.
>>
>> Surprisingly, lots of reference counts were wrong, and kept incrementing
>> every time check_refcnts() was called.
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 36536b6a8834acd2..f3e346e19c69d1f2 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -791,8 +791,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>> if (!np)
>> np = of_node_get(of_root);
>> while (np && *path == '/') {
>> + struct device_node *parent = np;
>> path++; /* Increment past '/' delimiter */
>> - np = __of_find_node_by_path(np, path);
>> + np = __of_find_node_by_path(parent, path);
>> + of_node_put(parent);
>> path = strchrnul(path, '/');
>> }
>> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> This doesn't look /quite/ the best. __for_each_child_of_node() is
> fiddling with refcounts, but the '__' of functions shouldn't need to do
> that since they are called under the spinlock (nothing is going to
> change while they are called). __of_find_all_nodes() for instance
> doesn't do refcounting, but of_find_all_nodes() does.
>
> Does the following also solve the problem?
Yes, it does.
Tested-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 36536b6a8834..0357b51a7440 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -626,9 +626,8 @@ static struct device_node *__of_get_next_child(const struct device_node *node,
>
> next = prev ? prev->sibling : node->child;
> for (; next; next = next->sibling)
> - if (of_node_get(next))
> + if (next)
> break;
> - of_node_put(prev);
> return next;
> }
> #define __for_each_child_of_node(parent, child) \
> @@ -650,7 +649,8 @@ struct device_node *of_get_next_child(const struct device_node *node,
> unsigned long flags;
>
> raw_spin_lock_irqsave(&devtree_lock, flags);
> - next = __of_get_next_child(node, prev);
> + next = of_node_get(__of_get_next_child(node, prev));
> + of_node_put(prev);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
> return next;
> }
> @@ -789,12 +789,13 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
> /* Step down the tree matching path components */
> raw_spin_lock_irqsave(&devtree_lock, flags);
> if (!np)
> - np = of_node_get(of_root);
> + np = of_root;
> while (np && *path == '/') {
> path++; /* Increment past '/' delimiter */
> np = __of_find_node_by_path(np, path);
> path = strchrnul(path, '/');
> }
> + of_node_get(np);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
> return np;
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-22 20:35 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2015-01-22 20:35 UTC (permalink / raw)
To: Grant Likely
Cc: Geert Uytterhoeven, Rob Herring, Gaurav Minocha, devicetree,
Linux Kernel Mailing List
Hi Grant,
On Thu, Jan 22, 2015 at 5:18 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Thu, Jan 22, 2015 at 4:14 PM, Grant Likely <grant.likely@linaro.org> wrote:
>> On Wed, 14 Jan 2015 16:45:56 +0100
>> , Geert Uytterhoeven <geert+renesas@glider.be>
>> wrote:
>>> When traversing all nodes and moving to a new path component, the old
>>> one must be released by calling of_node_put(). Else the refcounts of the
>>> parent node(s) will not be decremented.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
> [...]
>>> Feel free to (ab)use the code below and derive a unittest from it...
>
> BTW, can you do this please? They are pretty important for core
> changes now, and I'm stuck with doing them if the person supplying a
> patch does not (and I'm already too much of a bottleneck on the DT
> code).
I'll see whether I can get to it...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path()
@ 2015-01-22 20:35 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2015-01-22 20:35 UTC (permalink / raw)
To: Grant Likely
Cc: Geert Uytterhoeven, Rob Herring, Gaurav Minocha,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List
Hi Grant,
On Thu, Jan 22, 2015 at 5:18 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Jan 22, 2015 at 4:14 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Wed, 14 Jan 2015 16:45:56 +0100
>> , Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>> wrote:
>>> When traversing all nodes and moving to a new path component, the old
>>> one must be released by calling of_node_put(). Else the refcounts of the
>>> parent node(s) will not be decremented.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>>> ---
> [...]
>>> Feel free to (ab)use the code below and derive a unittest from it...
>
> BTW, can you do this please? They are pretty important for core
> changes now, and I'm stuck with doing them if the person supplying a
> patch does not (and I'm already too much of a bottleneck on the DT
> code).
I'll see whether I can get to it...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-01-22 20:36 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-14 15:45 [PATCH] of: Add missing of_node_put() in of_find_node_by_path() Geert Uytterhoeven
2015-01-14 15:45 ` Geert Uytterhoeven
2015-01-22 16:14 ` Grant Likely
2015-01-22 16:14 ` Grant Likely
2015-01-22 16:18 ` Grant Likely
2015-01-22 20:35 ` Geert Uytterhoeven
2015-01-22 20:35 ` Geert Uytterhoeven
2015-01-22 20:35 ` Geert Uytterhoeven
2015-01-22 20:35 ` Geert Uytterhoeven
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.