On Thu, Dec 16, 2010 at 09:40:55AM +1100, Michael Ellerman wrote:

On Wed, 2010-12-15 at 10:35 -0800, Hollis Blanchard wrote:

quoted

On Wed, Dec 8, 2010 at 7:09 PM, David Gibson
[off-list ref] wrote:

quoted

On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote:

quoted

On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote:

quoted

On Wed, 8 Dec 2010 11:29:44 -0800
Deepak Saxena [off-list ref] wrote:

quoted

We only return the next child if the device is available.

Signed-off-by: Hollis Blanchard <redacted>
Signed-off-by: Deepak Saxena <redacted>
---
 drivers/of/base.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5d269a4..81b2601 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c

@@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
  *
  *       Returns a node pointer with refcount incremented, use
  *       of_node_put() on it when done.
+ *
+ *       Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_get_next_child(const struct device_node *node,
  struct device_node *prev)

@@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
  read_lock(&devtree_lock);
  next = prev ? prev->sibling : node->child;
  for (; next; next = next->sibling)
-         if (of_node_get(next))
+         if (of_device_is_available(next) && of_node_get(next))
                  break;
  of_node_put(prev);
  read_unlock(&devtree_lock);

This seems like too low-level a place to put this.  Some code may know
how to un-disable a device in certain situations, or it may be part of
debug code trying to dump the whole device tree, etc.  Looking
further[1], I see a raw version of this function, but not other things
like of_find_compatible_node.

Yeah I agree. I think we'll eventually end up with __ versions of all or
lots of them. Not to mention there might be cases you've missed where
code expects to see unavailable nodes. The right approach is to add
_new_ routines that don't return unavailable nodes, and convert code
that you know wants to use them.

Actually, I don't think we really want these status-skipping
iterators at all.  The device tree iterators should give us the device
tree, as it is.  Those old-style drivers which seach for a node rather
than using the bus probing logic can keep individual checks of the
status property until they're converted to the new scheme.

So the patch should look something like this?

@@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct

device_node *node)
 *
 *     Returns a node pointer with refcount incremented, use
 *     of_node_put() on it when done.
+ *
+ *     Do not use this function.
 */
 struct device_node *of_get_next_child(const struct device_node *node,
       struct device_node *prev)

Haha. No it should say "this function doesn't lie to you".

And the patch should say "this patch _doesn't_ subtly change all callers
of of_get_next_child() without carefully auditing them".

Heh, Yes. The comments made on this patch are totally on-base.  Not
all nodes are devices, and not all callers will want to skip nodes;
regardless of the reason for skipping.  Case in point: the
/proc/device-tree support code.

If a caller needs a version of the function that skips unavailable
nodes, then that behaviour should be explicitly asked for.  In this
case it should be a new function with a new name.  Don't change the
behaviour out from under the existing users.

g.