of: Implement iterator for phandles
Subsystems (2)
Joerg Roedel
maintainercover 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(-)
Signature · 1 line
1.9.1
Joerg Roedel
maintainerPATCH 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().
--- drivers/of/base.c | 99 +++++++++++++++++++++++++++++++++--------------------- include/linux/of.h | 33 ++++++++++++++++++ 2 files changed, 93 insertions(+), 39 deletions(-)
drivers/of/base.c +60 -39
@@ -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; }
include/linux/of.h +33 -0
@@ -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 ;
Signature · 1 line
1.9.1
Joerg Roedel
maintainerPATCH 2/6 of: Move phandle walking to of_phandle_iterator_next()
In reply to Joerg RoedelFrom: 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().
--- drivers/of/base.c | 130 ++++++++++++++++++++++++++++++----------------------- include/linux/of.h | 7 +++ 2 files changed, 81 insertions(+), 56 deletions(-)
drivers/of/base.c +74 -56
@@ -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);
include/linux/of.h +7 -0
@@ -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 ;
Signature · 1 line
1.9.1
Joerg Roedel
maintainerPATCH 3/6 of: Remove counting special case from __of_parse_phandle_with_args()
In reply to Joerg RoedelFrom: 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.
--- drivers/of/base.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
drivers/of/base.c +14 -5
@@ -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);
Signature · 1 line
1.9.1
Joerg Roedel
maintainerPATCH 4/6 of: Introduce of_for_each_phandle() helper macro
In reply to Joerg RoedelFrom: 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.
--- drivers/of/base.c | 7 +------ include/linux/of.h | 6 ++++++ 2 files changed, 7 insertions(+), 6 deletions(-)
drivers/of/base.c +1 -6
@@ -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
include/linux/of.h +6 -0
@@ -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); \
Signature · 1 line
1.9.1
Joerg Roedel
maintainerPATCH 5/6 of: Introduce of_phandle_iterator_args()
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
This helper function can be used to copy the arguments of a phandle to an array.
--- drivers/of/base.c | 29 +++++++++++++++++++++++------ include/linux/of.h | 10 ++++++++++ 2 files changed, 33 insertions(+), 6 deletions(-)
drivers/of/base.c +23 -6
@@ -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); }
include/linux/of.h +10 -0
@@ -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 ;
Signature · 1 line
1.9.1
Joerg Roedel
maintainerPATCH 6/6 iommu/arm-smmu: Make use of phandle iterators in device-tree parsing
In reply to Joerg RoedelFrom: 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.
--- drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)
drivers/iommu/arm-smmu.c +22 -6
@@ -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);
Signature · 1 line
1.9.1
Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing
Quoted 47 lines · Joerg Roedel
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(-)drivers/iommu/arm-smmu.c +8
-2@@ -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;
drivers/iommu/arm-smmu.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 1726
Quoted 6 lines
drivers/iommu/arm-smmu.c +2
-1@@ -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?
Quoted 3 lines
drivers/iommu/arm-smmu.c +0
-0int num_irqs, i, err; smmu =devm_kzalloc (dev,sizeof (*smmu),GFP_KERNEL );
Joerg Roedel
maintainerPATCH v2 1/1 iommu/arm-smmu: Make use of phandle iterators in device-tree device-tree parsing
Quoted 4 lines · Rob Herring
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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
Quoted 1 line
From
85995d1edea7f61aae3c31e0fbd3258622d0b5aeMon 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.
--- drivers/iommu/arm-smmu.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
drivers/iommu/arm-smmu.c +33 -8
@@ -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 &&
Signature · 1 line
2.4.3
kbuild test robot
build botRe: [PATCH v2] iommu/arm-smmu: Make use of phandle iterators in device-tree device-tree parsing
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…
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 21203 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160323/74b47574/attachment-0001.obj>
Re: [PATCH v2] iommu/arm-smmu: Make use of phandle iterators in device-tree device-tree parsing
Quoted 13 lines · kbuild test robot
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; ^Quoted 1 line
Quoted 1 line
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, ^Quoted 1 line
Quoted 1 line
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; ^
Quoted 5 lines
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(structarm_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
Robin Murphy
reviewerRe: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing
In reply to Joerg RoedelHi Joerg,
Quoted 5 lines · Joerg Roedel
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
Quoted 80 lines
Signed-off-by Joerg Roedel <[email protected]>--- drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)drivers/iommu/arm-smmu.c +22
-6@@ -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);
Joerg Roedel
maintainerRe: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing
Hi Robin,
Quoted 6 lines · Robin Murphy
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?
Quoted 4 lines
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_STREAMIDSaltogether, 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
Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing
Quoted 10 lines · Joerg Roedel
Quoted 6 lines · Robin Murphy
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
Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing
In reply to Joerg RoedelHi Joerg,
Quoted 75 lines · Joerg Roedel
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(-)drivers/iommu/arm-smmu.c +18
-6@@ -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",
drivers/iommu/arm-smmu.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 1806
Quoted 6 lines
drivers/iommu/arm-smmu.c +3
-0@@ -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
Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing
Hi Will,
Quoted 6 lines · Will Deacon
drivers/iommu/arm-smmu.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 1806
Quoted 3 lines
drivers/iommu/arm-smmu.c +3
-0+ +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_masterswon'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.
Quoted 3 lines
In fact, I'm not completely clear on how the
of_noderefcounting 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
Quoted 8 lines · 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.
Quoted 6 lines
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:
Joerg Roedel
maintainerHi Rob,
Quoted 6 lines · Rob Herring
Quoted 1 line · Joerg Roedel
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
Quoted 6 lines · Joerg Roedel
Hi Rob,
Quoted 3 lines · Rob Herring
Quoted 1 line · Joerg Roedel
Please review. Patches are based on v4.5.
Quoted 7 lines
Quoted 3 lines
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
Quoted 6 lines · Rob Herring
Quoted 3 lines · Joerg Roedel
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