[PATCH 0/6] of: Implement iterator for phandles

[PATCH 0/6] of: Implement iterator for phandles

[Joerg Roedel]

Hi,

here is an implementation of the iterator over phandles
concept which Rob Herring suggested to me some time ago. My
approach is a little bit different from what the diff showed
back then, but it gets rid of the allocation and 'struct
'struct of_phandle_args' misuse.

I also converted the arm-smmu driver to make use of the
iterator. The resulting kernel boots on my AMD Seattle
system and fixes the warning triggered there. The patches
now also pass all dt-unittests in my kvm environment.

Please review. Patches are based on v4.5.

Thanks,

	Joerg

Changes since RFC post:

* Reordered members of 'struct of_phandle_iterator' and did
  some renaming

* Removed index counting from the iterator

* Split up iterator implementation into multiple patches

* Fixed the code to survive all dt-unittests, tested with each
  patch in this series

* Re-added and updated some comments which got lost during the
  conversion.

* Added of_for_each_phandle macro for easier handling

* Moved the counting special-case from __of_parse_phandle_with_args
  directly to of_count_phandle_with_args for code
  simplification

* Removed some iterator helper functions

* Formatting and style changes

Joerg Roedel (6):
  of: Introduce struct of_phandle_iterator
  of: Move phandle walking to of_phandle_iterator_next()
  of: Remove counting special case from __of_parse_phandle_with_args()
  of: Introduce of_for_each_phandle() helper macro
  of: Introduce of_phandle_iterator_args()
  iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

 drivers/iommu/arm-smmu.c |  28 +++++--
 drivers/of/base.c        | 206 ++++++++++++++++++++++++++++++-----------------
 include/linux/of.h       |  56 +++++++++++++
 3 files changed, 211 insertions(+), 79 deletions(-)

-- 
1.9.1

[PATCH 1/6] of: Introduce struct of_phandle_iterator

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

This struct carrys all necessary information to iterate over
a list of phandles and extract the arguments. Add an
init-function for the iterator and make use of it in
__of_parse_phandle_with_args().

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/of/base.c  | 99 +++++++++++++++++++++++++++++++++---------------------
 include/linux/of.h | 33 ++++++++++++++++++
 2 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 017dd94..bfdb09b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1439,35 +1439,56 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 	printk("\n");
 }
 
+int of_phandle_iterator_init(struct of_phandle_iterator *it,
+		const struct device_node *np,
+		const char *list_name,
+		const char *cells_name,
+		int cell_count)
+{
+	const __be32 *list;
+	int size;
+
+	memset(it, 0, sizeof(*it));
+
+	list = of_get_property(np, list_name, &size);
+	if (!list)
+		return -ENOENT;
+
+	it->cells_name = cells_name;
+	it->cell_count = cell_count;
+	it->parent = np;
+	it->list_end = list + size / sizeof(*list);
+	it->phandle_end = list;
+	it->cur = list;
+
+	return 0;
+}
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
 					int cell_count, int index,
 					struct of_phandle_args *out_args)
 {
-	const __be32 *list, *list_end;
-	int rc = 0, size, cur_index = 0;
-	uint32_t count = 0;
-	struct device_node *node = NULL;
-	phandle phandle;
+	struct of_phandle_iterator it;
+	int rc, cur_index = 0;
 
-	/* Retrieve the phandle list property */
-	list = of_get_property(np, list_name, &size);
-	if (!list)
-		return -ENOENT;
-	list_end = list + size / sizeof(*list);
+	rc = of_phandle_iterator_init(&it, np, list_name,
+				      cells_name, cell_count);
+	if (rc)
+		return rc;
 
 	/* Loop over the phandles until all the requested entry is found */
-	while (list < list_end) {
+	while (it.cur < it.list_end) {
 		rc = -EINVAL;
-		count = 0;
+		it.cur_count = 0;
 
 		/*
 		 * If phandle is 0, then it is an empty entry with no
 		 * arguments.  Skip forward to the next entry.
 		 */
-		phandle = be32_to_cpup(list++);
-		if (phandle) {
+		it.phandle = be32_to_cpup(it.cur++);
+		if (it.phandle) {
 			/*
 			 * Find the provider node and parse the #*-cells
 			 * property to determine the argument length.
@@ -1477,34 +1498,34 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 			 * except when we're going to return the found node
 			 * below.
 			 */
-			if (cells_name || cur_index == index) {
-				node = of_find_node_by_phandle(phandle);
-				if (!node) {
+			if (it.cells_name || cur_index == index) {
+				it.node = of_find_node_by_phandle(it.phandle);
+				if (!it.node) {
 					pr_err("%s: could not find phandle\n",
-						np->full_name);
+						it.parent->full_name);
 					goto err;
 				}
 			}
 
-			if (cells_name) {
-				if (of_property_read_u32(node, cells_name,
-							 &count)) {
+			if (it.cells_name) {
+				if (of_property_read_u32(it.node, it.cells_name,
+							 &it.cur_count)) {
 					pr_err("%s: could not get %s for %s\n",
-						np->full_name, cells_name,
-						node->full_name);
+						it.parent->full_name, it.cells_name,
+						it.node->full_name);
 					goto err;
 				}
 			} else {
-				count = cell_count;
+				it.cur_count = it.cell_count;
 			}
 
 			/*
 			 * Make sure that the arguments actually fit in the
 			 * remaining property data length
 			 */
-			if (list + count > list_end) {
+			if (it.cur + it.cur_count > it.list_end) {
 				pr_err("%s: arguments longer than property\n",
-					 np->full_name);
+					 it.parent->full_name);
 				goto err;
 			}
 		}
@@ -1517,28 +1538,28 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 		 */
 		rc = -ENOENT;
 		if (cur_index == index) {
-			if (!phandle)
+			if (!it.phandle)
 				goto err;
 
 			if (out_args) {
 				int i;
-				if (WARN_ON(count > MAX_PHANDLE_ARGS))
-					count = MAX_PHANDLE_ARGS;
-				out_args->np = node;
-				out_args->args_count = count;
-				for (i = 0; i < count; i++)
-					out_args->args[i] = be32_to_cpup(list++);
+				if (WARN_ON(it.cur_count > MAX_PHANDLE_ARGS))
+					it.cur_count = MAX_PHANDLE_ARGS;
+				out_args->np = it.node;
+				out_args->args_count = it.cur_count;
+				for (i = 0; i < it.cur_count; i++)
+					out_args->args[i] = be32_to_cpup(it.cur++);
 			} else {
-				of_node_put(node);
+				of_node_put(it.node);
 			}
 
 			/* Found it! return success */
 			return 0;
 		}
 
-		of_node_put(node);
-		node = NULL;
-		list += count;
+		of_node_put(it.node);
+		it.node = NULL;
+		it.cur += it.cur_count;
 		cur_index++;
 	}
 
@@ -1550,8 +1571,8 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	 */
 	rc = index < 0 ? cur_index : -ENOENT;
  err:
-	if (node)
-		of_node_put(node);
+	if (it.node)
+		of_node_put(it.node);
 	return rc;
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index dc6e396..70c2bdb 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -75,6 +75,23 @@ struct of_phandle_args {
 	uint32_t args[MAX_PHANDLE_ARGS];
 };
 
+struct of_phandle_iterator {
+	/* Common iterator information */
+	const char *cells_name;
+	int cell_count;
+	const struct device_node *parent;
+
+	/* List size information */
+	const __be32 *list_end;
+	const __be32 *phandle_end;
+
+	/* Current position state */
+	const __be32 *cur;
+	uint32_t cur_count;
+	phandle phandle;
+	struct device_node *node;
+};
+
 struct of_reconfig_data {
 	struct device_node	*dn;
 	struct property		*prop;
@@ -334,6 +351,13 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+/* phandle iterator functions */
+extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
+				    const struct device_node *np,
+				    const char *list_name,
+				    const char *cells_name,
+				    int cell_count);
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);
@@ -608,6 +632,15 @@ static inline int of_count_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
+					   const struct device_node *np,
+					   const char *list_name,
+					   const char *cells_name,
+					   int cell_count)
+{
+	return -ENOSYS;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
-- 
1.9.1

[PATCH 2/6] of: Move phandle walking to of_phandle_iterator_next()

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Move the code to walk over the phandles out of the loop in
__of_parse_phandle_with_args() to a separate function that
just works with the iterator handle: of_phandle_iterator_next().

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/of/base.c  | 130 ++++++++++++++++++++++++++++++-----------------------
 include/linux/of.h |   7 +++
 2 files changed, 81 insertions(+), 56 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index bfdb09b..4036512 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1464,6 +1464,75 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
 	return 0;
 }
 
+int of_phandle_iterator_next(struct of_phandle_iterator *it)
+{
+	uint32_t count = 0;
+
+	if (it->node) {
+		of_node_put(it->node);
+		it->node = NULL;
+	}
+
+	if (!it->cur || it->phandle_end >= it->list_end)
+		return -ENOENT;
+
+	it->cur = it->phandle_end;
+
+	/* If phandle is 0, then it is an empty entry with no arguments. */
+	it->phandle = be32_to_cpup(it->cur++);
+
+	if (it->phandle) {
+
+		/*
+		 * Find the provider node and parse the #*-cells property to
+		 * determine the argument length.
+		 */
+		it->node = of_find_node_by_phandle(it->phandle);
+
+		if (it->cells_name) {
+			if (!it->node) {
+				pr_err("%s: could not find phandle\n",
+				       it->parent->full_name);
+				goto err;
+			}
+
+			if (of_property_read_u32(it->node, it->cells_name,
+						 &count)) {
+				pr_err("%s: could not get %s for %s\n",
+				       it->parent->full_name,
+				       it->cells_name,
+				       it->node->full_name);
+				goto err;
+			}
+		} else {
+			count = it->cell_count;
+		}
+
+		/*
+		 * Make sure that the arguments actually fit in the remaining
+		 * property data length
+		 */
+		if (it->cur + count > it->list_end) {
+			pr_err("%s: arguments longer than property\n",
+			       it->parent->full_name);
+			goto err;
+		}
+	}
+
+	it->phandle_end = it->cur + count;
+	it->cur_count = count;
+
+	return 0;
+
+err:
+	if (it->node) {
+		of_node_put(it->node);
+		it->node = NULL;
+	}
+
+	return -EINVAL;
+}
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
@@ -1479,59 +1548,9 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 		return rc;
 
 	/* Loop over the phandles until all the requested entry is found */
-	while (it.cur < it.list_end) {
-		rc = -EINVAL;
-		it.cur_count = 0;
-
-		/*
-		 * If phandle is 0, then it is an empty entry with no
-		 * arguments.  Skip forward to the next entry.
-		 */
-		it.phandle = be32_to_cpup(it.cur++);
-		if (it.phandle) {
-			/*
-			 * Find the provider node and parse the #*-cells
-			 * property to determine the argument length.
-			 *
-			 * This is not needed if the cell count is hard-coded
-			 * (i.e. cells_name not set, but cell_count is set),
-			 * except when we're going to return the found node
-			 * below.
-			 */
-			if (it.cells_name || cur_index == index) {
-				it.node = of_find_node_by_phandle(it.phandle);
-				if (!it.node) {
-					pr_err("%s: could not find phandle\n",
-						it.parent->full_name);
-					goto err;
-				}
-			}
-
-			if (it.cells_name) {
-				if (of_property_read_u32(it.node, it.cells_name,
-							 &it.cur_count)) {
-					pr_err("%s: could not get %s for %s\n",
-						it.parent->full_name, it.cells_name,
-						it.node->full_name);
-					goto err;
-				}
-			} else {
-				it.cur_count = it.cell_count;
-			}
-
-			/*
-			 * Make sure that the arguments actually fit in the
-			 * remaining property data length
-			 */
-			if (it.cur + it.cur_count > it.list_end) {
-				pr_err("%s: arguments longer than property\n",
-					 it.parent->full_name);
-				goto err;
-			}
-		}
-
+	while ((rc = of_phandle_iterator_next(&it)) == 0) {
 		/*
-		 * All of the error cases above bail out of the loop, so at
+		 * All of the error cases bail out of the loop, so at
 		 * this point, the parsing is successful. If the requested
 		 * index matches, then fill the out_args structure and return,
 		 * or return -ENOENT for an empty entry.
@@ -1557,9 +1576,6 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 			return 0;
 		}
 
-		of_node_put(it.node);
-		it.node = NULL;
-		it.cur += it.cur_count;
 		cur_index++;
 	}
 
@@ -1569,7 +1585,9 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	 * -EINVAL : parsing error on data
 	 * [1..n]  : Number of phandle (count mode; when index = -1)
 	 */
-	rc = index < 0 ? cur_index : -ENOENT;
+	if (rc == -ENOENT && index < 0)
+		rc = cur_index;
+
  err:
 	if (it.node)
 		of_node_put(it.node);
diff --git a/include/linux/of.h b/include/linux/of.h
index 70c2bdb..d94388b0 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -358,6 +358,8 @@ extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
 				    const char *cells_name,
 				    int cell_count);
 
+extern int of_phandle_iterator_next(struct of_phandle_iterator *it);
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);
@@ -641,6 +643,11 @@ static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
 	return -ENOSYS;
 }
 
+static inline int of_phandle_iterator_next(struct of_phandle_iterator *it)
+{
+	return -ENOSYS;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
-- 
1.9.1

[PATCH 3/6] of: Remove counting special case from __of_parse_phandle_with_args()

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

The index = -1 case in __of_parse_phandle_with_args() is
used to just return the number of phandles. That special
case needs extra handling, so move it to the place where it
is needed: of_count_phandle_with_args().

This allows to further simplify __of_parse_phandle_with_args()
later on.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/of/base.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 4036512..15593cd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1583,10 +1583,7 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	 * Unlock node before returning result; will be one of:
 	 * -ENOENT : index is for empty phandle
 	 * -EINVAL : parsing error on data
-	 * [1..n]  : Number of phandle (count mode; when index = -1)
 	 */
-	if (rc == -ENOENT && index < 0)
-		rc = cur_index;
 
  err:
 	if (it.node)
@@ -1722,8 +1719,20 @@ EXPORT_SYMBOL(of_parse_phandle_with_fixed_args);
 int of_count_phandle_with_args(const struct device_node *np, const char *list_name,
 				const char *cells_name)
 {
-	return __of_parse_phandle_with_args(np, list_name, cells_name, 0, -1,
-					    NULL);
+	struct of_phandle_iterator it;
+	int rc, cur_index = 0;
+
+	rc = of_phandle_iterator_init(&it, np, list_name, cells_name, 0);
+	if (rc)
+		return rc;
+
+	while ((rc = of_phandle_iterator_next(&it)) == 0)
+		cur_index += 1;
+
+	if (rc != -ENOENT)
+		return rc;
+
+	return cur_index;
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
-- 
1.9.1

[PATCH 4/6] of: Introduce of_for_each_phandle() helper macro

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

With this macro any user can easily iterate over a list of
phandles. The patch also converts __of_parse_phandle_with_args()
to make use of the macro.

The of_count_phandle_with_args() function is not converted,
because the macro hides the return value of of_phandle_iterator_init(),
which is needed in there.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/of/base.c  | 7 +------
 include/linux/of.h | 6 ++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 15593cd..471d3d9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1542,13 +1542,8 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	struct of_phandle_iterator it;
 	int rc, cur_index = 0;
 
-	rc = of_phandle_iterator_init(&it, np, list_name,
-				      cells_name, cell_count);
-	if (rc)
-		return rc;
-
 	/* Loop over the phandles until all the requested entry is found */
-	while ((rc = of_phandle_iterator_next(&it)) == 0) {
+	of_for_each_phandle(&it, rc, np, list_name, cells_name, cell_count) {
 		/*
 		 * All of the error cases bail out of the loop, so at
 		 * this point, the parsing is successful. If the requested
diff --git a/include/linux/of.h b/include/linux/of.h
index d94388b0..05e38ed 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -908,6 +908,12 @@ static inline int of_property_read_s32(const struct device_node *np,
 	return of_property_read_u32(np, propname, (u32*) out_value);
 }
 
+#define of_for_each_phandle(it, err, np, ln, cn, cc)			\
+	for (of_phandle_iterator_init((it), (np), (ln), (cn), (cc)),	\
+	     err = of_phandle_iterator_next(it);			\
+	     err == 0;							\
+	     err = of_phandle_iterator_next(it))
+
 #define of_property_for_each_u32(np, propname, prop, p, u)	\
 	for (prop = of_find_property(np, propname, NULL),	\
 		p = of_prop_next_u32(prop, NULL, &u);		\
-- 
1.9.1

[PATCH 5/6] of: Introduce of_phandle_iterator_args()

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

This helper function can be used to copy the arguments of a
phandle to an array.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/of/base.c  | 29 +++++++++++++++++++++++------
 include/linux/of.h | 10 ++++++++++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 471d3d9..008988b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1533,6 +1533,23 @@ err:
 	return -EINVAL;
 }
 
+int of_phandle_iterator_args(struct of_phandle_iterator *it,
+			     uint32_t *args,
+			     int size)
+{
+	int i, count;
+
+	count = it->cur_count;
+
+	if (WARN_ON(size < count))
+		count = size;
+
+	for (i = 0; i < count; i++)
+		args[i] = be32_to_cpup(it->cur++);
+
+	return count;
+}
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
@@ -1556,13 +1573,13 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 				goto err;
 
 			if (out_args) {
-				int i;
-				if (WARN_ON(it.cur_count > MAX_PHANDLE_ARGS))
-					it.cur_count = MAX_PHANDLE_ARGS;
+				int c;
+
+				c = of_phandle_iterator_args(&it,
+							     out_args->args,
+							     MAX_PHANDLE_ARGS);
 				out_args->np = it.node;
-				out_args->args_count = it.cur_count;
-				for (i = 0; i < it.cur_count; i++)
-					out_args->args[i] = be32_to_cpup(it.cur++);
+				out_args->args_count = c;
 			} else {
 				of_node_put(it.node);
 			}
diff --git a/include/linux/of.h b/include/linux/of.h
index 05e38ed..c91f8b1 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -359,6 +359,9 @@ extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
 				    int cell_count);
 
 extern int of_phandle_iterator_next(struct of_phandle_iterator *it);
+extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
+				    uint32_t *args,
+				    int size);
 
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
@@ -648,6 +651,13 @@ static inline int of_phandle_iterator_next(struct of_phandle_iterator *it)
 	return -ENOSYS;
 }
 
+static inline int of_phandle_iterator_args(struct of_phandle_iterator *it,
+					   uint32_t *args,
+					   int size)
+{
+	return 0;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
-- 
1.9.1

[PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Remove the usage of of_parse_phandle_with_args() and replace
it by the phandle-iterator implementation so that we can
parse out all of the potentially present 128 stream-ids.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8..413bd64 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -48,7 +48,7 @@
 #include "io-pgtable.h"
 
 /* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
+#define MAX_MASTER_STREAMIDS		128
 
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
@@ -349,6 +349,12 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };
 
+struct arm_smmu_phandle_args {
+	struct device_node *np;
+	int args_count;
+	uint32_t args[MAX_MASTER_STREAMIDS];
+};
+
 static struct iommu_ops arm_smmu_ops;
 
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
@@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct arm_smmu_phandle_args *masterspec)
 {
 	int i;
 	struct arm_smmu_master *master;
@@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
 	struct rb_node *node;
-	struct of_phandle_args masterspec;
+	struct of_phandle_iterator it;
+	struct arm_smmu_phandle_args masterspec;
 	int num_irqs, i, err;
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
@@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	i = 0;
 	smmu->masters = RB_ROOT;
-	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
-					   "#stream-id-cells", i,
-					   &masterspec)) {
+
+	of_for_each_phandle(&it, err, dev->of_node,
+			    "mmu-masters", "#stream-id-cells", 0) {
+		int count = of_phandle_iterator_args(&it, masterspec.args,
+						     MAX_MASTER_STREAMIDS);
+		masterspec.np		= of_node_get(it.node);
+		masterspec.args_count	= count;
+
 		err = register_smmu_master(smmu, dev, &masterspec);
 		if (err) {
 			dev_err(dev, "failed to add master %s\n",
@@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 		i++;
 	}
+
+	if (i == 0)
+		goto out_put_masters;
+
 	dev_notice(dev, "registered %d master devices\n", i);
 
 	parse_driver_options(smmu);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Rob Herring]

On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
>
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.
>
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..413bd64 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,7 +48,7 @@
>  #include "io-pgtable.h"
>
>  /* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS           MAX_PHANDLE_ARGS
> +#define MAX_MASTER_STREAMIDS           128
>
>  /* Maximum number of context banks per SMMU */
>  #define ARM_SMMU_MAX_CBS               128
> @@ -349,6 +349,12 @@ struct arm_smmu_domain {
>         struct iommu_domain             domain;
>  };
>
> +struct arm_smmu_phandle_args {
> +       struct device_node *np;
> +       int args_count;
> +       uint32_t args[MAX_MASTER_STREAMIDS];
> +};
> +
>  static struct iommu_ops arm_smmu_ops;
>
>  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>
>  static int register_smmu_master(struct arm_smmu_device *smmu,
>                                 struct device *dev,
> -                               struct of_phandle_args *masterspec)
> +                               struct arm_smmu_phandle_args *masterspec)
>  {
>         int i;
>         struct arm_smmu_master *master;
> @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>         struct arm_smmu_device *smmu;
>         struct device *dev = &pdev->dev;
>         struct rb_node *node;
> -       struct of_phandle_args masterspec;
> +       struct of_phandle_iterator it;
> +       struct arm_smmu_phandle_args masterspec;

Isn't this a bit big to put on the stack being ~512 bytes?

>         int num_irqs, i, err;
>
>         smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);

[PATCH v2] iommu/arm-smmu: Make use of phandle iterators in device-tree device-tree parsing

[Joerg Roedel]

On Tue, Mar 22, 2016 at 01:38:06PM -0500, Rob Herring wrote:
> > +       struct of_phandle_iterator it;
> > +       struct arm_smmu_phandle_args masterspec;
> 
> Isn't this a bit big to put on the stack being ~512 bytes?

Yeah, you might be right. I havn't seen any problems booting with this
being allocated on the stack, but to be on the safe side I changed the
patch to allocate the masterspec with kmalloc, patch below.


	Joerg

>From 85995d1edea7f61aae3c31e0fbd3258622d0b5ae Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Date: Wed, 16 Mar 2016 17:10:10 +0100
Subject: [PATCH] iommu/arm-smmu: Make use of phandle iterators in device-tree
 parsing

Remove the usage of of_parse_phandle_with_args() and replace
it by the phandle-iterator implementation so that we can
parse out all of the potentially present 128 stream-ids.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8..fa9b98c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -48,7 +48,7 @@
 #include "io-pgtable.h"
 
 /* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
+#define MAX_MASTER_STREAMIDS		128
 
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
@@ -349,6 +349,12 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };
 
+struct arm_smmu_phandle_args {
+	struct device_node *np;
+	int args_count;
+	uint32_t args[MAX_MASTER_STREAMIDS];
+};
+
 static struct iommu_ops arm_smmu_ops;
 
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
@@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct arm_smmu_phandle_args *masterspec)
 {
 	int i;
 	struct arm_smmu_master *master;
@@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
 	struct rb_node *node;
-	struct of_phandle_args masterspec;
+	struct of_phandle_iterator it;
+	struct arm_smmu_phandle_args *masterspec;
 	int num_irqs, i, err;
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
@@ -1777,20 +1784,38 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	i = 0;
 	smmu->masters = RB_ROOT;
-	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
-					   "#stream-id-cells", i,
-					   &masterspec)) {
-		err = register_smmu_master(smmu, dev, &masterspec);
+
+	err = -ENOMEM;
+	/* No need to zero the memory for masterspec */
+	masterspec = kmalloc(sizeof(*masterspec), GFP_KERNEL);
+	if (!masterspec)
+		goto out_put_masters;
+
+	of_for_each_phandle(&it, err, dev->of_node,
+			    "mmu-masters", "#stream-id-cells", 0) {
+		int count = of_phandle_iterator_args(&it, masterspec->args,
+						     MAX_MASTER_STREAMIDS);
+		masterspec->np		= of_node_get(it.node);
+		masterspec->args_count	= count;
+
+		err = register_smmu_master(smmu, dev, masterspec);
 		if (err) {
 			dev_err(dev, "failed to add master %s\n",
-				masterspec.np->name);
+				masterspec->np->name);
+			kfree(masterspec);
 			goto out_put_masters;
 		}
 
 		i++;
 	}
+
+	if (i == 0)
+		goto out_put_masters;
+
 	dev_notice(dev, "registered %d master devices\n", i);
 
+	kfree(masterspec);
+
 	parse_driver_options(smmu);
 
 	if (smmu->version > ARM_SMMU_V1 &&
-- 
2.4.3

Re: [PATCH v2] iommu/arm-smmu: Make use of phandle iterators in device-tree device-tree parsing

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4668 bytes --]

Hi Joerg,

[auto build test ERROR on iommu/next]
[also build test ERROR on v4.5 next-20160323]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Joerg-Roedel/iommu-arm-smmu-Make-use-of-phandle-iterators-in-device-tree-device-tree-parsing/20160323-194824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm64-defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/iommu/arm-smmu.c: In function 'arm_smmu_device_dt_probe':
   drivers/iommu/arm-smmu.c:1746:29: error: storage size of 'it' isn't known
     struct of_phandle_iterator it;
                                ^
>> drivers/iommu/arm-smmu.c:1815:2: error: implicit declaration of function 'of_for_each_phandle' [-Werror=implicit-function-declaration]
     of_for_each_phandle(&it, err, dev->of_node,
     ^
>> drivers/iommu/arm-smmu.c:1816:46: error: expected ';' before '{' token
           "mmu-masters", "#stream-id-cells", 0) {
                                                 ^
   drivers/iommu/arm-smmu.c:1746:29: warning: unused variable 'it' [-Wunused-variable]
     struct of_phandle_iterator it;
                                ^
   drivers/iommu/arm-smmu.c: At top level:
   drivers/iommu/arm-smmu.c:473:12: warning: 'register_smmu_master' defined but not used [-Wunused-function]
    static int register_smmu_master(struct arm_smmu_device *smmu,
               ^
   cc1: some warnings being treated as errors

vim +/of_for_each_phandle +1815 drivers/iommu/arm-smmu.c

  1740	{
  1741		const struct of_device_id *of_id;
  1742		struct resource *res;
  1743		struct arm_smmu_device *smmu;
  1744		struct device *dev = &pdev->dev;
  1745		struct rb_node *node;
> 1746		struct of_phandle_iterator it;
  1747		struct arm_smmu_phandle_args *masterspec;
  1748		int num_irqs, i, err;
  1749	
  1750		smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
  1751		if (!smmu) {
  1752			dev_err(dev, "failed to allocate arm_smmu_device\n");
  1753			return -ENOMEM;
  1754		}
  1755		smmu->dev = dev;
  1756	
  1757		of_id = of_match_node(arm_smmu_of_match, dev->of_node);
  1758		smmu->version = (enum arm_smmu_arch_version)of_id->data;
  1759	
  1760		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  1761		smmu->base = devm_ioremap_resource(dev, res);
  1762		if (IS_ERR(smmu->base))
  1763			return PTR_ERR(smmu->base);
  1764		smmu->size = resource_size(res);
  1765	
  1766		if (of_property_read_u32(dev->of_node, "#global-interrupts",
  1767					 &smmu->num_global_irqs)) {
  1768			dev_err(dev, "missing #global-interrupts property\n");
  1769			return -ENODEV;
  1770		}
  1771	
  1772		num_irqs = 0;
  1773		while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, num_irqs))) {
  1774			num_irqs++;
  1775			if (num_irqs > smmu->num_global_irqs)
  1776				smmu->num_context_irqs++;
  1777		}
  1778	
  1779		if (!smmu->num_context_irqs) {
  1780			dev_err(dev, "found %d interrupts but expected at least %d\n",
  1781				num_irqs, smmu->num_global_irqs + 1);
  1782			return -ENODEV;
  1783		}
  1784	
  1785		smmu->irqs = devm_kzalloc(dev, sizeof(*smmu->irqs) * num_irqs,
  1786					  GFP_KERNEL);
  1787		if (!smmu->irqs) {
  1788			dev_err(dev, "failed to allocate %d irqs\n", num_irqs);
  1789			return -ENOMEM;
  1790		}
  1791	
  1792		for (i = 0; i < num_irqs; ++i) {
  1793			int irq = platform_get_irq(pdev, i);
  1794	
  1795			if (irq < 0) {
  1796				dev_err(dev, "failed to get irq index %d\n", i);
  1797				return -ENODEV;
  1798			}
  1799			smmu->irqs[i] = irq;
  1800		}
  1801	
  1802		err = arm_smmu_device_cfg_probe(smmu);
  1803		if (err)
  1804			return err;
  1805	
  1806		i = 0;
  1807		smmu->masters = RB_ROOT;
  1808	
  1809		err = -ENOMEM;
  1810		/* No need to zero the memory for masterspec */
  1811		masterspec = kmalloc(sizeof(*masterspec), GFP_KERNEL);
  1812		if (!masterspec)
  1813			goto out_put_masters;
  1814	
> 1815		of_for_each_phandle(&it, err, dev->of_node,
> 1816				    "mmu-masters", "#stream-id-cells", 0) {
  1817			int count = of_phandle_iterator_args(&it, masterspec->args,
  1818							     MAX_MASTER_STREAMIDS);
  1819			masterspec->np		= of_node_get(it.node);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21203 bytes --]

Re: [PATCH v2] iommu/arm-smmu: Make use of phandle iterators in device-tree device-tree parsing

[Joerg Roedel]

On Wed, Mar 23, 2016 at 11:18:25PM +0800, kbuild test robot wrote:
>    drivers/iommu/arm-smmu.c: In function 'arm_smmu_device_dt_probe':
>    drivers/iommu/arm-smmu.c:1746:29: error: storage size of 'it' isn't known
>      struct of_phandle_iterator it;
>                                 ^
> >> drivers/iommu/arm-smmu.c:1815:2: error: implicit declaration of function 'of_for_each_phandle' [-Werror=implicit-function-declaration]
>      of_for_each_phandle(&it, err, dev->of_node,
>      ^
> >> drivers/iommu/arm-smmu.c:1816:46: error: expected ';' before '{' token
>            "mmu-masters", "#stream-id-cells", 0) {
>                                                  ^
>    drivers/iommu/arm-smmu.c:1746:29: warning: unused variable 'it' [-Wunused-variable]
>      struct of_phandle_iterator it;
>                                 ^
>    drivers/iommu/arm-smmu.c: At top level:
>    drivers/iommu/arm-smmu.c:473:12: warning: 'register_smmu_master' defined but not used [-Wunused-function]
>     static int register_smmu_master(struct arm_smmu_device *smmu,
>                ^
>    cc1: some warnings being treated as errors

Looks like this patch got compiled without the other patch in this set,
right? Because the config builds perfectly fine here for me.


	Joerg

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Robin Murphy]

Hi Joerg,

On 22/03/16 17:58, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
>
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.

In a stream-matching implementation, a device may quite legitimately own 
anything up to _all_ of the stream IDs (32768, or 65536 if we ever 
implement support for the SMMUv2 EXID extension), so this is only a 
genuine limit for stream indexing (and if anyone ever actually made one 
of those, I don't think they're running mainline on it).

Alternatively, how straightforward is it to change the DT on your 
machine? I'll be getting a v2 of [1] out in a couple of weeks (after 
imminent holidays), which already gets rid of MAX_MASTER_STREAMIDS 
altogether, and might also have grown proper SMR support by then.

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12454

> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..413bd64 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,7 +48,7 @@
>   #include "io-pgtable.h"
>
>   /* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
> +#define MAX_MASTER_STREAMIDS		128
>
>   /* Maximum number of context banks per SMMU */
>   #define ARM_SMMU_MAX_CBS		128
> @@ -349,6 +349,12 @@ struct arm_smmu_domain {
>   	struct iommu_domain		domain;
>   };
>
> +struct arm_smmu_phandle_args {
> +	struct device_node *np;
> +	int args_count;
> +	uint32_t args[MAX_MASTER_STREAMIDS];
> +};
> +
>   static struct iommu_ops arm_smmu_ops;
>
>   static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>
>   static int register_smmu_master(struct arm_smmu_device *smmu,
>   				struct device *dev,
> -				struct of_phandle_args *masterspec)
> +				struct arm_smmu_phandle_args *masterspec)
>   {
>   	int i;
>   	struct arm_smmu_master *master;
> @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>   	struct arm_smmu_device *smmu;
>   	struct device *dev = &pdev->dev;
>   	struct rb_node *node;
> -	struct of_phandle_args masterspec;
> +	struct of_phandle_iterator it;
> +	struct arm_smmu_phandle_args masterspec;
>   	int num_irqs, i, err;
>
>   	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> @@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>
>   	i = 0;
>   	smmu->masters = RB_ROOT;
> -	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> -					   "#stream-id-cells", i,
> -					   &masterspec)) {
> +
> +	of_for_each_phandle(&it, err, dev->of_node,
> +			    "mmu-masters", "#stream-id-cells", 0) {
> +		int count = of_phandle_iterator_args(&it, masterspec.args,
> +						     MAX_MASTER_STREAMIDS);
> +		masterspec.np		= of_node_get(it.node);
> +		masterspec.args_count	= count;
> +
>   		err = register_smmu_master(smmu, dev, &masterspec);
>   		if (err) {
>   			dev_err(dev, "failed to add master %s\n",
> @@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>
>   		i++;
>   	}
> +
> +	if (i == 0)
> +		goto out_put_masters;
> +
>   	dev_notice(dev, "registered %d master devices\n", i);
>
>   	parse_driver_options(smmu);
>

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

Hi Robin,

On Tue, Mar 22, 2016 at 06:53:48PM +0000, Robin Murphy wrote:
> In a stream-matching implementation, a device may quite legitimately
> own anything up to _all_ of the stream IDs (32768, or 65536 if we
> ever implement support for the SMMUv2 EXID extension), so this is
> only a genuine limit for stream indexing (and if anyone ever
> actually made one of those, I don't think they're running mainline
> on it).

Do you mean we might see a lot more than the currently 128 supported
stream-ids for an smmu?

> Alternatively, how straightforward is it to change the DT on your
> machine? I'll be getting a v2 of [1] out in a couple of weeks (after
> imminent holidays), which already gets rid of MAX_MASTER_STREAMIDS
> altogether, and might also have grown proper SMR support by then.

Hmm, I don't know how to change the DT of this Seattle machine. I think
it is provided by the ACPI BIOS. At least it boots with grub2 and not
u-boot and there is no DT in /boot.


	Joerg

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Will Deacon]

On Wed, Mar 23, 2016 at 12:51:28PM +0100, Joerg Roedel wrote:
> On Tue, Mar 22, 2016 at 06:53:48PM +0000, Robin Murphy wrote:
> > In a stream-matching implementation, a device may quite legitimately
> > own anything up to _all_ of the stream IDs (32768, or 65536 if we
> > ever implement support for the SMMUv2 EXID extension), so this is
> > only a genuine limit for stream indexing (and if anyone ever
> > actually made one of those, I don't think they're running mainline
> > on it).
> 
> Do you mean we might see a lot more than the currently 128 supported
> stream-ids for an smmu?

We might, but this patch is still an improvement for now.

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Will Deacon]

Hi Joerg,

On Tue, Mar 22, 2016 at 06:58:29PM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..413bd64 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,7 +48,7 @@
>  #include "io-pgtable.h"
>  
>  /* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
> +#define MAX_MASTER_STREAMIDS		128
>  
>  /* Maximum number of context banks per SMMU */
>  #define ARM_SMMU_MAX_CBS		128
> @@ -349,6 +349,12 @@ struct arm_smmu_domain {
>  	struct iommu_domain		domain;
>  };
>  
> +struct arm_smmu_phandle_args {
> +	struct device_node *np;
> +	int args_count;
> +	uint32_t args[MAX_MASTER_STREAMIDS];
> +};
> +
>  static struct iommu_ops arm_smmu_ops;
>  
>  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>  
>  static int register_smmu_master(struct arm_smmu_device *smmu,
>  				struct device *dev,
> -				struct of_phandle_args *masterspec)
> +				struct arm_smmu_phandle_args *masterspec)
>  {
>  	int i;
>  	struct arm_smmu_master *master;
> @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  	struct arm_smmu_device *smmu;
>  	struct device *dev = &pdev->dev;
>  	struct rb_node *node;
> -	struct of_phandle_args masterspec;
> +	struct of_phandle_iterator it;
> +	struct arm_smmu_phandle_args masterspec;
>  	int num_irqs, i, err;
>  
>  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> @@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  
>  	i = 0;
>  	smmu->masters = RB_ROOT;
> -	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> -					   "#stream-id-cells", i,
> -					   &masterspec)) {
> +
> +	of_for_each_phandle(&it, err, dev->of_node,
> +			    "mmu-masters", "#stream-id-cells", 0) {
> +		int count = of_phandle_iterator_args(&it, masterspec.args,
> +						     MAX_MASTER_STREAMIDS);
> +		masterspec.np		= of_node_get(it.node);
> +		masterspec.args_count	= count;
> +
>  		err = register_smmu_master(smmu, dev, &masterspec);
>  		if (err) {
>  			dev_err(dev, "failed to add master %s\n",
> @@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  
>  		i++;
>  	}
> +
> +	if (i == 0)
> +		goto out_put_masters;

I'm confused by this hunk. If i == 0, then we shouldn't have registered
any masters and therefore out_put_masters won't have anything to do.

In fact, I'm not completely clear on how the of_node refcounting interacts
with your iterators. Does the iterator put the node after you call the
"next" function, or does it increment each thing exactly once?

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

Hi Will,

On Tue, Mar 29, 2016 at 06:22:16PM +0100, Will Deacon wrote:
> > +
> > +	if (i == 0)
> > +		goto out_put_masters;
> 
> I'm confused by this hunk. If i == 0, then we shouldn't have registered
> any masters and therefore out_put_masters won't have anything to do.

The idea was that there is nothing more to do in the function when it
didn't find any masters and so it can safely skip the rest of the
function.

But the original code doesn't do this either, so it certainly doesn't
belong into this patch. I remove it for the next post.

> In fact, I'm not completely clear on how the of_node refcounting interacts
> with your iterators. Does the iterator put the node after you call the
> "next" function, or does it increment each thing exactly once?

The iterator will put the current node at the following _next call, so
when you want to use each node, you need your own reference.

It works like the pci_dev iterators, so if you break out of the loop you
have to manually put the last node it returned.


	Joerg

Re: [PATCH 0/6] of: Implement iterator for phandles

[Rob Herring]

On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> Hi,
>
> here is an implementation of the iterator over phandles
> concept which Rob Herring suggested to me some time ago. My
> approach is a little bit different from what the diff showed
> back then, but it gets rid of the allocation and 'struct
> 'struct of_phandle_args' misuse.
>
> I also converted the arm-smmu driver to make use of the
> iterator. The resulting kernel boots on my AMD Seattle
> system and fixes the warning triggered there. The patches
> now also pass all dt-unittests in my kvm environment.
>
> Please review. Patches are based on v4.5.

Other than my one comment, this looks good to me. For the series:

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Re: [PATCH 0/6] of: Implement iterator for phandles

[Joerg Roedel]

Hi Rob,

On Tue, Mar 22, 2016 at 01:45:41PM -0500, Rob Herring wrote:
> On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> > Please review. Patches are based on v4.5.
> 
> Other than my one comment, this looks good to me. For the series:
> 
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks a lot for your fast reply! I guess these patches will go through
the DT tree, or should I carry them in the IOMMU tree? The DT tree
probably makes more sense.



	Joerg

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] of: Implement iterator for phandles

[Rob Herring]

On Wed, Mar 23, 2016 at 6:54 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> Hi Rob,
>
> On Tue, Mar 22, 2016 at 01:45:41PM -0500, Rob Herring wrote:
>> On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
>> > Please review. Patches are based on v4.5.
>>
>> Other than my one comment, this looks good to me. For the series:
>>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Thanks a lot for your fast reply! I guess these patches will go through
> the DT tree, or should I carry them in the IOMMU tree? The DT tree
> probably makes more sense.

Sure, I can take them.

Rob

Re: [PATCH 0/6] of: Implement iterator for phandles

[Joerg Roedel]

On Wed, Mar 23, 2016 at 03:37:44PM -0500, Rob Herring wrote:
> On Wed, Mar 23, 2016 at 6:54 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> > Thanks a lot for your fast reply! I guess these patches will go through
> > the DT tree, or should I carry them in the IOMMU tree? The DT tree
> > probably makes more sense.
> 
> Sure, I can take them.

Thanks! I am about to post a new version with all changes collected
together and rebased to the latest upstream kernel.


	Joerg

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/6 v2] of: Implement iterator for phandles

[Joerg Roedel]

Hi,

here is a new version of the implementation of the iterator
over phandles concept which Rob Herring suggested to me some
time ago. My approach is a little bit different from what
the diff showed back then, but it gets rid of the allocation
and 'struct of_phandle_args' misuse.

I also converted the arm-smmu driver to make use of the
iterator. The resulting kernel boots on my AMD Seattle
system and fixes the warning triggered there. The patches
now also pass all dt-unittests in my kvm environment.

Patches are based on v4.6-rc2

Thanks,

	Joerg

Changes since v1:

* Rebased to v4.6-rc2

* Removed the 'if (i == 0)...' hunk from the last patch

* Tested again with DT unit-tests and on the seattle system

Changes since RFC post:

* Reordered members of 'struct of_phandle_iterator' and did
  some renaming

* Removed index counting from the iterator

* Split up iterator implementation into multiple patches

* Fixed the code to survive all dt-unittests, tested with each
  patch in this series

* Re-added and updated some comments which got lost during the
  conversion.

* Added of_for_each_phandle macro for easier handling

* Moved the counting special-case from __of_parse_phandle_with_args
  directly to of_count_phandle_with_args for code
  simplification

* Removed some iterator helper functions

* Formatting and style changes

Joerg Roedel (6):
  of: Introduce struct of_phandle_iterator
  of: Move phandle walking to of_phandle_iterator_next()
  of: Remove counting special case from __of_parse_phandle_with_args()
  of: Introduce of_for_each_phandle() helper macro
  of: Introduce of_phandle_iterator_args()
  iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

 drivers/iommu/arm-smmu.c |  38 +++++++--
 drivers/of/base.c        | 206 ++++++++++++++++++++++++++++++-----------------
 include/linux/of.h       |  56 +++++++++++++
 3 files changed, 219 insertions(+), 81 deletions(-)

-- 
1.9.1

[PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Remove the usage of of_parse_phandle_with_args() and replace
it by the phandle-iterator implementation so that we can
parse out all of the potentially present 128 stream-ids.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 2409e3b..a1c965c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -49,7 +49,7 @@
 #include "io-pgtable.h"
 
 /* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
+#define MAX_MASTER_STREAMIDS		128
 
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
@@ -357,6 +357,12 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };
 
+struct arm_smmu_phandle_args {
+	struct device_node *np;
+	int args_count;
+	uint32_t args[MAX_MASTER_STREAMIDS];
+};
+
 static struct iommu_ops arm_smmu_ops;
 
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
@@ -466,7 +472,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct arm_smmu_phandle_args *masterspec)
 {
 	int i;
 	struct arm_smmu_master *master;
@@ -1737,7 +1743,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
 	struct rb_node *node;
-	struct of_phandle_args masterspec;
+	struct of_phandle_iterator it;
+	struct arm_smmu_phandle_args *masterspec;
 	int num_irqs, i, err;
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
@@ -1798,20 +1805,35 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	i = 0;
 	smmu->masters = RB_ROOT;
-	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
-					   "#stream-id-cells", i,
-					   &masterspec)) {
-		err = register_smmu_master(smmu, dev, &masterspec);
+
+	err = -ENOMEM;
+	/* No need to zero the memory for masterspec */
+	masterspec = kmalloc(sizeof(*masterspec), GFP_KERNEL);
+	if (!masterspec)
+		goto out_put_masters;
+
+	of_for_each_phandle(&it, err, dev->of_node,
+			    "mmu-masters", "#stream-id-cells", 0) {
+		int count = of_phandle_iterator_args(&it, masterspec->args,
+						     MAX_MASTER_STREAMIDS);
+		masterspec->np		= of_node_get(it.node);
+		masterspec->args_count	= count;
+
+		err = register_smmu_master(smmu, dev, masterspec);
 		if (err) {
 			dev_err(dev, "failed to add master %s\n",
-				masterspec.np->name);
+				masterspec->np->name);
+			kfree(masterspec);
 			goto out_put_masters;
 		}
 
 		i++;
 	}
+
 	dev_notice(dev, "registered %d master devices\n", i);
 
+	kfree(masterspec);
+
 	parse_driver_options(smmu);
 
 	if (smmu->version > ARM_SMMU_V1 &&
-- 
1.9.1

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Will Deacon]

Hi Joerg,

On Mon, Apr 04, 2016 at 05:49:22PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)

Looks good to me. Given that you probably want to keep this with the
related DT changes:

Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Thanks,

Will