Re: [PATCH v5 39/42] drivers/of: Unflatten nodes equal or deeper than specified level

[Grant Likely <grant.likely@linaro.org>]

On Thu,  4 Jun 2015 16:42:08 +1000
, Gavin Shan <gwshan@linux.vnet.ibm.com>
 wrote:
> unflatten_dt_node() is called recursively to unflatten FDT nodes
> with the assumption that FDT blob has only one root node, which
> isn't true when the FDT blob represents device sub-tree. The
> patch improves the function to supporting device sub-tree that
> have multiple root nodes:
> 
>    * Rename original unflatten_dt_node() to __unflatten_dt_node().
>    * Wrapper unflatten_dt_node() calls __unflatten_dt_node() with
>      adjusted current node depth to 1 to avoid underflow.
> 
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
> v5:
>   * Split from PATCH[v4 19/21]
>   * Fixed "line over 80 characters" from checkpatch.pl
> ---
>  drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index cde35c5d01..b87c157 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -28,6 +28,8 @@
>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>  #include <asm/page.h>
>  
> +static int cur_node_depth;
> +

eeeek! We'll never be able to call this concurrently this way. That will
create theoretical race conditions in the overlay code. (actually, you
didn't introduce this problem, see below...)

>  /*
>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>   * @limit: maximum entries
> @@ -161,27 +163,26 @@ static void *unflatten_dt_alloc(void **mem, unsigned long size,
>  }
>  
>  /**
> - * unflatten_dt_node - Alloc and populate a device_node from the flat tree
> + * __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
>   * @fpsize: Size of the node path up at the current depth.
>   */
> -static void * unflatten_dt_node(void *blob,
> -				void *mem,
> -				int *poffset,
> -				struct device_node *dad,
> -				struct device_node **nodepp,
> -				unsigned long fpsize,
> -				bool dryrun)
> +static void *__unflatten_dt_node(void *blob,
> +				 void *mem,
> +				 int *poffset,
> +				 struct device_node *dad,
> +				 struct device_node **nodepp,
> +				 unsigned long fpsize,
> +				 bool dryrun)

nitpick: If you resist the temptation to reflow indentation, then the
diffstat is smaller.

>  {
>  	const __be32 *p;
>  	struct device_node *np;
>  	struct property *pp, **prev_pp = NULL;
>  	const char *pathp;
>  	unsigned int l, allocl;
> -	static int depth = 0;

Hmmmm.. looks like the race condition is already there. Well that's no
good. If you move *depth into the parameters to unflatten_dt_node(), then
you can solve both problems at once without having to create a __
version of the function. That will be a cleaner solution overall.

>  	int old_depth;
>  	int offset;
>  	int has_name = 0;
> @@ -334,13 +335,19 @@ static void * unflatten_dt_node(void *blob,
>  			np->type = "<NULL>";
>  	}
>  
> -	old_depth = depth;
> -	*poffset = fdt_next_node(blob, *poffset, &depth);
> -	if (depth < 0)
> -		depth = 0;
> -	while (*poffset > 0 && depth > old_depth)
> -		mem = unflatten_dt_node(blob, mem, poffset, np, NULL,
> -					fpsize, dryrun);
> +	old_depth = cur_node_depth;
> +	*poffset = fdt_next_node(blob, *poffset, &cur_node_depth);
> +	while (*poffset > 0) {

What is the reasoning here? Why change to looking for poffset > 0?

> +		if (cur_node_depth < old_depth)
> +			break;
> +
> +		if (cur_node_depth == old_depth)
> +			mem = __unflatten_dt_node(blob, mem, poffset,
> +						  dad, NULL, fpsize, dryrun);
> +		else if (cur_node_depth > old_depth)
> +			mem = __unflatten_dt_node(blob, mem, poffset,
> +						  np, NULL, fpsize, dryrun);

Ditto here, please describe the purpose of the new logic.

> +	}
>  
>  	if (*poffset < 0 && *poffset != -FDT_ERR_NOTFOUND)
>  		pr_err("unflatten: error %d processing FDT\n", *poffset);
> @@ -366,6 +373,18 @@ static void * unflatten_dt_node(void *blob,
>  	return mem;
>  }
>  
> +static void *unflatten_dt_node(void *blob,
> +			       void *mem,
> +			       int *poffset,
> +			       struct device_node *dad,
> +			       struct device_node **nodepp,
> +			       bool dryrun)
> +{
> +	cur_node_depth = 1;
> +	return __unflatten_dt_node(blob, mem, poffset,
> +				   dad, nodepp, 0, dryrun);
> +}
> +
>  /**
>   * __unflatten_device_tree - create tree of device_nodes from flat blob
>   *
> @@ -405,7 +424,8 @@ static void __unflatten_device_tree(void *blob,
>  
>  	/* First pass, scan for size */
>  	start = 0;
> -	size = (unsigned long)unflatten_dt_node(blob, NULL, &start, NULL, NULL, 0, true);
> +	size = (unsigned long)unflatten_dt_node(blob, NULL, &start,
> +						NULL, NULL, true);
>  	size = ALIGN(size, 4);
>  
>  	pr_debug("  size is %lx, allocating...\n", size);
> @@ -420,7 +440,7 @@ static void __unflatten_device_tree(void *blob,
>  
>  	/* Second pass, do actual unflattening */
>  	start = 0;
> -	unflatten_dt_node(blob, mem, &start, NULL, mynodes, 0, false);
> +	unflatten_dt_node(blob, mem, &start, NULL, mynodes, false);
>  	if (be32_to_cpup(mem + size) != 0xdeadbeef)
>  		pr_warning("End of tree marker overwritten: %08x\n",
>  			   be32_to_cpup(mem + size));
> -- 
> 2.1.0
>