[Skiboot] [PATCH v2] core/device.c: Fix dt_find_compatible_node

[Alistair Popple]

dt_find_compatible_node() and dt_find_compatible_node_on_chip() are used to
find device nodes under a parent/root node with a given compatible
property.

dt_next(root, prev) is used to walk the child nodes of the given parent and
takes two arguments - root contains the parent node to walk whilst prev
contains the previous child to search from so that it can be used as an
iterator over all children nodes.

The first iteration of dt_find_compatible_node(root, prev) calls
dt_next(root, root) which is not a well defined operation as prev is
assumed to be child of the root node. The result is that when a node
contains no children it will start returning the parent nodes siblings
until it hits the top of the tree at which point a NULL derefence is
attempted when looking for the root nodes parent.

Dereferencing NULL can result in undesirable data exceptions during system
boot and untimely non-hilarious system crashes. dt_next() should not be
called with prev == root. Instead we add a check to dt_next() such that
passing prev = NULL will cause it to start iterating from the first child
node (if any).

Also add a unit test for this case to run-device.c.

Signed-off-by: Alistair Popple <alistair at popple.id.au>
---

Changes from v1:
	- Make sure the first direct child node is returned

 core/device.c          | 19 +++++++++++++------
 core/test/run-device.c | 31 ++++++++++++++++++++++++++-----
 2 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/core/device.c b/core/device.c
index fc1db568..50fbb186 100644
--- a/core/device.c
+++ b/core/device.c
@@ -608,6 +608,15 @@ struct dt_node *dt_first(const struct dt_node *root)
 struct dt_node *dt_next(const struct dt_node *root,
 			const struct dt_node *prev)
 {
+	if (!prev) {
+		struct dt_node *first = dt_first(root);
+
+		if (!first)
+			return NULL;
+		else
+			return first;
+	}
+
 	/* Children? */
 	if (!list_empty(&prev->children))
 		return dt_first(prev);
@@ -719,10 +728,9 @@ struct dt_node *dt_find_compatible_node(struct dt_node *root,
 					struct dt_node *prev,
 					const char *compat)
 {
-	struct dt_node *node;
+	struct dt_node *node = prev;
 
-	node = prev ? dt_next(root, prev) : root;
-	for (; node; node = dt_next(root, node))
+	while ((node = dt_next(root, node)))
 		if (dt_node_is_compatible(node, compat))
 			return node;
 	return NULL;
@@ -964,10 +972,9 @@ struct dt_node *dt_find_compatible_node_on_chip(struct dt_node *root,
 						const char *compat,
 						uint32_t chip_id)
 {
-	struct dt_node *node;
+	struct dt_node *node = prev;
 
-	node = prev ? dt_next(root, prev) : root;
-	for (; node; node = dt_next(root, node)) {
+	while ((node = dt_next(root, node))) {
 		u32 cid = __dt_get_chip_id(node);
 		if (cid == chip_id &&
 		    dt_node_is_compatible(node, compat))
diff --git a/core/test/run-device.c b/core/test/run-device.c
index 326be3ff..c3e46793 100644
--- a/core/test/run-device.c
+++ b/core/test/run-device.c
@@ -323,14 +323,27 @@ int main(void)
 	gc1 = dt_new(c1, "coprocessor1");
 	dt_add_property_strings(gc1, "compatible",
 				"specific-fake-coprocessor");
+	gc2 = dt_new(gc1, "coprocessor2");
+	dt_add_property_strings(gc2, "compatible",
+				"specific-fake-coprocessor");
+	gc3 = dt_new(c1, "coprocessor3");
+	dt_add_property_strings(gc3, "compatible",
+				"specific-fake-coprocessor");
 
-	gc2 = dt_new(c1, "node-without-compatible");
-	assert(__dt_find_property(gc2, "compatible") == NULL);
-	assert(!dt_node_is_compatible(gc2, "any-property"));
 
 	assert(dt_find_compatible_node(root, NULL, "generic-fake-bus") == c2);
 	assert(dt_find_compatible_node(root, c2, "generic-fake-bus") == NULL);
 
+	/* we can find all compatible nodes */
+	assert(dt_find_compatible_node(c1, NULL, "specific-fake-coprocessor") == gc1);
+	assert(dt_find_compatible_node(c1, gc1, "specific-fake-coprocessor") == gc2);
+	assert(dt_find_compatible_node(c1, gc2, "specific-fake-coprocessor") == gc3);
+	assert(dt_find_compatible_node(c1, gc3, "specific-fake-coprocessor") == NULL);
+	assert(dt_find_compatible_node(root, NULL, "specific-fake-coprocessor") == gc1);
+	assert(dt_find_compatible_node(root, gc1, "specific-fake-coprocessor") == gc2);
+	assert(dt_find_compatible_node(root, gc2, "specific-fake-coprocessor") == gc3);
+	assert(dt_find_compatible_node(root, gc3, "specific-fake-coprocessor") == NULL);
+
 	/* we can find the coprocessor once on the cpu */
 	assert(dt_find_compatible_node_on_chip(root,
 					       NULL,
@@ -339,6 +352,14 @@ int main(void)
 	assert(dt_find_compatible_node_on_chip(root,
 					       gc1,
 					       "specific-fake-coprocessor",
+					       0xcafe) == gc2);
+	assert(dt_find_compatible_node_on_chip(root,
+					       gc2,
+					       "specific-fake-coprocessor",
+					       0xcafe) == gc3);
+	assert(dt_find_compatible_node_on_chip(root,
+					       gc3,
+					       "specific-fake-coprocessor",
 					       0xcafe) == NULL);
 
 	/* we can't find the coprocessor on the bus */
@@ -349,9 +370,9 @@ int main(void)
 
 	/* Test phandles. We override the automatically generated one. */
 	phandle = 0xf00;
-	dt_add_property(gc2, "phandle", (const void *)&phandle, 4);
+	dt_add_property(gc3, "phandle", (const void *)&phandle, 4);
 	assert(last_phandle == 0xf00);
-	assert(dt_find_by_phandle(root, 0xf00) == gc2);
+	assert(dt_find_by_phandle(root, 0xf00) == gc3);
 	assert(dt_find_by_phandle(root, 0xf0f) == NULL);
 
 	dt_free(root);
-- 
2.11.0