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

[Andres Salomon]

On Sat, 12 Mar 2011 02:10:56 -0700
Grant Likely <grant.likely@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> 
> 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
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/