Skip to contentSkip to conversation

OF: base: import of_find_matching_node_and_match from Linux OF API

PATCH NISHIMOTO Hiroki 2 participants 2 messages Jul 2, 2013 – Jul 3, 2013 Device tree

lore
Share & tools
lore
#0 lore raw

Hi,

on master/next branch,

of_find_matching_node_and_match macro can make infinite loop.

This is because, of_tree_for_each_node traverse all nodes each time. But a callee of of_find_matching_node_and_match,
such as for_each_matching_node, calls it by changing "from" arg.
So if "matches" have two or more nodes, previously matched node can be re-matched.

Below change can fix the issue.
Or is it better to port node->allnext from linux kernel?

---
 drivers/of/base.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/of/base.c +10 -4
index 63ff647..36774de 100644
@@ -36,6 +36,9 @@
 #define of_tree_for_each_node(node, root) \
 	list_for_each_entry(node, &root->list, list)
 
+#define of_get_next_node(np) \
+	list_first_entry(&np->list, typeof(*np), list)
+
 /**
  * struct alias_prop - Alias property in 'aliases' node
  * @link:	List node to link the structure in aliases_lookup list
@@ -444,17 +447,20 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
 	if (match)
 		*match = NULL;
 
-	if (!from)
-		from = root_node;
+	np = from ? of_get_next_node(from) : root_node;
 
-	of_tree_for_each_node(np, from) {
+	if (from != NULL && np == root_node)
+		return NULL;
+
+	do {
 		const struct of_device_id *m = of_match_node(matches, np);
 		if (m) {
 			if (match)
 				*match = m;
 			return np;
 		}
-	}
+		np = of_get_next_node(np);
+	} while (np != root_node);
 
 	return NULL;
 }
Signature · 1 line
1.8.1.2
Quoted 109 lines

This imports of_find_matching_node_and_match and corresponding helpers from Linux OF API.

---
Cc: barebox@xxxxxxxxxxxxxxxxxxx

---
 drivers/of/base.c |   37 +++++++++++++++++++++++++++++++++++++
 include/of.h      |   24 ++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 0 deletions(-)
drivers/of/base.c +37 -0
index 50a3c22..218cb5a 100644
@@ -389,6 +389,43 @@ const struct of_device_id *of_match_node(const struct of_device_id *matches,
 	return NULL;
 }
 
+/**
+ *	of_find_matching_node_and_match - Find a node based on an of_device_id
+ *					  match table.
+ *	@from:		The node to start searching from or NULL, the node
+ *			you pass will not be searched, only the next one
+ *			will; typically, you pass what the previous call
+ *			returned.
+ *	@matches:	array of of device match structures to search in
+ *	@match		Updated to point at the matches entry which matched
+ *
+ *	Returns a pointer to the node found or NULL.
+ */
+struct device_node *of_find_matching_node_and_match(struct device_node *from,
+					const struct of_device_id *matches,
+					const struct of_device_id **match)
+{
+	struct device_node *np;
+
+	if (match)
+		*match = NULL;
+
+	if (!from)
+		from = root_node;
+
+	of_tree_for_each_node(np, from) {
+		const struct of_device_id *m = of_match_node(matches, np);
+		if (m) {
+			if (match)
+				*match = m;
+			return np;
+		}
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(of_find_matching_node_and_match);
+
 int of_match(struct device_d *dev, struct driver_d *drv)
 {
 	const struct of_device_id *id;
include/of.h +24 -0
index e170e2b..c21e73d 100644
@@ -187,6 +187,10 @@ extern struct device_node *of_find_node_by_name(struct device_node *from,
 extern struct device_node *of_find_node_by_path(const char *path);
 extern struct device_node *of_find_compatible_node(struct device_node *from,
 	const char *type, const char *compat);
+extern struct device_node *of_find_matching_node_and_match(
+	struct device_node *from,
+	const struct of_device_id *matches,
+	const struct of_device_id **match);
 extern int of_device_is_available(const struct device_node *device);
 
 extern void of_alias_scan(void);
@@ -259,6 +263,14 @@ static inline struct device_node *of_find_compatible_node(
 	return NULL;
 }
 
+static inline struct device_node *of_find_matching_node_and_match(
+					struct device_node *from,
+					const struct of_device_id *matches,
+					const struct of_device_id **match)
+{
+	return NULL;
+}
+
 static inline int of_device_is_available(const struct device_node *device)
 {
 	return 0;
@@ -285,5 +297,17 @@ static inline const char *of_alias_get(struct device_node *np)
 #define for_each_compatible_node(dn, type, compatible) \
 	for (dn = of_find_compatible_node(NULL, type, compatible); dn; \
 	     dn = of_find_compatible_node(dn, type, compatible))
+static inline struct device_node *of_find_matching_node(
+	struct device_node *from,
+	const struct of_device_id *matches)
+{
+	return of_find_matching_node_and_match(from, matches, NULL);
+}
+#define for_each_matching_node(dn, matches) \
+	for (dn = of_find_matching_node(NULL, matches); dn; \
+	     dn = of_find_matching_node(dn, matches))
+#define for_each_matching_node_and_match(dn, matches, match) \
+	for (dn = of_find_matching_node_and_match(NULL, matches, match); \
+	     dn; dn = of_find_matching_node_and_match(dn, matches, match))
 
 #endif /* __OF_H */
Signature · 1 line
1.7.2.5

_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox

#1 lore raw
Quoted 3 lines · NISHIMOTO Hiroki

on master/next branch,

of_find_matching_node_and_match macro can make infinite loop.

Hiroki,

I confirmed that infinite loop but there are more functions that suffer from this infinite loop.

Quoted 6 lines

This is because, of_tree_for_each_node traverse all nodes each time. But a callee of of_find_matching_node_and_match,
such as for_each_matching_node, calls it by changing "from" arg.
So if "matches" have two or more nodes, previously matched node can be re-matched.

Below change can fix the issue.

@Sascha: please confirm the below. It is what I understand from your last comments about OF API improvements.

Actually, root_node is only the list head for the currently active device tree. There can be other device trees you want to use the of_for_each_.. on, so we need to find another way to check for the list head of the tree the "from" node is in.

From what I can see from linux linked lists API, there is no safe way to determine the start of a linked list without knowing what has been the first list node in the first place.

Sascha knows barebox struct device_node lists for sure, maybe he can point us to the best way to solve this?

Sebastian

Quoted 36 lines

Or is it better to port node->allnext from linux kernel?

---
  drivers/of/base.c | 14 ++++++++++----
  1 file changed, 10 insertions(+), 4 deletions(-)
drivers/of/base.c +8 -3
index 63ff647..36774de 100644
@@ -36,6 +36,9 @@
  #define of_tree_for_each_node(node, root) \
  	list_for_each_entry(node,&root->list, list)
 
+#define of_get_next_node(np) \
+	list_first_entry(&np->list, typeof(*np), list)
+
  /**
   * struct alias_prop - Alias property in 'aliases' node
   * @link:	List node to link the structure in aliases_lookup list
@@ -444,17 +447,20 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
  	if (match)
  		*match = NULL;
 
-	if (!from)
-		from = root_node;
+	np = from ? of_get_next_node(from) : root_node;
 
-	of_tree_for_each_node(np, from) {
+	if (from != NULL&&  np == root_node)
+		return NULL;
+
+	do {
  		const struct of_device_id *m = of_match_node(matches, np);
  		if (m) {

drivers/of/base.c struct device_node *of_find_matching_node_and_match(struct device_node *from, · line 466

Quoted 10 lines
drivers/of/base.c +2 -1
  			if (match)
  				*match = m;
  			return np;
  		}
-	}
+		np = of_get_next_node(np);
+	} while (np != root_node);
 
  	return NULL;
  }

_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox