[PATCH 0/3] switch DT creation to using of_attach_node

[PATCH 0/3] switch DT creation to using of_attach_node

[Andres Salomon]

These patches cause of_attach_node to be used by OF core, both for promtree
and flattree building.

[PATCH 1/3] of: remove OF_DYNAMIC config option

[Andres Salomon]

Make of_attach_node/of_detach_node available without a specific config option.
CONFIG_OF_DYNAMIC wasn't actually set by anything; the drivers that use
of_attach_node (in arch/powerpc/platforms/) didn't actually depend upon
or select CONFIG_OF_DYNAMIC.

While we're at it, make of_attach_node safe to call with the node's parent
being NULL; the only time this should happen is with the root node.  Later
patches will be using of_attach_node to link the root node.

Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
---
 drivers/of/Kconfig |    4 ----
 drivers/of/base.c  |   17 ++++-------------
 include/linux/of.h |    4 +---
 3 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index d06a637..ba90122 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -26,10 +26,6 @@ config OF_EARLY_FLATTREE
 config OF_PROMTREE
 	bool
 
-config OF_DYNAMIC
-	def_bool y
-	depends on PPC_OF
-
 config OF_ADDRESS
 	def_bool y
 	depends on !SPARC
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 710b53b..825e6d1 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -849,15 +849,6 @@ int prom_update_property(struct device_node *np,
 	return 0;
 }
 
-#if defined(CONFIG_OF_DYNAMIC)
-/*
- * Support for dynamic device trees.
- *
- * On some platforms, the device tree can be manipulated at runtime.
- * The routines in this section support adding, removing and changing
- * device tree nodes.
- */
-
 /**
  * of_attach_node - Plug a device node into the tree and global list.
  */
@@ -866,9 +857,11 @@ void of_attach_node(struct device_node *np)
 	unsigned long flags;
 
 	write_lock_irqsave(&devtree_lock, flags);
-	np->sibling = np->parent->child;
+	if (np->parent) {
+		np->sibling = np->parent->child;
+		np->parent->child = np;
+	}
 	np->allnext = allnodes;
-	np->parent->child = np;
 	allnodes = np;
 	write_unlock_irqrestore(&devtree_lock, flags);
 }
@@ -917,5 +910,3 @@ void of_detach_node(struct device_node *np)
 out_unlock:
 	write_unlock_irqrestore(&devtree_lock, flags);
 }
-#endif /* defined(CONFIG_OF_DYNAMIC) */
-
diff --git a/include/linux/of.h b/include/linux/of.h
index bfc0ed1..bb36473 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -221,11 +221,9 @@ extern int prom_update_property(struct device_node *np,
 				struct property *newprop,
 				struct property *oldprop);
 
-#if defined(CONFIG_OF_DYNAMIC)
-/* For updating the device tree at runtime */
+/* For updating the device tree */
 extern void of_attach_node(struct device_node *);
 extern void of_detach_node(struct device_node *);
-#endif
 
 #else
 
-- 
1.7.2.3

Re: [PATCH 1/3] of: remove OF_DYNAMIC config option

[Grant Likely]

On Wed, Mar 09, 2011 at 04:16:05PM -0800, Andres Salomon wrote:
> Make of_attach_node/of_detach_node available without a specific config option.
> CONFIG_OF_DYNAMIC wasn't actually set by anything; the drivers that use
> of_attach_node (in arch/powerpc/platforms/) didn't actually depend upon
> or select CONFIG_OF_DYNAMIC.
> 
> While we're at it, make of_attach_node safe to call with the node's parent
> being NULL; the only time this should happen is with the root node.  Later
> patches will be using of_attach_node to link the root node.
> 
> Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>

Looks good to me.

g.

> ---
>  drivers/of/Kconfig |    4 ----
>  drivers/of/base.c  |   17 ++++-------------
>  include/linux/of.h |    4 +---
>  3 files changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index d06a637..ba90122 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -26,10 +26,6 @@ config OF_EARLY_FLATTREE
>  config OF_PROMTREE
>  	bool
>  
> -config OF_DYNAMIC
> -	def_bool y
> -	depends on PPC_OF
> -
>  config OF_ADDRESS
>  	def_bool y
>  	depends on !SPARC
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 710b53b..825e6d1 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -849,15 +849,6 @@ int prom_update_property(struct device_node *np,
>  	return 0;
>  }
>  
> -#if defined(CONFIG_OF_DYNAMIC)
> -/*
> - * Support for dynamic device trees.
> - *
> - * On some platforms, the device tree can be manipulated at runtime.
> - * The routines in this section support adding, removing and changing
> - * device tree nodes.
> - */
> -
>  /**
>   * of_attach_node - Plug a device node into the tree and global list.
>   */
> @@ -866,9 +857,11 @@ void of_attach_node(struct device_node *np)
>  	unsigned long flags;
>  
>  	write_lock_irqsave(&devtree_lock, flags);
> -	np->sibling = np->parent->child;
> +	if (np->parent) {
> +		np->sibling = np->parent->child;
> +		np->parent->child = np;
> +	}
>  	np->allnext = allnodes;
> -	np->parent->child = np;
>  	allnodes = np;
>  	write_unlock_irqrestore(&devtree_lock, flags);
>  }
> @@ -917,5 +910,3 @@ void of_detach_node(struct device_node *np)
>  out_unlock:
>  	write_unlock_irqrestore(&devtree_lock, flags);
>  }
> -#endif /* defined(CONFIG_OF_DYNAMIC) */
> -
> diff --git a/include/linux/of.h b/include/linux/of.h
> index bfc0ed1..bb36473 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -221,11 +221,9 @@ extern int prom_update_property(struct device_node *np,
>  				struct property *newprop,
>  				struct property *oldprop);
>  
> -#if defined(CONFIG_OF_DYNAMIC)
> -/* For updating the device tree at runtime */
> +/* For updating the device tree */
>  extern void of_attach_node(struct device_node *);
>  extern void of_detach_node(struct device_node *);
> -#endif
>  
>  #else
>  
> -- 
> 1.7.2.3
> 

[PATCH 2/3] of/promtree: switch to building the DT using of_attach_node

[Andres Salomon]

Use common functions (of_attach_node) to build the device tree.  This
allows us to drop a bit of code, and provides a more understandable
tree building.  It does change the order of the allnodes/allnext list;
the old tree looked something like this:

allnodes --> root (/)
             allnext ------> np1 (/pci)
                             allnext --> np2 (/pci/foo)
                                         allnext=NULL

Pretend that was a tree.  The new tree looks something like this:

allnodes     root (/)
      |      allnext=NULL    np1 (/pci)
      |        ^------------ allnext     np2 (/pci/foo)
      |                        ^-------- allnext
      |_____________________________________^

That shouldn't actually matter for anything, but it's worth noting in case
anyone notices odd behavior changes.  Plus, it's a good excuse to make
beautiful art.

Also note that this changes what gets passed to the of_pdt_build_more()
hook.  The only user of this hook is sparc's leon_kernel.c, which ends
up using it to call prom_amba_init.  However, prom_amba_init isn't
actually set in the kernel, so I can't tell whether this actually breaks
behavior or not.

Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
---
 drivers/of/pdt.c |   39 ++++++++++++---------------------------
 1 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 4d87b5d..5933127 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -193,56 +193,41 @@ static struct device_node * __init of_pdt_create_node(phandle node,
 	return dp;
 }
 
-static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
-						   phandle node,
-						   struct device_node ***nextp)
+static void __init of_pdt_build_tree(struct device_node *parent, phandle node)
 {
-	struct device_node *ret = NULL, *prev_sibling = NULL;
 	struct device_node *dp;
 
-	while (1) {
+	while (node) {
 		dp = of_pdt_create_node(node, parent);
 		if (!dp)
 			break;
 
-		if (prev_sibling)
-			prev_sibling->sibling = dp;
-
-		if (!ret)
-			ret = dp;
-		prev_sibling = dp;
-
-		*(*nextp) = dp;
-		*nextp = &dp->allnext;
-
 		dp->full_name = of_pdt_build_full_name(dp);
+		of_attach_node(dp);
 
-		dp->child = of_pdt_build_tree(dp,
-				of_pdt_prom_ops->getchild(node), nextp);
+		/* process child nodes prior to siblings */
+		of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(node));
 
 		if (of_pdt_build_more)
-			of_pdt_build_more(dp, nextp);
+			of_pdt_build_more(dp, NULL);
 
 		node = of_pdt_prom_ops->getsibling(node);
 	}
-
-	return ret;
 }
 
 void __init of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops)
 {
-	struct device_node **nextp;
+	struct device_node *dp;
 
 	BUG_ON(!ops);
 	of_pdt_prom_ops = ops;
 
-	allnodes = of_pdt_create_node(root_node, NULL);
+	dp = of_pdt_create_node(root_node, NULL);
 #if defined(CONFIG_SPARC)
-	allnodes->path_component_name = "";
+	dp->path_component_name = "";
 #endif
-	allnodes->full_name = "/";
+	dp->full_name = "/";
 
-	nextp = &allnodes->allnext;
-	allnodes->child = of_pdt_build_tree(allnodes,
-			of_pdt_prom_ops->getchild(allnodes->phandle), &nextp);
+	of_attach_node(dp);
+	of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(dp->phandle));
 }
-- 
1.7.2.3

Re: [PATCH 2/3] of/promtree: switch to building the DT using of_attach_node

[Grant Likely]

On Wed, Mar 09, 2011 at 04:16:06PM -0800, Andres Salomon wrote:
> Use common functions (of_attach_node) to build the device tree.  This
> allows us to drop a bit of code, and provides a more understandable
> tree building.  It does change the order of the allnodes/allnext list;
> the old tree looked something like this:
> 
> allnodes --> root (/)
>              allnext ------> np1 (/pci)
>                              allnext --> np2 (/pci/foo)
>                                          allnext=NULL
> 
> Pretend that was a tree.  The new tree looks something like this:
> 
> allnodes     root (/)
>       |      allnext=NULL    np1 (/pci)
>       |        ^------------ allnext     np2 (/pci/foo)
>       |                        ^-------- allnext
>       |_____________________________________^
> 
> That shouldn't actually matter for anything, but it's worth noting in case
> anyone notices odd behavior changes.  Plus, it's a good excuse to make
> beautiful art.

Hmmm, I hadn't thought of that detail.  This will very likely cause
breakage in subtle ways.  Though we *shouldn't* depend on the order of
the device nodes in the tree, I know for certain that there are some
platform which will end up renumbering devices if the order is in
reverse (uarts in particular) in a way that breaks booting.  Ugh.

Now, this might not be a problem at all for SPARC, and this patch only
affects OLPC and SPARC at the moment.  If David ack's it, then I'll
pick it up.  However, it will require some more thought before
unflatten_device_tree() can be converted to use it.

g.

> 
> Also note that this changes what gets passed to the of_pdt_build_more()
> hook.  The only user of this hook is sparc's leon_kernel.c, which ends
> up using it to call prom_amba_init.  However, prom_amba_init isn't
> actually set in the kernel, so I can't tell whether this actually breaks
> behavior or not.
> 
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
>  drivers/of/pdt.c |   39 ++++++++++++---------------------------
>  1 files changed, 12 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 4d87b5d..5933127 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -193,56 +193,41 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>  	return dp;
>  }
>  
> -static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> -						   phandle node,
> -						   struct device_node ***nextp)
> +static void __init of_pdt_build_tree(struct device_node *parent, phandle node)
>  {
> -	struct device_node *ret = NULL, *prev_sibling = NULL;
>  	struct device_node *dp;
>  
> -	while (1) {
> +	while (node) {
>  		dp = of_pdt_create_node(node, parent);
>  		if (!dp)
>  			break;
>  
> -		if (prev_sibling)
> -			prev_sibling->sibling = dp;
> -
> -		if (!ret)
> -			ret = dp;
> -		prev_sibling = dp;
> -
> -		*(*nextp) = dp;
> -		*nextp = &dp->allnext;
> -
>  		dp->full_name = of_pdt_build_full_name(dp);
> +		of_attach_node(dp);
>  
> -		dp->child = of_pdt_build_tree(dp,
> -				of_pdt_prom_ops->getchild(node), nextp);
> +		/* process child nodes prior to siblings */
> +		of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(node));
>  
>  		if (of_pdt_build_more)
> -			of_pdt_build_more(dp, nextp);
> +			of_pdt_build_more(dp, NULL);
>  
>  		node = of_pdt_prom_ops->getsibling(node);
>  	}
> -
> -	return ret;
>  }
>  
>  void __init of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops)
>  {
> -	struct device_node **nextp;
> +	struct device_node *dp;
>  
>  	BUG_ON(!ops);
>  	of_pdt_prom_ops = ops;
>  
> -	allnodes = of_pdt_create_node(root_node, NULL);
> +	dp = of_pdt_create_node(root_node, NULL);
>  #if defined(CONFIG_SPARC)
> -	allnodes->path_component_name = "";
> +	dp->path_component_name = "";
>  #endif
> -	allnodes->full_name = "/";
> +	dp->full_name = "/";
>  
> -	nextp = &allnodes->allnext;
> -	allnodes->child = of_pdt_build_tree(allnodes,
> -			of_pdt_prom_ops->getchild(allnodes->phandle), &nextp);
> +	of_attach_node(dp);
> +	of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(dp->phandle));
>  }
> -- 
> 1.7.2.3
> 

[PATCH 3/3] of/flattree: use of_attach_node to build tree, and associated cleanups

[Andres Salomon]

Use a common function (of_attach_node) to build the device tree.  This
simplifies the flat device tree creation a bit, and as an added bonus allows
us to drop a (now unused) field from the device_node struct.

There are a few minor cleanups snuck in here as well (comment additions, etc).

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/of/fdt.c   |   42 ++++++++++++++++--------------------------
 include/linux/of.h |    1 -
 2 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index af824e7..e70cee8 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -139,16 +139,17 @@ static void *unflatten_dt_alloc(unsigned long *mem, unsigned long size,
 /**
  * unflatten_dt_node - Alloc and populate a device_node from the flat tree
  * @blob: The parent device tree blob
+ * @mem: memory chunk to use for allocating device nodes and properties
  * @p: pointer to node in flat tree
  * @dad: Parent struct device_node
- * @allnextpp: pointer to ->allnext from last allocated device_node
+ * @size_scan: are we figuring out amount of memory to allocate?
  * @fpsize: Size of the node path up at the current depth.
  */
 unsigned long unflatten_dt_node(struct boot_param_header *blob,
 				unsigned long mem,
 				unsigned long *p,
 				struct device_node *dad,
-				struct device_node ***allnextpp,
+				bool size_scan,
 				unsigned long fpsize)
 {
 	struct device_node *np;
@@ -195,7 +196,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
 
 	np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl,
 				__alignof__(struct device_node));
-	if (allnextpp) {
+	if (!size_scan) {
 		memset(np, 0, sizeof(*np));
 		np->full_name = ((char *)np) + sizeof(struct device_node);
 		if (new_format) {
@@ -217,19 +218,10 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
 		} else
 			memcpy(np->full_name, pathp, l);
 		prev_pp = &np->properties;
-		**allnextpp = np;
-		*allnextpp = &np->allnext;
-		if (dad != NULL) {
-			np->parent = dad;
-			/* we temporarily use the next field as `last_child'*/
-			if (dad->next == NULL)
-				dad->child = np;
-			else
-				dad->next->sibling = np;
-			dad->next = np;
-		}
+		np->parent = dad;
 		kref_init(&np->kref);
 	}
+	/* process properties */
 	while (1) {
 		u32 sz, noff;
 		char *pname;
@@ -258,7 +250,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
 		l = strlen(pname) + 1;
 		pp = unflatten_dt_alloc(&mem, sizeof(struct property),
 					__alignof__(struct property));
-		if (allnextpp) {
+		if (!size_scan) {
 			/* We accept flattened tree phandles either in
 			 * ePAPR-style "phandle" properties, or the
 			 * legacy "linux,phandle" properties.  If both
@@ -301,7 +293,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
 		sz = (pa - ps) + 1;
 		pp = unflatten_dt_alloc(&mem, sizeof(struct property) + sz,
 					__alignof__(struct property));
-		if (allnextpp) {
+		if (!size_scan) {
 			pp->name = "name";
 			pp->length = sz;
 			pp->value = pp + 1;
@@ -313,7 +305,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
 				(char *)pp->value);
 		}
 	}
-	if (allnextpp) {
+	if (!size_scan) {
 		*prev_pp = NULL;
 		np->name = of_get_property(np, "name", NULL);
 		np->type = of_get_property(np, "device_type", NULL);
@@ -322,12 +314,14 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
 			np->name = "<NULL>";
 		if (!np->type)
 			np->type = "<NULL>";
+
+		of_attach_node(np);
 	}
 	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
 		if (tag == OF_DT_NOP)
 			*p += 4;
 		else
-			mem = unflatten_dt_node(blob, mem, p, np, allnextpp,
+			mem = unflatten_dt_node(blob, mem, p, np, size_scan,
 						fpsize);
 		tag = be32_to_cpup((__be32 *)(*p));
 	}
@@ -347,16 +341,13 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
  * pointers of the nodes so the normal device-tree walking functions
  * can be used.
  * @blob: The blob to expand
- * @mynodes: The device_node tree created by the call
  * @dt_alloc: An allocator that provides a virtual address to memory
  * for the resulting tree
  */
 void __unflatten_device_tree(struct boot_param_header *blob,
-			     struct device_node **mynodes,
 			     void * (*dt_alloc)(u64 size, u64 align))
 {
 	unsigned long start, mem, size;
-	struct device_node **allnextp = mynodes;
 
 	pr_debug(" -> unflatten_device_tree()\n");
 
@@ -378,7 +369,7 @@ void __unflatten_device_tree(struct boot_param_header *blob,
 	/* First pass, scan for size */
 	start = ((unsigned long)blob) +
 		be32_to_cpu(blob->off_dt_struct);
-	size = unflatten_dt_node(blob, 0, &start, NULL, NULL, 0);
+	size = unflatten_dt_node(blob, 0, &start, NULL, true, 0);
 	size = (size | 3) + 1;
 
 	pr_debug("  size is %lx, allocating...\n", size);
@@ -394,13 +385,12 @@ void __unflatten_device_tree(struct boot_param_header *blob,
 	/* Second pass, do actual unflattening */
 	start = ((unsigned long)blob) +
 		be32_to_cpu(blob->off_dt_struct);
-	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
+	unflatten_dt_node(blob, mem, &start, NULL, false, 0);
 	if (be32_to_cpup((__be32 *)start) != OF_DT_END)
 		pr_warning("Weird tag at end of tree: %08x\n", *((u32 *)start));
 	if (be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeef)
 		pr_warning("End of tree marker overwritten: %08x\n",
 			   be32_to_cpu(((__be32 *)mem)[size / 4]));
-	*allnextp = NULL;
 
 	pr_debug(" <- unflatten_device_tree()\n");
 }
@@ -423,7 +413,7 @@ void of_fdt_unflatten_tree(unsigned long *blob,
 {
 	struct boot_param_header *device_tree =
 		(struct boot_param_header *)blob;
-	__unflatten_device_tree(device_tree, mynodes, &kernel_tree_alloc);
+	__unflatten_device_tree(device_tree, &kernel_tree_alloc);
 }
 EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
 
@@ -702,7 +692,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
  */
 void __init unflatten_device_tree(void)
 {
-	__unflatten_device_tree(initial_boot_params, &allnodes,
+	__unflatten_device_tree(initial_boot_params,
 				early_init_dt_alloc_memory_arch);
 
 	/* Get pointer to OF "/chosen" node for use everywhere */
diff --git a/include/linux/of.h b/include/linux/of.h
index bb36473..95754ca 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -50,7 +50,6 @@ struct device_node {
 	struct	device_node *parent;
 	struct	device_node *child;
 	struct	device_node *sibling;
-	struct	device_node *next;	/* next device of same type */
 	struct	device_node *allnext;	/* next in list of all nodes */
 	struct	proc_dir_entry *pde;	/* this node's proc directory */
 	struct	kref kref;
-- 
1.7.2.3

Re: [PATCH 3/3] of/flattree: use of_attach_node to build tree, and associated cleanups

[Grant Likely]

On Wed, Mar 09, 2011 at 04:16:07PM -0800, Andres Salomon wrote:
> Use a common function (of_attach_node) to build the device tree.  This
> simplifies the flat device tree creation a bit, and as an added bonus allows
> us to drop a (now unused) field from the device_node struct.
> 
> There are a few minor cleanups snuck in here as well (comment additions, etc).
> 
> Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>

In addition to my comment about changing the tree order on the last
patch, unfortunately this patch will also break the newly added
of_fdt_unflatten_tree().  of_fdt_unflatten_tree() allows a driver to
unflatten a private dtb for its own use without it being attached to
the global tree or the global list of all nodes.  I had also forgotten
about this.  Shoot.

The solution would be a variant of of_attach_node which accepts a
private allnodes pointer.  That would also help with the ordering
issues because the order of the list could be explicitly reversed
before assigning the value to the real allnodes pointer.  Another
possible option would be an optional 'tail' pointer argument to
of_attach_node() which if present it would use as the insertion point
for adding the node, which would preserve the current sort order.

g.

> ---
>  drivers/of/fdt.c   |   42 ++++++++++++++++--------------------------
>  include/linux/of.h |    1 -
>  2 files changed, 16 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index af824e7..e70cee8 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -139,16 +139,17 @@ static void *unflatten_dt_alloc(unsigned long *mem, unsigned long size,
>  /**
>   * unflatten_dt_node - Alloc and populate a device_node from the flat tree
>   * @blob: The parent device tree blob
> + * @mem: memory chunk to use for allocating device nodes and properties
>   * @p: pointer to node in flat tree
>   * @dad: Parent struct device_node
> - * @allnextpp: pointer to ->allnext from last allocated device_node
> + * @size_scan: are we figuring out amount of memory to allocate?
>   * @fpsize: Size of the node path up at the current depth.
>   */
>  unsigned long unflatten_dt_node(struct boot_param_header *blob,
>  				unsigned long mem,
>  				unsigned long *p,
>  				struct device_node *dad,
> -				struct device_node ***allnextpp,
> +				bool size_scan,
>  				unsigned long fpsize)
>  {
>  	struct device_node *np;
> @@ -195,7 +196,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
>  
>  	np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl,
>  				__alignof__(struct device_node));
> -	if (allnextpp) {
> +	if (!size_scan) {
>  		memset(np, 0, sizeof(*np));
>  		np->full_name = ((char *)np) + sizeof(struct device_node);
>  		if (new_format) {
> @@ -217,19 +218,10 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
>  		} else
>  			memcpy(np->full_name, pathp, l);
>  		prev_pp = &np->properties;
> -		**allnextpp = np;
> -		*allnextpp = &np->allnext;
> -		if (dad != NULL) {
> -			np->parent = dad;
> -			/* we temporarily use the next field as `last_child'*/
> -			if (dad->next == NULL)
> -				dad->child = np;
> -			else
> -				dad->next->sibling = np;
> -			dad->next = np;
> -		}
> +		np->parent = dad;
>  		kref_init(&np->kref);
>  	}
> +	/* process properties */
>  	while (1) {
>  		u32 sz, noff;
>  		char *pname;
> @@ -258,7 +250,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
>  		l = strlen(pname) + 1;
>  		pp = unflatten_dt_alloc(&mem, sizeof(struct property),
>  					__alignof__(struct property));
> -		if (allnextpp) {
> +		if (!size_scan) {
>  			/* We accept flattened tree phandles either in
>  			 * ePAPR-style "phandle" properties, or the
>  			 * legacy "linux,phandle" properties.  If both
> @@ -301,7 +293,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
>  		sz = (pa - ps) + 1;
>  		pp = unflatten_dt_alloc(&mem, sizeof(struct property) + sz,
>  					__alignof__(struct property));
> -		if (allnextpp) {
> +		if (!size_scan) {
>  			pp->name = "name";
>  			pp->length = sz;
>  			pp->value = pp + 1;
> @@ -313,7 +305,7 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
>  				(char *)pp->value);
>  		}
>  	}
> -	if (allnextpp) {
> +	if (!size_scan) {
>  		*prev_pp = NULL;
>  		np->name = of_get_property(np, "name", NULL);
>  		np->type = of_get_property(np, "device_type", NULL);
> @@ -322,12 +314,14 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
>  			np->name = "<NULL>";
>  		if (!np->type)
>  			np->type = "<NULL>";
> +
> +		of_attach_node(np);
>  	}
>  	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
>  		if (tag == OF_DT_NOP)
>  			*p += 4;
>  		else
> -			mem = unflatten_dt_node(blob, mem, p, np, allnextpp,
> +			mem = unflatten_dt_node(blob, mem, p, np, size_scan,
>  						fpsize);
>  		tag = be32_to_cpup((__be32 *)(*p));
>  	}
> @@ -347,16 +341,13 @@ unsigned long unflatten_dt_node(struct boot_param_header *blob,
>   * pointers of the nodes so the normal device-tree walking functions
>   * can be used.
>   * @blob: The blob to expand
> - * @mynodes: The device_node tree created by the call
>   * @dt_alloc: An allocator that provides a virtual address to memory
>   * for the resulting tree
>   */
>  void __unflatten_device_tree(struct boot_param_header *blob,
> -			     struct device_node **mynodes,
>  			     void * (*dt_alloc)(u64 size, u64 align))
>  {
>  	unsigned long start, mem, size;
> -	struct device_node **allnextp = mynodes;
>  
>  	pr_debug(" -> unflatten_device_tree()\n");
>  
> @@ -378,7 +369,7 @@ void __unflatten_device_tree(struct boot_param_header *blob,
>  	/* First pass, scan for size */
>  	start = ((unsigned long)blob) +
>  		be32_to_cpu(blob->off_dt_struct);
> -	size = unflatten_dt_node(blob, 0, &start, NULL, NULL, 0);
> +	size = unflatten_dt_node(blob, 0, &start, NULL, true, 0);
>  	size = (size | 3) + 1;
>  
>  	pr_debug("  size is %lx, allocating...\n", size);
> @@ -394,13 +385,12 @@ void __unflatten_device_tree(struct boot_param_header *blob,
>  	/* Second pass, do actual unflattening */
>  	start = ((unsigned long)blob) +
>  		be32_to_cpu(blob->off_dt_struct);
> -	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
> +	unflatten_dt_node(blob, mem, &start, NULL, false, 0);
>  	if (be32_to_cpup((__be32 *)start) != OF_DT_END)
>  		pr_warning("Weird tag at end of tree: %08x\n", *((u32 *)start));
>  	if (be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeef)
>  		pr_warning("End of tree marker overwritten: %08x\n",
>  			   be32_to_cpu(((__be32 *)mem)[size / 4]));
> -	*allnextp = NULL;
>  
>  	pr_debug(" <- unflatten_device_tree()\n");
>  }
> @@ -423,7 +413,7 @@ void of_fdt_unflatten_tree(unsigned long *blob,
>  {
>  	struct boot_param_header *device_tree =
>  		(struct boot_param_header *)blob;
> -	__unflatten_device_tree(device_tree, mynodes, &kernel_tree_alloc);
> +	__unflatten_device_tree(device_tree, &kernel_tree_alloc);
>  }
>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>  
> @@ -702,7 +692,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>   */
>  void __init unflatten_device_tree(void)
>  {
> -	__unflatten_device_tree(initial_boot_params, &allnodes,
> +	__unflatten_device_tree(initial_boot_params,
>  				early_init_dt_alloc_memory_arch);
>  
>  	/* Get pointer to OF "/chosen" node for use everywhere */
> diff --git a/include/linux/of.h b/include/linux/of.h
> index bb36473..95754ca 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -50,7 +50,6 @@ struct device_node {
>  	struct	device_node *parent;
>  	struct	device_node *child;
>  	struct	device_node *sibling;
> -	struct	device_node *next;	/* next device of same type */
>  	struct	device_node *allnext;	/* next in list of all nodes */
>  	struct	proc_dir_entry *pde;	/* this node's proc directory */
>  	struct	kref kref;
> -- 
> 1.7.2.3
> 

Re: [PATCH 3/3] of/flattree: use of_attach_node to build tree, and associated cleanups

[Andres Salomon]

On Sat, 12 Mar 2011 02:10:56 -0700
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Wed, Mar 09, 2011 at 04:16:07PM -0800, Andres Salomon wrote:
> > Use a common function (of_attach_node) to build the device tree.
> > This simplifies the flat device tree creation a bit, and as an
> > added bonus allows us to drop a (now unused) field from the
> > device_node struct.
> > 
> > There are a few minor cleanups snuck in here as well (comment
> > additions, etc).
> > 
> > Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
> 
> In addition to my comment about changing the tree order on the last
> patch, unfortunately this patch will also break the newly added
> of_fdt_unflatten_tree().  of_fdt_unflatten_tree() allows a driver to
> unflatten a private dtb for its own use without it being attached to
> the global tree or the global list of all nodes.  I had also forgotten
> about this.  Shoot.

Ah, I was wondering what that was all about.  So we'd probably end up
with something like:

void of_attach_node(struct device_node *dp)
{
	unsigned long flags;

	write_lock_irqsave(devtree_lock, &flags);
	__of_attach_node(allnodes, dp);
	write_unlock_irqrestore(devtree_lock, &flags);
}

Most stuff could get away with just calling of_attach_node, with the
unflatten_dt_node calling __of_attach_node (and either not caring
about devtree_lock, as is currently the case, or grabbing it from
unflatten_device_tree).


> 
> The solution would be a variant of of_attach_node which accepts a
> private allnodes pointer.  That would also help with the ordering
> issues because the order of the list could be explicitly reversed
> before assigning the value to the real allnodes pointer.  Another
> possible option would be an optional 'tail' pointer argument to
> of_attach_node() which if present it would use as the insertion point
> for adding the node, which would preserve the current sort order.

I was leaning towards a tail pointer, but I need to take a closer look
at the two options.



> 
> g.
> 
> > ---
> >  drivers/of/fdt.c   |   42
> > ++++++++++++++++-------------------------- include/linux/of.h |
> > 1 - 2 files changed, 16 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index af824e7..e70cee8 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -139,16 +139,17 @@ static void *unflatten_dt_alloc(unsigned long
> > *mem, unsigned long size, /**
> >   * unflatten_dt_node - Alloc and populate a device_node from the
> > flat tree
> >   * @blob: The parent device tree blob
> > + * @mem: memory chunk to use for allocating device nodes and
> > properties
> >   * @p: pointer to node in flat tree
> >   * @dad: Parent struct device_node
> > - * @allnextpp: pointer to ->allnext from last allocated device_node
> > + * @size_scan: are we figuring out amount of memory to allocate?
> >   * @fpsize: Size of the node path up at the current depth.
> >   */
> >  unsigned long unflatten_dt_node(struct boot_param_header *blob,
> >  				unsigned long mem,
> >  				unsigned long *p,
> >  				struct device_node *dad,
> > -				struct device_node ***allnextpp,
> > +				bool size_scan,
> >  				unsigned long fpsize)
> >  {
> >  	struct device_node *np;
> > @@ -195,7 +196,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, 
> >  	np = unflatten_dt_alloc(&mem, sizeof(struct device_node) +
> > allocl, __alignof__(struct device_node));
> > -	if (allnextpp) {
> > +	if (!size_scan) {
> >  		memset(np, 0, sizeof(*np));
> >  		np->full_name = ((char *)np) + sizeof(struct
> > device_node); if (new_format) {
> > @@ -217,19 +218,10 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, } else
> >  			memcpy(np->full_name, pathp, l);
> >  		prev_pp = &np->properties;
> > -		**allnextpp = np;
> > -		*allnextpp = &np->allnext;
> > -		if (dad != NULL) {
> > -			np->parent = dad;
> > -			/* we temporarily use the next field as
> > `last_child'*/
> > -			if (dad->next == NULL)
> > -				dad->child = np;
> > -			else
> > -				dad->next->sibling = np;
> > -			dad->next = np;
> > -		}
> > +		np->parent = dad;
> >  		kref_init(&np->kref);
> >  	}
> > +	/* process properties */
> >  	while (1) {
> >  		u32 sz, noff;
> >  		char *pname;
> > @@ -258,7 +250,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, l = strlen(pname) + 1;
> >  		pp = unflatten_dt_alloc(&mem, sizeof(struct
> > property), __alignof__(struct property));
> > -		if (allnextpp) {
> > +		if (!size_scan) {
> >  			/* We accept flattened tree phandles
> > either in
> >  			 * ePAPR-style "phandle" properties, or the
> >  			 * legacy "linux,phandle" properties.  If
> > both @@ -301,7 +293,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, sz = (pa - ps) + 1;
> >  		pp = unflatten_dt_alloc(&mem, sizeof(struct
> > property) + sz, __alignof__(struct property));
> > -		if (allnextpp) {
> > +		if (!size_scan) {
> >  			pp->name = "name";
> >  			pp->length = sz;
> >  			pp->value = pp + 1;
> > @@ -313,7 +305,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, (char *)pp->value);
> >  		}
> >  	}
> > -	if (allnextpp) {
> > +	if (!size_scan) {
> >  		*prev_pp = NULL;
> >  		np->name = of_get_property(np, "name", NULL);
> >  		np->type = of_get_property(np, "device_type",
> > NULL); @@ -322,12 +314,14 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, np->name = "<NULL>";
> >  		if (!np->type)
> >  			np->type = "<NULL>";
> > +
> > +		of_attach_node(np);
> >  	}
> >  	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
> >  		if (tag == OF_DT_NOP)
> >  			*p += 4;
> >  		else
> > -			mem = unflatten_dt_node(blob, mem, p, np,
> > allnextpp,
> > +			mem = unflatten_dt_node(blob, mem, p, np,
> > size_scan, fpsize);
> >  		tag = be32_to_cpup((__be32 *)(*p));
> >  	}
> > @@ -347,16 +341,13 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob,
> >   * pointers of the nodes so the normal device-tree walking
> > functions
> >   * can be used.
> >   * @blob: The blob to expand
> > - * @mynodes: The device_node tree created by the call
> >   * @dt_alloc: An allocator that provides a virtual address to
> > memory
> >   * for the resulting tree
> >   */
> >  void __unflatten_device_tree(struct boot_param_header *blob,
> > -			     struct device_node **mynodes,
> >  			     void * (*dt_alloc)(u64 size, u64
> > align)) {
> >  	unsigned long start, mem, size;
> > -	struct device_node **allnextp = mynodes;
> >  
> >  	pr_debug(" -> unflatten_device_tree()\n");
> >  
> > @@ -378,7 +369,7 @@ void __unflatten_device_tree(struct
> > boot_param_header *blob, /* First pass, scan for size */
> >  	start = ((unsigned long)blob) +
> >  		be32_to_cpu(blob->off_dt_struct);
> > -	size = unflatten_dt_node(blob, 0, &start, NULL, NULL, 0);
> > +	size = unflatten_dt_node(blob, 0, &start, NULL, true, 0);
> >  	size = (size | 3) + 1;
> >  
> >  	pr_debug("  size is %lx, allocating...\n", size);
> > @@ -394,13 +385,12 @@ void __unflatten_device_tree(struct
> > boot_param_header *blob, /* Second pass, do actual unflattening */
> >  	start = ((unsigned long)blob) +
> >  		be32_to_cpu(blob->off_dt_struct);
> > -	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
> > +	unflatten_dt_node(blob, mem, &start, NULL, false, 0);
> >  	if (be32_to_cpup((__be32 *)start) != OF_DT_END)
> >  		pr_warning("Weird tag at end of tree: %08x\n",
> > *((u32 *)start)); if (be32_to_cpu(((__be32 *)mem)[size / 4]) !=
> > 0xdeadbeef) pr_warning("End of tree marker overwritten: %08x\n",
> >  			   be32_to_cpu(((__be32 *)mem)[size / 4]));
> > -	*allnextp = NULL;
> >  
> >  	pr_debug(" <- unflatten_device_tree()\n");
> >  }
> > @@ -423,7 +413,7 @@ void of_fdt_unflatten_tree(unsigned long *blob,
> >  {
> >  	struct boot_param_header *device_tree =
> >  		(struct boot_param_header *)blob;
> > -	__unflatten_device_tree(device_tree, mynodes,
> > &kernel_tree_alloc);
> > +	__unflatten_device_tree(device_tree, &kernel_tree_alloc);
> >  }
> >  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
> >  
> > @@ -702,7 +692,7 @@ int __init early_init_dt_scan_chosen(unsigned
> > long node, const char *uname, */
> >  void __init unflatten_device_tree(void)
> >  {
> > -	__unflatten_device_tree(initial_boot_params, &allnodes,
> > +	__unflatten_device_tree(initial_boot_params,
> >  				early_init_dt_alloc_memory_arch);
> >  
> >  	/* Get pointer to OF "/chosen" node for use everywhere */
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index bb36473..95754ca 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -50,7 +50,6 @@ struct device_node {
> >  	struct	device_node *parent;
> >  	struct	device_node *child;
> >  	struct	device_node *sibling;
> > -	struct	device_node *next;	/* next device of
> > same type */ struct	device_node *allnext;	/* next in
> > list of all nodes */ struct	proc_dir_entry *pde;	/*
> > this node's proc directory */ struct	kref kref;
> > -- 
> > 1.7.2.3
> > 

Re: [PATCH 3/3] of/flattree: use of_attach_node to build tree, and associated cleanups

[Grant Likely]

On Sat, Mar 12, 2011 at 11:37:26AM -0800, Andres Salomon wrote:
> On Sat, 12 Mar 2011 02:10:56 -0700
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> > On Wed, Mar 09, 2011 at 04:16:07PM -0800, Andres Salomon wrote:
> > > Use a common function (of_attach_node) to build the device tree.
> > > This simplifies the flat device tree creation a bit, and as an
> > > added bonus allows us to drop a (now unused) field from the
> > > device_node struct.
> > > 
> > > There are a few minor cleanups snuck in here as well (comment
> > > additions, etc).
> > > 
> > > Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
> > 
> > In addition to my comment about changing the tree order on the last
> > patch, unfortunately this patch will also break the newly added
> > of_fdt_unflatten_tree().  of_fdt_unflatten_tree() allows a driver to
> > unflatten a private dtb for its own use without it being attached to
> > the global tree or the global list of all nodes.  I had also forgotten
> > about this.  Shoot.
> 
> Ah, I was wondering what that was all about.  So we'd probably end up
> with something like:
> 
> void of_attach_node(struct device_node *dp)
> {
> 	unsigned long flags;
> 
> 	write_lock_irqsave(devtree_lock, &flags);
> 	__of_attach_node(allnodes, dp);
> 	write_unlock_irqrestore(devtree_lock, &flags);
> }
> 
> Most stuff could get away with just calling of_attach_node, with the
> unflatten_dt_node calling __of_attach_node (and either not caring
> about devtree_lock, as is currently the case, or grabbing it from
> unflatten_device_tree).

Yes.  The caller would be responsible for locking its own private dt
structure.

> > 
> > The solution would be a variant of of_attach_node which accepts a
> > private allnodes pointer.  That would also help with the ordering
> > issues because the order of the list could be explicitly reversed
> > before assigning the value to the real allnodes pointer.  Another
> > possible option would be an optional 'tail' pointer argument to
> > of_attach_node() which if present it would use as the insertion point
> > for adding the node, which would preserve the current sort order.
> 
> I was leaning towards a tail pointer, but I need to take a closer look
> at the two options.

okay.

g.