Usage of for_each_child_of_node()
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 devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · 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 callingof_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.
Quoted 7 lines
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.
Quoted 1 line
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 18 lines · Rob Herring
Quoted 9 lines · 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 callingof_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.Quoted 7 lines
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 ?
Quoted 6 lines
Certainly.
Quoted 1 line
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
Quoted 26 lines · Thierry Reding
Quoted 18 lines · Rob Herring
Quoted 9 lines · 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 callingof_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.Quoted 7 lines
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 ?
Quoted 6 lines
Certainly.
Quoted 1 line
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.
Quoted 7 lines
Actually, I think it actually happens to be correct most of the time. The reason is that
for_each_child_of_node()internally calls theof_get_next_child()to iterate over all children. And that function already callsof_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.
Good, that reduces the scope of the problem significantly.
Quoted 6 lines
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 42 lines · Guenter Roeck
Quoted 26 lines · Thierry Reding
Quoted 18 lines · Rob Herring
Quoted 9 lines · 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 callingof_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.Quoted 7 lines
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 ?
Quoted 6 lines
Certainly.
Quoted 1 line
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.
Quoted 7 lines
Actually, I think it actually happens to be correct most of the time. The reason is that
for_each_child_of_node()internally calls theof_get_next_child()to iterate over all children. And that function already callsof_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.Good, that reduces the scope of the problem significantly.
Quoted 6 lines
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.
Quoted 4 lines
Unfortunately, there are many cases with code such as
if (error) return; /* or break; */
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.
Quoted 5 lines
or even
if (found node) return of_node_get(node);in the loop.
Yeah, I think all of those are probably wrong too.
Thierry
Quoted 43 lines · Thierry Reding
Quoted 42 lines · Guenter Roeck
Quoted 26 lines · Thierry Reding
Quoted 18 lines · Rob Herring
Quoted 9 lines · 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 callingof_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.Quoted 7 lines
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 ?
Quoted 6 lines
Certainly.
Quoted 1 line
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.
Quoted 7 lines
Actually, I think it actually happens to be correct most of the time. The reason is that
for_each_child_of_node()internally calls theof_get_next_child()to iterate over all children. And that function already callsof_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.Good, that reduces the scope of the problem significantly.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 4 lines
Unfortunately, there are many cases with code such as
if (error) return; /* or break; */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
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 ;-).
Guenter
Quoted 52 lines · Guenter Roeck
Quoted 43 lines · Thierry Reding
Quoted 42 lines · Guenter Roeck
Quoted 26 lines · Thierry Reding
Quoted 18 lines · Rob Herring
Quoted 9 lines · 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 callingof_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.Quoted 7 lines
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 ?
Quoted 6 lines
Certainly.
Quoted 1 line
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.
Quoted 7 lines
Actually, I think it actually happens to be correct most of the time. The reason is that
for_each_child_of_node()internally calls theof_get_next_child()to iterate over all children. And that function already callsof_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.Good, that reduces the scope of the problem significantly.
Quoted 6 lines
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.Quoted 7 lines
Quoted 4 lines
Unfortunately, there are many cases with code such as
if (error) return; /* or break; */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
Quoted 6 lines
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 ;-).
Well, that's better than boring, isn't it? =)
Thierry