On Wed, Mar 16, 2016 at 11:42 AM, Joerg Roedel <joro@xxxxxxxxxx> wrote:
> From: Joerg Roedel <jroedel@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
