2016-03-22 17:58:41

by Joerg Roedel

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

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


2016-03-22 17:58:45

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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

2016-03-22 17:58:52

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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

2016-03-22 17:58:50

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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

2016-03-22 17:59:39

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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

Signed-off-by: Joerg Roedel <[email protected]>
---
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

2016-03-22 17:59:57

by Joerg Roedel

[permalink] [raw]
Subject: [PATCH 1/6] of: Introduce struct of_phandle_iterator

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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

2016-03-22 17:59:53

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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

2016-03-22 18:38:31

by Rob Herring

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

On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <[email protected]> wrote:
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);

2016-03-22 18:46:09

by Rob Herring

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

On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <[email protected]> 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 <[email protected]>

2016-03-22 18:53:54

by Robin Murphy

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

Hi Joerg,

On 22/03/16 17:58, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
>

2016-03-23 11:47:20

by Joerg Roedel

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

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 <[email protected]>
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 <[email protected]>
---
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

2016-03-23 11:51:40

by Joerg Roedel

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

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

2016-03-23 11:55:02

by Joerg Roedel

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

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 <[email protected]> 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 <[email protected]>

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

2016-03-23 15:19:14

by kernel test robot

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

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


Attachments:
(No filename) (4.56 kB)
.config.gz (20.71 kB)
Download all attachments

2016-03-23 20:38:08

by Rob Herring

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

On Wed, Mar 23, 2016 at 6:54 AM, Joerg Roedel <[email protected]> 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 <[email protected]> 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 <[email protected]>
>
> 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

2016-03-29 17:19:58

by Will Deacon

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

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

2016-03-29 17:22:01

by Will Deacon

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

Hi Joerg,

On Tue, Mar 22, 2016 at 06:58:29PM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2016-04-04 14:24:15

by Jörg Rödel

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

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

2016-04-04 14:25:21

by Jörg Rödel

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

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

2016-04-04 15:47:32

by Jörg Rödel

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

On Wed, Mar 23, 2016 at 03:37:44PM -0500, Rob Herring wrote:
> On Wed, Mar 23, 2016 at 6:54 AM, Joerg Roedel <[email protected]> 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