Re: [PATCH v3 10/12] libfdt: Add overlay application function

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 15408 bytes --]

On Fri, Jun 24, 2016 at 04:27:55PM +0200, 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  include/libfdt.h         |  30 ++++
>  lib/Kconfig              |   5 +
>  lib/libfdt/Makefile      |   2 +
>  lib/libfdt/fdt_overlay.c | 381 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 418 insertions(+)
>  create mode 100644 lib/libfdt/fdt_overlay.c
> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index b8758de3ae54..039ecb0c1ba7 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1747,6 +1747,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.
> + *
> + * 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/Kconfig b/lib/Kconfig
> index 02ca4058d37e..16ff01a2cd37 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -139,6 +139,11 @@ config OF_LIBFDT
>  	  particular compatible nodes. The library operates on a flattened
>  	  version of the device tree.
>  
> +config OF_LIBFDT_OVERLAY
> +	bool "Enable the FDT library overlay support"
> +	help
> +	  This enables the FDT library (libfdt) overlay support.
> +
>  config SPL_OF_LIBFDT
>  	bool "Enable the FDT library for SPL"
>  	default y if SPL_OF_CONTROL
> diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
> index 8b86c15904cb..6ef8290f4e09 100644
> --- a/lib/libfdt/Makefile
> +++ b/lib/libfdt/Makefile
> @@ -15,3 +15,5 @@ obj-y += \
>  	fdt_empty_tree.o \
>  	fdt_addresses.o \
>  	fdt_region.o
> +
> +obj-$(CONFIG_OF_LIBFDT_OVERLAY) += fdt_overlay.o
> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> new file mode 100644
> index 000000000000..5835a90afafe
> --- /dev/null
> +++ b/lib/libfdt/fdt_overlay.c
> @@ -0,0 +1,381 @@
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
> +{
> +	const uint32_t *val;
> +	int len;
> +
> +	val = fdt_getprop(fdto, fragment, "target", &len);
> +	if (!val || (*val == 0xffffffff) || (len != sizeof(*val)))
> +		return 0;

This doesn't distinguish between a missing property (which may
indicate a valid overlay using a target-path or some other method)
and a badly formatted 'target' property, which is definitely an error
in the overlay.

I think those should be treated differently.

> +	return fdt32_to_cpu(*val);
> +}
> +
> +static int 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 = 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 overlay_phandle_add_offset(void *fdt, int node,
> +				      const char *name, uint32_t delta)
> +{
> +	const uint32_t *val;
> +	uint32_t adj_val;
> +	int len;
> +
> +	val = fdt_getprop(fdt, node, name, &len);
> +	if (!val)
> +		return len;
> +
> +	if (len != sizeof(*val))
> +		return -FDT_ERR_BADSTRUCTURE;
> +
> +	adj_val = fdt32_to_cpu(*val);
> +	adj_val += delta;

You should probably check for overflow here.

> +
> +	return fdt_setprop_inplace_u32(fdt, node, name, adj_val);
> +}
> +
> +static int overlay_adjust_node_phandles(void *fdto, int node,
> +					uint32_t delta)
> +{
> +	bool found = false;
> +	int child;
> +	int ret;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;
> +
> +	if (!ret)
> +		found = true;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;

I think the check for phandle vs. linux,phandle should be folded into
overlay_phandle_add_offset().

> +	/*
> +	 * If neither phandle nor linux,phandle have been found return
> +	 * an error.
> +	 */
> +	if (!found && !ret)
> +		return ret;
> +
> +	fdt_for_each_subnode(fdto, child, node)
> +		overlay_adjust_node_phandles(fdto, child, delta);
> +
> +	return 0;
> +}
> +
> +static int overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> +{
> +	/*
> +	 * Start adjusting the phandles from the overlay root
> +	 */
> +	return overlay_adjust_node_phandles(fdto, 0, delta);
> +}
> +
> +static int overlay_update_local_node_references(void *fdto,
> +						int tree_node,
> +						int fixup_node,
> +						uint32_t delta)
> +{
> +	int fixup_prop;
> +	int fixup_child;
> +	int ret;
> +
> +	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> +		const uint32_t *val = NULL;
> +		uint32_t adj_val, index;
> +		const char *name;
> +		int fixup_len;
> +		int tree_len;
> +
> +		val = fdt_getprop_by_offset(fdto, fixup_prop,
> +					    &name, &fixup_len);
> +		if (!val)
> +			return fixup_len;
> +		index = fdt32_to_cpu(*val);

This still doesn't handle the case of multiple fixups within the same
property.  That would be entirely plausible for something like an
interrupt-map property.

> +		val = fdt_getprop(fdto, tree_node, name, &tree_len);
> +		if (!val)
> +			return tree_len;
> +
> +		adj_val = fdt32_to_cpu(*(val + (index / sizeof(uint32_t))));

phandle values need not be aligned within the property containint them
(e.g. properties which mix strings and integer values).  So you're
going to need to work directly with a byte offset here.

> +		adj_val += delta;
> +		adj_val = cpu_to_fdt32(adj_val);
> +
> +		ret = fdt_setprop_inplace_namelen_by_index(fdto, tree_node,
> +							   name, strlen(name),
> +							   index, &adj_val,
> +							   sizeof(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;
> +
> +		tree_child = fdt_subnode_offset(fdto, tree_node,
> +						fixup_child_name);
> +		if (tree_child < 0)
> +			return tree_child;
> +
> +		ret = overlay_update_local_node_references(fdto,
> +							   tree_child,
> +							   fixup_child,
> +							   delta);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int overlay_update_local_references(void *dto, uint32_t delta)
> +{
> +	int fixups;
> +
> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> +	if (fixups < 0) {
> +		/*
> +		 * There's no local phandles to adjust, bail out
> +		 */
> +		if (fixups == -FDT_ERR_NOTFOUND)
> +			return 0;
> +
> +		return fixups;
> +	}
> +
> +	/*
> +	 * Update our local references from the root of the tree
> +	 */
> +	return overlay_update_local_node_references(dto, 0, fixups,
> +						    delta);
> +}
> +
> +static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> +				     int symbols_off,
> +				     const char *path, uint32_t path_len,
> +				     const char *name, uint32_t name_len,
> +				     int index, const char *label)
> +{
> +	const char *symbol_path;
> +	uint32_t phandle;
> +	int symbol_off, fixup_off;
> +	int prop_len;
> +
> +	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_namelen(fdto, path, path_len);
> +	if (fixup_off < 0)
> +		return fixup_off;
> +
> +	phandle = cpu_to_fdt32(phandle);
> +	return fdt_setprop_inplace_namelen_by_index(fdto, fixup_off,
> +						    name, name_len, index,
> +						    &phandle, sizeof(phandle));

This will be broken on BE systems because fdt_get_phandle() byteswaps
to native order, but setprop_inplace doesn't byteswap back (because
it's input is a byte array, not a u32).

> +};
> +
> +static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +				 int property)
> +{
> +	const char *value;
> +	const char *label;
> +	int len;
> +
> +	value = fdt_getprop_by_offset(fdto, property,
> +				      &label, &len);
> +	if (!value)
> +		return len;
> +
> +	do {
> +		const char *prop_string = value;
> +		const char *path, *name;
> +		uint32_t path_len, name_len;
> +		char *sep, *endptr;
> +		int index;
> +		int ret;
> +
> +		path = prop_string;
> +		sep = strchr(prop_string, ':');
> +		path_len = sep - path;

You need to check for strchr() returning NULL (indicating a badly
formatted fixup missing a separator).

Also... strchr() is not safe.  Again, if the fixup string is badly
formatted and doesn't have a terminating \0, this could overrun, so
you want memchr() instead.

> +		name = sep + 1;
> +		sep = strchr(name, ':');
> +		name_len = sep - name;

And again.

> +		index = strtoul(sep + 1, &endptr, 10);
> +		if (endptr <= (sep + 1))
> +			return -FDT_ERR_BADSTRUCTURE;

IIRC the index value is supposed to go to the end of that portion of
the fixup, so you should be able to check for *endptr == '\0'.

Except.. strtoul() could also overrun if the fixup property doesn't
contain a \0 like it should.

So I think you'd be better off using a memchr() (or strnlen()) to
search for the \0 before you attempt parsing the innards of a single
fixup string.

Ugh.  I hate that we're parsing text numbers in decimal here, but
we're pretty much stuck with that format here.  Doubly so that it's a
gratuitously different way of encoding the (node, property, offset)
tuple from local_fixups.

> +		index = strtoul(sep + 1, &endptr, 10);
> +		if (endptr <= (sep + 1))
> +			return -FDT_ERR_BADSTRUCTURE;
> +
> +		len -= strlen(prop_string) + 1;
> +		value += strlen(prop_string) + 1;

Please don't double call strlen() on the same string - remember it's
O(n) in C.

> +
> +		ret = overlay_fixup_one_phandle(fdt, fdto, symbols_off,
> +						path, path_len, name, name_len,
> +						index, label);
> +		if (ret)
> +			return ret;
> +	} while (len > 0);
> +
> +	return 0;
> +}
> +
> +static int 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_offset(property, dto, fixups_off)
> +		overlay_fixup_phandle(dt, dto, symbols_off, property);
> +
> +	return 0;
> +}
> +
> +static int apply_overlay_node(void *dt, int target,
> +			      void *dto, int overlay)

I think 'overlay' should be 'fragment' here for consistency?

> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property_offset(property, dto, overlay) {
> +		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;

Actually, you probably should check the error code returned in
prop_len.  Getting a NOTFOUND would indicate an internal error, but
you could also get BADSTRUCTURE or similar errors which would indicate
an error in input.

> +
> +		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 == -FDT_ERR_EXISTS)
> +			nnode = fdt_subnode_offset(dt, target, name);
> +
> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = apply_overlay_node(dt, nnode, dto, node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int overlay_merge(void *dt, void *dto)
> +{
> +	int fragment;
> +
> +	fdt_for_each_subnode(dto, fragment, 0) {
> +		int overlay;
> +		int target;
> +		int ret;
> +
> +		target = overlay_get_target(dt, dto, fragment);
> +		if (target < 0)
> +			continue;
> +
> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> +		if (overlay < 0)
> +			return overlay;
> +
> +		ret = apply_overlay_node(dt, target, dto, overlay);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int fdt_overlay_apply(void *fdt, void *fdto)
> +{
> +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> +	int ret;
> +
> +	FDT_CHECK_HEADER(fdt);
> +	FDT_CHECK_HEADER(fdto);
> +
> +	ret = overlay_adjust_local_phandles(fdto, delta);
> +	if (ret)
> +		goto err;
> +
> +	ret = overlay_update_local_references(fdto, delta);
> +	if (ret)
> +		goto err;
> +
> +	ret = overlay_fixup_phandles(fdt, fdto);
> +	if (ret)
> +		goto err;
> +
> +	ret = overlay_merge(fdt, fdto);
> +	if (!ret)
> +		goto out;
> +
> +err:

This is a confusing use of gotos - this looks like it is in the exit
path for both success and failure cases, but it's not due to the
easy-to-miss goto out above.

> +	/*
> +	 * The base device tree might have been damaged, erase its
> +	 * magic.
> +	 */
> +	fdt_set_magic(fdt, ~0);
> +
> +out:
> +	/*
> +	 * The overlay has been damaged, erase its magic.
> +	 */
> +	fdt_set_magic(fdto, ~0);
> +
> +	return ret;
> +}

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]