| Message ID | 20101208192944.GE32473@mentor.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Grant Likely |
| Headers | show |
On Wed, 8 Dec 2010 11:29:44 -0800 Deepak Saxena <deepak_saxena@mentor.com> wrote: > We only return the next child if the device is available. > > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> > --- > 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. Could this be done more othogonally, so that the caller specifies in a parameter whether to skip based on status? -Scott [1] For some reason I received some of these patches from linuxppc-dev, and others from devicetree-discuss. I wish lists wouldn't try to be "smart" about discarding duplicates -- it messes with filters.
On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote: > On Wed, 8 Dec 2010 11:29:44 -0800 > Deepak Saxena <deepak_saxena@mentor.com> wrote: > > > We only return the next child if the device is available. > > > > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> > > --- > > 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. cheers
On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote: > On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote: > > On Wed, 8 Dec 2010 11:29:44 -0800 > > Deepak Saxena <deepak_saxena@mentor.com> wrote: > > > > > We only return the next child if the device is available. > > > > > > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> > > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> > > > --- > > > 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.
On Wed, Dec 8, 2010 at 7:09 PM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote: >> On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote: >> > On Wed, 8 Dec 2010 11:29:44 -0800 >> > Deepak Saxena <deepak_saxena@mentor.com> wrote: >> > >> > > We only return the next child if the device is available. >> > > >> > > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> >> > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> >> > > --- >> > > 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) ... + struct device_node *of_get_next_available_child(const struct device_node *node, + struct device_node *prev) + ... + } And then (almost) all the of_get_next_child() sites should be changed to call the new function? -Hollis
On Wed, 2010-12-15 at 10:35 -0800, Hollis Blanchard wrote: > On Wed, Dec 8, 2010 at 7:09 PM, David Gibson > <david@gibson.dropbear.id.au> wrote: > > On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote: > >> On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote: > >> > On Wed, 8 Dec 2010 11:29:44 -0800 > >> > Deepak Saxena <deepak_saxena@mentor.com> wrote: > >> > > >> > > We only return the next child if the device is available. > >> > > > >> > > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> > >> > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> > >> > > --- > >> > > 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". cheers
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: > > On Wed, Dec 8, 2010 at 7:09 PM, David Gibson > > <david@gibson.dropbear.id.au> wrote: > > > On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote: > > >> On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote: > > >> > On Wed, 8 Dec 2010 11:29:44 -0800 > > >> > Deepak Saxena <deepak_saxena@mentor.com> wrote: > > >> > > > >> > > We only return the next child if the device is available. > > >> > > > > >> > > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> > > >> > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> > > >> > > --- > > >> > > 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.
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);