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

Submitted by Madhavan Srinivasan on April 3, 2017, 7:41 a.m.

Details

Message ID 1491205307-20408-3-git-send-email-maddy@linux.vnet.ibm.com
State New
Headers show

Commit Message

Madhavan Srinivasan April 3, 2017, 7:41 a.m.
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(+)

Comments

Stewart Smith April 6, 2017, 11:31 p.m.
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.
Benjamin Herrenschmidt April 6, 2017, 11:58 p.m.
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.

Patch hide | download patch | download mbox

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);
+			}
+		}
+	}
+	return 0;
+}
+
 u64 dt_get_number(const void *pdata, unsigned int cells)
 {
 	const u32 *p = pdata;
diff --git a/include/device.h b/include/device.h
index f659675764df..87e06b81786d 100644
--- a/include/device.h
+++ b/include/device.h
@@ -47,6 +47,22 @@  struct dt_node {
 	u32 phandle;
 };
 
+/*
+ * Data structure for device tree fixup functions
+ */
+struct dt_fixup_p {
+	u32 events;
+	struct list_head children;
+};
+
+struct dt_fixup_c {
+	u32 events;
+	const char *name;
+	struct dt_node *node;
+	struct list_node list;
+	struct list_head children;
+};
+
 /* This is shared with device_tree.c .. make it static when
  * the latter is gone (hopefully soon)
  */
@@ -263,4 +279,9 @@  u64 dt_translate_address(const struct dt_node *node, unsigned int index,
  */
 int dt_cmp_subnodes(const struct dt_node *a,  const struct dt_node *b);
 
+/* Helpers for dt phandle fixup */
+void dt_fixup_list_free(struct dt_fixup_p *parent);
+int dt_fixup_phandle(struct dt_node *dev, struct dt_fixup_p *parent);
+int dt_fixup_populate_llist(struct dt_node *lr_node, struct dt_fixup_p *parent, const char *name);
+
 #endif /* __DEVICE_H */