[v8,02/10] dt: Add phandle fixup helpers

Patch to add a phandle fixup function for a new
incoming subtree. When attaching a "subtree" to
main dt, subtree could have "property" referring
different nodes or multiple nodes via phandle as
"labels". For this reason, new subtree will have
"phandle" already assigned to some of it's nodes.
If this new subtree is attached to main dt without
phandle fixup, could lead to duplicate phandle errors.

Patch also carries other helper routines for phandle
fixup function.

Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
 core/device.c    | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/device.h |  21 +++++++++++
 2 files changed, 131 insertions(+)

Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes:
> Patch to add a phandle fixup function for a new
> incoming subtree. When attaching a "subtree" to
> main dt, subtree could have "property" referring
> different nodes or multiple nodes via phandle as
> "labels". For this reason, new subtree will have
> "phandle" already assigned to some of it's nodes.
> If this new subtree is attached to main dt without
> phandle fixup, could lead to duplicate phandle errors.
>
> Patch also carries other helper routines for phandle
> fixup function.
>
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
>  core/device.c    | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/device.h |  21 +++++++++++
>  2 files changed, 131 insertions(+)

Please add some unit tests for this new functionality in
core/test/run-dt-insert-subtree.c or similar, I'd like to see this code
regularly run under valgrind and with a variety of inputs.

>
> diff --git a/core/device.c b/core/device.c
> index 170e0ee7bbb5..33cbf8dbc13e 100644
> --- a/core/device.c
> +++ b/core/device.c
> @@ -849,6 +849,116 @@ void dt_expand(const void *fdt)
>  		abort();
>  }
>
> +/*
> + * Helper to free malloced dt_fixup _list_ variables
> + */
> +void dt_fixup_list_free(struct dt_fixup_p *parent)
> +{
> +	struct dt_fixup_c *cptr, *ccptr;
> +
> +	list_for_each(&parent->children, cptr, list) {
> +		list_for_each(&cptr->children, ccptr, list)
> +			free(ccptr);
> +		free(cptr);
> +	}
> +}
> +
> +/*
> + * Function to capture the "property" and the node
> + * that needs phandle fixup via _list_
> + *
> + * Function first parser the new subtree for a "property"
> + * and adds to a _list_. If the "property" supports more than
> + * one "phandle" value, then the _list_ becomes 2D. i.e,
> + * say the "property" here is "events",
> + *
> + * <parent>
> + *    |
> + * <events <1> > - <node 1> - <node 2> -- <node n>
> + *    |
> + * <events <2> > - <node 1> - <node 2> -- <node n>
> + *    |
> + * <events <n> > - <node 1> - <node 2> -- <node n>
> + *
> + * < node *>  have events "property" and needs an
> + * update after phandle change.
> + */
> +int dt_fixup_populate_llist(struct dt_node *lr_node, struct dt_fixup_p *parent, const char *name)
> +{
> +	u32 ev_value;
> +	struct dt_fixup_c *cptr, *ccptr;
> +	bool flag = true;
> +	struct dt_node *node;
> +
> +	/* Initialize */
> +	list_head_init(&parent->children);
> +
> +	/* Loop through each node in the subtree */
> +	dt_for_each_node(lr_node, node) {
> +		/* Look for the given "property */
> +		if (dt_find_property(node, name)) {
> +			/* extract the "phandle" value */
> +			ev_value = dt_prop_get_u32(node, name);
> +			list_for_each(&parent->children, cptr, list) {
> +				if (cptr->events == ev_value) {
> +					flag = false;
> +					/* Grow the list horizontally */
> +					ccptr = malloc(sizeof(struct dt_fixup_c));
> +					if (!ccptr)
> +						return -ENOMEM;
> +
> +					/* capture information */
> +					ccptr->node = node;
> +					ccptr->name = name;
> +					list_add(&cptr->children, &ccptr->list);
> +					break;
> +				}
> +			}
> +			if (flag){
> +				cptr = malloc(sizeof(struct dt_fixup_c));
> +				if (!cptr)
> +					return -ENOMEM;
> +
> +				cptr->events = ev_value;
> +				cptr->node = node;
> +
> +				/* Grow the list vertically */
> +				list_add(&parent->children, &cptr->list);
> +				list_head_init(&cptr->children);
> +				ccptr = malloc(sizeof(struct dt_fixup_c));
> +				if (!ccptr)
> +					return -ENOMEM;
> +
> +				ccptr->node = node;
> +				ccptr->name = name;
> +				list_add(&cptr->children, &ccptr->list);
> +			}
> +			flag = true;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int dt_fixup_phandle(struct dt_node *dev, struct dt_fixup_p *parent)
> +{
> +	struct dt_node *node;
> +	struct dt_property *prop;
> +	struct dt_fixup_c *cptr, *ccptr;
> +
> +	list_for_each(&parent->children, cptr, list) {
> +		node = dt_find_by_phandle(dev, cptr->events);
> +		if (node->phandle == cptr->events) {
> +			node->phandle = increment_return_last_phandle();
> +			list_for_each(&cptr->children, ccptr, list) {
> +				prop = __dt_find_property(ccptr->node, "events");
> +				memcpy(prop->prop, &node->phandle,
> prop->len);

This makes the code look specific to nodes with "events" properties?
Why? For something sitting in core/device.c, it should be generic.

On Fri, 2017-04-07 at 09:31 +1000, Stewart Smith wrote:
> This makes the code look specific to nodes with "events" properties?
> Why? For something sitting in core/device.c, it should be generic.

We don't know what properties may contain phandles. We might need
something like a generic table of properties that may, along with
corresponding "handler" functions that know how to locate the phandles
within the property to fix them up.

I would recommend not using a linked list for the translation table
though or we'll end up O^2 like hell. We should add some simple hash
table primitive to OPAL.

Cheers,
Ben.