[PATCH 0/4] libfdt: Add support for device tree overlays

[PATCH 0/4] libfdt: Add support for device tree overlays

[Maxime Ripard]

Hi,

The device tree overlays are a great solution to the issue raised by
the bunch expandable boards we find everywhere these days, like the
Beaglebone, Raspberry Pi or CHIP.

Although for now Linux is the only available tool that can deal with
overlays, some other components like bootloaders or user-space tools
might need to apply these overlays on top of a base device tree.

To address these use cases, we introduce a new function to the libfdt,
fdt_overlay_apply, that does just that.

You can find a test program here: http://code.bulix.org/792zum-99476?raw

Let me know what you think!
Maxime

Maxime Ripard (3):
  libfdt: Add iterator over properties
  libfdt: Add max phandle retrieval function
  libfdt: Add overlay application function

Thierry Reding (1):
  libfdt: Add a subnodes iterator macro

 libfdt/Makefile.libfdt |   2 +-
 libfdt/fdt_overlay.c   | 414 +++++++++++++++++++++++++++++++++++++++++++++++++
 libfdt/fdt_ro.c        |  15 ++
 libfdt/libfdt.h        |  92 +++++++++++
 libfdt/libfdt_env.h    |   1 +
 5 files changed, 523 insertions(+), 1 deletion(-)
 create mode 100644 libfdt/fdt_overlay.c

-- 
2.8.2

[PATCH] fdt: Add a subnodes iterator macro

[Maxime Ripard]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The fdt_for_each_subnode() iterator macro provided by this patch can be
used to iterate over a device tree node's subnodes. At each iteration a
loop variable will be set to the next subnode.

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 libfdt/libfdt.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 2dfc6d9e5ce7..f3cbb637be41 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -163,6 +163,31 @@ int fdt_first_subnode(const void *fdt, int offset);
  */
 int fdt_next_subnode(const void *fdt, int offset);
 
+/**
+ * fdt_for_each_subnode - iterate over all subnodes of a parent
+ *
+ * This is actually a wrapper around a for loop and would be used like so:
+ *
+ *	fdt_for_each_subnode(fdt, node, parent) {
+ *		...
+ *		use node
+ *		...
+ *	}
+ *
+ * Note that this is implemented as a macro and node is used as iterator in
+ * the loop. It should therefore be a locally allocated variable. The parent
+ * variable on the other hand is never modified, so it can be constant or
+ * even a literal.
+ *
+ * @fdt:	FDT blob (const void *)
+ * @node:	child node (int)
+ * @parent:	parent node (int)
+ */
+#define fdt_for_each_subnode(fdt, node, parent)		\
+	for (node = fdt_first_subnode(fdt, parent);	\
+	     node >= 0;					\
+	     node = fdt_next_subnode(fdt, node))
+
 /**********************************************************************/
 /* General functions                                                  */
 /**********************************************************************/
-- 
2.8.2

[PATCH 1/4] libfdt: Add a subnodes iterator macro

[Maxime Ripard]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The fdt_for_each_subnode() iterator macro provided by this patch can be
used to iterate over a device tree node's subnodes. At each iteration a
loop variable will be set to the next subnode.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 libfdt/libfdt.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 36222fd4a6f4..331f32dd04b5 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -168,6 +168,31 @@ int fdt_first_subnode(const void *fdt, int offset);
  */
 int fdt_next_subnode(const void *fdt, int offset);
 
+/**
+ * fdt_for_each_subnode - iterate over all subnodes of a parent
+ *
+ * This is actually a wrapper around a for loop and would be used like so:
+ *
+ *	fdt_for_each_subnode(fdt, node, parent) {
+ *		...
+ *		use node
+ *		...
+ *	}
+ *
+ * Note that this is implemented as a macro and node is used as iterator in
+ * the loop. It should therefore be a locally allocated variable. The parent
+ * variable on the other hand is never modified, so it can be constant or
+ * even a literal.
+ *
+ * @fdt:	FDT blob (const void *)
+ * @node:	child node (int)
+ * @parent:	parent node (int)
+ */
+#define fdt_for_each_subnode(fdt, node, parent)		\
+	for (node = fdt_first_subnode(fdt, parent);	\
+	     node >= 0;					\
+	     node = fdt_next_subnode(fdt, node))
+
 /**********************************************************************/
 /* General functions                                                  */
 /**********************************************************************/
-- 
2.8.2

Re: [PATCH 1/4] libfdt: Add a subnodes iterator macro

[David Gibson]

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

On Fri, May 27, 2016 at 10:28:46AM +0200, Maxime Ripard wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The fdt_for_each_subnode() iterator macro provided by this patch can be
> used to iterate over a device tree node's subnodes. At each iteration a
> loop variable will be set to the next subnode.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Looks good, but needs a testcase.  Extending subnode_iterate would
probably make sense.

> ---
>  libfdt/libfdt.h | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 36222fd4a6f4..331f32dd04b5 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -168,6 +168,31 @@ int fdt_first_subnode(const void *fdt, int offset);
>   */
>  int fdt_next_subnode(const void *fdt, int offset);
>  
> +/**
> + * fdt_for_each_subnode - iterate over all subnodes of a parent
> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_subnode(fdt, node, parent) {
> + *		...
> + *		use node
> + *		...
> + *	}
> + *
> + * Note that this is implemented as a macro and node is used as iterator in
> + * the loop. It should therefore be a locally allocated variable. The parent
> + * variable on the other hand is never modified, so it can be constant or
> + * even a literal.
> + *
> + * @fdt:	FDT blob (const void *)
> + * @node:	child node (int)
> + * @parent:	parent node (int)
> + */
> +#define fdt_for_each_subnode(fdt, node, parent)		\
> +	for (node = fdt_first_subnode(fdt, parent);	\
> +	     node >= 0;					\
> +	     node = fdt_next_subnode(fdt, node))
> +
>  /**********************************************************************/
>  /* General functions                                                  */
>  /**********************************************************************/

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

Re: [PATCH 1/4] libfdt: Add a subnodes iterator macro

[David Gibson]

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

On Tue, May 31, 2016 at 10:47:19AM +1000, David Gibson wrote:
> On Fri, May 27, 2016 at 10:28:46AM +0200, Maxime Ripard wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > The fdt_for_each_subnode() iterator macro provided by this patch can be
> > used to iterate over a device tree node's subnodes. At each iteration a
> > loop variable will be set to the next subnode.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> Looks good, but needs a testcase.  Extending subnode_iterate would
> probably make sense.

Sorry, one other thing, as I noted on 2/4, I prefer the convention
where the loop counter (so the subnode offset in this case) is the
first argument to foreach style macros.

> 
> > ---
> >  libfdt/libfdt.h | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index 36222fd4a6f4..331f32dd04b5 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -168,6 +168,31 @@ int fdt_first_subnode(const void *fdt, int offset);
> >   */
> >  int fdt_next_subnode(const void *fdt, int offset);
> >  
> > +/**
> > + * fdt_for_each_subnode - iterate over all subnodes of a parent
> > + *
> > + * This is actually a wrapper around a for loop and would be used like so:
> > + *
> > + *	fdt_for_each_subnode(fdt, node, parent) {
> > + *		...
> > + *		use node
> > + *		...
> > + *	}
> > + *
> > + * Note that this is implemented as a macro and node is used as iterator in
> > + * the loop. It should therefore be a locally allocated variable. The parent
> > + * variable on the other hand is never modified, so it can be constant or
> > + * even a literal.
> > + *
> > + * @fdt:	FDT blob (const void *)
> > + * @node:	child node (int)
> > + * @parent:	parent node (int)
> > + */
> > +#define fdt_for_each_subnode(fdt, node, parent)		\
> > +	for (node = fdt_first_subnode(fdt, parent);	\
> > +	     node >= 0;					\
> > +	     node = fdt_next_subnode(fdt, node))
> > +
> >  /**********************************************************************/
> >  /* General functions                                                  */
> >  /**********************************************************************/
> 



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

[PATCH 2/4] libfdt: Add iterator over properties

[Maxime Ripard]

Implement a macro based on fdt_first_property_offset and
fdt_next_property_offset that provides a convenience to iterate over all
the properties of a given node.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 libfdt/libfdt.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 331f32dd04b5..6589e08db7ab 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -456,6 +456,30 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset);
 int fdt_next_property_offset(const void *fdt, int offset);
 
 /**
+ * fdt_for_each_property - iterate over all properties of a node
+ * @fdt:	FDT blob (const void *)
+ * @node:	node offset (int)
+ * @property:	property offset (int)
+ *
+ * This is actually a wrapper around a for loop and would be used like so:
+ *
+ *	fdt_for_each_property(fdt, node, property) {
+ *		...
+ *		use property
+ *		...
+ *	}
+ *
+ * Note that this is implemented as a macro and property is used as
+ * iterator in the loop. It should therefore be a locally allocated
+ * variable. The node variable on the other hand is never modified, so
+ * it can be constant or even a literal.
+ */
+#define fdt_for_each_property(fdt, node, property)		\
+	for (property = fdt_first_property_offset(fdt, node);	\
+	     property >= 0;					\
+	     property = fdt_next_property_offset(fdt, property))
+
+/**
  * fdt_get_property_by_offset - retrieve the property at a given offset
  * @fdt: pointer to the device tree blob
  * @offset: offset of the property to retrieve
-- 
2.8.2

Re: [PATCH 2/4] libfdt: Add iterator over properties

[David Gibson]

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

On Fri, May 27, 2016 at 10:28:47AM +0200, Maxime Ripard wrote:
> Implement a macro based on fdt_first_property_offset and
> fdt_next_property_offset that provides a convenience to iterate over all
> the properties of a given node.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Looks, ok but a couple of nits:

1) Needs a testcase.

2) I prefer the convention that in for_each_XX style macros, the first
argument should be the "loop counter" (i.e. the property offset in
this case).

3) Please make the offset parameter called 'offset' or
'property_offset', and change the macro name to
'fdt_for_each_property_offset'.  I know this is pedantic, but the
naming convention is designed to repeatedly remind the user that
offsets are offsets and so *cannot* be used as persistent handles.

> ---
>  libfdt/libfdt.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 331f32dd04b5..6589e08db7ab 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -456,6 +456,30 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset);
>  int fdt_next_property_offset(const void *fdt, int offset);
>  
>  /**
> + * fdt_for_each_property - iterate over all properties of a node
> + * @fdt:	FDT blob (const void *)
> + * @node:	node offset (int)
> + * @property:	property offset (int)
> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_property(fdt, node, property) {
> + *		...
> + *		use property
> + *		...
> + *	}
> + *
> + * Note that this is implemented as a macro and property is used as
> + * iterator in the loop. It should therefore be a locally allocated
> + * variable. The node variable on the other hand is never modified, so
> + * it can be constant or even a literal.
> + */
> +#define fdt_for_each_property(fdt, node, property)		\
> +	for (property = fdt_first_property_offset(fdt, node);	\
> +	     property >= 0;					\
> +	     property = fdt_next_property_offset(fdt, property))
> +
> +/**
>   * fdt_get_property_by_offset - retrieve the property at a given offset
>   * @fdt: pointer to the device tree blob
>   * @offset: offset of the property to retrieve

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

[PATCH 3/4] libfdt: Add max phandle retrieval function

[Maxime Ripard]

Add a function to retrieve the highest phandle in a given device tree.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 libfdt/fdt_ro.c | 15 +++++++++++++++
 libfdt/libfdt.h | 13 +++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 50cce864283c..d5a14178190f 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -88,6 +88,21 @@ static int _fdt_string_eq(const void *fdt, int stroffset,
 	return (strlen(p) == len) && (memcmp(p, s, len) == 0);
 }
 
+uint32_t fdt_get_max_phandle(const void *fdt)
+{
+	uint32_t max_phandle = 0, phandle;
+	int offset;
+
+	for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
+	     offset = fdt_next_node(fdt, offset, NULL)) {
+		phandle = fdt_get_phandle(fdt, offset);
+		if (phandle > max_phandle)
+			max_phandle = phandle;
+	}
+
+	return max_phandle;
+}
+
 int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
 {
 	FDT_CHECK_HEADER(fdt);
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 6589e08db7ab..7156b264b1e9 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -284,6 +284,19 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
 const char *fdt_string(const void *fdt, int stroffset);
 
 /**
+ * fdt_get_max_phandle - retrieves the highest phandle in a tree
+ * @fdt: pointer to the device tree blob
+ *
+ * fdt_get_max_phandle retrieves the highest phandlle in the given
+ * device tree
+ *
+ * returns:
+ *      the highest phandle on success
+ *      0, if an error occured
+ */
+uint32_t fdt_get_max_phandle(const void *fdt);
+
+/**
  * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
  * @fdt: pointer to the device tree blob
  *
-- 
2.8.2

Re: [PATCH 3/4] libfdt: Add max phandle retrieval function

[David Gibson]

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

On Fri, May 27, 2016 at 10:28:48AM +0200, Maxime Ripard wrote:
> Add a function to retrieve the highest phandle in a given device tree.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Needs a testcase, plus a couple of smallish problems:

> ---
>  libfdt/fdt_ro.c | 15 +++++++++++++++
>  libfdt/libfdt.h | 13 +++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 50cce864283c..d5a14178190f 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -88,6 +88,21 @@ static int _fdt_string_eq(const void *fdt, int stroffset,
>  	return (strlen(p) == len) && (memcmp(p, s, len) == 0);
>  }
>  
> +uint32_t fdt_get_max_phandle(const void *fdt)
> +{
> +	uint32_t max_phandle = 0, phandle;
> +	int offset;
> +
> +	for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
> +	     offset = fdt_next_node(fdt, offset, NULL)) {
> +		phandle = fdt_get_phandle(fdt, offset);

I think you need to check for the case of an invalid -1 valued phandle
- otherwise if there are any of those, it will come up as the max
(unsigned comparison).

> +		if (phandle > max_phandle)
> +			max_phandle = phandle;
> +	}

You need to check for offset != -FDT_ERR_NOTFOUND, which would
indicate some other error from fdt_next_node.

> +
> +	return max_phandle;
> +}
> +
>  int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
>  {
>  	FDT_CHECK_HEADER(fdt);
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 6589e08db7ab..7156b264b1e9 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -284,6 +284,19 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
>  const char *fdt_string(const void *fdt, int stroffset);
>  
>  /**
> + * fdt_get_max_phandle - retrieves the highest phandle in a tree
> + * @fdt: pointer to the device tree blob
> + *
> + * fdt_get_max_phandle retrieves the highest phandlle in the given
> + * device tree
> + *
> + * returns:
> + *      the highest phandle on success
> + *      0, if an error occured
> + */
> +uint32_t fdt_get_max_phandle(const void *fdt);
> +
> +/**
>   * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
>   * @fdt: pointer to the device tree blob
>   *

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

[PATCH 4/4] libfdt: Add overlay application function

[Maxime Ripard]

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>
---
 libfdt/Makefile.libfdt |   2 +-
 libfdt/fdt_overlay.c   | 414 +++++++++++++++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h        |  30 ++++
 libfdt/libfdt_env.h    |   1 +
 4 files changed, 446 insertions(+), 1 deletion(-)
 create mode 100644 libfdt/fdt_overlay.c

diff --git a/libfdt/Makefile.libfdt b/libfdt/Makefile.libfdt
index 09c322ed82ba..098b3f36e668 100644
--- a/libfdt/Makefile.libfdt
+++ b/libfdt/Makefile.libfdt
@@ -7,5 +7,5 @@ LIBFDT_soname = libfdt.$(SHAREDLIB_EXT).1
 LIBFDT_INCLUDES = fdt.h libfdt.h libfdt_env.h
 LIBFDT_VERSION = version.lds
 LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c fdt_empty_tree.c \
-	fdt_addresses.c
+	fdt_addresses.c fdt_overlay.c
 LIBFDT_OBJS = $(LIBFDT_SRCS:%.c=%.o)
diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
new file mode 100644
index 000000000000..1e68e903c734
--- /dev/null
+++ b/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;
+
+		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
+	 */
+	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;
+}
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 7156b264b1e9..ebee808a7f92 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1716,6 +1716,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/libfdt/libfdt_env.h b/libfdt/libfdt_env.h
index 9dea97dfff81..99f936dacc60 100644
--- a/libfdt/libfdt_env.h
+++ b/libfdt/libfdt_env.h
@@ -54,6 +54,7 @@
 
 #include <stddef.h>
 #include <stdint.h>
+#include <stdlib.h>
 #include <string.h>
 
 #ifdef __CHECKER__
-- 
2.8.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] libfdt: Add overlay application function

[David Gibson]

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

On Fri, May 27, 2016 at 10:28:49AM +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>
> ---
>  libfdt/Makefile.libfdt |   2 +-
>  libfdt/fdt_overlay.c   | 414 +++++++++++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h        |  30 ++++
>  libfdt/libfdt_env.h    |   1 +
>  4 files changed, 446 insertions(+), 1 deletion(-)
>  create mode 100644 libfdt/fdt_overlay.c
> 
> diff --git a/libfdt/Makefile.libfdt b/libfdt/Makefile.libfdt
> index 09c322ed82ba..098b3f36e668 100644
> --- a/libfdt/Makefile.libfdt
> +++ b/libfdt/Makefile.libfdt
> @@ -7,5 +7,5 @@ LIBFDT_soname = libfdt.$(SHAREDLIB_EXT).1
>  LIBFDT_INCLUDES = fdt.h libfdt.h libfdt_env.h
>  LIBFDT_VERSION = version.lds
>  LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c fdt_empty_tree.c \
> -	fdt_addresses.c
> +	fdt_addresses.c fdt_overlay.c
>  LIBFDT_OBJS = $(LIBFDT_SRCS:%.c=%.o)
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> new file mode 100644
> index 000000000000..1e68e903c734
> --- /dev/null
> +++ b/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)

I'd prefer to see 'node' renamed to indicate what node is needed
here.  Maybe 'fragment' as below.

Btw, static functions don't need the fdt_ prefix, and are probably
better off without it, since it provides an additional visual hint
that a function is not exported.

> +{
> +	const uint32_t *val;
> +	int len;
> +
> +	val = fdt_getprop(fdto, node, "target", &len);
> +	if (!val || (len != sizeof(*val)))
> +		return 0;

For consistency, it's probably worth returning 0 in all failure cases,
including (val == 0xffffffff).

> +	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) {

There's no need for this loop - you can just use two
fdt_setprop_inplace() calls, ignoring FDT_ERR_NOTFOUND (as long as
only one of them returns that).

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

fdt_path_offset(xx, "/") is *never* necessary.  The offset of the root
node is always 0 by definition.

> +
> +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
> +}
> +
> +static int _fdt_overlay_update_local_references(void *fdto,

Don't use _ prefixes on function names - they're reserved in C (the
kernel gets away with it because it controls the whole environment).

> +						int tree_node,
> +						int fixup_node,
> +						uint32_t delta)
> +{
> +	int fixup_prop;
> +	int fixup_child;

This is pretty complicated, but that's mostly due to the overly
complicated encoding of local fixups in the overlay format.  I've been
discussing that elsewhere.

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

Hrm.. phandle properties in the fixups node would suggest a broken
overlay, but this is probably safest.

> +		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;
> +		}

You already have the name so you can just use getprop() (or
get_property()) rather than explicitly iterating through the
properties.

> +
> +		if (!val || !tree_len)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (!tree_name)
> +			return -FDT_ERR_NOTFOUND;

You don't seem to test for the case that the loop across properties
never hit a match - this doesn't catch it, since tree_name could still
have a value from the last iteration.  I don't really see what the
point of checking tree_name here is at all.

> +		if (tree_len != 4)
> +			return -FDT_ERR_NOTFOUND;

This is bogus.  Any property can include a reference to a phandle, not
just a 4-byte property which has *only* a reference to a phandle.  To
correctly apply the fixup you need to parse the contents of the
property in the __local__fixups__ subtree to determine the offset
within the property that needs adjustment.

> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
> +					      adj_val);

Likewise this needs adjustment to handle non-cell properties.

> +		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;
> +		}

Again, you can just use fdt_get_subnode() to find the matching node of
the main tree rather than manually iterating.

> +
> +		_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;

As above, no need for path_offset /.

> +	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);

libfdt isn't supposed to use malloc(), since it's designed to work in
restricted environments like tiny bootloaders which may not have an
allocator.



> +	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;
> +	}

This needs fixing - newer things should use -1 instead of 0xdeadbeef
as the placeholder value (since 0xdeadbeef is a valid phandle)

> +
> +	fixup_val[index] = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop_inplace(fdto, fixup_off,
> +				  name, fixup_val,
> +				  prop_len);

Looks like we want a setprop_inplace_partial() or something to avoid
the need to malloc() a whole extra copy of the property just to alter
4 bytes.

> +
> +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);

Hm, I think you should be able to roll the two loops into one.

> +
> +	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);

Again, no malloc().

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

the *_namelen() functions are designed specifically to avoid this
fiddly dicking around with making copies of strings and inserting
extra \0 characters.  Using those should let you avoid the malloc()
and copies.

Looks like a path_offset_namelen() might need to be added.

> +
> +		index = strtol(prop_tmp, NULL, 10);
> +		prop_tmp += strlen(prop_tmp) + 1;

strtol() is a new dependency for libfdt.  That's ok, but it should be
noted in libfdt_env.h.

Also it should probably be strtoul() since offsets are necessarily
non-negative.

..and you should pass an endptr value so you can check for errors.

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

In general I'd prefer to see parameters that relate to the base DT
grouped together and parameters that relate to the overlay DT group
together.  So in this case: (dt, target, dto, 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;

You need to ignore FDT_ERR_EXISTS here, since overlays are allowed to
update existing nodes.

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

No path_offset(/)

> +	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
> +	 */
> +	fdto_copy = malloc(fdt_totalsize(fdto));

Again, no malloc() in libfdt - let the caller do allocation if they
want.

> +	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;
> +}
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 7156b264b1e9..ebee808a7f92 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1716,6 +1716,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/libfdt/libfdt_env.h b/libfdt/libfdt_env.h
> index 9dea97dfff81..99f936dacc60 100644
> --- a/libfdt/libfdt_env.h
> +++ b/libfdt/libfdt_env.h
> @@ -54,6 +54,7 @@
>  
>  #include <stddef.h>
>  #include <stdint.h>
> +#include <stdlib.h>
>  #include <string.h>
>  
>  #ifdef __CHECKER__

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