Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties
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 da...@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_sax...@mentor.com wrote: We only return the next child if the device is available. Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com Signed-off-by: Deepak Saxena deepak_sax...@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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties
On Wed, Dec 8, 2010 at 7:09 PM, David Gibson da...@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_sax...@mentor.com wrote: We only return the next child if the device is available. Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com Signed-off-by: Deepak Saxena deepak_sax...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties
On Wed, 2010-12-15 at 10:35 -0800, Hollis Blanchard wrote: On Wed, Dec 8, 2010 at 7:09 PM, David Gibson da...@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_sax...@mentor.com wrote: We only return the next child if the device is available. Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com Signed-off-by: Deepak Saxena deepak_sax...@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 signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 3/5] of/device: Make of_get_next_child() check status properties
We only return the next child if the device is available. Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com Signed-off-by: Deepak Saxena deepak_sax...@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); -- 1.6.3.3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties
On Wed, 8 Dec 2010 11:29:44 -0800 Deepak Saxena deepak_sax...@mentor.com wrote: We only return the next child if the device is available. Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com Signed-off-by: Deepak Saxena deepak_sax...@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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties
On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote: On Wed, 8 Dec 2010 11:29:44 -0800 Deepak Saxena deepak_sax...@mentor.com wrote: We only return the next child if the device is available. Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com Signed-off-by: Deepak Saxena deepak_sax...@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 signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties
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_sax...@mentor.com wrote: We only return the next child if the device is available. Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com Signed-off-by: Deepak Saxena deepak_sax...@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. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
