Usage of for_each_child_of_node()

[Guenter Roeck]

Hi all,

for_each_child_of_node() and similar functions increase the refcount
on each returned node and expect the caller to release the node by
calling of_node_put() when done.

Looking through the kernel code, it appears this is hardly ever done,
if at all. Some code even calls of_node_get() on returned nodes again.

I guess this doesn't matter in cases where devicetree is a static entity.
However, this is not (or no longer) the case with devicetree overlays,
or more generically in cases where devicetree nodes are added and
removed dynamically.

Fundamental question: Would patches to fix this problem be accepted upstream ?

Or, of course, stepping a bit back: Am I missing something essential ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Rob Herring]

On Sat, Oct 12, 2013 at 3:54 PM, Guenter Roeck <li...@roeck-us.net> wrote:
> Hi all,
>
> for_each_child_of_node() and similar functions increase the refcount
> on each returned node and expect the caller to release the node by
> calling of_node_put() when done.
>
> Looking through the kernel code, it appears this is hardly ever done,
> if at all. Some code even calls of_node_get() on returned nodes again.
>
> I guess this doesn't matter in cases where devicetree is a static entity.
> However, this is not (or no longer) the case with devicetree overlays,
> or more generically in cases where devicetree nodes are added and
> removed dynamically.
>
> Fundamental question: Would patches to fix this problem be accepted upstream
> ?

Certainly.

> Or, of course, stepping a bit back: Am I missing something essential ?

No. I think this is frequently wrong since it typically doesn't matter
for static entries as you mention.

Rob

[Thierry Reding]

On Sat, Oct 12, 2013 at 10:15:03PM -0500, Rob Herring wrote:
> On Sat, Oct 12, 2013 at 3:54 PM, Guenter Roeck <li...@roeck-us.net> wrote:
> > Hi all,
> >
> > for_each_child_of_node() and similar functions increase the refcount
> > on each returned node and expect the caller to release the node by
> > calling of_node_put() when done.
> >
> > Looking through the kernel code, it appears this is hardly ever done,
> > if at all. Some code even calls of_node_get() on returned nodes again.
> >
> > I guess this doesn't matter in cases where devicetree is a static entity.
> > However, this is not (or no longer) the case with devicetree overlays,
> > or more generically in cases where devicetree nodes are added and
> > removed dynamically.
> >
> > Fundamental question: Would patches to fix this problem be accepted upstream
> > ?
>
> Certainly.
>
> > Or, of course, stepping a bit back: Am I missing something essential ?
>
> No. I think this is frequently wrong since it typically doesn't matter
> for static entries as you mention.

Actually, I think it actually happens to be correct most of the time.
The reason is that for_each_child_of_node() internally calls the
of_get_next_child() to iterate over all children. And that function
already calls of_node_put() on the "previous" node. So if all the code
does is to iterate over all nodes to query them, then all should be
fine.

The only case where you actually need to drop the reference on a node is
if you break out of the loop (so that of_get_next_child() will not be
called). But that's usually the case when you need to perform some
operation on the node, in which case it is the right thing to hold on to
a reference until you're done with the node.

Thierry

[Guenter Roeck]

Good, that reduces the scope of the problem significantly.

> The only case where you actually need to drop the reference on a node is
> if you break out of the loop (so that of_get_next_child() will not be
> called). But that's usually the case when you need to perform some
> operation on the node, in which case it is the right thing to hold on to
> a reference until you're done with the node.
>

Unfortunately, there are many cases with code such as

if (error)
return; /* or break; */

or even
if (found node)
return of_node_get(node);

in the loop.

Guenter

[Thierry Reding]

Well, a break isn't necessarily bad, since you could be using the node
subsequently. I imagine that depending on the exact block following the
if statement the node could also be assigned to some field within a
structure or similar, in which case this might still be valid. So it
really needs to be evaluated on a case by case basis.

If the above is actually verbatim, then yes, that's certainly an error.

> or even
> if (found node)
> return of_node_get(node);
>
> in the loop.

Yeah, I think all of those are probably wrong too.

Thierry

[Guenter Roeck]

Correct, but I meant the error case. Randomly looking through several
drivers, most of them get error return handling wrong. "Winner" so far
is of_regulator_match(), which doesn't release the node on error return,
but does not acquire references for use afterwards either.

Something to do with my non-existing free time ;-).

[Thierry Reding]

Well, that's better than boring, isn't it? =)

Thierry