| Message ID | 1458146527-1133-2-git-send-email-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 16, 2016 at 11:42 AM, Joerg Roedel <joro@8bytes.org> wrote: > From: Joerg Roedel <jroedel@suse.de> > > Getting the arguments of phandles is somewhat limited at the > moement, because the number of arguments supported by core > code is limited to MAX_PHANDLE_ARGS, which is set to 16 > currently. > > In case of the arm smmu this is not enough, as the 128 > supported stream-ids are encoded in device-tree with > arguments to phandles. On some systems with more than 16 > stream-ids this causes a WARN_ON being triggered at boot and > an incomplete initialization of the smmu. > > This patch-set implements an iterator interface over > phandles which allows to parse out arbitrary numbers of > arguments per phandle. The existing function for parsing > them, __of_parse_phandle_with_args(), is converted to make > use of the iterator. This mostly looks fine to me, but it is kind of a lot of functions just for this one thing. For example, I think the caller can track the index themselves if they care about it. I'd also like to see a more standard style for_each type iterator define rather than open coded while loops. Some minor style comments below. > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > drivers/of/base.c | 219 ++++++++++++++++++++++++++++++----------------------- > include/linux/of.h | 95 +++++++++++++++++++++++ > 2 files changed, 220 insertions(+), 94 deletions(-) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index 017dd94..9a5f199 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -1439,119 +1439,150 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args) > printk("\n"); > } > > -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) > +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, *list_end; > - int rc = 0, size, cur_index = 0; > - uint32_t count = 0; > - struct device_node *node = NULL; > - phandle phandle; > + const __be32 *list; > + int size; > > /* Retrieve the phandle list property */ > list = of_get_property(np, list_name, &size); > if (!list) > return -ENOENT; > - list_end = list + size / sizeof(*list); > > - /* Loop over the phandles until all the requested entry is found */ > - while (list < list_end) { > - rc = -EINVAL; > - count = 0; > + it->np = np; > + it->node = NULL; > + it->list = list; > + it->phandle_end = list; > + it->list_end = list + size / sizeof(*list); > + it->cells_name = cells_name; > + it->cell_count = cell_count; > + it->cur_index = -1; > > - /* > - * 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) { > - /* > - * 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 (cells_name || cur_index == index) { > - node = of_find_node_by_phandle(phandle); > - if (!node) { > - pr_err("%s: could not find phandle\n", > - np->full_name); > - goto err; > - } > - } > + return 0; > +} > > - if (cells_name) { > - if (of_property_read_u32(node, cells_name, > - &count)) { > - pr_err("%s: could not get %s for %s\n", > - np->full_name, cells_name, > - node->full_name); > - goto err; > - } > - } else { > - count = cell_count; > - } > +int of_phandle_iterator_next(struct of_phandle_iterator *it) > +{ > + phandle phandle = 0; > + uint32_t count = 0; > > - /* > - * Make sure that the arguments actually fit in the > - * remaining property data length > - */ > - if (list + count > list_end) { > - pr_err("%s: arguments longer than property\n", > - np->full_name); > - goto err; > - } > + if (it->phandle_end >= it->list_end) > + return -ENOENT; > + > + if (it->node) { > + of_node_put(it->node); > + it->node = NULL; > + } > + > + it->list = it->phandle_end; > + it->cur_index += 1; it->cur_index++; > + phandle = be32_to_cpup(it->list++); Please, just <space>=<space>. > + > + if (phandle) { > + if (it->cells_name) { > + it->node = of_find_node_by_phandle(phandle); > + if (!it->node) > + goto no_node; All these goto's are pretty pointless and can just be printk and return. > + > + if (of_property_read_u32(it->node, > + it->cells_name, > + &count)) > + goto no_property; > + } else { > + count = it->cell_count; > } > > - /* > - * All of the error cases above 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. > - */ > + if (it->list + count > it->list_end) > + goto too_many_args; > + } > + > + it->phandle_end = it->list + count; > + it->cur_count = count; > + it->phandle = phandle; single space. > + > + return 0; > + > +no_node: > + pr_err("%s: could not find phandle\n", it->np->full_name); > + > + return -EINVAL; > + > +no_property: > + pr_err("%s: could not get %s for %s\n", it->np->full_name, > + it->cells_name, it->node->full_name); > + > + return -EINVAL; > + > +too_many_args: > + pr_err("%s: arguments longer than property\n", it->np->full_name); > + > + 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->list++); > + > + return count; > +} > + > +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) > +{ > + struct of_phandle_iterator it; > + int rc; > + > + rc = of_phandle_iterator_init(&it, np, list_name, > + cells_name, cell_count); > + if (rc) > + return rc; > + > + while ((rc = of_phandle_iterator_next(&it)) == 0) { > + uint32_t count; > + int cur_index; > + > + count = of_phandle_iterator_count(&it); > + cur_index = of_phandle_iterator_index(&it); > + > rc = -ENOENT; > if (cur_index == index) { > - if (!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++); > - } else { > - of_node_put(node); > - } > + if (!of_phandle_iterator_phandle(&it)) > + break; > + > + rc = 0; > > /* Found it! return success */ > - return 0; > - } > + if (!out_args) > + break; > > - of_node_put(node); > - node = NULL; > - list += count; > - cur_index++; > + out_args->np = of_phandle_iterator_node(&it); > + out_args->args_count = of_phandle_iterator_args(&it, > + out_args->args, > + MAX_PHANDLE_ARGS); > + > + break; > + } > } > > - /* > - * 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) > - */ > - rc = index < 0 ? cur_index : -ENOENT; > - err: > - if (node) > - of_node_put(node); > + of_phandle_iterator_destroy(&it); > + > return rc; > } > > diff --git a/include/linux/of.h b/include/linux/of.h > index dc6e396..31e6ee9 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -75,6 +75,19 @@ struct of_phandle_args { > uint32_t args[MAX_PHANDLE_ARGS]; > }; > > +struct of_phandle_iterator { > + const struct device_node *np; > + const __be32 *list; > + const __be32 *list_end; > + const __be32 *phandle_end; > + phandle phandle; > + struct device_node *node; np and node? If you need both, name them based on what they point to. > + const char *cells_name; > + int cell_count; > + int cur_index; > + uint32_t cur_count; > +}; > + > struct of_reconfig_data { > struct device_node *dn; > struct property *prop; > @@ -334,6 +347,50 @@ 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); > + > +static inline void of_phandle_iterator_destroy(struct of_phandle_iterator *it) > +{ > + if (it->node) > + of_node_put(it->node); > +} > + > +extern int of_phandle_iterator_next(struct of_phandle_iterator *it); > + > +static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it) > +{ > + return it->cur_index; > +} > + > +static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it) > +{ > + return it->cur_count; > +} > + > +static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it) > +{ > + return it->phandle; > +} > + > +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it) > +{ > + if (!it->node) > + it->node = of_find_node_by_phandle(it->phandle); > + > + if (it->node) > + of_node_get(it->node); The above function may have already done the get. Not sure offhand. > + > + return it->node; > +} > + > +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); > extern int of_alias_get_highest_id(const char *stem); > @@ -608,6 +665,44 @@ 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 void of_phandle_iterator_destroy(struct of_phandle_iterator *it) > +{ > +} > + > +static inline int of_phandle_iterator_next(struct of_phandle_iterator *it) > +{ > + return -ENOSYS; > +} > + > +static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it) > +{ > + return -1; > +} > + > +static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it) > +{ > + return 0; > +} > + > +static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it) > +{ > + return 0; > +} > + > +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it) > +{ > + return NULL; > +} > + > static inline int of_alias_get_id(struct device_node *np, const char *stem) > { > return -ENOSYS; > -- > 1.9.1 >
Hi Rob, On Fri, Mar 18, 2016 at 10:54:57AM -0500, Rob Herring wrote: > This mostly looks fine to me, but it is kind of a lot of functions > just for this one thing. For example, I think the caller can track the > index themselves if they care about it. I'd also like to see a more > standard style for_each type iterator define rather than open coded > while loops. Thanks for your feedback. I think I worked everything into the patches and am about to send a new version. > > +struct of_phandle_iterator { > > + const struct device_node *np; > > + const __be32 *list; > > + const __be32 *list_end; > > + const __be32 *phandle_end; > > + phandle phandle; > > + struct device_node *node; > > np and node? If you need both, name them based on what they point to. Yes, 'np' is the parent node, while 'node' is the current node the iterator points to. The parent node is needed for the error messages later. > > +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it) > > +{ > > + if (!it->node) > > + it->node = of_find_node_by_phandle(it->phandle); > > + > > + if (it->node) > > + of_node_get(it->node); > > The above function may have already done the get. Not sure offhand. Yes, it does. But when the iterator moves on the node will be put again. So when it is returned here we get an extra reference. But it doesn't matter anymore as I changed the code to unconditionally get the node in of_phandle_iterator_next now. Joerg
diff --git a/drivers/of/base.c b/drivers/of/base.c index 017dd94..9a5f199 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -1439,119 +1439,150 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args) printk("\n"); } -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) +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, *list_end; - int rc = 0, size, cur_index = 0; - uint32_t count = 0; - struct device_node *node = NULL; - phandle phandle; + const __be32 *list; + int size; /* Retrieve the phandle list property */ list = of_get_property(np, list_name, &size); if (!list) return -ENOENT; - list_end = list + size / sizeof(*list); - /* Loop over the phandles until all the requested entry is found */ - while (list < list_end) { - rc = -EINVAL; - count = 0; + it->np = np; + it->node = NULL; + it->list = list; + it->phandle_end = list; + it->list_end = list + size / sizeof(*list); + it->cells_name = cells_name; + it->cell_count = cell_count; + it->cur_index = -1; - /* - * 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) { - /* - * 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 (cells_name || cur_index == index) { - node = of_find_node_by_phandle(phandle); - if (!node) { - pr_err("%s: could not find phandle\n", - np->full_name); - goto err; - } - } + return 0; +} - if (cells_name) { - if (of_property_read_u32(node, cells_name, - &count)) { - pr_err("%s: could not get %s for %s\n", - np->full_name, cells_name, - node->full_name); - goto err; - } - } else { - count = cell_count; - } +int of_phandle_iterator_next(struct of_phandle_iterator *it) +{ + phandle phandle = 0; + uint32_t count = 0; - /* - * Make sure that the arguments actually fit in the - * remaining property data length - */ - if (list + count > list_end) { - pr_err("%s: arguments longer than property\n", - np->full_name); - goto err; - } + if (it->phandle_end >= it->list_end) + return -ENOENT; + + if (it->node) { + of_node_put(it->node); + it->node = NULL; + } + + it->list = it->phandle_end; + it->cur_index += 1; + phandle = be32_to_cpup(it->list++); + + if (phandle) { + if (it->cells_name) { + it->node = of_find_node_by_phandle(phandle); + if (!it->node) + goto no_node; + + if (of_property_read_u32(it->node, + it->cells_name, + &count)) + goto no_property; + } else { + count = it->cell_count; } - /* - * All of the error cases above 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. - */ + if (it->list + count > it->list_end) + goto too_many_args; + } + + it->phandle_end = it->list + count; + it->cur_count = count; + it->phandle = phandle; + + return 0; + +no_node: + pr_err("%s: could not find phandle\n", it->np->full_name); + + return -EINVAL; + +no_property: + pr_err("%s: could not get %s for %s\n", it->np->full_name, + it->cells_name, it->node->full_name); + + return -EINVAL; + +too_many_args: + pr_err("%s: arguments longer than property\n", it->np->full_name); + + 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->list++); + + return count; +} + +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) +{ + struct of_phandle_iterator it; + int rc; + + rc = of_phandle_iterator_init(&it, np, list_name, + cells_name, cell_count); + if (rc) + return rc; + + while ((rc = of_phandle_iterator_next(&it)) == 0) { + uint32_t count; + int cur_index; + + count = of_phandle_iterator_count(&it); + cur_index = of_phandle_iterator_index(&it); + rc = -ENOENT; if (cur_index == index) { - if (!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++); - } else { - of_node_put(node); - } + if (!of_phandle_iterator_phandle(&it)) + break; + + rc = 0; /* Found it! return success */ - return 0; - } + if (!out_args) + break; - of_node_put(node); - node = NULL; - list += count; - cur_index++; + out_args->np = of_phandle_iterator_node(&it); + out_args->args_count = of_phandle_iterator_args(&it, + out_args->args, + MAX_PHANDLE_ARGS); + + break; + } } - /* - * 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) - */ - rc = index < 0 ? cur_index : -ENOENT; - err: - if (node) - of_node_put(node); + of_phandle_iterator_destroy(&it); + return rc; } diff --git a/include/linux/of.h b/include/linux/of.h index dc6e396..31e6ee9 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -75,6 +75,19 @@ struct of_phandle_args { uint32_t args[MAX_PHANDLE_ARGS]; }; +struct of_phandle_iterator { + const struct device_node *np; + const __be32 *list; + const __be32 *list_end; + const __be32 *phandle_end; + phandle phandle; + struct device_node *node; + const char *cells_name; + int cell_count; + int cur_index; + uint32_t cur_count; +}; + struct of_reconfig_data { struct device_node *dn; struct property *prop; @@ -334,6 +347,50 @@ 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); + +static inline void of_phandle_iterator_destroy(struct of_phandle_iterator *it) +{ + if (it->node) + of_node_put(it->node); +} + +extern int of_phandle_iterator_next(struct of_phandle_iterator *it); + +static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it) +{ + return it->cur_index; +} + +static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it) +{ + return it->cur_count; +} + +static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it) +{ + return it->phandle; +} + +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it) +{ + if (!it->node) + it->node = of_find_node_by_phandle(it->phandle); + + if (it->node) + of_node_get(it->node); + + return it->node; +} + +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); extern int of_alias_get_highest_id(const char *stem); @@ -608,6 +665,44 @@ 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 void of_phandle_iterator_destroy(struct of_phandle_iterator *it) +{ +} + +static inline int of_phandle_iterator_next(struct of_phandle_iterator *it) +{ + return -ENOSYS; +} + +static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it) +{ + return -1; +} + +static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it) +{ + return 0; +} + +static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it) +{ + return 0; +} + +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it) +{ + return NULL; +} + static inline int of_alias_get_id(struct device_node *np, const char *stem) { return -ENOSYS;