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

[Stewart Smith]

Madhavan Srinivasan <maddy at 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 at linux.vnet.ibm.com>
> Signed-off-by: Madhavan Srinivasan <maddy at 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.

-- 
Stewart Smith
OPAL Architect, IBM.