[U-Boot,v2,7/9] libfdt: Add overlay application function

The device tree overlays are a good way to deal with user-modifyable
boards or boards with some kind of an expansion mechanism where we can
easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).

Add a new function to merge overlays with a base device tree.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 include/libfdt.h         |  30 ++++
 lib/libfdt/Makefile      |   2 +-
 lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 445 insertions(+), 1 deletion(-)
 create mode 100644 lib/libfdt/fdt_overlay.c

On 2016-05-27 02:13, Maxime Ripard wrote:
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> 
> Add a new function to merge overlays with a base device tree.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  include/libfdt.h         |  30 ++++
>  lib/libfdt/Makefile      |   2 +-
>  lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 445 insertions(+), 1 deletion(-)
>  create mode 100644 lib/libfdt/fdt_overlay.c
> 

How big is the bloat? Did you thought about making it optional? e.g.
CONFIG_OF_LIBFDT_OVERLAY

What is the general opinion on that?


> diff --git a/include/libfdt.h b/include/libfdt.h
> index 1e01b2c7ebdf..783067e841a1 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int
> parentoffset, const char *name);
>   */
>  int fdt_del_node(void *fdt, int nodeoffset);
>  
> +/**
> + * fdt_overlay_apply - Applies a DT overlay on a base DT
> + * @fdt: pointer to the base device tree blob
> + * @fdto: pointer to the device tree overlay blob
> + *
> + * fdt_overlay_apply() will apply the given device tree overlay on the
> + * given base device tree.
> + *
> + * Expect the base device tree to be modified, even if the function
> + * returns an error.

Hm this sounds somewhat contradicting to a comment inside that
function...

> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
> + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
> + *		properties in the base DT
> + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
> + *		magic
> + *	-FDT_ERR_INTERNAL,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADOFFSET,
> + *	-FDT_ERR_BADPATH,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_overlay_apply(void *fdt, void *fdto);
> +
>  /**********************************************************************/
>  /* Debugging / informational functions                                */
>  /**********************************************************************/
> diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
> index 934d6142c3e9..4935bf012a75 100644
> --- a/lib/libfdt/Makefile
> +++ b/lib/libfdt/Makefile
> @@ -6,4 +6,4 @@
>  #
>  
>  obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
> -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
> +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> new file mode 100644
> index 000000000000..1e68e903c734
> --- /dev/null
> +++ b/lib/libfdt/fdt_overlay.c
> @@ -0,0 +1,414 @@
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
> +{
> +	const uint32_t *val;
> +	int len;
> +
> +	val = fdt_getprop(fdto, node, "target", &len);
> +	if (!val || (len != sizeof(*val)))
> +		return 0;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> +				  int fragment)
> +{
> +	uint32_t phandle;
> +	const char *path;
> +
> +	/* Try first to do a phandle based lookup */
> +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> +	if (phandle)
> +		return fdt_node_offset_by_phandle(fdt, phandle);
> +
> +	/* And then a path based lookup */
> +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> +	if (!path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	return fdt_path_offset(fdt, path);
> +}
> +
> +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
> +					    uint32_t delta)
> +{
> +	int property;
> +	int child;
> +
> +	fdt_for_each_property(fdto, node, property) {
> +		const uint32_t *val;
> +		const char *name;
> +		uint32_t adj_val;
> +		int len;
> +		int ret;
> +
> +		val = fdt_getprop_by_offset(fdto, property,
> +					    &name, &len);
> +		if (!val || (len != sizeof(*val)))
> +			continue;
> +
> +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> +			continue;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, child, node)
> +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> +{
> +	int root;
> +
> +	root = fdt_path_offset(fdto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
> +}
> +
> +static int _fdt_overlay_update_local_references(void *fdto,
> +						int tree_node,
> +						int fixup_node,
> +						uint32_t delta)
> +{
> +	int fixup_prop;
> +	int fixup_child;
> +
> +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
> +		const char *fixup_name, *tree_name;
> +		const uint32_t *val = NULL;
> +		uint32_t adj_val;
> +		int fixup_len;
> +		int tree_prop;
> +		int tree_len;
> +		int ret;
> +
> +		fdt_getprop_by_offset(fdto, fixup_prop,
> +				      &fixup_name, &fixup_len);
> +
> +		if (!strcmp(fixup_name, "phandle") ||
> +		    !strcmp(fixup_name, "linux,phandle"))
> +			continue;
> +
> +		fdt_for_each_property(fdto, tree_node, tree_prop) {
> +			val = fdt_getprop_by_offset(fdto, tree_prop,
> +						    &tree_name, &tree_len);
> +
> +			if (!strcmp(tree_name, fixup_name))
> +				break;
> +		}
> +
> +		if (!val || !tree_len)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (!tree_name)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (tree_len != 4)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
> +					      adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
> +		const char *fixup_child_name = fdt_get_name(fdto,
> +							    fixup_child, NULL);
> +		int tree_child;
> +
> +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
> +			const char *tree_child_name = fdt_get_name(fdto,
> +								   tree_child,
> +								   NULL);
> +
> +			if (!strcmp(fixup_child_name, tree_child_name))
> +				break;
> +		}
> +
> +		_fdt_overlay_update_local_references(fdto,
> +						     tree_child,
> +						     fixup_child,
> +						     delta);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> +{
> +	int root, fixups;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> +	if (root < 0)
> +		return root;
> +
> +	return _fdt_overlay_update_local_references(dto, root, fixups,
> +						    delta);
> +}
> +
> +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
> +				      int symbols_off,
> +				      const char *path, const char *name,
> +				      int index, const char *label)
> +{
> +	const uint32_t *prop_val;
> +	const char *symbol_path;
> +	uint32_t *fixup_val;
> +	uint32_t phandle;
> +	int symbol_off, fixup_off;
> +	int prop_len;
> +	int ret;
> +
> +	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +				  &prop_len);
> +	if (!symbol_path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	symbol_off = fdt_path_offset(fdt, symbol_path);
> +	if (symbol_off < 0)
> +		return symbol_off;
> +
> +	phandle = fdt_get_phandle(fdt, symbol_off);
> +	if (!phandle)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_off = fdt_path_offset(fdto, path);
> +	if (fixup_off < 0)
> +		return fixup_off;
> +
> +	prop_val = fdt_getprop(fdto, fixup_off, name,
> +			       &prop_len);
> +	if (!prop_val)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_val = malloc(prop_len);
> +	if (!fixup_val)
> +		return -FDT_ERR_INTERNAL;
> +	memcpy(fixup_val, prop_val, prop_len);
> +
> +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
> +		ret = -FDT_ERR_BADPHANDLE;
> +		goto out;
> +	}
> +
> +	fixup_val[index] = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop_inplace(fdto, fixup_off,
> +				  name, fixup_val,
> +				  prop_len);
> +
> +out:
> +	free(fixup_val);
> +	return ret;
> +};
> +
> +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +				     int property)
> +{
> +	const char *value, *tmp_value;
> +	const char *label;
> +	int tmp_len, len, next;
> +	int table = 0;
> +	int i;
> +
> +	value = fdt_getprop_by_offset(fdto, property,
> +				      &label, &len);
> +	tmp_value = value;
> +	tmp_len = len;
> +
> +	do {
> +		next = strlen(tmp_value) + 1;
> +		tmp_len -= next;
> +		tmp_value += next;
> +		table++;
> +	} while (tmp_len > 0);
> +
> +	for (i = 0; i < table; i++) {
> +		const char *prop_string = value;
> +		const char *path, *name;
> +		char *prop_cpy, *prop_tmp;
> +		int index, stlen;
> +		char *sep;
> +
> +		stlen = strlen(prop_string);
> +		prop_cpy = malloc(stlen + 1);
> +		if (!prop_cpy)
> +			return -FDT_ERR_INTERNAL;

A malloc in a inner loop sounds somewhat scary to me, but it probably
doesn't matter that much, not sure how memory management in U-Boot
exactly works.

I tried to find a maximum property length in order to use a stack
allocated variable, but it seems that the property length is
unlimited...

What you might try is using realloc. It should only resize the
allocation if the requested size is larger.
Initialize with 255, and then realloc in here, and free at the end of
the function....



> +
> +		strncpy(prop_cpy, prop_string, stlen);
> +		prop_cpy[stlen] = '\0';
> +
> +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
> +		     prop_tmp += ((sep - prop_tmp) + 1))
> +			*sep = '\0';
> +
> +		prop_tmp = prop_cpy;
> +		path = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		name = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		index = strtol(prop_tmp, NULL, 10);
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		value += stlen + 1;
> +		len -= stlen + 1;
> +
> +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
> +					   path, name, index, label);
> +
> +		free(prop_cpy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> +{
> +	int fixups_off, symbols_off;
> +	int property;
> +
> +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> +
> +	fdt_for_each_property(dto, fixups_off, property)
> +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
> +
> +	return 0;
> +}
> +
> +static int fdt_apply_overlay_node(void *dt, void *dto,
> +				  int target, int overlay)
> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property(dto, overlay, property) {
> +		const char *name;
> +		const void *prop;
> +		int prop_len;
> +		int ret;
> +
> +		prop = fdt_getprop_by_offset(dto, property, &name,
> +					     &prop_len);
> +		if (!prop)
> +			return -FDT_ERR_INTERNAL;
> +
> +		ret = fdt_setprop(dt, target, name, prop, prop_len);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(dto, node, overlay) {
> +		const char *name = fdt_get_name(dto, node, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(dt, target, name);
> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_merge(void *dt, void *dto)
> +{
> +	int root, fragment;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fdt_for_each_subnode(dto, fragment, root) {
> +		const char *name = fdt_get_name(dto, fragment, NULL);
> +		uint32_t target;
> +		int overlay;
> +		int ret;
> +
> +		if (strncmp(name, "fragment", 8))
> +			continue;
> +
> +		target = fdt_overlay_get_target(dt, dto, fragment);
> +		if (target < 0)
> +			return target;
> +
> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> +		if (overlay < 0)
> +			return overlay;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int fdt_overlay_apply(void *fdt, void *fdto)
> +{
> +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> +	void *fdto_copy;
> +	int ret;
> +
> +	FDT_CHECK_HEADER(fdt);
> +	FDT_CHECK_HEADER(fdto);
> +
> +	/*
> +	 * We're going to modify the overlay so that we can apply it.
> +	 *
> +	 * Make sure sure we don't destroy the original
> +	 */

... here.

> +	fdto_copy = malloc(fdt_totalsize(fdto));
> +	if (!fdto_copy)
> +		return -FDT_ERR_INTERNAL;
> +
> +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_merge(fdt, fdto_copy);
> +
> +out:
> +	free(fdto_copy);
> +	return ret;
> +}


--
Stefan

Hi Maxime,

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> 
> Add a new function to merge overlays with a base device tree.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> include/libfdt.h         |  30 ++++
> lib/libfdt/Makefile      |   2 +-
> lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 445 insertions(+), 1 deletion(-)
> create mode 100644 lib/libfdt/fdt_overlay.c
> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 1e01b2c7ebdf..783067e841a1 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
>  */
> int fdt_del_node(void *fdt, int nodeoffset);
> 
> +/**
> + * fdt_overlay_apply - Applies a DT overlay on a base DT
> + * @fdt: pointer to the base device tree blob
> + * @fdto: pointer to the device tree overlay blob
> + *
> + * fdt_overlay_apply() will apply the given device tree overlay on the
> + * given base device tree.
> + *
> + * Expect the base device tree to be modified, even if the function
> + * returns an error.
> + *

If the base tree has been modified on an error it is unsafe to use it
for booting. A valid strategy would be to scribble over the DT magic
number so that that blob is invalidated.

What are the other people’s opinion on this?

> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
> + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
> + *		properties in the base DT
> + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
> + *		magic
> + *	-FDT_ERR_INTERNAL,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADOFFSET,
> + *	-FDT_ERR_BADPATH,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_overlay_apply(void *fdt, void *fdto);
> +
> /**********************************************************************/
> /* Debugging / informational functions                                */
> /**********************************************************************/
> diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
> index 934d6142c3e9..4935bf012a75 100644
> --- a/lib/libfdt/Makefile
> +++ b/lib/libfdt/Makefile
> @@ -6,4 +6,4 @@
> #
> 
> obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
> -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
> +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> new file mode 100644
> index 000000000000..1e68e903c734
> --- /dev/null
> +++ b/lib/libfdt/fdt_overlay.c
> @@ -0,0 +1,414 @@
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
> +{
> +	const uint32_t *val;
> +	int len;
> +
> +	val = fdt_getprop(fdto, node, "target", &len);
> +	if (!val || (len != sizeof(*val)))
> +		return 0;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> +				  int fragment)
> +{
> +	uint32_t phandle;
> +	const char *path;
> +
> +	/* Try first to do a phandle based lookup */
> +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> +	if (phandle)
> +		return fdt_node_offset_by_phandle(fdt, phandle);
> +
> +	/* And then a path based lookup */
> +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> +	if (!path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	return fdt_path_offset(fdt, path);
> +}
> +

Those targets are fine; beware there are patches with more target options.

> +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
> +					    uint32_t delta)
> +{
> +	int property;
> +	int child;
> +
> +	fdt_for_each_property(fdto, node, property) {
> +		const uint32_t *val;
> +		const char *name;
> +		uint32_t adj_val;
> +		int len;
> +		int ret;
> +
> +		val = fdt_getprop_by_offset(fdto, property,
> +					    &name, &len);
> +		if (!val || (len != sizeof(*val)))

Superfluous parentheses.

> +			continue;
> +
> +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> +			continue;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, child, node)
> +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> +{
> +	int root;
> +
> +	root = fdt_path_offset(fdto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
> +}
> +
> +static int _fdt_overlay_update_local_references(void *fdto,
> +						int tree_node,
> +						int fixup_node,
> +						uint32_t delta)
> +{
> +	int fixup_prop;
> +	int fixup_child;
> +
> +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
> +		const char *fixup_name, *tree_name;
> +		const uint32_t *val = NULL;
> +		uint32_t adj_val;
> +		int fixup_len;
> +		int tree_prop;
> +		int tree_len;
> +		int ret;
> +
> +		fdt_getprop_by_offset(fdto, fixup_prop,
> +				      &fixup_name, &fixup_len);
> +
> +		if (!strcmp(fixup_name, "phandle") ||
> +		    !strcmp(fixup_name, "linux,phandle"))
> +			continue;
> +
> +		fdt_for_each_property(fdto, tree_node, tree_prop) {
> +			val = fdt_getprop_by_offset(fdto, tree_prop,
> +						    &tree_name, &tree_len);
> +
> +			if (!strcmp(tree_name, fixup_name))
> +				break;
> +		}
> +
> +		if (!val || !tree_len)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (!tree_name)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (tree_len != 4)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
> +					      adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
> +		const char *fixup_child_name = fdt_get_name(fdto,
> +							    fixup_child, NULL);
> +		int tree_child;
> +
> +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
> +			const char *tree_child_name = fdt_get_name(fdto,
> +								   tree_child,
> +								   NULL);
> +
> +			if (!strcmp(fixup_child_name, tree_child_name))
> +				break;
> +		}
> +
> +		_fdt_overlay_update_local_references(fdto,
> +						     tree_child,
> +						     fixup_child,
> +						     delta);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> +{
> +	int root, fixups;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> +	if (root < 0)
> +		return root;
> +
> +	return _fdt_overlay_update_local_references(dto, root, fixups,
> +						    delta);
> +}
> +
> +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
> +				      int symbols_off,
> +				      const char *path, const char *name,
> +				      int index, const char *label)
> +{
> +	const uint32_t *prop_val;
> +	const char *symbol_path;
> +	uint32_t *fixup_val;
> +	uint32_t phandle;
> +	int symbol_off, fixup_off;
> +	int prop_len;
> +	int ret;
> +
> +	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +				  &prop_len);
> +	if (!symbol_path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	symbol_off = fdt_path_offset(fdt, symbol_path);
> +	if (symbol_off < 0)
> +		return symbol_off;
> +
> +	phandle = fdt_get_phandle(fdt, symbol_off);
> +	if (!phandle)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_off = fdt_path_offset(fdto, path);
> +	if (fixup_off < 0)
> +		return fixup_off;
> +
> +	prop_val = fdt_getprop(fdto, fixup_off, name,
> +			       &prop_len);
> +	if (!prop_val)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_val = malloc(prop_len);
> +	if (!fixup_val)
> +		return -FDT_ERR_INTERNAL;
> +	memcpy(fixup_val, prop_val, prop_len);
> +
> +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
> +		ret = -FDT_ERR_BADPHANDLE;
> +		goto out;
> +	}
> +
> +	fixup_val[index] = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop_inplace(fdto, fixup_off,
> +				  name, fixup_val,
> +				  prop_len);
> +
> +out:
> +	free(fixup_val);
> +	return ret;
> +};
> +
> +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +				     int property)
> +{
> +	const char *value, *tmp_value;
> +	const char *label;
> +	int tmp_len, len, next;
> +	int table = 0;
> +	int i;
> +
> +	value = fdt_getprop_by_offset(fdto, property,
> +				      &label, &len);
> +	tmp_value = value;
> +	tmp_len = len;
> +
> +	do {
> +		next = strlen(tmp_value) + 1;
> +		tmp_len -= next;
> +		tmp_value += next;
> +		table++;
> +	} while (tmp_len > 0);
> +
> +	for (i = 0; i < table; i++) {
> +		const char *prop_string = value;
> +		const char *path, *name;
> +		char *prop_cpy, *prop_tmp;
> +		int index, stlen;
> +		char *sep;
> +
> +		stlen = strlen(prop_string);
> +		prop_cpy = malloc(stlen + 1);
> +		if (!prop_cpy)
> +			return -FDT_ERR_INTERNAL;
> +
> +		strncpy(prop_cpy, prop_string, stlen);
> +		prop_cpy[stlen] = '\0';
> +
> +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
> +		     prop_tmp += ((sep - prop_tmp) + 1))
> +			*sep = '\0';
> +
> +		prop_tmp = prop_cpy;
> +		path = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		name = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		index = strtol(prop_tmp, NULL, 10);
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		value += stlen + 1;
> +		len -= stlen + 1;
> +
> +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
> +					   path, name, index, label);
> +
> +		free(prop_cpy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> +{
> +	int fixups_off, symbols_off;
> +	int property;
> +
> +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> +
> +	fdt_for_each_property(dto, fixups_off, property)
> +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
> +
> +	return 0;
> +}
> +
> +static int fdt_apply_overlay_node(void *dt, void *dto,
> +				  int target, int overlay)
> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property(dto, overlay, property) {
> +		const char *name;
> +		const void *prop;
> +		int prop_len;
> +		int ret;
> +
> +		prop = fdt_getprop_by_offset(dto, property, &name,
> +					     &prop_len);
> +		if (!prop)
> +			return -FDT_ERR_INTERNAL;
> +
> +		ret = fdt_setprop(dt, target, name, prop, prop_len);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(dto, node, overlay) {
> +		const char *name = fdt_get_name(dto, node, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(dt, target, name);
> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_merge(void *dt, void *dto)
> +{
> +	int root, fragment;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fdt_for_each_subnode(dto, fragment, root) {
> +		const char *name = fdt_get_name(dto, fragment, NULL);
> +		uint32_t target;
> +		int overlay;
> +		int ret;
> +
> +		if (strncmp(name, "fragment", 8))
> +			continue;
> +

This is incorrect. The use of “fragment” is a convention only.
The real test whether the node is an overlay fragment is that
it contains a target property.

> +		target = fdt_overlay_get_target(dt, dto, fragment);
> +		if (target < 0)
> +			return target;
> +

So you could do ‘if (target < 0) continue;’ or handle a more complex error code.

> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> +		if (overlay < 0)
> +			return overlay;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int fdt_overlay_apply(void *fdt, void *fdto)
> +{
> +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> +	void *fdto_copy;
> +	int ret;
> +
> +	FDT_CHECK_HEADER(fdt);
> +	FDT_CHECK_HEADER(fdto);
> +
> +	/*
> +	 * We're going to modify the overlay so that we can apply it.
> +	 *
> +	 * Make sure sure we don't destroy the original
> +	 */
> +	fdto_copy = malloc(fdt_totalsize(fdto));
> +	if (!fdto_copy)
> +		return -FDT_ERR_INTERNAL;
> +
> +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_merge(fdt, fdto_copy);
> +
> +out:
> +	free(fdto_copy);
> +	return ret;
> +}
> -- 
> 2.8.2
> 

I would caution against the liberal use of malloc in libfdt. We’re possibly running in a constrained
environment; a custom extents based (non freeing) allocator should be better.

We need some figures about memory consumption when this is enabled, and a CONFIG option to disable it.

Regards

— Pantelis

Hi Pantelis,

On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> Hi Maxime,
> 
> > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > The device tree overlays are a good way to deal with user-modifyable
> > boards or boards with some kind of an expansion mechanism where we can
> > easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> > 
> > Add a new function to merge overlays with a base device tree.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> > include/libfdt.h         |  30 ++++
> > lib/libfdt/Makefile      |   2 +-
> > lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 445 insertions(+), 1 deletion(-)
> > create mode 100644 lib/libfdt/fdt_overlay.c
> > 
> > diff --git a/include/libfdt.h b/include/libfdt.h
> > index 1e01b2c7ebdf..783067e841a1 100644
> > --- a/include/libfdt.h
> > +++ b/include/libfdt.h
> > @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
> >  */
> > int fdt_del_node(void *fdt, int nodeoffset);
> > 
> > +/**
> > + * fdt_overlay_apply - Applies a DT overlay on a base DT
> > + * @fdt: pointer to the base device tree blob
> > + * @fdto: pointer to the device tree overlay blob
> > + *
> > + * fdt_overlay_apply() will apply the given device tree overlay on the
> > + * given base device tree.
> > + *
> > + * Expect the base device tree to be modified, even if the function
> > + * returns an error.
> > + *
> 
> If the base tree has been modified on an error it is unsafe to use it
> for booting. A valid strategy would be to scribble over the DT magic
> number so that that blob is invalidated.
> 
> What are the other people’s opinion on this?

That would probably be safer yes, I'll change that.

> > +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> > +				  int fragment)
> > +{
> > +	uint32_t phandle;
> > +	const char *path;
> > +
> > +	/* Try first to do a phandle based lookup */
> > +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> > +	if (phandle)
> > +		return fdt_node_offset_by_phandle(fdt, phandle);
> > +
> > +	/* And then a path based lookup */
> > +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> > +	if (!path)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	return fdt_path_offset(fdt, path);
> > +}
> > +
> 
> Those targets are fine; beware there are patches with more target options.

Ack.

> > +static int fdt_overlay_merge(void *dt, void *dto)
> > +{
> > +	int root, fragment;
> > +
> > +	root = fdt_path_offset(dto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	fdt_for_each_subnode(dto, fragment, root) {
> > +		const char *name = fdt_get_name(dto, fragment, NULL);
> > +		uint32_t target;
> > +		int overlay;
> > +		int ret;
> > +
> > +		if (strncmp(name, "fragment", 8))
> > +			continue;
> > +
> 
> This is incorrect. The use of “fragment” is a convention only.
> The real test whether the node is an overlay fragment is that
> it contains a target property.
> 
> > +		target = fdt_overlay_get_target(dt, dto, fragment);
> > +		if (target < 0)
> > +			return target;
> > +
> 
> So you could do ‘if (target < 0) continue;’ or handle a more complex error code.

Ok, will change.

> I would caution against the liberal use of malloc in libfdt. We’re
> possibly running in a constrained environment; a custom extents
> based (non freeing) allocator should be better.

David had the same comments, and I will drop the mallocs entirely.

> We need some figures about memory consumption when this is enabled,
> and a CONFIG option to disable it.

The current code (before and after that patch) adds 18kB to a
sandbox_defconfig.

Maxime

On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> Hi Maxime,
> 
> > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > The device tree overlays are a good way to deal with user-modifyable
> > boards or boards with some kind of an expansion mechanism where we can
> > easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> > 
> > Add a new function to merge overlays with a base device tree.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> > include/libfdt.h         |  30 ++++
> > lib/libfdt/Makefile      |   2 +-
> > lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 445 insertions(+), 1 deletion(-)
> > create mode 100644 lib/libfdt/fdt_overlay.c
> > 
> > diff --git a/include/libfdt.h b/include/libfdt.h
> > index 1e01b2c7ebdf..783067e841a1 100644
> > --- a/include/libfdt.h
> > +++ b/include/libfdt.h
> > @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
> >  */
> > int fdt_del_node(void *fdt, int nodeoffset);
> > 
> > +/**
> > + * fdt_overlay_apply - Applies a DT overlay on a base DT
> > + * @fdt: pointer to the base device tree blob
> > + * @fdto: pointer to the device tree overlay blob
> > + *
> > + * fdt_overlay_apply() will apply the given device tree overlay on the
> > + * given base device tree.
> > + *
> > + * Expect the base device tree to be modified, even if the function
> > + * returns an error.
> > + *
> 
> If the base tree has been modified on an error it is unsafe to use it
> for booting. A valid strategy would be to scribble over the DT magic
> number so that that blob is invalidated.
> 
> What are the other people’s opinion on this?
> 
> > + * returns:
> > + *	0, on success
> > + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
> > + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
> > + *		properties in the base DT
> > + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
> > + *		magic
> > + *	-FDT_ERR_INTERNAL,
> > + *	-FDT_ERR_BADLAYOUT,
> > + *	-FDT_ERR_BADMAGIC,
> > + *	-FDT_ERR_BADOFFSET,
> > + *	-FDT_ERR_BADPATH,
> > + *	-FDT_ERR_BADVERSION,
> > + *	-FDT_ERR_BADSTRUCTURE,
> > + *	-FDT_ERR_BADSTATE,
> > + *	-FDT_ERR_TRUNCATED, standard meanings
> > + */
> > +int fdt_overlay_apply(void *fdt, void *fdto);
> > +
> > /**********************************************************************/
> > /* Debugging / informational functions                                */
> > /**********************************************************************/
> > diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
> > index 934d6142c3e9..4935bf012a75 100644
> > --- a/lib/libfdt/Makefile
> > +++ b/lib/libfdt/Makefile
> > @@ -6,4 +6,4 @@
> > #
> > 
> > obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
> > -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
> > +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
> > diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> > new file mode 100644
> > index 000000000000..1e68e903c734
> > --- /dev/null
> > +++ b/lib/libfdt/fdt_overlay.c
> > @@ -0,0 +1,414 @@
> > +#include "libfdt_env.h"
> > +
> > +#include <fdt.h>
> > +#include <libfdt.h>
> > +
> > +#include "libfdt_internal.h"
> > +
> > +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
> > +{
> > +	const uint32_t *val;
> > +	int len;
> > +
> > +	val = fdt_getprop(fdto, node, "target", &len);
> > +	if (!val || (len != sizeof(*val)))
> > +		return 0;
> > +
> > +	return fdt32_to_cpu(*val);
> > +}
> > +
> > +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> > +				  int fragment)
> > +{
> > +	uint32_t phandle;
> > +	const char *path;
> > +
> > +	/* Try first to do a phandle based lookup */
> > +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> > +	if (phandle)
> > +		return fdt_node_offset_by_phandle(fdt, phandle);
> > +
> > +	/* And then a path based lookup */
> > +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> > +	if (!path)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	return fdt_path_offset(fdt, path);
> > +}
> > +
> 
> Those targets are fine; beware there are patches with more target options.
> 
> > +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
> > +					    uint32_t delta)
> > +{
> > +	int property;
> > +	int child;
> > +
> > +	fdt_for_each_property(fdto, node, property) {
> > +		const uint32_t *val;
> > +		const char *name;
> > +		uint32_t adj_val;
> > +		int len;
> > +		int ret;
> > +
> > +		val = fdt_getprop_by_offset(fdto, property,
> > +					    &name, &len);
> > +		if (!val || (len != sizeof(*val)))
> 
> Superfluous parentheses.
> 
> > +			continue;
> > +
> > +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> > +			continue;
> > +
> > +		adj_val = fdt32_to_cpu(*val);
> > +		adj_val += delta;
> > +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	fdt_for_each_subnode(fdto, child, node)
> > +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> > +{
> > +	int root;
> > +
> > +	root = fdt_path_offset(fdto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
> > +}
> > +
> > +static int _fdt_overlay_update_local_references(void *fdto,
> > +						int tree_node,
> > +						int fixup_node,
> > +						uint32_t delta)
> > +{
> > +	int fixup_prop;
> > +	int fixup_child;
> > +
> > +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
> > +		const char *fixup_name, *tree_name;
> > +		const uint32_t *val = NULL;
> > +		uint32_t adj_val;
> > +		int fixup_len;
> > +		int tree_prop;
> > +		int tree_len;
> > +		int ret;
> > +
> > +		fdt_getprop_by_offset(fdto, fixup_prop,
> > +				      &fixup_name, &fixup_len);
> > +
> > +		if (!strcmp(fixup_name, "phandle") ||
> > +		    !strcmp(fixup_name, "linux,phandle"))
> > +			continue;
> > +
> > +		fdt_for_each_property(fdto, tree_node, tree_prop) {
> > +			val = fdt_getprop_by_offset(fdto, tree_prop,
> > +						    &tree_name, &tree_len);
> > +
> > +			if (!strcmp(tree_name, fixup_name))
> > +				break;
> > +		}
> > +
> > +		if (!val || !tree_len)
> > +			return -FDT_ERR_NOTFOUND;
> > +
> > +		if (!tree_name)
> > +			return -FDT_ERR_NOTFOUND;
> > +
> > +		if (tree_len != 4)
> > +			return -FDT_ERR_NOTFOUND;
> > +
> > +		adj_val = fdt32_to_cpu(*val);
> > +		adj_val += delta;
> > +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
> > +					      adj_val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
> > +		const char *fixup_child_name = fdt_get_name(fdto,
> > +							    fixup_child, NULL);
> > +		int tree_child;
> > +
> > +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
> > +			const char *tree_child_name = fdt_get_name(fdto,
> > +								   tree_child,
> > +								   NULL);
> > +
> > +			if (!strcmp(fixup_child_name, tree_child_name))
> > +				break;
> > +		}
> > +
> > +		_fdt_overlay_update_local_references(fdto,
> > +						     tree_child,
> > +						     fixup_child,
> > +						     delta);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> > +{
> > +	int root, fixups;
> > +
> > +	root = fdt_path_offset(dto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	return _fdt_overlay_update_local_references(dto, root, fixups,
> > +						    delta);
> > +}
> > +
> > +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
> > +				      int symbols_off,
> > +				      const char *path, const char *name,
> > +				      int index, const char *label)
> > +{
> > +	const uint32_t *prop_val;
> > +	const char *symbol_path;
> > +	uint32_t *fixup_val;
> > +	uint32_t phandle;
> > +	int symbol_off, fixup_off;
> > +	int prop_len;
> > +	int ret;
> > +
> > +	symbol_path = fdt_getprop(fdt, symbols_off, label,
> > +				  &prop_len);
> > +	if (!symbol_path)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	symbol_off = fdt_path_offset(fdt, symbol_path);
> > +	if (symbol_off < 0)
> > +		return symbol_off;
> > +
> > +	phandle = fdt_get_phandle(fdt, symbol_off);
> > +	if (!phandle)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	fixup_off = fdt_path_offset(fdto, path);
> > +	if (fixup_off < 0)
> > +		return fixup_off;
> > +
> > +	prop_val = fdt_getprop(fdto, fixup_off, name,
> > +			       &prop_len);
> > +	if (!prop_val)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	fixup_val = malloc(prop_len);
> > +	if (!fixup_val)
> > +		return -FDT_ERR_INTERNAL;
> > +	memcpy(fixup_val, prop_val, prop_len);
> > +
> > +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
> > +		ret = -FDT_ERR_BADPHANDLE;
> > +		goto out;
> > +	}
> > +
> > +	fixup_val[index] = cpu_to_fdt32(phandle);
> > +	ret = fdt_setprop_inplace(fdto, fixup_off,
> > +				  name, fixup_val,
> > +				  prop_len);
> > +
> > +out:
> > +	free(fixup_val);
> > +	return ret;
> > +};
> > +
> > +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> > +				     int property)
> > +{
> > +	const char *value, *tmp_value;
> > +	const char *label;
> > +	int tmp_len, len, next;
> > +	int table = 0;
> > +	int i;
> > +
> > +	value = fdt_getprop_by_offset(fdto, property,
> > +				      &label, &len);
> > +	tmp_value = value;
> > +	tmp_len = len;
> > +
> > +	do {
> > +		next = strlen(tmp_value) + 1;
> > +		tmp_len -= next;
> > +		tmp_value += next;
> > +		table++;
> > +	} while (tmp_len > 0);
> > +
> > +	for (i = 0; i < table; i++) {
> > +		const char *prop_string = value;
> > +		const char *path, *name;
> > +		char *prop_cpy, *prop_tmp;
> > +		int index, stlen;
> > +		char *sep;
> > +
> > +		stlen = strlen(prop_string);
> > +		prop_cpy = malloc(stlen + 1);
> > +		if (!prop_cpy)
> > +			return -FDT_ERR_INTERNAL;
> > +
> > +		strncpy(prop_cpy, prop_string, stlen);
> > +		prop_cpy[stlen] = '\0';
> > +
> > +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
> > +		     prop_tmp += ((sep - prop_tmp) + 1))
> > +			*sep = '\0';
> > +
> > +		prop_tmp = prop_cpy;
> > +		path = prop_tmp;
> > +		prop_tmp += strlen(prop_tmp) + 1;
> > +
> > +		name = prop_tmp;
> > +		prop_tmp += strlen(prop_tmp) + 1;
> > +
> > +		index = strtol(prop_tmp, NULL, 10);
> > +		prop_tmp += strlen(prop_tmp) + 1;
> > +
> > +		value += stlen + 1;
> > +		len -= stlen + 1;
> > +
> > +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
> > +					   path, name, index, label);
> > +
> > +		free(prop_cpy);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> > +{
> > +	int fixups_off, symbols_off;
> > +	int property;
> > +
> > +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> > +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> > +
> > +	fdt_for_each_property(dto, fixups_off, property)
> > +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_apply_overlay_node(void *dt, void *dto,
> > +				  int target, int overlay)
> > +{
> > +	int property;
> > +	int node;
> > +
> > +	fdt_for_each_property(dto, overlay, property) {
> > +		const char *name;
> > +		const void *prop;
> > +		int prop_len;
> > +		int ret;
> > +
> > +		prop = fdt_getprop_by_offset(dto, property, &name,
> > +					     &prop_len);
> > +		if (!prop)
> > +			return -FDT_ERR_INTERNAL;
> > +
> > +		ret = fdt_setprop(dt, target, name, prop, prop_len);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	fdt_for_each_subnode(dto, node, overlay) {
> > +		const char *name = fdt_get_name(dto, node, NULL);
> > +		int nnode;
> > +		int ret;
> > +
> > +		nnode = fdt_add_subnode(dt, target, name);
> > +		if (nnode < 0)
> > +			return nnode;
> > +
> > +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_merge(void *dt, void *dto)
> > +{
> > +	int root, fragment;
> > +
> > +	root = fdt_path_offset(dto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	fdt_for_each_subnode(dto, fragment, root) {
> > +		const char *name = fdt_get_name(dto, fragment, NULL);
> > +		uint32_t target;
> > +		int overlay;
> > +		int ret;
> > +
> > +		if (strncmp(name, "fragment", 8))
> > +			continue;
> > +
> 
> This is incorrect. The use of “fragment” is a convention only.
> The real test whether the node is an overlay fragment is that
> it contains a target property.

Hmm.. I dislike that approach.  First, it means that if new target
types are introduced in future, older code is likely to silently
ignore such fragments instead of realizing that it doesn't know how to
apply themm.  Second, it raises weird issues if some node down within
a fragment also happens to have a property called "target".


> > +		target = fdt_overlay_get_target(dt, dto, fragment);
> > +		if (target < 0)
> > +			return target;
> > +
> 
> So you could do ‘if (target < 0) continue;’ or handle a more complex error code.
> 
> > +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> > +		if (overlay < 0)
> > +			return overlay;
> > +
> > +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int fdt_overlay_apply(void *fdt, void *fdto)
> > +{
> > +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> > +	void *fdto_copy;
> > +	int ret;
> > +
> > +	FDT_CHECK_HEADER(fdt);
> > +	FDT_CHECK_HEADER(fdto);
> > +
> > +	/*
> > +	 * We're going to modify the overlay so that we can apply it.
> > +	 *
> > +	 * Make sure sure we don't destroy the original
> > +	 */
> > +	fdto_copy = malloc(fdt_totalsize(fdto));
> > +	if (!fdto_copy)
> > +		return -FDT_ERR_INTERNAL;
> > +
> > +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_merge(fdt, fdto_copy);
> > +
> > +out:
> > +	free(fdto_copy);
> > +	return ret;
> > +}
> 
> I would caution against the liberal use of malloc in libfdt. We’re possibly running in a constrained
> environment; a custom extents based (non freeing) allocator should be better.
> 
> We need some figures about memory consumption when this is enabled, and a CONFIG option to disable it.
> 
> Regards
> 
> — Pantelis
> 
>

Hi David,

> On Jun 14, 2016, at 03:25 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
>> Hi Maxime,
>> 
>>> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>>> 
>>> The device tree overlays are a good way to deal with user-modifyable
>>> boards or boards with some kind of an expansion mechanism where we can
>>> easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
>>> 
>>> Add a new function to merge overlays with a base device tree.
>>> 
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>> include/libfdt.h         |  30 ++++
>>> lib/libfdt/Makefile      |   2 +-
>>> lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 445 insertions(+), 1 deletion(-)
>>> create mode 100644 lib/libfdt/fdt_overlay.c
>>> 
>>> diff --git a/include/libfdt.h b/include/libfdt.h
>>> index 1e01b2c7ebdf..783067e841a1 100644
>>> --- a/include/libfdt.h
>>> +++ b/include/libfdt.h
>>> @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
>>> */
>>> int fdt_del_node(void *fdt, int nodeoffset);
>>> 
>>> +/**
>>> + * fdt_overlay_apply - Applies a DT overlay on a base DT
>>> + * @fdt: pointer to the base device tree blob
>>> + * @fdto: pointer to the device tree overlay blob
>>> + *
>>> + * fdt_overlay_apply() will apply the given device tree overlay on the
>>> + * given base device tree.
>>> + *
>>> + * Expect the base device tree to be modified, even if the function
>>> + * returns an error.
>>> + *
>> 
>> If the base tree has been modified on an error it is unsafe to use it
>> for booting. A valid strategy would be to scribble over the DT magic
>> number so that that blob is invalidated.
>> 
>> What are the other people’s opinion on this?
>> 
>>> + * returns:
>>> + *	0, on success
>>> + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
>>> + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
>>> + *		properties in the base DT
>>> + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
>>> + *		magic
>>> + *	-FDT_ERR_INTERNAL,
>>> + *	-FDT_ERR_BADLAYOUT,
>>> + *	-FDT_ERR_BADMAGIC,
>>> + *	-FDT_ERR_BADOFFSET,
>>> + *	-FDT_ERR_BADPATH,
>>> + *	-FDT_ERR_BADVERSION,
>>> + *	-FDT_ERR_BADSTRUCTURE,
>>> + *	-FDT_ERR_BADSTATE,
>>> + *	-FDT_ERR_TRUNCATED, standard meanings
>>> + */
>>> +int fdt_overlay_apply(void *fdt, void *fdto);
>>> +
>>> /**********************************************************************/
>>> /* Debugging / informational functions                                */
>>> /**********************************************************************/
>>> diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
>>> index 934d6142c3e9..4935bf012a75 100644
>>> --- a/lib/libfdt/Makefile
>>> +++ b/lib/libfdt/Makefile
>>> @@ -6,4 +6,4 @@
>>> #
>>> 
>>> obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
>>> -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
>>> +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
>>> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
>>> new file mode 100644
>>> index 000000000000..1e68e903c734
>>> --- /dev/null
>>> +++ b/lib/libfdt/fdt_overlay.c
>>> @@ -0,0 +1,414 @@
>>> +#include "libfdt_env.h"
>>> +
>>> +#include <fdt.h>
>>> +#include <libfdt.h>
>>> +
>>> +#include "libfdt_internal.h"
>>> +
>>> +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
>>> +{
>>> +	const uint32_t *val;
>>> +	int len;
>>> +
>>> +	val = fdt_getprop(fdto, node, "target", &len);
>>> +	if (!val || (len != sizeof(*val)))
>>> +		return 0;
>>> +
>>> +	return fdt32_to_cpu(*val);
>>> +}
>>> +
>>> +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
>>> +				  int fragment)
>>> +{
>>> +	uint32_t phandle;
>>> +	const char *path;
>>> +
>>> +	/* Try first to do a phandle based lookup */
>>> +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
>>> +	if (phandle)
>>> +		return fdt_node_offset_by_phandle(fdt, phandle);
>>> +
>>> +	/* And then a path based lookup */
>>> +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
>>> +	if (!path)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	return fdt_path_offset(fdt, path);
>>> +}
>>> +
>> 
>> Those targets are fine; beware there are patches with more target options.
>> 
>>> +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
>>> +					    uint32_t delta)
>>> +{
>>> +	int property;
>>> +	int child;
>>> +
>>> +	fdt_for_each_property(fdto, node, property) {
>>> +		const uint32_t *val;
>>> +		const char *name;
>>> +		uint32_t adj_val;
>>> +		int len;
>>> +		int ret;
>>> +
>>> +		val = fdt_getprop_by_offset(fdto, property,
>>> +					    &name, &len);
>>> +		if (!val || (len != sizeof(*val)))
>> 
>> Superfluous parentheses.
>> 
>>> +			continue;
>>> +
>>> +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
>>> +			continue;
>>> +
>>> +		adj_val = fdt32_to_cpu(*val);
>>> +		adj_val += delta;
>>> +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	fdt_for_each_subnode(fdto, child, node)
>>> +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
>>> +{
>>> +	int root;
>>> +
>>> +	root = fdt_path_offset(fdto, "/");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
>>> +}
>>> +
>>> +static int _fdt_overlay_update_local_references(void *fdto,
>>> +						int tree_node,
>>> +						int fixup_node,
>>> +						uint32_t delta)
>>> +{
>>> +	int fixup_prop;
>>> +	int fixup_child;
>>> +
>>> +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
>>> +		const char *fixup_name, *tree_name;
>>> +		const uint32_t *val = NULL;
>>> +		uint32_t adj_val;
>>> +		int fixup_len;
>>> +		int tree_prop;
>>> +		int tree_len;
>>> +		int ret;
>>> +
>>> +		fdt_getprop_by_offset(fdto, fixup_prop,
>>> +				      &fixup_name, &fixup_len);
>>> +
>>> +		if (!strcmp(fixup_name, "phandle") ||
>>> +		    !strcmp(fixup_name, "linux,phandle"))
>>> +			continue;
>>> +
>>> +		fdt_for_each_property(fdto, tree_node, tree_prop) {
>>> +			val = fdt_getprop_by_offset(fdto, tree_prop,
>>> +						    &tree_name, &tree_len);
>>> +
>>> +			if (!strcmp(tree_name, fixup_name))
>>> +				break;
>>> +		}
>>> +
>>> +		if (!val || !tree_len)
>>> +			return -FDT_ERR_NOTFOUND;
>>> +
>>> +		if (!tree_name)
>>> +			return -FDT_ERR_NOTFOUND;
>>> +
>>> +		if (tree_len != 4)
>>> +			return -FDT_ERR_NOTFOUND;
>>> +
>>> +		adj_val = fdt32_to_cpu(*val);
>>> +		adj_val += delta;
>>> +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
>>> +					      adj_val);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
>>> +		const char *fixup_child_name = fdt_get_name(fdto,
>>> +							    fixup_child, NULL);
>>> +		int tree_child;
>>> +
>>> +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
>>> +			const char *tree_child_name = fdt_get_name(fdto,
>>> +								   tree_child,
>>> +								   NULL);
>>> +
>>> +			if (!strcmp(fixup_child_name, tree_child_name))
>>> +				break;
>>> +		}
>>> +
>>> +		_fdt_overlay_update_local_references(fdto,
>>> +						     tree_child,
>>> +						     fixup_child,
>>> +						     delta);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
>>> +{
>>> +	int root, fixups;
>>> +
>>> +	root = fdt_path_offset(dto, "/");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	return _fdt_overlay_update_local_references(dto, root, fixups,
>>> +						    delta);
>>> +}
>>> +
>>> +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
>>> +				      int symbols_off,
>>> +				      const char *path, const char *name,
>>> +				      int index, const char *label)
>>> +{
>>> +	const uint32_t *prop_val;
>>> +	const char *symbol_path;
>>> +	uint32_t *fixup_val;
>>> +	uint32_t phandle;
>>> +	int symbol_off, fixup_off;
>>> +	int prop_len;
>>> +	int ret;
>>> +
>>> +	symbol_path = fdt_getprop(fdt, symbols_off, label,
>>> +				  &prop_len);
>>> +	if (!symbol_path)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	symbol_off = fdt_path_offset(fdt, symbol_path);
>>> +	if (symbol_off < 0)
>>> +		return symbol_off;
>>> +
>>> +	phandle = fdt_get_phandle(fdt, symbol_off);
>>> +	if (!phandle)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	fixup_off = fdt_path_offset(fdto, path);
>>> +	if (fixup_off < 0)
>>> +		return fixup_off;
>>> +
>>> +	prop_val = fdt_getprop(fdto, fixup_off, name,
>>> +			       &prop_len);
>>> +	if (!prop_val)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	fixup_val = malloc(prop_len);
>>> +	if (!fixup_val)
>>> +		return -FDT_ERR_INTERNAL;
>>> +	memcpy(fixup_val, prop_val, prop_len);
>>> +
>>> +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
>>> +		ret = -FDT_ERR_BADPHANDLE;
>>> +		goto out;
>>> +	}
>>> +
>>> +	fixup_val[index] = cpu_to_fdt32(phandle);
>>> +	ret = fdt_setprop_inplace(fdto, fixup_off,
>>> +				  name, fixup_val,
>>> +				  prop_len);
>>> +
>>> +out:
>>> +	free(fixup_val);
>>> +	return ret;
>>> +};
>>> +
>>> +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>>> +				     int property)
>>> +{
>>> +	const char *value, *tmp_value;
>>> +	const char *label;
>>> +	int tmp_len, len, next;
>>> +	int table = 0;
>>> +	int i;
>>> +
>>> +	value = fdt_getprop_by_offset(fdto, property,
>>> +				      &label, &len);
>>> +	tmp_value = value;
>>> +	tmp_len = len;
>>> +
>>> +	do {
>>> +		next = strlen(tmp_value) + 1;
>>> +		tmp_len -= next;
>>> +		tmp_value += next;
>>> +		table++;
>>> +	} while (tmp_len > 0);
>>> +
>>> +	for (i = 0; i < table; i++) {
>>> +		const char *prop_string = value;
>>> +		const char *path, *name;
>>> +		char *prop_cpy, *prop_tmp;
>>> +		int index, stlen;
>>> +		char *sep;
>>> +
>>> +		stlen = strlen(prop_string);
>>> +		prop_cpy = malloc(stlen + 1);
>>> +		if (!prop_cpy)
>>> +			return -FDT_ERR_INTERNAL;
>>> +
>>> +		strncpy(prop_cpy, prop_string, stlen);
>>> +		prop_cpy[stlen] = '\0';
>>> +
>>> +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
>>> +		     prop_tmp += ((sep - prop_tmp) + 1))
>>> +			*sep = '\0';
>>> +
>>> +		prop_tmp = prop_cpy;
>>> +		path = prop_tmp;
>>> +		prop_tmp += strlen(prop_tmp) + 1;
>>> +
>>> +		name = prop_tmp;
>>> +		prop_tmp += strlen(prop_tmp) + 1;
>>> +
>>> +		index = strtol(prop_tmp, NULL, 10);
>>> +		prop_tmp += strlen(prop_tmp) + 1;
>>> +
>>> +		value += stlen + 1;
>>> +		len -= stlen + 1;
>>> +
>>> +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
>>> +					   path, name, index, label);
>>> +
>>> +		free(prop_cpy);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
>>> +{
>>> +	int fixups_off, symbols_off;
>>> +	int property;
>>> +
>>> +	symbols_off = fdt_path_offset(dt, "/__symbols__");
>>> +	fixups_off = fdt_path_offset(dto, "/__fixups__");
>>> +
>>> +	fdt_for_each_property(dto, fixups_off, property)
>>> +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_apply_overlay_node(void *dt, void *dto,
>>> +				  int target, int overlay)
>>> +{
>>> +	int property;
>>> +	int node;
>>> +
>>> +	fdt_for_each_property(dto, overlay, property) {
>>> +		const char *name;
>>> +		const void *prop;
>>> +		int prop_len;
>>> +		int ret;
>>> +
>>> +		prop = fdt_getprop_by_offset(dto, property, &name,
>>> +					     &prop_len);
>>> +		if (!prop)
>>> +			return -FDT_ERR_INTERNAL;
>>> +
>>> +		ret = fdt_setprop(dt, target, name, prop, prop_len);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	fdt_for_each_subnode(dto, node, overlay) {
>>> +		const char *name = fdt_get_name(dto, node, NULL);
>>> +		int nnode;
>>> +		int ret;
>>> +
>>> +		nnode = fdt_add_subnode(dt, target, name);
>>> +		if (nnode < 0)
>>> +			return nnode;
>>> +
>>> +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_merge(void *dt, void *dto)
>>> +{
>>> +	int root, fragment;
>>> +
>>> +	root = fdt_path_offset(dto, "/");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	fdt_for_each_subnode(dto, fragment, root) {
>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
>>> +		uint32_t target;
>>> +		int overlay;
>>> +		int ret;
>>> +
>>> +		if (strncmp(name, "fragment", 8))
>>> +			continue;
>>> +
>> 
>> This is incorrect. The use of “fragment” is a convention only.
>> The real test whether the node is an overlay fragment is that
>> it contains a target property.
> 
> Hmm.. I dislike that approach.  First, it means that if new target
> types are introduced in future, older code is likely to silently
> ignore such fragments instead of realizing that it doesn't know how to
> apply themm.  Second, it raises weird issues if some node down within
> a fragment also happens to have a property called "target”.

Not really. If new targets are introduced then the fragment will be ignored.

We can have an argument about what is better to do (report an error or 
ignore a fragment) but what it comes down to is that that applicator
does not know how to handle the new target method.

There is no issues with any target properties inside a fragment because
the check is not made recursively.
 
> 

> 
>>> +		target = fdt_overlay_get_target(dt, dto, fragment);
>>> +		if (target < 0)
>>> +			return target;
>>> +
>> 
>> So you could do ‘if (target < 0) continue;’ or handle a more complex error code.
>> 
>>> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
>>> +		if (overlay < 0)
>>> +			return overlay;
>>> +
>>> +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int fdt_overlay_apply(void *fdt, void *fdto)
>>> +{
>>> +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
>>> +	void *fdto_copy;
>>> +	int ret;
>>> +
>>> +	FDT_CHECK_HEADER(fdt);
>>> +	FDT_CHECK_HEADER(fdto);
>>> +
>>> +	/*
>>> +	 * We're going to modify the overlay so that we can apply it.
>>> +	 *
>>> +	 * Make sure sure we don't destroy the original
>>> +	 */
>>> +	fdto_copy = malloc(fdt_totalsize(fdto));
>>> +	if (!fdto_copy)
>>> +		return -FDT_ERR_INTERNAL;
>>> +
>>> +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_merge(fdt, fdto_copy);
>>> +
>>> +out:
>>> +	free(fdto_copy);
>>> +	return ret;
>>> +}
>> 
>> I would caution against the liberal use of malloc in libfdt. We’re possibly running in a constrained
>> environment; a custom extents based (non freeing) allocator should be better.
>> 
>> We need some figures about memory consumption when this is enabled, and a CONFIG option to disable it.
>> 
>> Regards
>> 
>> — Pantelis
>> 
>> 
> 

Regards

— Pantelis

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
> Hi David,
> > On Jun 14, 2016, at 03:25 , David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
[snip]
> >>> +static int fdt_overlay_merge(void *dt, void *dto)
> >>> +{
> >>> +	int root, fragment;
> >>> +
> >>> +	root = fdt_path_offset(dto, "/");
> >>> +	if (root < 0)
> >>> +		return root;
> >>> +
> >>> +	fdt_for_each_subnode(dto, fragment, root) {
> >>> +		const char *name = fdt_get_name(dto, fragment, NULL);
> >>> +		uint32_t target;
> >>> +		int overlay;
> >>> +		int ret;
> >>> +
> >>> +		if (strncmp(name, "fragment", 8))
> >>> +			continue;
> >>> +
> >> 
> >> This is incorrect. The use of “fragment” is a convention only.
> >> The real test whether the node is an overlay fragment is that
> >> it contains a target property.
> > 
> > Hmm.. I dislike that approach.  First, it means that if new target
> > types are introduced in future, older code is likely to silently
> > ignore such fragments instead of realizing that it doesn't know how to
> > apply themm.  Second, it raises weird issues if some node down within
> > a fragment also happens to have a property called "target”.
> 
> Not really. If new targets are introduced then the fragment will be ignored.

Yes.. and that's bad.

> We can have an argument about what is better to do (report an error or 
> ignore a fragment) but what it comes down to is that that applicator
> does not know how to handle the new target method.

Sure, let's have the argument.  The overlay is constructed on the
assumption that all the pieces will be applied, or none of them.  A
silent, partial application is an awful, awful failure mode.  We
absolutely should report an error, and in order to do so we need to
know what are applicable fragments, whether or not we understand the
target designation (or any other meta-data the fragment has).

Given what's established so far, checking the name seems the obvious
way to do that.

> There is no issues with any target properties inside a fragment because
> the check is not made recursively.

Ok, so the real test you're proposing is "at the top level AND has a
target property".

Hi David,

> On Jun 15, 2016, at 06:14 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>>> On Jun 14, 2016, at 03:25 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> [snip]
>>>>> +static int fdt_overlay_merge(void *dt, void *dto)
>>>>> +{
>>>>> +	int root, fragment;
>>>>> +
>>>>> +	root = fdt_path_offset(dto, "/");
>>>>> +	if (root < 0)
>>>>> +		return root;
>>>>> +
>>>>> +	fdt_for_each_subnode(dto, fragment, root) {
>>>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
>>>>> +		uint32_t target;
>>>>> +		int overlay;
>>>>> +		int ret;
>>>>> +
>>>>> +		if (strncmp(name, "fragment", 8))
>>>>> +			continue;
>>>>> +
>>>> 
>>>> This is incorrect. The use of “fragment” is a convention only.
>>>> The real test whether the node is an overlay fragment is that
>>>> it contains a target property.
>>> 
>>> Hmm.. I dislike that approach.  First, it means that if new target
>>> types are introduced in future, older code is likely to silently
>>> ignore such fragments instead of realizing that it doesn't know how to
>>> apply themm.  Second, it raises weird issues if some node down within
>>> a fragment also happens to have a property called "target”.
>> 
>> Not really. If new targets are introduced then the fragment will be ignored.
> 
> Yes.. and that's bad.
> 

That’s arguable.

>> We can have an argument about what is better to do (report an error or 
>> ignore a fragment) but what it comes down to is that that applicator
>> does not know how to handle the new target method.
> 
> Sure, let's have the argument.  The overlay is constructed on the
> assumption that all the pieces will be applied, or none of them.  A
> silent, partial application is an awful, awful failure mode.  We
> absolutely should report an error, and in order to do so we need to
> know what are applicable fragments, whether or not we understand the
> target designation (or any other meta-data the fragment has).
> 

This way also allows having nodes being something other than fragments.

So instead of being painted into a corner (all subnodes that are not
named ‘fragment@X’ are errors), we have flexibility in introducing
nodes that contain configuration data for the loader.
 
> Given what's established so far, checking the name seems the obvious
> way to do that.
> 

Again, it’s arguable. Stricter checking against future-proofing.

>> There is no issues with any target properties inside a fragment because
>> the check is not made recursively.
> 
> Ok, so the real test you're proposing is "at the top level AND has a
> target property”.

Yes

> 

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Regards

— Pantelis

On Wed, Jun 15, 2016 at 12:34:00PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Jun 15, 2016, at 06:14 , David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
> >> Hi David,
> >>> On Jun 14, 2016, at 03:25 , David Gibson <david@gibson.dropbear.id.au> wrote:
> >>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> > [snip]
> >>>>> +static int fdt_overlay_merge(void *dt, void *dto)
> >>>>> +{
> >>>>> +	int root, fragment;
> >>>>> +
> >>>>> +	root = fdt_path_offset(dto, "/");
> >>>>> +	if (root < 0)
> >>>>> +		return root;
> >>>>> +
> >>>>> +	fdt_for_each_subnode(dto, fragment, root) {
> >>>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
> >>>>> +		uint32_t target;
> >>>>> +		int overlay;
> >>>>> +		int ret;
> >>>>> +
> >>>>> +		if (strncmp(name, "fragment", 8))
> >>>>> +			continue;
> >>>>> +
> >>>> 
> >>>> This is incorrect. The use of “fragment” is a convention only.
> >>>> The real test whether the node is an overlay fragment is that
> >>>> it contains a target property.
> >>> 
> >>> Hmm.. I dislike that approach.  First, it means that if new target
> >>> types are introduced in future, older code is likely to silently
> >>> ignore such fragments instead of realizing that it doesn't know how to
> >>> apply themm.  Second, it raises weird issues if some node down within
> >>> a fragment also happens to have a property called "target”.
> >> 
> >> Not really. If new targets are introduced then the fragment will be ignored.
> > 
> > Yes.. and that's bad.
> 
> That’s arguable.

!?!  No, really, silent partial application is just horrible.

> >> We can have an argument about what is better to do (report an error or 
> >> ignore a fragment) but what it comes down to is that that applicator
> >> does not know how to handle the new target method.
> > 
> > Sure, let's have the argument.  The overlay is constructed on the
> > assumption that all the pieces will be applied, or none of them.  A
> > silent, partial application is an awful, awful failure mode.  We
> > absolutely should report an error, and in order to do so we need to
> > know what are applicable fragments, whether or not we understand the
> > target designation (or any other meta-data the fragment has).
> 
> This way also allows having nodes being something other than fragments.
> 
> So instead of being painted into a corner (all subnodes that are not
> named ‘fragment@X’ are errors), we have flexibility in introducing
> nodes that contain configuration data for the loader.

There's no significant difference between the approaches from this
point of view.  Metadata nodes are certainly possible (we already have
__symbols__ and __fixups__) but calling them something other than
fragment@ is no harder than leaving off the target property.  In fact
even if it was workable, calling new metadata nodes fragment@ would be
stupidly confusing.

> > Given what's established so far, checking the name seems the obvious
> > way to do that.
> > 
> 
> Again, it’s arguable. Stricter checking against future-proofing.
> 
> >> There is no issues with any target properties inside a fragment because
> >> the check is not made recursively.
> > 
> > Ok, so the real test you're proposing is "at the top level AND has a
> > target property”.
> 
> Yes

Hi David,

> On Jun 15, 2016, at 13:19 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> On Wed, Jun 15, 2016 at 12:34:00PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>> 
>>> On Jun 15, 2016, at 06:14 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>> 
>>> On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
>>>> Hi David,
>>>>> On Jun 14, 2016, at 03:25 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
>>> [snip]
>>>>>>> +static int fdt_overlay_merge(void *dt, void *dto)
>>>>>>> +{
>>>>>>> +	int root, fragment;
>>>>>>> +
>>>>>>> +	root = fdt_path_offset(dto, "/");
>>>>>>> +	if (root < 0)
>>>>>>> +		return root;
>>>>>>> +
>>>>>>> +	fdt_for_each_subnode(dto, fragment, root) {
>>>>>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
>>>>>>> +		uint32_t target;
>>>>>>> +		int overlay;
>>>>>>> +		int ret;
>>>>>>> +
>>>>>>> +		if (strncmp(name, "fragment", 8))
>>>>>>> +			continue;
>>>>>>> +
>>>>>> 
>>>>>> This is incorrect. The use of “fragment” is a convention only.
>>>>>> The real test whether the node is an overlay fragment is that
>>>>>> it contains a target property.
>>>>> 
>>>>> Hmm.. I dislike that approach.  First, it means that if new target
>>>>> types are introduced in future, older code is likely to silently
>>>>> ignore such fragments instead of realizing that it doesn't know how to
>>>>> apply themm.  Second, it raises weird issues if some node down within
>>>>> a fragment also happens to have a property called "target”.
>>>> 
>>>> Not really. If new targets are introduced then the fragment will be ignored.
>>> 
>>> Yes.. and that's bad.
>> 
>> That’s arguable.
> 
> !?!  No, really, silent partial application is just horrible.
> 
>>>> We can have an argument about what is better to do (report an error or 
>>>> ignore a fragment) but what it comes down to is that that applicator
>>>> does not know how to handle the new target method.
>>> 
>>> Sure, let's have the argument.  The overlay is constructed on the
>>> assumption that all the pieces will be applied, or none of them.  A
>>> silent, partial application is an awful, awful failure mode.  We
>>> absolutely should report an error, and in order to do so we need to
>>> know what are applicable fragments, whether or not we understand the
>>> target designation (or any other meta-data the fragment has).
>> 
>> This way also allows having nodes being something other than fragments.
>> 
>> So instead of being painted into a corner (all subnodes that are not
>> named ‘fragment@X’ are errors), we have flexibility in introducing
>> nodes that contain configuration data for the loader.
> 
> There's no significant difference between the approaches from this
> point of view.  Metadata nodes are certainly possible (we already have
> __symbols__ and __fixups__) but calling them something other than
> fragment@ is no harder than leaving off the target property.  In fact
> even if it was workable, calling new metadata nodes fragment@ would be
> stupidly confusing.
> 

Err, we won’t ever call metadata nodes fragment@, that would be awfully confusing.

We’re bikeshedding, it’s just a philosophical argument right now.

The correct way is to get cracking on the machine readable yaml based bindings
and enforce it through there.

>>> Given what's established so far, checking the name seems the obvious
>>> way to do that.
>>> 
>> 
>> Again, it’s arguable. Stricter checking against future-proofing.
>> 
>>>> There is no issues with any target properties inside a fragment because
>>>> the check is not made recursively.
>>> 
>>> Ok, so the real test you're proposing is "at the top level AND has a
>>> target property”.
>> 
>> Yes
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Regards

— Pantelis

> On Jun 15, 2016, at 4:23 AM, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:

> The correct way is to get cracking on the machine readable yaml based bindings
> and enforce it through there.

Machine readable bindings with enforcement would be enabling technology to solve
a lot of different problems (both technical and non-technical). Coupled with overlays
(whatever philosophy wins and I hope one wins soon), these things will greatly improve
the flexibility of the currently somewhat rigid DTB…

Warner