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

[PATCH v2 0/6] 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

This is the last patches sent to U-boot, with the modifications
suggested by David, and the additional test cases.

Let me know what you think!
Maxime

Changes from U-Boot v4:
  - Added test cases for the functions
  - Changed the fdt_for_each_subnode argument order
  - Don't error out if -1 phandle is found in fdt_get_max_phandle
  - Used libfdt's fdt_path_offset_namelen

Changes from U-Boot v3:
  - Moved the patch to introduce fdt_getprop_namelen_w earlier to keep
    bisectability
  - Renamed fdt_setprop_inplace_namelen_by_index in
    fdt_setprop_inplace_namelen_partial
  - Reintroduced the check on the property length in fdt_setprop_inplace
  - Made sure the code was taking the non 32bits-aligned phandles
  - Used memchr instead of strchr in the fixup parsing code, and made
    sure the cases where the fixup format was wrong was reported as an
    error.
  - Fixed a bug where a property in a overlay having multiple phandles
    local to that overlay would only resolve the first one. Also added
    a test case for this
  - Added kerneldocs for all the overlay functions
  - Added a patch to fix a typo in separator
  - A few fixes, function renamings, error checking changes here and there

Changes from U-boot v2 / libfdt v1
  - Reworked the code to deal with Pantelis and David numerous
    comments, among which:
    * Remove the need for malloc in the overlay code, and added some
      libfdt functions to do that
    * Remove the DT magic in case of an error to not be able to use it
      anymore
    * Removed the fdt_ and _ function prefix for the static functions
    * Plus the usual bunch of rework, error checking and optimizations.				    


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

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

 libfdt/Makefile.libfdt     |   2 +-
 libfdt/fdt_overlay.c       | 618 +++++++++++++++++++++++++++++++++++++++++++++
 libfdt/fdt_ro.c            |  26 ++
 libfdt/fdt_wip.c           |  29 ++-
 libfdt/libfdt.h            | 121 +++++++++
 libfdt/libfdt_env.h        |   1 +
 tests/.gitignore           |   2 +
 tests/Makefile.tests       |   4 +-
 tests/get_phandle.c        |   6 +
 tests/overlay.c            | 228 +++++++++++++++++
 tests/overlay_base.dts     |  21 ++
 tests/overlay_overlay.dts  |  96 +++++++
 tests/property_iterate.c   |  97 +++++++
 tests/property_iterate.dts |  24 ++
 tests/run_tests.sh         |  11 +
 tests/subnode_iterate.c    |   8 +-
 16 files changed, 1282 insertions(+), 12 deletions(-)
 create mode 100644 libfdt/fdt_overlay.c
 create mode 100644 tests/overlay.c
 create mode 100644 tests/overlay_base.dts
 create mode 100644 tests/overlay_overlay.dts
 create mode 100644 tests/property_iterate.c
 create mode 100644 tests/property_iterate.dts

-- 
2.9.0

[PATCH v2 1/6] 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 +++++++++++++++++++++++++
 tests/subnode_iterate.c |  8 ++------
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 36222fd4a6f4..0cf46872b54e 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.
+ *
+ * @node:	child node (int)
+ * @fdt:	FDT blob (const void *)
+ * @parent:	parent node (int)
+ */
+#define fdt_for_each_subnode(node, fdt, parent)		\
+	for (node = fdt_first_subnode(fdt, parent);	\
+	     node >= 0;					\
+	     node = fdt_next_subnode(fdt, node))
+
 /**********************************************************************/
 /* General functions                                                  */
 /**********************************************************************/
diff --git a/tests/subnode_iterate.c b/tests/subnode_iterate.c
index b9f379d5963c..0fb5c901ebd7 100644
--- a/tests/subnode_iterate.c
+++ b/tests/subnode_iterate.c
@@ -48,9 +48,7 @@ static void test_node(void *fdt, int parent_offset)
 	subnodes = cpu_to_fdt32(*prop);
 
 	count = 0;
-	for (offset = fdt_first_subnode(fdt, parent_offset);
-	     offset >= 0;
-	     offset = fdt_next_subnode(fdt, offset))
+	fdt_for_each_subnode(offset, fdt, parent_offset)
 		count++;
 
 	if (count != subnodes) {
@@ -65,9 +63,7 @@ static void check_fdt_next_subnode(void *fdt)
 	int offset;
 	int count = 0;
 
-	for (offset = fdt_first_subnode(fdt, 0);
-	     offset >= 0;
-	     offset = fdt_next_subnode(fdt, offset)) {
+	fdt_for_each_subnode(offset, fdt, 0) {
 		test_node(fdt, offset);
 		count++;
 	}
-- 
2.9.0

[PATCH v2 2/6] 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>
Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 libfdt/libfdt.h            | 24 ++++++++++++
 tests/.gitignore           |  1 +
 tests/Makefile.tests       |  1 +
 tests/property_iterate.c   | 97 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/property_iterate.dts | 24 ++++++++++++
 tests/run_tests.sh         |  3 ++
 6 files changed, 150 insertions(+)
 create mode 100644 tests/property_iterate.c
 create mode 100644 tests/property_iterate.dts

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 0cf46872b54e..9d3c9b234274 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -455,6 +455,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
+ * @property_offset:	property offset (int)
+ * @fdt:		FDT blob (const void *)
+ * @node:		node 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_offset(property, fdt, node)	\
+	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
diff --git a/tests/.gitignore b/tests/.gitignore
index e4532da30bf5..fa4616ba28c2 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -40,6 +40,7 @@ tmp.*
 /path_offset
 /path_offset_aliases
 /phandle_format
+/property_iterate
 /propname_escapes
 /references
 /root_node
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index f7c3a4b00ead..196518c83eda 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -23,6 +23,7 @@ LIB_TESTS_L = get_mem_rsv \
 	add_subnode_with_nops path_offset_aliases \
 	utilfdt_test \
 	integer-expressions \
+	property_iterate \
 	subnode_iterate
 LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
 
diff --git a/tests/property_iterate.c b/tests/property_iterate.c
new file mode 100644
index 000000000000..0f3959cb8c22
--- /dev/null
+++ b/tests/property_iterate.c
@@ -0,0 +1,97 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Tests that fdt_next_subnode() works as expected
+ *
+ * Copyright (C) 2013 Google, Inc
+ *
+ * Copyright (C) 2007 David Gibson, IBM Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+static void test_node(void *fdt, int parent_offset)
+{
+	fdt32_t properties;
+	const fdt32_t *prop;
+	int offset, property;
+	int count;
+	int len;
+
+	/*
+	 * This property indicates the number of properties in our
+	 * test node to expect
+	 */
+	prop = fdt_getprop(fdt, parent_offset, "test-properties", &len);
+	if (!prop || len != sizeof(fdt32_t)) {
+		FAIL("Missing/invalid test-properties property at '%s'",
+		     fdt_get_name(fdt, parent_offset, NULL));
+	}
+	properties = cpu_to_fdt32(*prop);
+
+	count = 0;
+	offset = fdt_first_subnode(fdt, parent_offset);
+	if (offset < 0)
+		FAIL("Missing test node\n");
+
+	fdt_for_each_property_offset(property, fdt, offset)
+		count++;
+
+	if (count != properties) {
+		FAIL("Node '%s': Expected %d properties, got %d\n",
+		     fdt_get_name(fdt, parent_offset, NULL), properties,
+		     count);
+	}
+}
+
+static void check_fdt_next_subnode(void *fdt)
+{
+	int offset;
+	int count = 0;
+
+	fdt_for_each_subnode(offset, fdt, 0) {
+		test_node(fdt, offset);
+		count++;
+	}
+
+	if (count != 2)
+		FAIL("Expected %d tests, got %d\n", 2, count);
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+
+	test_init(argc, argv);
+	if (argc != 2)
+		CONFIG("Usage: %s <dtb file>", argv[0]);
+
+	fdt = load_blob(argv[1]);
+	if (!fdt)
+		FAIL("No device tree available");
+
+	check_fdt_next_subnode(fdt);
+
+	PASS();
+}
diff --git a/tests/property_iterate.dts b/tests/property_iterate.dts
new file mode 100644
index 000000000000..2ed677e7e6ea
--- /dev/null
+++ b/tests/property_iterate.dts
@@ -0,0 +1,24 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	test1 {
+		test-properties = <3>;
+
+		test {
+			linux,phandle = <0x1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+
+	test2 {
+		test-properties = <0>;
+
+		test {
+		};
+	};
+};
+
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 7eb9b3d33108..6a2662b2abaf 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -269,6 +269,9 @@ libfdt_tests () {
     run_dtc_test -I dts -O dtb -o subnode_iterate.dtb subnode_iterate.dts
     run_test subnode_iterate subnode_iterate.dtb
 
+    run_dtc_test -I dts -O dtb -o property_iterate.dtb property_iterate.dts
+    run_test property_iterate property_iterate.dtb
+
     # Tests for behaviour on various sorts of corrupted trees
     run_test truncated_property
 
-- 
2.9.0

[PATCH v2 3/6] 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>
Reviewed-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 libfdt/fdt_ro.c     | 26 ++++++++++++++++++++++++++
 libfdt/libfdt.h     | 14 ++++++++++++++
 tests/get_phandle.c |  6 ++++++
 3 files changed, 46 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 50cce864283c..04590984bd51 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -88,6 +88,32 @@ 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;
+	int offset;
+
+	for (offset = fdt_next_node(fdt, -1, NULL);;
+	     offset = fdt_next_node(fdt, offset, NULL)) {
+		uint32_t phandle;
+
+		if (offset == -FDT_ERR_NOTFOUND)
+			return max_phandle;
+
+		if (offset < 0)
+			return (uint32_t)-1;
+
+		phandle = fdt_get_phandle(fdt, offset);
+		if (phandle == (uint32_t)-1)
+			continue;
+
+		if (phandle > max_phandle)
+			max_phandle = phandle;
+	}
+
+	return 0;
+}
+
 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 9d3c9b234274..812937fede44 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -283,6 +283,20 @@ 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 phandle in the given
+ * device tree
+ *
+ * returns:
+ *      the highest phandle on success
+ *      0, if no phandle was found in the device tree
+ *      -1, if an error occurred
+ */
+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
diff --git a/tests/get_phandle.c b/tests/get_phandle.c
index 2079591d4c49..22bd7b81b3f0 100644
--- a/tests/get_phandle.c
+++ b/tests/get_phandle.c
@@ -44,6 +44,7 @@ static void check_phandle(void *fdt, const char *path, uint32_t checkhandle)
 
 int main(int argc, char *argv[])
 {
+	uint32_t max;
 	void *fdt;
 
 	test_init(argc, argv);
@@ -53,5 +54,10 @@ int main(int argc, char *argv[])
 	check_phandle(fdt, "/subnode@2", PHANDLE_1);
 	check_phandle(fdt, "/subnode@2/subsubnode@0", PHANDLE_2);
 
+	max = fdt_get_max_phandle(fdt);
+	if (max != PHANDLE_2)
+		FAIL("fdt_get_max_phandle returned 0x%x instead of 0x%x\n",
+		     max, PHANDLE_2);
+
 	PASS();
 }
-- 
2.9.0

[PATCH v2 4/6] libfdt: Add fdt_getprop_namelen_w

[Maxime Ripard]

Add a function to retrieve a writeable property only by the first
characters of its name.

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

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 812937fede44..1da2c540b607 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -622,6 +622,13 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
  */
 const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 				const char *name, int namelen, int *lenp);
+static inline void *fdt_getprop_namelen_w(void *fdt, int nodeoffset,
+					  const char *name, int namelen,
+					  int *lenp)
+{
+	return (void *)(uintptr_t)fdt_getprop_namelen(fdt, nodeoffset, name,
+						      namelen, lenp);
+}
 
 /**
  * fdt_getprop - retrieve the value of a given property
-- 
2.9.0

[PATCH v2 5/6] libfdt: Add fdt_setprop_inplace_namelen_partial

[Maxime Ripard]

Add a function to modify inplace only a portion of a property..

This is especially useful when the property is an array of values, and you
want to update one of them without changing the DT size.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 libfdt/fdt_wip.c | 29 +++++++++++++++++++++++++----
 libfdt/libfdt.h  | 21 +++++++++++++++++++++
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
index c5bbb68d3273..fd53e0b67416 100644
--- a/libfdt/fdt_wip.c
+++ b/libfdt/fdt_wip.c
@@ -55,21 +55,42 @@
 
 #include "libfdt_internal.h"
 
+int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
+					const char *name, int namelen,
+					uint32_t index, const void *val,
+					int len)
+{
+	void *propval;
+	int proplen;
+
+	propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen,
+					&proplen);
+	if (!propval)
+		return proplen;
+
+	if (proplen < (len + index))
+		return -FDT_ERR_NOSPACE;
+
+	memcpy((unsigned char *)propval + index, val, len);
+	return 0;
+}
+
 int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
 			const void *val, int len)
 {
-	void *propval;
+	const void *propval;
 	int proplen;
 
-	propval = fdt_getprop_w(fdt, nodeoffset, name, &proplen);
+	propval = fdt_getprop(fdt, nodeoffset, name, &proplen);
 	if (! propval)
 		return proplen;
 
 	if (proplen != len)
 		return -FDT_ERR_NOSPACE;
 
-	memcpy(propval, val, len);
-	return 0;
+	return fdt_setprop_inplace_namelen_partial(fdt, nodeoffset, name,
+						   strlen(name), 0,
+						   val, len);
 }
 
 static void _fdt_nop_region(void *start, int len)
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 1da2c540b607..a22a16b6878a 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1072,6 +1072,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset);
 /* Write-in-place functions                                           */
 /**********************************************************************/
 
+/**
+ * fdt_setprop_inplace_namelen_partial - change a property's value,
+ *                                       but not its size
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node whose property to change
+ * @name: name of the property to change
+ * @namelen: number of characters of name to consider
+ * @index: index of the property to change in the array
+ * @val: pointer to data to replace the property value with
+ * @len: length of the property value
+ *
+ * Identical to fdt_setprop_inplace(), but modifies the given property
+ * starting from the given index, and using only the first characters
+ * of the name. It is useful when you want to manipulate only one value of
+ * an array and you have a string that doesn't end with \0.
+ */
+int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
+					const char *name, int namelen,
+					uint32_t index, const void *val,
+					int len);
+
 /**
  * fdt_setprop_inplace - change a property's value, but not its size
  * @fdt: pointer to the device tree blob
-- 
2.9.0

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

[PATCH v2 6/6] 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      | 618 ++++++++++++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h           |  30 +++
 libfdt/libfdt_env.h       |   1 +
 tests/.gitignore          |   1 +
 tests/Makefile.tests      |   3 +-
 tests/overlay.c           | 228 +++++++++++++++++
 tests/overlay_base.dts    |  21 ++
 tests/overlay_overlay.dts |  96 +++++++
 tests/run_tests.sh        |   8 +
 10 files changed, 1006 insertions(+), 2 deletions(-)
 create mode 100644 libfdt/fdt_overlay.c
 create mode 100644 tests/overlay.c
 create mode 100644 tests/overlay_base.dts
 create mode 100644 tests/overlay_overlay.dts

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..40ad3b86c13e
--- /dev/null
+++ b/libfdt/fdt_overlay.c
@@ -0,0 +1,618 @@
+#include "libfdt_env.h"
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "libfdt_internal.h"
+
+/**
+ * overlay_get_target_phandle - retrieves the target phandle of a fragment
+ * @fdto: pointer to the device tree overlay blob
+ * @fragment: node offset of the fragment in the overlay
+ *
+ * overlay_get_target_phandle() retrieves the target phandle of an
+ * overlay fragment when that fragment uses a phandle (target
+ * property) instead of a path (target-path property).
+ *
+ * returns:
+ *      the phandle pointed by the target property
+ *      0, if the phandle was not found
+ *	-1, if the phandle was malformed
+ */
+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)
+		return 0;
+
+	if ((*val == (uint32_t)-1) || (len != sizeof(*val)))
+		return (uint32_t)-1;
+
+	return fdt32_to_cpu(*val);
+}
+
+/**
+ * overlay_get_target - retrieves the target phandle of a fragment
+ * @fdt: Base device tree blob
+ * @fdto: Device tree overlay blob
+ * @fragment: node offset of the fragment in the overlay
+ *
+ * overlay_get_target() retrieves the target phandle in the base
+ * device tree of a fragment, no matter how the actual targetting is
+ * done (through a phandle or a path)
+ *
+ * returns:
+ *      the targetted node offset in the base device tree
+ *      Negative error code on error
+ */
+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 == (uint32_t)-1)
+		return -FDT_ERR_BADPHANDLE;
+
+	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);
+}
+
+/**
+ * overlay_phandle_add_offset - Increases a phandle by an offset
+ * @fdt: Base device tree blob
+ * @node: Device tree overlay blob
+ * @name: Name of the property to modify (phandle or linux,phandle)
+ * @delta: offset to apply
+ *
+ * overlay_phandle_add_offset() increments a node phandle by a given
+ * offset.
+ *
+ * returns:
+ *      0 on success.
+ *      Negative error code on error
+ */
+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);
+	if ((adj_val + delta) < adj_val)
+		return -FDT_ERR_BADPHANDLE;
+
+	adj_val += delta;
+	return fdt_setprop_inplace_u32(fdt, node, name, adj_val);
+}
+
+/**
+ * overlay_adjust_node_phandles - Offsets the phandles of a node
+ * @fdto: Device tree overlay blob
+ * @node: Offset of the node we want to adjust
+ * @delta: Offset to shift the phandles of
+ *
+ * overlay_adjust_node_phandles() adds a constant to all the phandles
+ * of a given node. This is mainly use as part of the overlay
+ * application process, when we want to update all the overlay
+ * phandles to not conflict with the overlays of the base device tree.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_adjust_node_phandles(void *fdto, int node,
+					uint32_t delta)
+{
+	unsigned char found = 0;
+	int child;
+	int ret;
+
+	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
+	if (ret && ret != -FDT_ERR_NOTFOUND)
+		return ret;
+
+	if (!ret)
+		found = 1;
+
+	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
+	if (ret && ret != -FDT_ERR_NOTFOUND)
+		return ret;
+
+	/*
+	 * If neither phandle nor linux,phandle have been found return
+	 * an error.
+	 */
+	if (!found && !ret)
+		return ret;
+
+	fdt_for_each_subnode(child, fdto, node)
+		overlay_adjust_node_phandles(fdto, child, delta);
+
+	return 0;
+}
+
+/**
+ * overlay_adjust_local_phandles - Adjust the phandles of a whole overlay
+ * @fdto: Device tree overlay blob
+ * @delta: Offset to shift the phandles of
+ *
+ * overlay_adjust_local_phandles() adds a constant to all the
+ * phandles of an overlay. This is mainly use as part of the overlay
+ * application process, when we want to update all the overlay
+ * phandles to not conflict with the overlays of the base device tree.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+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);
+}
+
+/**
+ * overlay_update_local_node_references - Adjust the overlay references
+ * @fdto: Device tree overlay blob
+ * @tree_node: Node offset of the node to operate on
+ * @fixup_node: Node offset of the matching local fixups node
+ * @delta: Offset to shift the phandles of
+ *
+ * overlay_update_local_nodes_references() update the phandles
+ * pointing to a node within the device tree overlay by adding a
+ * constant delta.
+ *
+ * This is mainly used as part of a device tree application process,
+ * where you want the device tree overlays phandles to not conflict
+ * with the ones from the base device tree before merging them.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+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 unsigned char *fixup_val, *tree_val;
+		const char *name;
+		int fixup_len;
+		int tree_len;
+		int i;
+
+		fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
+						  &name, &fixup_len);
+		if (!fixup_val)
+			return fixup_len;
+
+		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
+		if (!tree_val)
+			return tree_len;
+
+		for (i = 0; i < fixup_len; i += sizeof(uint32_t)) {
+			uint32_t adj_val, index;
+
+			index = *(const uint32_t *)(fixup_val + i);
+			index = fdt32_to_cpu(index);
+
+			/*
+			 * phandles to fixup can be unaligned.
+			 *
+			 * Use a memcpy for the architectures that do
+			 * not support unaligned accesses.
+			 */
+			memcpy(&adj_val, tree_val + index, sizeof(uint32_t));
+
+			adj_val = fdt32_to_cpu(adj_val);
+			adj_val += delta;
+			adj_val = cpu_to_fdt32(adj_val);
+
+			ret = fdt_setprop_inplace_namelen_partial(fdto,
+								  tree_node,
+								  name,
+								  strlen(name),
+								  index,
+								  &adj_val,
+								  sizeof(adj_val));
+			if (ret)
+				return ret;
+		}
+	}
+
+	fdt_for_each_subnode(fixup_child, fdto, 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;
+}
+
+/**
+ * overlay_update_local_references - Adjust the overlay references
+ * @fdto: Device tree overlay blob
+ * @delta: Offset to shift the phandles of
+ *
+ * overlay_update_local_references() update all the phandles pointing
+ * to a node within the device tree overlay by adding a constant
+ * delta to not conflict with the base overlay.
+ *
+ * This is mainly used as part of a device tree application process,
+ * where you want the device tree overlays phandles to not conflict
+ * with the ones from the base device tree before merging them.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_update_local_references(void *fdto, uint32_t delta)
+{
+	int fixups;
+
+	fixups = fdt_path_offset(fdto, "/__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(fdto, 0, fixups,
+						    delta);
+}
+
+/**
+ * overlay_fixup_one_phandle - Set an overlay phandle to the base one
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ * @symbols_off: Node offset of the symbols node in the base device tree
+ * @path: Path to a node holding a phandle in the overlay
+ * @path_len: number of path characters to consider
+ * @name: Name of the property holding the phandle reference in the overlay
+ * @name_len: number of name characters to consider
+ * @index: Index in the overlay property where the phandle is stored
+ * @label: Label of the node referenced by the phandle
+ *
+ * overlay_fixup_one_phandle() resolves an overlay phandle pointing to
+ * a node in the base device tree.
+ *
+ * This is part of the device tree overlay application process, when
+ * you want all the phandles in the overlay to point to the actual
+ * base dt nodes.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+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_partial(fdto, fixup_off,
+						   name, name_len, index,
+						   &phandle, sizeof(phandle));
+};
+
+/**
+ * overlay_fixup_phandle - Set an overlay phandle to the base one
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ * @symbols_off: Node offset of the symbols node in the base device tree
+ * @property: Property offset in the overlay holding the list of fixups
+ *
+ * overlay_fixup_phandle() resolves all the overlay phandles pointed
+ * to in a __local_fixup__ property, and updates them to match the
+ * phandles in use in the base device tree.
+ *
+ * This is part of the device tree overlay application process, when
+ * you want all the phandles in the overlay to point to the actual
+ * base dt nodes.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+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 prop_len = strlen(value);
+		uint32_t path_len, name_len;
+		char *sep, *endptr;
+		int index;
+		int ret;
+
+		path = prop_string;
+		sep = memchr(prop_string, ':', prop_len);
+		if (*sep != ':')
+			return -FDT_ERR_BADSTRUCTURE;
+		path_len = sep - path;
+
+		name = sep + 1;
+		sep = memchr(name, ':', prop_len);
+		if (*sep != ':')
+			return -FDT_ERR_BADSTRUCTURE;
+		name_len = sep - name;
+
+		index = strtoul(sep + 1, &endptr, 10);
+		if ((*endptr != '\0') || (endptr <= (sep + 1)))
+			return -FDT_ERR_BADSTRUCTURE;
+
+		len -= prop_len + 1;
+		value += prop_len + 1;
+
+		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;
+}
+
+/**
+ * overlay_fixup_phandles - Resolve the overlay phandles to the base
+ *                          device tree
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ *
+ * overlay_fixup_phandles() resolves all the overlay phandles pointing
+ * to nodes in the base device tree.
+ *
+ * This is one of the steps of the device tree overlay application
+ * process, when you want all the phandles in the overlay to point to
+ * the actual base dt nodes.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_fixup_phandles(void *fdt, void *fdto)
+{
+	int fixups_off, symbols_off;
+	int property;
+
+	symbols_off = fdt_path_offset(fdt, "/__symbols__");
+	fixups_off = fdt_path_offset(fdto, "/__fixups__");
+
+	fdt_for_each_property_offset(property, fdto, fixups_off) {
+		int ret;
+
+		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * overlay_apply_node - Merge an overlay fragment into the base device tree
+ * @fdt: Base Device Tree blob
+ * @target: Node offset in the base device tree to apply the fragment to
+ * @fdto: Device tree overlay blob
+ * @fragment: Node offset in the overlay holding the changes to merge
+ *
+ * overlay_apply_node() merges an overlay fragment into a target base
+ * device tree node pointed.
+ *
+ * This is part of the final step in the device tree overlay
+ * application process, when all the phandles have been adjusted and
+ * resolved and you just have to merge overlay into the base device
+ * tree.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_apply_node(void *fdt, int target,
+			      void *fdto, int fragment)
+{
+	int property;
+	int node;
+
+	fdt_for_each_property_offset(property, fdto, fragment) {
+		const char *name;
+		const void *prop;
+		int prop_len;
+		int ret;
+
+		prop = fdt_getprop_by_offset(fdto, property, &name,
+					     &prop_len);
+		if (prop_len == -FDT_ERR_NOTFOUND)
+			return -FDT_ERR_INTERNAL;
+		if (prop_len < 0)
+			return prop_len;
+
+		ret = fdt_setprop(fdt, target, name, prop, prop_len);
+		if (ret)
+			return ret;
+	}
+
+	fdt_for_each_subnode(node, fdto, fragment) {
+		const char *name = fdt_get_name(fdto, node, NULL);
+		int nnode;
+		int ret;
+
+		nnode = fdt_add_subnode(fdt, target, name);
+		if (nnode == -FDT_ERR_EXISTS)
+			nnode = fdt_subnode_offset(fdt, target, name);
+
+		if (nnode < 0)
+			return nnode;
+
+		ret = overlay_apply_node(fdt, nnode, fdto, node);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * overlay_merge - Merge an overlay into its base device tree
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ *
+ * overlay_merge() merges an overlay into its base device tree.
+ *
+ * This is the final step in the device tree overlay application
+ * process, when all the phandles have been adjusted and resolved and
+ * you just have to merge overlay into the base device tree.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_merge(void *fdt, void *fdto)
+{
+	int fragment;
+
+	fdt_for_each_subnode(fragment, fdto, 0) {
+		int overlay;
+		int target;
+		int ret;
+
+		target = overlay_get_target(fdt, fdto, fragment);
+		if (target < 0)
+			continue;
+
+		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+		if (overlay < 0)
+			return overlay;
+
+		ret = overlay_apply_node(fdt, target, fdto, 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 err;
+
+	/*
+	 * The overlay has been damaged, erase its magic.
+	 */
+	fdt_set_magic(fdto, ~0);
+
+	return 0;
+
+err:
+	/*
+	 * The overlay might have been damaged, erase its magic.
+	 */
+	fdt_set_magic(fdto, ~0);
+
+	/*
+	 * The base device tree might have been damaged, erase its
+	 * magic.
+	 */
+	fdt_set_magic(fdt, ~0);
+
+	return ret;
+}
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index a22a16b6878a..a70b8767cabc 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1745,6 +1745,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__
diff --git a/tests/.gitignore b/tests/.gitignore
index fa4616ba28c2..6c97066c587e 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -35,6 +35,7 @@ tmp.*
 /nopulate
 /notfound
 /open_pack
+/overlay
 /parent_offset
 /path-references
 /path_offset
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index 196518c83eda..4aefbbd15253 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -24,7 +24,8 @@ LIB_TESTS_L = get_mem_rsv \
 	utilfdt_test \
 	integer-expressions \
 	property_iterate \
-	subnode_iterate
+	subnode_iterate \
+	overlay
 LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
 
 LIBTREE_TESTS_L = truncated_property
diff --git a/tests/overlay.c b/tests/overlay.c
new file mode 100644
index 000000000000..4637a777e50e
--- /dev/null
+++ b/tests/overlay.c
@@ -0,0 +1,228 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for DT overlays()
+ * Copyright (C) 2016 Free Electrons
+ * Copyright (C) 2016 NextThing Co.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <stdio.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+
+#define CHECK(code) \
+	{ \
+		if (code) \
+			FAIL(#code ": %s", fdt_strerror(code)); \
+	}
+
+/* 4k ought to be enough for anybody */
+#define FDT_COPY_SIZE	(4 * 1024)
+
+static int fdt_getprop_u32_by_index(void *fdt, const char *path,
+				    const char *name, int index,
+				    unsigned long *out)
+{
+	const fdt32_t *val;
+	int node_off;
+	int len;
+
+	node_off = fdt_path_offset(fdt, path);
+	if (node_off < 0)
+		return node_off;
+
+	val = fdt_getprop(fdt, node_off, name, &len);
+	if (!val || (len < (sizeof(uint32_t) * (index + 1))))
+		return -FDT_ERR_NOTFOUND;
+
+	*out = fdt32_to_cpu(*(val + index));
+
+	return 0;
+}
+
+static int check_getprop_string_by_name(void *fdt, const char *path,
+					const char *name, const char *val)
+{
+	int node_off;
+
+	node_off = fdt_path_offset(fdt, path);
+	if (node_off < 0)
+		return node_off;
+
+	check_getprop_string(fdt, node_off, name, val);
+
+	return RC_PASS;
+}
+
+static int check_getprop_u32_by_name(void *fdt, const char *path,
+				     const char *name, uint32_t val)
+{
+	int node_off;
+
+	node_off = fdt_path_offset(fdt, path);
+	if (node_off < 0)
+		return node_off;
+
+	check_getprop_cell(fdt, node_off, name, val);
+
+	return RC_PASS;
+}
+
+static int check_getprop_null_by_name(void *fdt, const char *path,
+				      const char *name)
+{
+	int node_off;
+
+	node_off = fdt_path_offset(fdt, path);
+	if (node_off < 0)
+		return node_off;
+
+	check_property(fdt, node_off, name, 0, NULL);
+
+	return RC_PASS;
+}
+
+static int fdt_overlay_change_int_property(void *fdt)
+{
+	return check_getprop_u32_by_name(fdt, "/test-node", "test-int-property",
+					 43);
+}
+
+static int fdt_overlay_change_str_property(void *fdt)
+{
+	return check_getprop_string_by_name(fdt, "/test-node",
+					    "test-str-property", "foobar");
+}
+
+static int fdt_overlay_add_str_property(void *fdt)
+{
+	return check_getprop_string_by_name(fdt, "/test-node",
+					    "test-str-property-2", "foobar2");
+}
+
+static int fdt_overlay_add_node_by_phandle(void *fdt)
+{
+	return check_getprop_null_by_name(fdt, "/test-node/new-node",
+					  "new-property");
+}
+
+static int fdt_overlay_add_node_by_path(void *fdt)
+{
+	return check_getprop_null_by_name(fdt, "/new-node", "new-property");
+}
+
+static int fdt_overlay_add_subnode_property(void *fdt)
+{
+	check_getprop_null_by_name(fdt, "/test-node/sub-test-node",
+				   "sub-test-property");
+	check_getprop_null_by_name(fdt, "/test-node/sub-test-node",
+				   "new-sub-test-property");
+
+	return RC_PASS;
+}
+
+static int fdt_overlay_local_phandle(void *fdt)
+{
+	uint32_t local_phandle;
+	unsigned long val = 0;
+	int off;
+
+	off = fdt_path_offset(fdt, "/new-local-node");
+	CHECK(off < 0);
+
+	local_phandle = fdt_get_phandle(fdt, off);
+	CHECK(!local_phandle);
+
+	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-several-phandle",
+				       0, &val));
+	CHECK(val != local_phandle);
+
+	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-several-phandle",
+				       1, &val));
+	CHECK(val != local_phandle);
+
+	return RC_PASS;
+}
+
+static int fdt_overlay_local_phandles(void *fdt)
+{
+	uint32_t local_phandle, test_phandle;
+	unsigned long val = 0;
+	int off;
+
+	off = fdt_path_offset(fdt, "/new-local-node");
+	CHECK(off < 0);
+
+	local_phandle = fdt_get_phandle(fdt, off);
+	CHECK(!local_phandle);
+
+	off = fdt_path_offset(fdt, "/test-node");
+	CHECK(off < 0);
+
+	test_phandle = fdt_get_phandle(fdt, off);
+	CHECK(!test_phandle);
+
+	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-phandle", 0, &val));
+	CHECK(test_phandle != val);
+
+	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-phandle", 1, &val));
+	CHECK(local_phandle != val);
+
+	return RC_PASS;
+}
+
+static void *open_dt(char *path)
+{
+	void *dt, *copy;
+
+	dt = load_blob(path);
+	copy = xmalloc(FDT_COPY_SIZE);
+
+	/*
+	 * Resize our DTs to 4k so that we have room to operate on
+	 */
+	CHECK(fdt_open_into(dt, copy, FDT_COPY_SIZE));
+
+	return copy;
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt_base, *fdt_overlay;
+
+	test_init(argc, argv);
+	if (argc != 3)
+		CONFIG("Usage: %s <base dtb> <overlay dtb>", argv[0]);
+
+	fdt_base = open_dt(argv[1]);
+	fdt_overlay = open_dt(argv[2]);
+
+	/* Apply the overlay */
+	CHECK(fdt_overlay_apply(fdt_base, fdt_overlay));
+
+	fdt_overlay_change_int_property(fdt_base);
+	fdt_overlay_change_str_property(fdt_base);
+	fdt_overlay_add_str_property(fdt_base);
+	fdt_overlay_add_node_by_phandle(fdt_base);
+	fdt_overlay_add_node_by_path(fdt_base);
+	fdt_overlay_add_subnode_property(fdt_base);
+	fdt_overlay_local_phandle(fdt_base);
+	fdt_overlay_local_phandles(fdt_base);
+
+	PASS();
+}
diff --git a/tests/overlay_base.dts b/tests/overlay_base.dts
new file mode 100644
index 000000000000..2603adb6821e
--- /dev/null
+++ b/tests/overlay_base.dts
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+	test: test-node {
+		test-int-property = <42>;
+		test-str-property = "foo";
+
+		subtest: sub-test-node {
+			sub-test-property;
+		};
+	};
+};
+
+
diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
new file mode 100644
index 000000000000..d30ecdf3661c
--- /dev/null
+++ b/tests/overlay_overlay.dts
@@ -0,0 +1,96 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+/ {
+	/* Test that we can change an int by another */
+	fragment@0 {
+		target = <&test>;
+
+		__overlay__ {
+			test-int-property = <43>;
+		};
+	};
+
+	/* Test that we can replace a string by a longer one */
+	fragment@1 {
+		target = <&test>;
+
+		__overlay__ {
+			test-str-property = "foobar";
+		};
+	};
+
+	/* Test that we add a new property */
+	fragment@2 {
+		target = <&test>;
+
+		__overlay__ {
+			test-str-property-2 = "foobar2";
+		};
+	};
+
+	/* Test that we add a new node (by phandle) */
+	fragment@3 {
+		target = <&test>;
+
+		__overlay__ {
+			new-node {
+				new-property;
+			};
+		};
+	};
+
+	/* Test that we add a new node (by path) */
+	fragment@4 {
+		target-path = "/";
+
+		__overlay__ {
+			new-node {
+				new-property;
+			};
+		};
+	};
+
+	fragment@5 {
+		target-path = "/";
+
+		__overlay__ {
+			local: new-local-node {
+				new-property;
+			};
+		};
+	};
+
+	fragment@6 {
+		target-path = "/";
+
+		__overlay__ {
+			test-phandle = <&test>, <&local>;
+		};
+	};
+
+	fragment@7 {
+		target-path = "/";
+
+		__overlay__ {
+			test-several-phandle = <&local>, <&local>;
+		};
+	};
+
+	fragment@8 {
+		target = <&test>;
+
+		__overlay__ {
+			sub-test-node {
+				new-sub-test-property;
+			};
+		};
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 6a2662b2abaf..054758298de9 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -280,6 +280,14 @@ libfdt_tests () {
     run_test get_alias aliases.dtb
     run_test path_offset_aliases aliases.dtb
 
+    # Overlay tests
+    echo "/dts-v1/; / {};" | $DTC -@ > /dev/null 2>&1
+    if [ $? -eq 0 ]; then
+        run_dtc_test -@ -I dts -O dtb -o overlay_base.dtb overlay_base.dts
+        run_dtc_test -I dts -O dtb -o overlay_overlay.dtb overlay_overlay.dts
+        run_test overlay overlay_base.dtb overlay_overlay.dtb
+    fi
+
     # Specific bug tests
     run_test add_subnode_with_nops
     run_dtc_test -I dts -O dts -o sourceoutput.test.dts sourceoutput.dts
-- 
2.9.0

Re: [PATCH v2 6/6] libfdt: Add overlay application function

[Phil Elwell]

On 11/07/2016 20:56, 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      | 618 ++++++++++++++++++++++++++++++++++++++++++++++
>   libfdt/libfdt.h           |  30 +++
>   libfdt/libfdt_env.h       |   1 +
>   tests/.gitignore          |   1 +
>   tests/Makefile.tests      |   3 +-
>   tests/overlay.c           | 228 +++++++++++++++++
>   tests/overlay_base.dts    |  21 ++
>   tests/overlay_overlay.dts |  96 +++++++
>   tests/run_tests.sh        |   8 +
>   10 files changed, 1006 insertions(+), 2 deletions(-)
>   create mode 100644 libfdt/fdt_overlay.c
>   create mode 100644 tests/overlay.c
>   create mode 100644 tests/overlay_base.dts
>   create mode 100644 tests/overlay_overlay.dts
>
> 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..40ad3b86c13e
> --- /dev/null
> +++ b/libfdt/fdt_overlay.c
> @@ -0,0 +1,618 @@
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +/**
> + * overlay_get_target_phandle - retrieves the target phandle of a fragment
> + * @fdto: pointer to the device tree overlay blob
> + * @fragment: node offset of the fragment in the overlay
> + *
> + * overlay_get_target_phandle() retrieves the target phandle of an
> + * overlay fragment when that fragment uses a phandle (target
> + * property) instead of a path (target-path property).
> + *
> + * returns:
> + *      the phandle pointed by the target property
> + *      0, if the phandle was not found
> + *	-1, if the phandle was malformed
> + */
> +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)
> +		return 0;
> +
> +	if ((*val == (uint32_t)-1) || (len != sizeof(*val)))
> +		return (uint32_t)-1;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +/**
> + * overlay_get_target - retrieves the target phandle of a fragment
> + * @fdt: Base device tree blob
> + * @fdto: Device tree overlay blob
> + * @fragment: node offset of the fragment in the overlay
> + *
> + * overlay_get_target() retrieves the target phandle in the base
> + * device tree of a fragment, no matter how the actual targetting is
> + * done (through a phandle or a path)
> + *
> + * returns:
> + *      the targetted node offset in the base device tree
> + *      Negative error code on error
> + */
> +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 == (uint32_t)-1)
> +		return -FDT_ERR_BADPHANDLE;
> +
> +	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);
> +}
> +
> +/**
> + * overlay_phandle_add_offset - Increases a phandle by an offset
> + * @fdt: Base device tree blob
> + * @node: Device tree overlay blob
> + * @name: Name of the property to modify (phandle or linux,phandle)
> + * @delta: offset to apply
> + *
> + * overlay_phandle_add_offset() increments a node phandle by a given
> + * offset.
> + *
> + * returns:
> + *      0 on success.
> + *      Negative error code on error
> + */
> +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);
> +	if ((adj_val + delta) < adj_val)
> +		return -FDT_ERR_BADPHANDLE;
> +
> +	adj_val += delta;
> +	return fdt_setprop_inplace_u32(fdt, node, name, adj_val);
> +}
> +
> +/**
> + * overlay_adjust_node_phandles - Offsets the phandles of a node
> + * @fdto: Device tree overlay blob
> + * @node: Offset of the node we want to adjust
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_adjust_node_phandles() adds a constant to all the phandles
> + * of a given node. This is mainly use as part of the overlay
> + * application process, when we want to update all the overlay
> + * phandles to not conflict with the overlays of the base device tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_adjust_node_phandles(void *fdto, int node,
> +					uint32_t delta)
> +{
> +	unsigned char found = 0;
> +	int child;
> +	int ret;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;
> +
> +	if (!ret)
> +		found = 1;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;
> +
> +	/*
> +	 * If neither phandle nor linux,phandle have been found return
> +	 * an error.
> +	 */
> +	if (!found && !ret)
> +		return ret;
> +
> +	fdt_for_each_subnode(child, fdto, node)
> +		overlay_adjust_node_phandles(fdto, child, delta);
> +
> +	return 0;
> +}
> +
> +/**
> + * overlay_adjust_local_phandles - Adjust the phandles of a whole overlay
> + * @fdto: Device tree overlay blob
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_adjust_local_phandles() adds a constant to all the
> + * phandles of an overlay. This is mainly use as part of the overlay
> + * application process, when we want to update all the overlay
> + * phandles to not conflict with the overlays of the base device tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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);
> +}
> +
> +/**
> + * overlay_update_local_node_references - Adjust the overlay references
> + * @fdto: Device tree overlay blob
> + * @tree_node: Node offset of the node to operate on
> + * @fixup_node: Node offset of the matching local fixups node
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_update_local_nodes_references() update the phandles
> + * pointing to a node within the device tree overlay by adding a
> + * constant delta.
> + *
> + * This is mainly used as part of a device tree application process,
> + * where you want the device tree overlays phandles to not conflict
> + * with the ones from the base device tree before merging them.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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 unsigned char *fixup_val, *tree_val;
> +		const char *name;
> +		int fixup_len;
> +		int tree_len;
> +		int i;
> +
> +		fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> +						  &name, &fixup_len);
> +		if (!fixup_val)
> +			return fixup_len;
> +
> +		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> +		if (!tree_val)
> +			return tree_len;
> +
> +		for (i = 0; i < fixup_len; i += sizeof(uint32_t)) {
> +			uint32_t adj_val, index;
> +
> +			index = *(const uint32_t *)(fixup_val + i);
> +			index = fdt32_to_cpu(index);
> +
> +			/*
> +			 * phandles to fixup can be unaligned.
> +			 *
> +			 * Use a memcpy for the architectures that do
> +			 * not support unaligned accesses.
> +			 */
> +			memcpy(&adj_val, tree_val + index, sizeof(uint32_t));
> +
> +			adj_val = fdt32_to_cpu(adj_val);
> +			adj_val += delta;
> +			adj_val = cpu_to_fdt32(adj_val);
> +
> +			ret = fdt_setprop_inplace_namelen_partial(fdto,
> +								  tree_node,
> +								  name,
> +								  strlen(name),
> +								  index,
> +								  &adj_val,
> +								  sizeof(adj_val));
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	fdt_for_each_subnode(fixup_child, fdto, 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;
> +}
> +
> +/**
> + * overlay_update_local_references - Adjust the overlay references
> + * @fdto: Device tree overlay blob
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_update_local_references() update all the phandles pointing
> + * to a node within the device tree overlay by adding a constant
> + * delta to not conflict with the base overlay.
> + *
> + * This is mainly used as part of a device tree application process,
> + * where you want the device tree overlays phandles to not conflict
> + * with the ones from the base device tree before merging them.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_update_local_references(void *fdto, uint32_t delta)
> +{
> +	int fixups;
> +
> +	fixups = fdt_path_offset(fdto, "/__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(fdto, 0, fixups,
> +						    delta);
> +}
> +
> +/**
> + * overlay_fixup_one_phandle - Set an overlay phandle to the base one
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbols_off: Node offset of the symbols node in the base device tree
> + * @path: Path to a node holding a phandle in the overlay
> + * @path_len: number of path characters to consider
> + * @name: Name of the property holding the phandle reference in the overlay
> + * @name_len: number of name characters to consider
> + * @index: Index in the overlay property where the phandle is stored
> + * @label: Label of the node referenced by the phandle
> + *
> + * overlay_fixup_one_phandle() resolves an overlay phandle pointing to
> + * a node in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when
> + * you want all the phandles in the overlay to point to the actual
> + * base dt nodes.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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_partial(fdto, fixup_off,
> +						   name, name_len, index,
> +						   &phandle, sizeof(phandle));
> +};
> +
> +/**
> + * overlay_fixup_phandle - Set an overlay phandle to the base one
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbols_off: Node offset of the symbols node in the base device tree
> + * @property: Property offset in the overlay holding the list of fixups
> + *
> + * overlay_fixup_phandle() resolves all the overlay phandles pointed
> + * to in a __local_fixup__ property, and updates them to match the
> + * phandles in use in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when
> + * you want all the phandles in the overlay to point to the actual
> + * base dt nodes.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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 prop_len = strlen(value);
> +		uint32_t path_len, name_len;
> +		char *sep, *endptr;
> +		int index;
> +		int ret;
> +
> +		path = prop_string;
> +		sep = memchr(prop_string, ':', prop_len);
> +		if (*sep != ':')
> +			return -FDT_ERR_BADSTRUCTURE;
> +		path_len = sep - path;
> +
> +		name = sep + 1;
> +		sep = memchr(name, ':', prop_len);
> +		if (*sep != ':')
> +			return -FDT_ERR_BADSTRUCTURE;
> +		name_len = sep - name;
> +
> +		index = strtoul(sep + 1, &endptr, 10);
> +		if ((*endptr != '\0') || (endptr <= (sep + 1)))
> +			return -FDT_ERR_BADSTRUCTURE;
> +
> +		len -= prop_len + 1;
> +		value += prop_len + 1;
> +
> +		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;
> +}
> +
> +/**
> + * overlay_fixup_phandles - Resolve the overlay phandles to the base
> + *                          device tree
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_fixup_phandles() resolves all the overlay phandles pointing
> + * to nodes in the base device tree.
> + *
> + * This is one of the steps of the device tree overlay application
> + * process, when you want all the phandles in the overlay to point to
> + * the actual base dt nodes.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_fixup_phandles(void *fdt, void *fdto)
> +{
> +	int fixups_off, symbols_off;
> +	int property;
> +
> +	symbols_off = fdt_path_offset(fdt, "/__symbols__");
> +	fixups_off = fdt_path_offset(fdto, "/__fixups__");
> +
> +	fdt_for_each_property_offset(property, fdto, fixups_off) {
> +		int ret;
> +
> +		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * overlay_apply_node - Merge an overlay fragment into the base device tree
> + * @fdt: Base Device Tree blob
> + * @target: Node offset in the base device tree to apply the fragment to
> + * @fdto: Device tree overlay blob
> + * @fragment: Node offset in the overlay holding the changes to merge
> + *
> + * overlay_apply_node() merges an overlay fragment into a target base
> + * device tree node pointed.
> + *
> + * This is part of the final step in the device tree overlay
> + * application process, when all the phandles have been adjusted and
> + * resolved and you just have to merge overlay into the base device
> + * tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_apply_node(void *fdt, int target,
> +			      void *fdto, int fragment)
> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property_offset(property, fdto, fragment) {
> +		const char *name;
> +		const void *prop;
> +		int prop_len;
> +		int ret;
> +
> +		prop = fdt_getprop_by_offset(fdto, property, &name,
> +					     &prop_len);
> +		if (prop_len == -FDT_ERR_NOTFOUND)
> +			return -FDT_ERR_INTERNAL;
> +		if (prop_len < 0)
> +			return prop_len;
> +
> +		ret = fdt_setprop(fdt, target, name, prop, prop_len);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(node, fdto, fragment) {
> +		const char *name = fdt_get_name(fdto, node, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(fdt, target, name);
> +		if (nnode == -FDT_ERR_EXISTS)
> +			nnode = fdt_subnode_offset(fdt, target, name);
> +
> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = overlay_apply_node(fdt, nnode, fdto, node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * overlay_merge - Merge an overlay into its base device tree
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_merge() merges an overlay into its base device tree.
> + *
> + * This is the final step in the device tree overlay application
> + * process, when all the phandles have been adjusted and resolved and
> + * you just have to merge overlay into the base device tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_merge(void *fdt, void *fdto)
> +{
> +	int fragment;
> +
> +	fdt_for_each_subnode(fragment, fdto, 0) {
> +		int overlay;
> +		int target;
> +		int ret;
> +
> +		target = overlay_get_target(fdt, fdto, fragment);
> +		if (target < 0)
> +			continue;
> +
> +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +		if (overlay < 0)
> +			return overlay;
Why does the absence of a target cause a fragment to be ignored but the absence of an "__overlay__" property cause the merging to be abandoned with an error? Can't we just ignore fragments that aren't recognised?
> +
> +		ret = overlay_apply_node(fdt, target, fdto, 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 err;
> +
> +	/*
> +	 * The overlay has been damaged, erase its magic.
> +	 */
> +	fdt_set_magic(fdto, ~0);
> +
> +	return 0;
> +
> +err:
> +	/*
> +	 * The overlay might have been damaged, erase its magic.
> +	 */
> +	fdt_set_magic(fdto, ~0);
> +
> +	/*
> +	 * The base device tree might have been damaged, erase its
> +	 * magic.
> +	 */
> +	fdt_set_magic(fdt, ~0);
> +
> +	return ret;
> +}
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index a22a16b6878a..a70b8767cabc 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1745,6 +1745,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__
> diff --git a/tests/.gitignore b/tests/.gitignore
> index fa4616ba28c2..6c97066c587e 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -35,6 +35,7 @@ tmp.*
>   /nopulate
>   /notfound
>   /open_pack
> +/overlay
>   /parent_offset
>   /path-references
>   /path_offset
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 196518c83eda..4aefbbd15253 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -24,7 +24,8 @@ LIB_TESTS_L = get_mem_rsv \
>   	utilfdt_test \
>   	integer-expressions \
>   	property_iterate \
> -	subnode_iterate
> +	subnode_iterate \
> +	overlay
>   LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
>   
>   LIBTREE_TESTS_L = truncated_property
> diff --git a/tests/overlay.c b/tests/overlay.c
> new file mode 100644
> index 000000000000..4637a777e50e
> --- /dev/null
> +++ b/tests/overlay.c
> @@ -0,0 +1,228 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + *	Testcase for DT overlays()
> + * Copyright (C) 2016 Free Electrons
> + * Copyright (C) 2016 NextThing Co.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public License
> + * as published by the Free Software Foundation; either version 2.1 of
> + * the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdio.h>
> +
> +#include <libfdt.h>
> +
> +#include "tests.h"
> +
> +#define CHECK(code) \
> +	{ \
> +		if (code) \
> +			FAIL(#code ": %s", fdt_strerror(code)); \
> +	}
> +
> +/* 4k ought to be enough for anybody */
> +#define FDT_COPY_SIZE	(4 * 1024)
> +
> +static int fdt_getprop_u32_by_index(void *fdt, const char *path,
> +				    const char *name, int index,
> +				    unsigned long *out)
> +{
> +	const fdt32_t *val;
> +	int node_off;
> +	int len;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	val = fdt_getprop(fdt, node_off, name, &len);
> +	if (!val || (len < (sizeof(uint32_t) * (index + 1))))
> +		return -FDT_ERR_NOTFOUND;
> +
> +	*out = fdt32_to_cpu(*(val + index));
> +
> +	return 0;
> +}
> +
> +static int check_getprop_string_by_name(void *fdt, const char *path,
> +					const char *name, const char *val)
> +{
> +	int node_off;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	check_getprop_string(fdt, node_off, name, val);
> +
> +	return RC_PASS;
> +}
> +
> +static int check_getprop_u32_by_name(void *fdt, const char *path,
> +				     const char *name, uint32_t val)
> +{
> +	int node_off;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	check_getprop_cell(fdt, node_off, name, val);
> +
> +	return RC_PASS;
> +}
> +
> +static int check_getprop_null_by_name(void *fdt, const char *path,
> +				      const char *name)
> +{
> +	int node_off;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	check_property(fdt, node_off, name, 0, NULL);
> +
> +	return RC_PASS;
> +}
> +
> +static int fdt_overlay_change_int_property(void *fdt)
> +{
> +	return check_getprop_u32_by_name(fdt, "/test-node", "test-int-property",
> +					 43);
> +}
> +
> +static int fdt_overlay_change_str_property(void *fdt)
> +{
> +	return check_getprop_string_by_name(fdt, "/test-node",
> +					    "test-str-property", "foobar");
> +}
> +
> +static int fdt_overlay_add_str_property(void *fdt)
> +{
> +	return check_getprop_string_by_name(fdt, "/test-node",
> +					    "test-str-property-2", "foobar2");
> +}
> +
> +static int fdt_overlay_add_node_by_phandle(void *fdt)
> +{
> +	return check_getprop_null_by_name(fdt, "/test-node/new-node",
> +					  "new-property");
> +}
> +
> +static int fdt_overlay_add_node_by_path(void *fdt)
> +{
> +	return check_getprop_null_by_name(fdt, "/new-node", "new-property");
> +}
> +
> +static int fdt_overlay_add_subnode_property(void *fdt)
> +{
> +	check_getprop_null_by_name(fdt, "/test-node/sub-test-node",
> +				   "sub-test-property");
> +	check_getprop_null_by_name(fdt, "/test-node/sub-test-node",
> +				   "new-sub-test-property");
> +
> +	return RC_PASS;
> +}
> +
> +static int fdt_overlay_local_phandle(void *fdt)
> +{
> +	uint32_t local_phandle;
> +	unsigned long val = 0;
> +	int off;
> +
> +	off = fdt_path_offset(fdt, "/new-local-node");
> +	CHECK(off < 0);
> +
> +	local_phandle = fdt_get_phandle(fdt, off);
> +	CHECK(!local_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-several-phandle",
> +				       0, &val));
> +	CHECK(val != local_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-several-phandle",
> +				       1, &val));
> +	CHECK(val != local_phandle);
> +
> +	return RC_PASS;
> +}
> +
> +static int fdt_overlay_local_phandles(void *fdt)
> +{
> +	uint32_t local_phandle, test_phandle;
> +	unsigned long val = 0;
> +	int off;
> +
> +	off = fdt_path_offset(fdt, "/new-local-node");
> +	CHECK(off < 0);
> +
> +	local_phandle = fdt_get_phandle(fdt, off);
> +	CHECK(!local_phandle);
> +
> +	off = fdt_path_offset(fdt, "/test-node");
> +	CHECK(off < 0);
> +
> +	test_phandle = fdt_get_phandle(fdt, off);
> +	CHECK(!test_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-phandle", 0, &val));
> +	CHECK(test_phandle != val);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-phandle", 1, &val));
> +	CHECK(local_phandle != val);
> +
> +	return RC_PASS;
> +}
> +
> +static void *open_dt(char *path)
> +{
> +	void *dt, *copy;
> +
> +	dt = load_blob(path);
> +	copy = xmalloc(FDT_COPY_SIZE);
> +
> +	/*
> +	 * Resize our DTs to 4k so that we have room to operate on
> +	 */
> +	CHECK(fdt_open_into(dt, copy, FDT_COPY_SIZE));
> +
> +	return copy;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	void *fdt_base, *fdt_overlay;
> +
> +	test_init(argc, argv);
> +	if (argc != 3)
> +		CONFIG("Usage: %s <base dtb> <overlay dtb>", argv[0]);
> +
> +	fdt_base = open_dt(argv[1]);
> +	fdt_overlay = open_dt(argv[2]);
> +
> +	/* Apply the overlay */
> +	CHECK(fdt_overlay_apply(fdt_base, fdt_overlay));
> +
> +	fdt_overlay_change_int_property(fdt_base);
> +	fdt_overlay_change_str_property(fdt_base);
> +	fdt_overlay_add_str_property(fdt_base);
> +	fdt_overlay_add_node_by_phandle(fdt_base);
> +	fdt_overlay_add_node_by_path(fdt_base);
> +	fdt_overlay_add_subnode_property(fdt_base);
> +	fdt_overlay_local_phandle(fdt_base);
> +	fdt_overlay_local_phandles(fdt_base);
> +
> +	PASS();
> +}
> diff --git a/tests/overlay_base.dts b/tests/overlay_base.dts
> new file mode 100644
> index 000000000000..2603adb6821e
> --- /dev/null
> +++ b/tests/overlay_base.dts
> @@ -0,0 +1,21 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +	test: test-node {
> +		test-int-property = <42>;
> +		test-str-property = "foo";
> +
> +		subtest: sub-test-node {
> +			sub-test-property;
> +		};
> +	};
> +};
> +
> +
> diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
> new file mode 100644
> index 000000000000..d30ecdf3661c
> --- /dev/null
> +++ b/tests/overlay_overlay.dts
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	/* Test that we can change an int by another */
> +	fragment@0 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-int-property = <43>;
> +		};
> +	};
> +
> +	/* Test that we can replace a string by a longer one */
> +	fragment@1 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-str-property = "foobar";
> +		};
> +	};
> +
> +	/* Test that we add a new property */
> +	fragment@2 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-str-property-2 = "foobar2";
> +		};
> +	};
> +
> +	/* Test that we add a new node (by phandle) */
> +	fragment@3 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			new-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	/* Test that we add a new node (by path) */
> +	fragment@4 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			new-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	fragment@5 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			local: new-local-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	fragment@6 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			test-phandle = <&test>, <&local>;
> +		};
> +	};
> +
> +	fragment@7 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			test-several-phandle = <&local>, <&local>;
> +		};
> +	};
> +
> +	fragment@8 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			sub-test-node {
> +				new-sub-test-property;
> +			};
> +		};
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 6a2662b2abaf..054758298de9 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -280,6 +280,14 @@ libfdt_tests () {
>       run_test get_alias aliases.dtb
>       run_test path_offset_aliases aliases.dtb
>   
> +    # Overlay tests
> +    echo "/dts-v1/; / {};" | $DTC -@ > /dev/null 2>&1
> +    if [ $? -eq 0 ]; then
> +        run_dtc_test -@ -I dts -O dtb -o overlay_base.dtb overlay_base.dts
> +        run_dtc_test -I dts -O dtb -o overlay_overlay.dtb overlay_overlay.dts
> +        run_test overlay overlay_base.dtb overlay_overlay.dtb
> +    fi
> +
>       # Specific bug tests
>       run_test add_subnode_with_nops
>       run_dtc_test -I dts -O dts -o sourceoutput.test.dts sourceoutput.dts

Phil Elwell (aka phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org)

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

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:56:18PM +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>

Code looks ok, but there are some nits in the comments.

> ---
>  libfdt/libfdt.h         | 25 +++++++++++++++++++++++++
>  tests/subnode_iterate.c |  8 ++------
>  2 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 36222fd4a6f4..0cf46872b54e 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

Parameter descriptions should go here.

> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_subnode(fdt, node, parent) {

Parameter order hasn't been updated for code revisions.

> + *		...
> + *		use node
> + *		...
> + *	}

One gotcha with this is what happens if there's an error during the
iteration.  I suggest adding something like:
	if ((node < 0) && (node != -FDT_ERR_NOTFOUND)) {
		/* error handling */
	}

to the example, to remind users that they should check for this.

> + *
> + * Note that this is implemented as a macro and node is used as iterator in

s/node/@node/ - makes it stand out as a parameter.

> + * the loop. It should therefore be a locally allocated variable. The parent
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Remove for brevity.

> + * variable on the other hand is never modified, so it can be constant or
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Remove for brevity.

> + * even a literal.
> + *
> + * @node:	child node (int)
> + * @fdt:	FDT blob (const void *)
> + * @parent:	parent node (int)
> + */
> +#define fdt_for_each_subnode(node, fdt, parent)		\
> +	for (node = fdt_first_subnode(fdt, parent);	\
> +	     node >= 0;					\
> +	     node = fdt_next_subnode(fdt, node))
> +
>  /**********************************************************************/
>  /* General functions                                                  */
>  /**********************************************************************/
> diff --git a/tests/subnode_iterate.c b/tests/subnode_iterate.c
> index b9f379d5963c..0fb5c901ebd7 100644
> --- a/tests/subnode_iterate.c
> +++ b/tests/subnode_iterate.c
> @@ -48,9 +48,7 @@ static void test_node(void *fdt, int parent_offset)
>  	subnodes = cpu_to_fdt32(*prop);
>  
>  	count = 0;
> -	for (offset = fdt_first_subnode(fdt, parent_offset);
> -	     offset >= 0;
> -	     offset = fdt_next_subnode(fdt, offset))
> +	fdt_for_each_subnode(offset, fdt, parent_offset)
>  		count++;
>  
>  	if (count != subnodes) {
> @@ -65,9 +63,7 @@ static void check_fdt_next_subnode(void *fdt)
>  	int offset;
>  	int count = 0;
>  
> -	for (offset = fdt_first_subnode(fdt, 0);
> -	     offset >= 0;
> -	     offset = fdt_next_subnode(fdt, offset)) {
> +	fdt_for_each_subnode(offset, fdt, 0) {
>  		test_node(fdt, offset);
>  		count++;
>  	}

-- 
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 v2 2/6] libfdt: Add iterator over properties

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:56:19PM +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>
> Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

As with 1/6 code looks ok, but there are a couple of nits in the comment.

> ---
>  libfdt/libfdt.h            | 24 ++++++++++++
>  tests/.gitignore           |  1 +
>  tests/Makefile.tests       |  1 +
>  tests/property_iterate.c   | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/property_iterate.dts | 24 ++++++++++++
>  tests/run_tests.sh         |  3 ++
>  6 files changed, 150 insertions(+)
>  create mode 100644 tests/property_iterate.c
>  create mode 100644 tests/property_iterate.dts
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 0cf46872b54e..9d3c9b234274 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -455,6 +455,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
> + * @property_offset:	property offset (int)
> + * @fdt:		FDT blob (const void *)
> + * @node:		node offset (int)
> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_property(fdt, node, property) {

Again, parameter order is out of date.

> + *		...
> + *		use property
> + *		...
> + *	}

Again add an error handling clause.

> + *
> + * 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.

Similar edits for brevity as 1/6.

> + */
> +#define fdt_for_each_property_offset(property, fdt, node)	\
> +	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
> diff --git a/tests/.gitignore b/tests/.gitignore
> index e4532da30bf5..fa4616ba28c2 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -40,6 +40,7 @@ tmp.*
>  /path_offset
>  /path_offset_aliases
>  /phandle_format
> +/property_iterate
>  /propname_escapes
>  /references
>  /root_node
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index f7c3a4b00ead..196518c83eda 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -23,6 +23,7 @@ LIB_TESTS_L = get_mem_rsv \
>  	add_subnode_with_nops path_offset_aliases \
>  	utilfdt_test \
>  	integer-expressions \
> +	property_iterate \
>  	subnode_iterate
>  LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
>  
> diff --git a/tests/property_iterate.c b/tests/property_iterate.c
> new file mode 100644
> index 000000000000..0f3959cb8c22
> --- /dev/null
> +++ b/tests/property_iterate.c
> @@ -0,0 +1,97 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + *	Tests that fdt_next_subnode() works as expected
> + *
> + * Copyright (C) 2013 Google, Inc
> + *
> + * Copyright (C) 2007 David Gibson, IBM Corporation.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public License
> + * as published by the Free Software Foundation; either version 2.1 of
> + * the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +#include <libfdt.h>
> +
> +#include "tests.h"
> +#include "testdata.h"
> +
> +static void test_node(void *fdt, int parent_offset)
> +{
> +	fdt32_t properties;
> +	const fdt32_t *prop;
> +	int offset, property;
> +	int count;
> +	int len;
> +
> +	/*
> +	 * This property indicates the number of properties in our
> +	 * test node to expect
> +	 */
> +	prop = fdt_getprop(fdt, parent_offset, "test-properties", &len);
> +	if (!prop || len != sizeof(fdt32_t)) {
> +		FAIL("Missing/invalid test-properties property at '%s'",
> +		     fdt_get_name(fdt, parent_offset, NULL));
> +	}
> +	properties = cpu_to_fdt32(*prop);
> +
> +	count = 0;
> +	offset = fdt_first_subnode(fdt, parent_offset);
> +	if (offset < 0)
> +		FAIL("Missing test node\n");
> +
> +	fdt_for_each_property_offset(property, fdt, offset)
> +		count++;
> +
> +	if (count != properties) {
> +		FAIL("Node '%s': Expected %d properties, got %d\n",
> +		     fdt_get_name(fdt, parent_offset, NULL), properties,
> +		     count);
> +	}
> +}
> +
> +static void check_fdt_next_subnode(void *fdt)
> +{
> +	int offset;
> +	int count = 0;
> +
> +	fdt_for_each_subnode(offset, fdt, 0) {
> +		test_node(fdt, offset);
> +		count++;
> +	}
> +
> +	if (count != 2)
> +		FAIL("Expected %d tests, got %d\n", 2, count);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	void *fdt;
> +
> +	test_init(argc, argv);
> +	if (argc != 2)
> +		CONFIG("Usage: %s <dtb file>", argv[0]);
> +
> +	fdt = load_blob(argv[1]);
> +	if (!fdt)
> +		FAIL("No device tree available");
> +
> +	check_fdt_next_subnode(fdt);
> +
> +	PASS();
> +}
> diff --git a/tests/property_iterate.dts b/tests/property_iterate.dts
> new file mode 100644
> index 000000000000..2ed677e7e6ea
> --- /dev/null
> +++ b/tests/property_iterate.dts
> @@ -0,0 +1,24 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	test1 {
> +		test-properties = <3>;
> +
> +		test {
> +			linux,phandle = <0x1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +	};
> +
> +	test2 {
> +		test-properties = <0>;
> +
> +		test {
> +		};
> +	};
> +};
> +
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 7eb9b3d33108..6a2662b2abaf 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -269,6 +269,9 @@ libfdt_tests () {
>      run_dtc_test -I dts -O dtb -o subnode_iterate.dtb subnode_iterate.dts
>      run_test subnode_iterate subnode_iterate.dtb
>  
> +    run_dtc_test -I dts -O dtb -o property_iterate.dtb property_iterate.dts
> +    run_test property_iterate property_iterate.dtb
> +
>      # Tests for behaviour on various sorts of corrupted trees
>      run_test truncated_property
>  

-- 
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 v2 2/6] libfdt: Add iterator over properties

[Robert P. J. Day]

On Tue, 12 Jul 2016, David Gibson wrote:

> On Mon, Jul 11, 2016 at 09:56:19PM +0200, Maxime Ripard wrote:
> > +/**
> > + * fdt_for_each_property - iterate over all properties of a node
> > + * @property_offset:	property offset (int)
> > + * @fdt:		FDT blob (const void *)
> > + * @node:		node offset (int)
> > + *
> > + * This is actually a wrapper around a for loop and would be used like so:
> > + *
> > + *	fdt_for_each_property(fdt, node, property) {
>
> Again, parameter order is out of date.

  also, when did kerneldoc content start specifying data types in the
comment? (int, const void*, ...). i don't think that's standard.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

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

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:56:20PM +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>
> Reviewed-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
> Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Although one little nit in the comment that might be worth addressing
if you respin the series.

> ---
>  libfdt/fdt_ro.c     | 26 ++++++++++++++++++++++++++
>  libfdt/libfdt.h     | 14 ++++++++++++++
>  tests/get_phandle.c |  6 ++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 50cce864283c..04590984bd51 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -88,6 +88,32 @@ 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;
> +	int offset;
> +
> +	for (offset = fdt_next_node(fdt, -1, NULL);;
> +	     offset = fdt_next_node(fdt, offset, NULL)) {
> +		uint32_t phandle;
> +
> +		if (offset == -FDT_ERR_NOTFOUND)
> +			return max_phandle;
> +
> +		if (offset < 0)
> +			return (uint32_t)-1;
> +
> +		phandle = fdt_get_phandle(fdt, offset);
> +		if (phandle == (uint32_t)-1)
> +			continue;
> +
> +		if (phandle > max_phandle)
> +			max_phandle = phandle;
> +	}
> +
> +	return 0;
> +}
> +
>  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 9d3c9b234274..812937fede44 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -283,6 +283,20 @@ 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 phandle in the given
> + * device tree

Perhaps worth mentioning here that this will effectively ignore badly
formatted phandle properties, or phandles with value 0 or -1.

> + * returns:
> + *      the highest phandle on success
> + *      0, if no phandle was found in the device tree
> + *      -1, if an error occurred
> + */
> +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
> diff --git a/tests/get_phandle.c b/tests/get_phandle.c
> index 2079591d4c49..22bd7b81b3f0 100644
> --- a/tests/get_phandle.c
> +++ b/tests/get_phandle.c
> @@ -44,6 +44,7 @@ static void check_phandle(void *fdt, const char *path, uint32_t checkhandle)
>  
>  int main(int argc, char *argv[])
>  {
> +	uint32_t max;
>  	void *fdt;
>  
>  	test_init(argc, argv);
> @@ -53,5 +54,10 @@ int main(int argc, char *argv[])
>  	check_phandle(fdt, "/subnode@2", PHANDLE_1);
>  	check_phandle(fdt, "/subnode@2/subsubnode@0", PHANDLE_2);
>  
> +	max = fdt_get_max_phandle(fdt);
> +	if (max != PHANDLE_2)
> +		FAIL("fdt_get_max_phandle returned 0x%x instead of 0x%x\n",
> +		     max, PHANDLE_2);
> +
>  	PASS();
>  }

-- 
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 v2 4/6] libfdt: Add fdt_getprop_namelen_w

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:56:21PM +0200, Maxime Ripard wrote:
> Add a function to retrieve a writeable property only by the first
> characters of its name.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

> ---
>  libfdt/libfdt.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 812937fede44..1da2c540b607 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -622,6 +622,13 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>   */
>  const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>  				const char *name, int namelen, int *lenp);
> +static inline void *fdt_getprop_namelen_w(void *fdt, int nodeoffset,
> +					  const char *name, int namelen,
> +					  int *lenp)
> +{
> +	return (void *)(uintptr_t)fdt_getprop_namelen(fdt, nodeoffset, name,
> +						      namelen, lenp);
> +}
>  
>  /**
>   * fdt_getprop - retrieve the value of a given property

-- 
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 v2 2/6] libfdt: Add iterator over properties

[David Gibson]

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

On Mon, Jul 11, 2016 at 06:57:46PM -0700, Robert P. J. Day wrote:
> On Tue, 12 Jul 2016, David Gibson wrote:
> 
> > On Mon, Jul 11, 2016 at 09:56:19PM +0200, Maxime Ripard wrote:
> > > +/**
> > > + * fdt_for_each_property - iterate over all properties of a node
> > > + * @property_offset:	property offset (int)
> > > + * @fdt:		FDT blob (const void *)
> > > + * @node:		node offset (int)
> > > + *
> > > + * This is actually a wrapper around a for loop and would be used like so:
> > > + *
> > > + *	fdt_for_each_property(fdt, node, property) {
> >
> > Again, parameter order is out of date.
> 
>   also, when did kerneldoc content start specifying data types in the
> comment? (int, const void*, ...). i don't think that's standard.

It's not, but because this is a macro, not a function, I think it's a
good idea to include.  The data types are constrained by the functions
the macro invokes, but unlike a function comment those aren't obvious
from the function signature immediately below.

Actually.. rather than the somewhat longwinded comment about @node, it
might be worth just noting that it's an lvalue in the parameter
description here.

-- 
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 v2 5/6] libfdt: Add fdt_setprop_inplace_namelen_partial

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:56:22PM +0200, Maxime Ripard wrote:
> Add a function to modify inplace only a portion of a property..
> 
> This is especially useful when the property is an array of values, and you
> want to update one of them without changing the DT size.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Now it just needs some testcases..

> ---
>  libfdt/fdt_wip.c | 29 +++++++++++++++++++++++++----
>  libfdt/libfdt.h  | 21 +++++++++++++++++++++
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
> index c5bbb68d3273..fd53e0b67416 100644
> --- a/libfdt/fdt_wip.c
> +++ b/libfdt/fdt_wip.c
> @@ -55,21 +55,42 @@
>  
>  #include "libfdt_internal.h"
>  
> +int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
> +					const char *name, int namelen,
> +					uint32_t index, const void *val,
> +					int len)
> +{
> +	void *propval;
> +	int proplen;
> +
> +	propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen,
> +					&proplen);
> +	if (!propval)
> +		return proplen;
> +
> +	if (proplen < (len + index))
> +		return -FDT_ERR_NOSPACE;
> +
> +	memcpy((unsigned char *)propval + index, val, len);
> +	return 0;
> +}
> +
>  int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
>  			const void *val, int len)
>  {
> -	void *propval;
> +	const void *propval;
>  	int proplen;
>  
> -	propval = fdt_getprop_w(fdt, nodeoffset, name, &proplen);
> +	propval = fdt_getprop(fdt, nodeoffset, name, &proplen);
>  	if (! propval)
>  		return proplen;
>  
>  	if (proplen != len)
>  		return -FDT_ERR_NOSPACE;
>  
> -	memcpy(propval, val, len);
> -	return 0;
> +	return fdt_setprop_inplace_namelen_partial(fdt, nodeoffset, name,
> +						   strlen(name), 0,
> +						   val, len);
>  }
>  
>  static void _fdt_nop_region(void *start, int len)
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 1da2c540b607..a22a16b6878a 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1072,6 +1072,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset);
>  /* Write-in-place functions                                           */
>  /**********************************************************************/
>  
> +/**
> + * fdt_setprop_inplace_namelen_partial - change a property's value,
> + *                                       but not its size
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to change
> + * @name: name of the property to change
> + * @namelen: number of characters of name to consider
> + * @index: index of the property to change in the array
> + * @val: pointer to data to replace the property value with
> + * @len: length of the property value
> + *
> + * Identical to fdt_setprop_inplace(), but modifies the given property
> + * starting from the given index, and using only the first characters
> + * of the name. It is useful when you want to manipulate only one value of
> + * an array and you have a string that doesn't end with \0.
> + */
> +int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
> +					const char *name, int namelen,
> +					uint32_t index, const void *val,
> +					int len);
> +
>  /**
>   * fdt_setprop_inplace - change a property's value, but not its size
>   * @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 --]

Re: [PATCH v2 6/6] libfdt: Add overlay application function

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:56:23PM +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      | 618 ++++++++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h           |  30 +++
>  libfdt/libfdt_env.h       |   1 +
>  tests/.gitignore          |   1 +
>  tests/Makefile.tests      |   3 +-
>  tests/overlay.c           | 228 +++++++++++++++++
>  tests/overlay_base.dts    |  21 ++
>  tests/overlay_overlay.dts |  96 +++++++
>  tests/run_tests.sh        |   8 +
>  10 files changed, 1006 insertions(+), 2 deletions(-)
>  create mode 100644 libfdt/fdt_overlay.c
>  create mode 100644 tests/overlay.c
>  create mode 100644 tests/overlay_base.dts
>  create mode 100644 tests/overlay_overlay.dts
> 
> 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..40ad3b86c13e
> --- /dev/null
> +++ b/libfdt/fdt_overlay.c
> @@ -0,0 +1,618 @@
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +/**
> + * overlay_get_target_phandle - retrieves the target phandle of a fragment
> + * @fdto: pointer to the device tree overlay blob
> + * @fragment: node offset of the fragment in the overlay
> + *
> + * overlay_get_target_phandle() retrieves the target phandle of an
> + * overlay fragment when that fragment uses a phandle (target
> + * property) instead of a path (target-path property).
> + *
> + * returns:
> + *      the phandle pointed by the target property
> + *      0, if the phandle was not found
> + *	-1, if the phandle was malformed
> + */
> +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)
> +		return 0;
> +
> +	if ((*val == (uint32_t)-1) || (len != sizeof(*val)))

You need to reverse the two if clauses.  If the len is not right, it's
not safe to dereference val.

> +		return (uint32_t)-1;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +/**
> + * overlay_get_target - retrieves the target phandle of a fragment

The description is wrong, since this returns an offset, not a phandle.

> + * @fdt: Base device tree blob
> + * @fdto: Device tree overlay blob
> + * @fragment: node offset of the fragment in the overlay
> + *
> + * overlay_get_target() retrieves the target phandle in the base

And again here.

> + * device tree of a fragment, no matter how the actual targetting is
> + * done (through a phandle or a path)
> + *
> + * returns:
> + *      the targetted node offset in the base device tree
> + *      Negative error code on error
> + */
> +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 == (uint32_t)-1)
> +		return -FDT_ERR_BADPHANDLE;
> +
> +	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);
> +}
> +
> +/**
> + * overlay_phandle_add_offset - Increases a phandle by an offset
> + * @fdt: Base device tree blob
> + * @node: Device tree overlay blob
> + * @name: Name of the property to modify (phandle or linux,phandle)
> + * @delta: offset to apply
> + *
> + * overlay_phandle_add_offset() increments a node phandle by a given
> + * offset.
> + *
> + * returns:
> + *      0 on success.
> + *      Negative error code on error
> + */
> +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);
> +	if ((adj_val + delta) < adj_val)
> +		return -FDT_ERR_BADPHANDLE;

Need to check for adj_val == (uint32_t)-1 as well, because it's an
unsigned comparison the test above won't catch it.

> +
> +	adj_val += delta;
> +	return fdt_setprop_inplace_u32(fdt, node, name, adj_val);
> +}
> +
> +/**
> + * overlay_adjust_node_phandles - Offsets the phandles of a node
> + * @fdto: Device tree overlay blob
> + * @node: Offset of the node we want to adjust
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_adjust_node_phandles() adds a constant to all the phandles
> + * of a given node. This is mainly use as part of the overlay
> + * application process, when we want to update all the overlay
> + * phandles to not conflict with the overlays of the base device tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_adjust_node_phandles(void *fdto, int node,
> +					uint32_t delta)
> +{
> +	unsigned char found = 0;

Just use an int.  I should probably think about introducing bool to libfdt.

> +	int child;
> +	int ret;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;
> +
> +	if (!ret)
> +		found = 1;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;
> +
> +	/*
> +	 * If neither phandle nor linux,phandle have been found return
> +	 * an error.
> +	 */
> +	if (!found && !ret)
> +		return ret;

Clearer to return 0 here, since ret must equal 0.

Except.. it's not mandatory for a node to have a phandle, so this just
looks wrong, anyway.

> +
> +	fdt_for_each_subnode(child, fdto, node)
> +		overlay_adjust_node_phandles(fdto, child, delta);
> +
> +	return 0;
> +}
> +
> +/**
> + * overlay_adjust_local_phandles - Adjust the phandles of a whole overlay
> + * @fdto: Device tree overlay blob
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_adjust_local_phandles() adds a constant to all the
> + * phandles of an overlay. This is mainly use as part of the overlay
> + * application process, when we want to update all the overlay
> + * phandles to not conflict with the overlays of the base device tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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);
> +}
> +
> +/**
> + * overlay_update_local_node_references - Adjust the overlay references
> + * @fdto: Device tree overlay blob
> + * @tree_node: Node offset of the node to operate on
> + * @fixup_node: Node offset of the matching local fixups node
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_update_local_nodes_references() update the phandles
> + * pointing to a node within the device tree overlay by adding a
> + * constant delta.
> + *
> + * This is mainly used as part of a device tree application process,
> + * where you want the device tree overlays phandles to not conflict
> + * with the ones from the base device tree before merging them.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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 unsigned char *fixup_val, *tree_val;

You might as well make fixup_val a u32 *, it should remove some casts
below.

> +		const char *name;
> +		int fixup_len;
> +		int tree_len;
> +		int i;
> +
> +		fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> +						  &name, &fixup_len);
> +		if (!fixup_val)
> +			return fixup_len;
> +
> +		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> +		if (!tree_val)
> +			return tree_len;
> +		for (i = 0; i < fixup_len; i += sizeof(uint32_t)) {

This could lead to an unsafe dereference if fixup_len is not a
multiple of 4 (i.e. badly formatted fixup properyt).  Another reason
to use a u32 *.

> +			uint32_t adj_val, index;
> +
> +			index = *(const uint32_t *)(fixup_val + i);
> +			index = fdt32_to_cpu(index);
> +
> +			/*
> +			 * phandles to fixup can be unaligned.
> +			 *
> +			 * Use a memcpy for the architectures that do
> +			 * not support unaligned accesses.
> +			 */
> +			memcpy(&adj_val, tree_val + index, sizeof(uint32_t));
> +
> +			adj_val = fdt32_to_cpu(adj_val);
> +			adj_val += delta;
> +			adj_val = cpu_to_fdt32(adj_val);
> +
> +			ret = fdt_setprop_inplace_namelen_partial(fdto,
> +								  tree_node,
> +								  name,
> +								  strlen(name),
> +								  index,
> +								  &adj_val,
> +								  sizeof(adj_val));
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	fdt_for_each_subnode(fixup_child, fdto, 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;
> +}
> +
> +/**
> + * overlay_update_local_references - Adjust the overlay references
> + * @fdto: Device tree overlay blob
> + * @delta: Offset to shift the phandles of
> + *
> + * overlay_update_local_references() update all the phandles pointing
> + * to a node within the device tree overlay by adding a constant
> + * delta to not conflict with the base overlay.
> + *
> + * This is mainly used as part of a device tree application process,
> + * where you want the device tree overlays phandles to not conflict
> + * with the ones from the base device tree before merging them.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_update_local_references(void *fdto, uint32_t delta)
> +{
> +	int fixups;
> +
> +	fixups = fdt_path_offset(fdto, "/__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(fdto, 0, fixups,
> +						    delta);
> +}
> +
> +/**
> + * overlay_fixup_one_phandle - Set an overlay phandle to the base one
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbols_off: Node offset of the symbols node in the base device tree
> + * @path: Path to a node holding a phandle in the overlay
> + * @path_len: number of path characters to consider
> + * @name: Name of the property holding the phandle reference in the overlay
> + * @name_len: number of name characters to consider
> + * @index: Index in the overlay property where the phandle is stored
> + * @label: Label of the node referenced by the phandle
> + *
> + * overlay_fixup_one_phandle() resolves an overlay phandle pointing to
> + * a node in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when
> + * you want all the phandles in the overlay to point to the actual
> + * base dt nodes.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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_partial(fdto, fixup_off,
> +						   name, name_len, index,
> +						   &phandle, sizeof(phandle));
> +};
> +
> +/**
> + * overlay_fixup_phandle - Set an overlay phandle to the base one
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbols_off: Node offset of the symbols node in the base device tree
> + * @property: Property offset in the overlay holding the list of fixups
> + *
> + * overlay_fixup_phandle() resolves all the overlay phandles pointed
> + * to in a __local_fixup__ property, and updates them to match the

Uh, this one's the __fixup__ rather than __local_fixups__ property, no?

> + * phandles in use in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when
> + * you want all the phandles in the overlay to point to the actual
> + * base dt nodes.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +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;

As with the case you have below, a NOTFOUND returned by
getprop_by_offset() should return an INTERNAL error from here, since
it means this has been called with bad parameters.

> +	do {
> +		const char *prop_string = value;
> +		const char *path, *name;
> +		uint32_t prop_len = strlen(value);

strlen() isn't safe here, because you could have a badly formatted
fixup with no \0.

> +		uint32_t path_len, name_len;
> +		char *sep, *endptr;
> +		int index;
> +		int ret;
> +
> +		path = prop_string;
> +		sep = memchr(prop_string, ':', prop_len);
> +		if (*sep != ':')

memchr() returns NULL if the separator is not found, so it's not safe
to dereference it without checking that first.

> +			return -FDT_ERR_BADSTRUCTURE;

I wonder if it might be worth adding an error code specifically for
bad fixups.

> +		path_len = sep - path;
> +
> +		name = sep + 1;

You need to check sep against prop_len, for the case of a badly
formatted fixup with a : as the last character.

> +		sep = memchr(name, ':', prop_len);
> +		if (*sep != ':')
> +			return -FDT_ERR_BADSTRUCTURE;
> +		name_len = sep - name;
> +
> +		index = strtoul(sep + 1, &endptr, 10);
> +		if ((*endptr != '\0') || (endptr <= (sep + 1)))
> +			return -FDT_ERR_BADSTRUCTURE;
> +
> +		len -= prop_len + 1;
> +		value += prop_len + 1;
> +
> +		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;
> +}
> +
> +/**
> + * overlay_fixup_phandles - Resolve the overlay phandles to the base
> + *                          device tree
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_fixup_phandles() resolves all the overlay phandles pointing
> + * to nodes in the base device tree.
> + *
> + * This is one of the steps of the device tree overlay application
> + * process, when you want all the phandles in the overlay to point to
> + * the actual base dt nodes.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_fixup_phandles(void *fdt, void *fdto)
> +{
> +	int fixups_off, symbols_off;
> +	int property;
> +
> +	symbols_off = fdt_path_offset(fdt, "/__symbols__");
> +	fixups_off = fdt_path_offset(fdto, "/__fixups__");
> +
> +	fdt_for_each_property_offset(property, fdto, fixups_off) {
> +		int ret;
> +
> +		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * overlay_apply_node - Merge an overlay fragment into the base device tree
> + * @fdt: Base Device Tree blob
> + * @target: Node offset in the base device tree to apply the fragment to
> + * @fdto: Device tree overlay blob
> + * @fragment: Node offset in the overlay holding the changes to merge

fragment is a bad name here - it's accurate on the initial call, but
not on the recursive subcalls when it's an offset to a particular
subtree of the fragment.

> + * overlay_apply_node() merges an overlay fragment into a target base
> + * device tree node pointed.
> + *
> + * This is part of the final step in the device tree overlay
> + * application process, when all the phandles have been adjusted and
> + * resolved and you just have to merge overlay into the base device
> + * tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_apply_node(void *fdt, int target,
> +			      void *fdto, int fragment)
> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property_offset(property, fdto, fragment) {
> +		const char *name;
> +		const void *prop;
> +		int prop_len;
> +		int ret;
> +
> +		prop = fdt_getprop_by_offset(fdto, property, &name,
> +					     &prop_len);
> +		if (prop_len == -FDT_ERR_NOTFOUND)
> +			return -FDT_ERR_INTERNAL;
> +		if (prop_len < 0)
> +			return prop_len;
> +
> +		ret = fdt_setprop(fdt, target, name, prop, prop_len);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(node, fdto, fragment) {
> +		const char *name = fdt_get_name(fdto, node, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(fdt, target, name);
> +		if (nnode == -FDT_ERR_EXISTS)
> +			nnode = fdt_subnode_offset(fdt, target, name);
> +
> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = overlay_apply_node(fdt, nnode, fdto, node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * overlay_merge - Merge an overlay into its base device tree
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_merge() merges an overlay into its base device tree.
> + *
> + * This is the final step in the device tree overlay application
> + * process, when all the phandles have been adjusted and resolved and
> + * you just have to merge overlay into the base device tree.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_merge(void *fdt, void *fdto)
> +{
> +	int fragment;
> +
> +	fdt_for_each_subnode(fragment, fdto, 0) {
> +		int overlay;
> +		int target;
> +		int ret;
> +
> +		target = overlay_get_target(fdt, fdto, fragment);
> +		if (target < 0)
> +			continue;

I really dislike this silent ignoring of nodes if we can't figure out
the target.  If we add new target types in future, then the code
should noisily fail, not silently work incompletely.

> +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +		if (overlay < 0)
> +			return overlay;

Yeah.. I definitely think we should add a new error code for a badly
formatted overlay, and return that here if we got a NOTFOUND.

> +
> +		ret = overlay_apply_node(fdt, target, fdto, 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 err;
> +
> +	/*
> +	 * The overlay has been damaged, erase its magic.
> +	 */
> +	fdt_set_magic(fdto, ~0);
> +
> +	return 0;
> +
> +err:
> +	/*
> +	 * The overlay might have been damaged, erase its magic.
> +	 */
> +	fdt_set_magic(fdto, ~0);
> +
> +	/*
> +	 * The base device tree might have been damaged, erase its
> +	 * magic.
> +	 */
> +	fdt_set_magic(fdt, ~0);
> +
> +	return ret;
> +}
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index a22a16b6878a..a70b8767cabc 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1745,6 +1745,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__
> diff --git a/tests/.gitignore b/tests/.gitignore
> index fa4616ba28c2..6c97066c587e 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -35,6 +35,7 @@ tmp.*
>  /nopulate
>  /notfound
>  /open_pack
> +/overlay
>  /parent_offset
>  /path-references
>  /path_offset
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 196518c83eda..4aefbbd15253 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -24,7 +24,8 @@ LIB_TESTS_L = get_mem_rsv \
>  	utilfdt_test \
>  	integer-expressions \
>  	property_iterate \
> -	subnode_iterate
> +	subnode_iterate \
> +	overlay
>  LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
>  
>  LIBTREE_TESTS_L = truncated_property
> diff --git a/tests/overlay.c b/tests/overlay.c
> new file mode 100644
> index 000000000000..4637a777e50e
> --- /dev/null
> +++ b/tests/overlay.c
> @@ -0,0 +1,228 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + *	Testcase for DT overlays()
> + * Copyright (C) 2016 Free Electrons
> + * Copyright (C) 2016 NextThing Co.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public License
> + * as published by the Free Software Foundation; either version 2.1 of
> + * the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdio.h>
> +
> +#include <libfdt.h>
> +
> +#include "tests.h"
> +
> +#define CHECK(code) \
> +	{ \
> +		if (code) \
> +			FAIL(#code ": %s", fdt_strerror(code)); \
> +	}
> +
> +/* 4k ought to be enough for anybody */
> +#define FDT_COPY_SIZE	(4 * 1024)
> +
> +static int fdt_getprop_u32_by_index(void *fdt, const char *path,
> +				    const char *name, int index,
> +				    unsigned long *out)
> +{
> +	const fdt32_t *val;
> +	int node_off;
> +	int len;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	val = fdt_getprop(fdt, node_off, name, &len);
> +	if (!val || (len < (sizeof(uint32_t) * (index + 1))))
> +		return -FDT_ERR_NOTFOUND;
> +
> +	*out = fdt32_to_cpu(*(val + index));
> +
> +	return 0;
> +}
> +
> +static int check_getprop_string_by_name(void *fdt, const char *path,
> +					const char *name, const char *val)
> +{
> +	int node_off;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	check_getprop_string(fdt, node_off, name, val);
> +
> +	return RC_PASS;
> +}
> +
> +static int check_getprop_u32_by_name(void *fdt, const char *path,
> +				     const char *name, uint32_t val)
> +{
> +	int node_off;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	check_getprop_cell(fdt, node_off, name, val);
> +
> +	return RC_PASS;
> +}
> +
> +static int check_getprop_null_by_name(void *fdt, const char *path,
> +				      const char *name)
> +{
> +	int node_off;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	check_property(fdt, node_off, name, 0, NULL);
> +
> +	return RC_PASS;
> +}
> +
> +static int fdt_overlay_change_int_property(void *fdt)
> +{
> +	return check_getprop_u32_by_name(fdt, "/test-node", "test-int-property",
> +					 43);
> +}
> +
> +static int fdt_overlay_change_str_property(void *fdt)
> +{
> +	return check_getprop_string_by_name(fdt, "/test-node",
> +					    "test-str-property", "foobar");
> +}
> +
> +static int fdt_overlay_add_str_property(void *fdt)
> +{
> +	return check_getprop_string_by_name(fdt, "/test-node",
> +					    "test-str-property-2", "foobar2");
> +}
> +
> +static int fdt_overlay_add_node_by_phandle(void *fdt)
> +{
> +	return check_getprop_null_by_name(fdt, "/test-node/new-node",
> +					  "new-property");
> +}
> +
> +static int fdt_overlay_add_node_by_path(void *fdt)
> +{
> +	return check_getprop_null_by_name(fdt, "/new-node", "new-property");
> +}
> +
> +static int fdt_overlay_add_subnode_property(void *fdt)
> +{
> +	check_getprop_null_by_name(fdt, "/test-node/sub-test-node",
> +				   "sub-test-property");
> +	check_getprop_null_by_name(fdt, "/test-node/sub-test-node",
> +				   "new-sub-test-property");
> +
> +	return RC_PASS;
> +}
> +
> +static int fdt_overlay_local_phandle(void *fdt)
> +{
> +	uint32_t local_phandle;
> +	unsigned long val = 0;
> +	int off;
> +
> +	off = fdt_path_offset(fdt, "/new-local-node");
> +	CHECK(off < 0);
> +
> +	local_phandle = fdt_get_phandle(fdt, off);
> +	CHECK(!local_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-several-phandle",
> +				       0, &val));
> +	CHECK(val != local_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-several-phandle",
> +				       1, &val));
> +	CHECK(val != local_phandle);
> +
> +	return RC_PASS;
> +}
> +
> +static int fdt_overlay_local_phandles(void *fdt)
> +{
> +	uint32_t local_phandle, test_phandle;
> +	unsigned long val = 0;
> +	int off;
> +
> +	off = fdt_path_offset(fdt, "/new-local-node");
> +	CHECK(off < 0);
> +
> +	local_phandle = fdt_get_phandle(fdt, off);
> +	CHECK(!local_phandle);
> +
> +	off = fdt_path_offset(fdt, "/test-node");
> +	CHECK(off < 0);
> +
> +	test_phandle = fdt_get_phandle(fdt, off);
> +	CHECK(!test_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-phandle", 0, &val));
> +	CHECK(test_phandle != val);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/", "test-phandle", 1, &val));
> +	CHECK(local_phandle != val);
> +
> +	return RC_PASS;
> +}
> +
> +static void *open_dt(char *path)
> +{
> +	void *dt, *copy;
> +
> +	dt = load_blob(path);
> +	copy = xmalloc(FDT_COPY_SIZE);
> +
> +	/*
> +	 * Resize our DTs to 4k so that we have room to operate on
> +	 */
> +	CHECK(fdt_open_into(dt, copy, FDT_COPY_SIZE));
> +
> +	return copy;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	void *fdt_base, *fdt_overlay;
> +
> +	test_init(argc, argv);
> +	if (argc != 3)
> +		CONFIG("Usage: %s <base dtb> <overlay dtb>", argv[0]);
> +
> +	fdt_base = open_dt(argv[1]);
> +	fdt_overlay = open_dt(argv[2]);
> +
> +	/* Apply the overlay */
> +	CHECK(fdt_overlay_apply(fdt_base, fdt_overlay));
> +
> +	fdt_overlay_change_int_property(fdt_base);
> +	fdt_overlay_change_str_property(fdt_base);
> +	fdt_overlay_add_str_property(fdt_base);
> +	fdt_overlay_add_node_by_phandle(fdt_base);
> +	fdt_overlay_add_node_by_path(fdt_base);
> +	fdt_overlay_add_subnode_property(fdt_base);
> +	fdt_overlay_local_phandle(fdt_base);
> +	fdt_overlay_local_phandles(fdt_base);
> +
> +	PASS();
> +}
> diff --git a/tests/overlay_base.dts b/tests/overlay_base.dts
> new file mode 100644
> index 000000000000..2603adb6821e
> --- /dev/null
> +++ b/tests/overlay_base.dts
> @@ -0,0 +1,21 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +	test: test-node {
> +		test-int-property = <42>;
> +		test-str-property = "foo";
> +
> +		subtest: sub-test-node {
> +			sub-test-property;
> +		};
> +	};
> +};
> +
> +
> diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
> new file mode 100644
> index 000000000000..d30ecdf3661c
> --- /dev/null
> +++ b/tests/overlay_overlay.dts
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	/* Test that we can change an int by another */
> +	fragment@0 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-int-property = <43>;
> +		};
> +	};
> +
> +	/* Test that we can replace a string by a longer one */
> +	fragment@1 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-str-property = "foobar";
> +		};
> +	};
> +
> +	/* Test that we add a new property */
> +	fragment@2 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-str-property-2 = "foobar2";
> +		};
> +	};
> +
> +	/* Test that we add a new node (by phandle) */
> +	fragment@3 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			new-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	/* Test that we add a new node (by path) */
> +	fragment@4 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			new-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	fragment@5 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			local: new-local-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	fragment@6 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			test-phandle = <&test>, <&local>;
> +		};
> +	};
> +
> +	fragment@7 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			test-several-phandle = <&local>, <&local>;
> +		};
> +	};
> +
> +	fragment@8 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			sub-test-node {
> +				new-sub-test-property;
> +			};
> +		};
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 6a2662b2abaf..054758298de9 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -280,6 +280,14 @@ libfdt_tests () {
>      run_test get_alias aliases.dtb
>      run_test path_offset_aliases aliases.dtb
>  
> +    # Overlay tests
> +    echo "/dts-v1/; / {};" | $DTC -@ > /dev/null 2>&1

       ^^^^^

What's this about?

> +    if [ $? -eq 0 ]; then
> +        run_dtc_test -@ -I dts -O dtb -o overlay_base.dtb overlay_base.dts
> +        run_dtc_test -I dts -O dtb -o overlay_overlay.dtb overlay_overlay.dts
> +        run_test overlay overlay_base.dtb overlay_overlay.dtb
> +    fi
> +
>      # Specific bug tests
>      run_test add_subnode_with_nops
>      run_dtc_test -I dts -O dts -o sourceoutput.test.dts sourceoutput.dts

-- 
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 v2 6/6] libfdt: Add overlay application function

[David Gibson]

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

On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> On 11/07/2016 20:56, Maxime Ripard wrote:
[snip]

> > +static int overlay_merge(void *fdt, void *fdto)
> > +{
> > +	int fragment;
> > +
> > +	fdt_for_each_subnode(fragment, fdto, 0) {
> > +		int overlay;
> > +		int target;
> > +		int ret;
> > +
> > +		target = overlay_get_target(fdt, fdto, fragment);
> > +		if (target < 0)
> > +			continue;
> > +
> > +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> > +		if (overlay < 0)
> > +			return overlay;

> Why does the absence of a target cause a fragment to be ignored but
> the absence of an "__overlay__" property cause the merging to be
> abandoned with an error? Can't we just ignore fragments that aren't
> recognised?

So, I had the same question.  But fragments we can't make sense MUST
cause failures, and not be silently ignored.

An incompletely applied overlay is almost certainly going to cause you
horrible grief at some point, so you absolutely want to know early if
your overlay is in a format your tool doesn't understand.

-- 
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 v2 6/6] libfdt: Add overlay application function

[Phil Elwell]

On Tue, Jul 12, 2016 at 3:34 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
>> On 11/07/2016 20:56, Maxime Ripard wrote:
> [snip]
>
>> > +static int overlay_merge(void *fdt, void *fdto)
>> > +{
>> > +   int fragment;
>> > +
>> > +   fdt_for_each_subnode(fragment, fdto, 0) {
>> > +           int overlay;
>> > +           int target;
>> > +           int ret;
>> > +
>> > +           target = overlay_get_target(fdt, fdto, fragment);
>> > +           if (target < 0)
>> > +                   continue;
>> > +
>> > +           overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
>> > +           if (overlay < 0)
>> > +                   return overlay;
>
>> Why does the absence of a target cause a fragment to be ignored but
>> the absence of an "__overlay__" property cause the merging to be
>> abandoned with an error? Can't we just ignore fragments that aren't
>> recognised?
>
> So, I had the same question.  But fragments we can't make sense MUST
> cause failures, and not be silently ignored.
>
> An incompletely applied overlay is almost certainly going to cause you
> horrible grief at some point, so you absolutely want to know early if
> your overlay is in a format your tool doesn't understand.

Cards on the table time - at Raspberry Pi we've found it convenient to
selectively enable or disable fragments within an overlay based on
user-supplied parameters. The way I've implemented this is by using
"__dormant__" as an antonym to "__overlay__" (partly chosen because it
is the same length), and then pre-processing the overlay before
applying it. This scheme works with the kernel overlay support and our
firmware loader (obviously).

Although I could invent a similar scheme that uses antonyms of
"target" and "target-path", I don't see why this is inherently safer.

Phil (phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org)

Re: [PATCH v2 6/6] libfdt: Add overlay application function

[David Gibson]

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

On Tue, Jul 12, 2016 at 04:07:49PM +0100, Phil Elwell wrote:
> On Tue, Jul 12, 2016 at 3:34 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> >> On 11/07/2016 20:56, Maxime Ripard wrote:
> > [snip]
> >
> >> > +static int overlay_merge(void *fdt, void *fdto)
> >> > +{
> >> > +   int fragment;
> >> > +
> >> > +   fdt_for_each_subnode(fragment, fdto, 0) {
> >> > +           int overlay;
> >> > +           int target;
> >> > +           int ret;
> >> > +
> >> > +           target = overlay_get_target(fdt, fdto, fragment);
> >> > +           if (target < 0)
> >> > +                   continue;
> >> > +
> >> > +           overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> >> > +           if (overlay < 0)
> >> > +                   return overlay;
> >
> >> Why does the absence of a target cause a fragment to be ignored but
> >> the absence of an "__overlay__" property cause the merging to be
> >> abandoned with an error? Can't we just ignore fragments that aren't
> >> recognised?
> >
> > So, I had the same question.  But fragments we can't make sense MUST
> > cause failures, and not be silently ignored.
> >
> > An incompletely applied overlay is almost certainly going to cause you
> > horrible grief at some point, so you absolutely want to know early if
> > your overlay is in a format your tool doesn't understand.
> 
> Cards on the table time - at Raspberry Pi we've found it convenient to
> selectively enable or disable fragments within an overlay based on
> user-supplied parameters. The way I've implemented this is by using
> "__dormant__" as an antonym to "__overlay__" (partly chosen because it
> is the same length), and then pre-processing the overlay before
> applying it. This scheme works with the kernel overlay support and our
> firmware loader (obviously).

Ok, if you want that feature, let's invent a mechanism specifically
for conditionally disabling chunks, rather than using a side-effect
which will break horribly as soon as we extend the overlay format.

Alternatively you could just use fdt_del_node() or fdt_nop_node() to
remove the fragments you don't wish to apply.

> Although I could invent a similar scheme that uses antonyms of
> "target" and "target-path", I don't see why this is inherently safer.



-- 
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 v2 6/6] libfdt: Add overlay application function

[Maxime Ripard]

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

Hi David,

On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
> On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> > On 11/07/2016 20:56, Maxime Ripard wrote:
> [snip]
> 
> > > +static int overlay_merge(void *fdt, void *fdto)
> > > +{
> > > +	int fragment;
> > > +
> > > +	fdt_for_each_subnode(fragment, fdto, 0) {
> > > +		int overlay;
> > > +		int target;
> > > +		int ret;
> > > +
> > > +		target = overlay_get_target(fdt, fdto, fragment);
> > > +		if (target < 0)
> > > +			continue;
> > > +
> > > +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> > > +		if (overlay < 0)
> > > +			return overlay;
> 
> > Why does the absence of a target cause a fragment to be ignored but
> > the absence of an "__overlay__" property cause the merging to be
> > abandoned with an error? Can't we just ignore fragments that aren't
> > recognised?
> 
> So, I had the same question.  But fragments we can't make sense MUST
> cause failures, and not be silently ignored.
> 
> An incompletely applied overlay is almost certainly going to cause you
> horrible grief at some point, so you absolutely want to know early if
> your overlay is in a format your tool doesn't understand.

I'm not sure how we can achieve that without applying it once, and see
if it fails. The obvious things are easy to detect (like a missing
__overlay__ node), but some others really aren't (like a poorly
formatted phandle, or one that overflows) without applying it
entirely. And that seems difficult without malloc.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 6/6] libfdt: Add overlay application function

[David Gibson]

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

On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:
> Hi David,
> 
> On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
> > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> > > On 11/07/2016 20:56, Maxime Ripard wrote:
> > [snip]
> > 
> > > > +static int overlay_merge(void *fdt, void *fdto)
> > > > +{
> > > > +	int fragment;
> > > > +
> > > > +	fdt_for_each_subnode(fragment, fdto, 0) {
> > > > +		int overlay;
> > > > +		int target;
> > > > +		int ret;
> > > > +
> > > > +		target = overlay_get_target(fdt, fdto, fragment);
> > > > +		if (target < 0)
> > > > +			continue;
> > > > +
> > > > +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> > > > +		if (overlay < 0)
> > > > +			return overlay;
> > 
> > > Why does the absence of a target cause a fragment to be ignored but
> > > the absence of an "__overlay__" property cause the merging to be
> > > abandoned with an error? Can't we just ignore fragments that aren't
> > > recognised?
> > 
> > So, I had the same question.  But fragments we can't make sense MUST
> > cause failures, and not be silently ignored.
> > 
> > An incompletely applied overlay is almost certainly going to cause you
> > horrible grief at some point, so you absolutely want to know early if
> > your overlay is in a format your tool doesn't understand.
> 
> I'm not sure how we can achieve that without applying it once, and see
> if it fails. The obvious things are easy to detect (like a missing
> __overlay__ node), but some others really aren't (like a poorly
> formatted phandle, or one that overflows) without applying it
> entirely. And that seems difficult without malloc.

So, atomically applying either the whole overlay or nothing would be a
nice property, but it is indeed infeasibly difficult to achieve
without malloc().  Well.. we sort of could by making apply_overlay()
take an output buffer separate from the base tree, but that's not what
I'm suggesting.

I'm fine with the base tree being trashed with an incomplete
application when apply_overlay() reports failure.  WHat I'm not ok
with is *silent* failure.  If you ignore fragments you don't
understand, then - if the overlay uses features that aren't supported
by this version of the code - you'll end up with an incompletely
applied overlay while the apply_overlay() function *reports success*.
That is a recipe for disaster.

-- 
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 v2 6/6] libfdt: Add overlay application function

[Maxime Ripard]

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

On Thu, Jul 14, 2016 at 01:07:45AM +1000, David Gibson wrote:
> On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:
> > Hi David,
> > 
> > On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
> > > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> > > > On 11/07/2016 20:56, Maxime Ripard wrote:
> > > [snip]
> > > 
> > > > > +static int overlay_merge(void *fdt, void *fdto)
> > > > > +{
> > > > > +	int fragment;
> > > > > +
> > > > > +	fdt_for_each_subnode(fragment, fdto, 0) {
> > > > > +		int overlay;
> > > > > +		int target;
> > > > > +		int ret;
> > > > > +
> > > > > +		target = overlay_get_target(fdt, fdto, fragment);
> > > > > +		if (target < 0)
> > > > > +			continue;
> > > > > +
> > > > > +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> > > > > +		if (overlay < 0)
> > > > > +			return overlay;
> > > 
> > > > Why does the absence of a target cause a fragment to be ignored but
> > > > the absence of an "__overlay__" property cause the merging to be
> > > > abandoned with an error? Can't we just ignore fragments that aren't
> > > > recognised?
> > > 
> > > So, I had the same question.  But fragments we can't make sense MUST
> > > cause failures, and not be silently ignored.
> > > 
> > > An incompletely applied overlay is almost certainly going to cause you
> > > horrible grief at some point, so you absolutely want to know early if
> > > your overlay is in a format your tool doesn't understand.
> > 
> > I'm not sure how we can achieve that without applying it once, and see
> > if it fails. The obvious things are easy to detect (like a missing
> > __overlay__ node), but some others really aren't (like a poorly
> > formatted phandle, or one that overflows) without applying it
> > entirely. And that seems difficult without malloc.
> 
> So, atomically applying either the whole overlay or nothing would be a
> nice property, but it is indeed infeasibly difficult to achieve
> without malloc().  Well.. we sort of could by making apply_overlay()
> take an output buffer separate from the base tree, but that's not what
> I'm suggesting.
> 
> I'm fine with the base tree being trashed with an incomplete
> application when apply_overlay() reports failure.  WHat I'm not ok
> with is *silent* failure.  If you ignore fragments you don't
> understand, then - if the overlay uses features that aren't supported
> by this version of the code - you'll end up with an incompletely
> applied overlay while the apply_overlay() function *reports success*.
> That is a recipe for disaster.

Ok, that makes sense. I'll return an error if the target is missing as
well then.

But then, I think we fall back to the discussion you had with
Pantelis: how do you identify an overlay node (that must have a
target) and some other "metadata" node that shouldn't be applied (and
will not have a target). In the first case, we need to report an error
if it's missing. In the second, we should just ignore the node
entirely.

Would turning that code the other way around, and if it has an
__overlay__ subnode, target or target-path is mandatory, and if not
just ignore the node entirely, work for you?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 6/6] libfdt: Add overlay application function

[Maxime Ripard]

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

Hi David,

I'll fix your comments, but I have a bunch of questions, see below.

On Wed, Jul 13, 2016 at 12:31:20AM +1000, David Gibson wrote:
> > +/**
> > + * overlay_adjust_node_phandles - Offsets the phandles of a node
> > + * @fdto: Device tree overlay blob
> > + * @node: Offset of the node we want to adjust
> > + * @delta: Offset to shift the phandles of
> > + *
> > + * overlay_adjust_node_phandles() adds a constant to all the phandles
> > + * of a given node. This is mainly use as part of the overlay
> > + * application process, when we want to update all the overlay
> > + * phandles to not conflict with the overlays of the base device tree.
> > + *
> > + * returns:
> > + *      0 on success
> > + *      Negative error code on failure
> > + */
> > +static int overlay_adjust_node_phandles(void *fdto, int node,
> > +					uint32_t delta)
> > +{
> > +	unsigned char found = 0;
> 
> Just use an int.  I should probably think about introducing bool to libfdt.
> 
> > +	int child;
> > +	int ret;
> > +
> > +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > +		return ret;
> > +
> > +	if (!ret)
> > +		found = 1;
> > +
> > +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > +		return ret;
> > +
> > +	/*
> > +	 * If neither phandle nor linux,phandle have been found return
> > +	 * an error.
> > +	 */
> > +	if (!found && !ret)
> > +		return ret;
> 
> Clearer to return 0 here, since ret must equal 0.
> 
> Except.. it's not mandatory for a node to have a phandle, so this just
> looks wrong, anyway.

Hmmm, that's true. I have no idea how that can possibly work
though. It should break at the very first node, since the root node
will not have one...

I'll look into this.

> 
> > +
> > +	fdt_for_each_subnode(child, fdto, node)
> > +		overlay_adjust_node_phandles(fdto, child, delta);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * overlay_adjust_local_phandles - Adjust the phandles of a whole overlay
> > + * @fdto: Device tree overlay blob
> > + * @delta: Offset to shift the phandles of
> > + *
> > + * overlay_adjust_local_phandles() adds a constant to all the
> > + * phandles of an overlay. This is mainly use as part of the overlay
> > + * application process, when we want to update all the overlay
> > + * phandles to not conflict with the overlays of the base device tree.
> > + *
> > + * returns:
> > + *      0 on success
> > + *      Negative error code on failure
> > + */
> > +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);
> > +}
> > +
> > +/**
> > + * overlay_update_local_node_references - Adjust the overlay references
> > + * @fdto: Device tree overlay blob
> > + * @tree_node: Node offset of the node to operate on
> > + * @fixup_node: Node offset of the matching local fixups node
> > + * @delta: Offset to shift the phandles of
> > + *
> > + * overlay_update_local_nodes_references() update the phandles
> > + * pointing to a node within the device tree overlay by adding a
> > + * constant delta.
> > + *
> > + * This is mainly used as part of a device tree application process,
> > + * where you want the device tree overlays phandles to not conflict
> > + * with the ones from the base device tree before merging them.
> > + *
> > + * returns:
> > + *      0 on success
> > + *      Negative error code on failure
> > + */
> > +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 unsigned char *fixup_val, *tree_val;
> 
> You might as well make fixup_val a u32 *, it should remove some casts
> below.

Yes, but then, if it's poorly formatted, we might get unaligned
accesses too.. And we can't really use memcpy here, since we don't
know the size of the property at compile time.

> > +		const char *name;
> > +		int fixup_len;
> > +		int tree_len;
> > +		int i;
> > +
> > +		fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> > +						  &name, &fixup_len);
> > +		if (!fixup_val)
> > +			return fixup_len;
> > +
> > +		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> > +		if (!tree_val)
> > +			return tree_len;
> > +		for (i = 0; i < fixup_len; i += sizeof(uint32_t)) {
> 
> This could lead to an unsafe dereference if fixup_len is not a
> multiple of 4 (i.e. badly formatted fixup properyt).  Another reason
> to use a u32 *.

Then maybe the safest would be to simply check that the length is a
multiple of 4, and return an error if it's not?

> > diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> > index 6a2662b2abaf..054758298de9 100755
> > --- a/tests/run_tests.sh
> > +++ b/tests/run_tests.sh
> > @@ -280,6 +280,14 @@ libfdt_tests () {
> >      run_test get_alias aliases.dtb
> >      run_test path_offset_aliases aliases.dtb
> >  
> > +    # Overlay tests
> > +    echo "/dts-v1/; / {};" | $DTC -@ > /dev/null 2>&1
> 
>        ^^^^^
> 
> What's this about?

In current master tree, calling dtc with -@ will fail, since the
support to compile the overlays is not upstream yet (afaik).

This is just to make sur that the dtc binary is actually able to
compile something we can run our test on. Otherwise we would have
failed tests, while the code might or might not work, which seems bad.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 6/6] libfdt: Add overlay application function

[David Gibson]

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

On Wed, Jul 13, 2016 at 09:37:57PM +0200, Maxime Ripard wrote:
> On Thu, Jul 14, 2016 at 01:07:45AM +1000, David Gibson wrote:
> > On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:
> > > Hi David,
> > > 
> > > On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
> > > > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> > > > > On 11/07/2016 20:56, Maxime Ripard wrote:
> > > > [snip]
> > > > 
> > > > > > +static int overlay_merge(void *fdt, void *fdto)
> > > > > > +{
> > > > > > +	int fragment;
> > > > > > +
> > > > > > +	fdt_for_each_subnode(fragment, fdto, 0) {
> > > > > > +		int overlay;
> > > > > > +		int target;
> > > > > > +		int ret;
> > > > > > +
> > > > > > +		target = overlay_get_target(fdt, fdto, fragment);
> > > > > > +		if (target < 0)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> > > > > > +		if (overlay < 0)
> > > > > > +			return overlay;
> > > > 
> > > > > Why does the absence of a target cause a fragment to be ignored but
> > > > > the absence of an "__overlay__" property cause the merging to be
> > > > > abandoned with an error? Can't we just ignore fragments that aren't
> > > > > recognised?
> > > > 
> > > > So, I had the same question.  But fragments we can't make sense MUST
> > > > cause failures, and not be silently ignored.
> > > > 
> > > > An incompletely applied overlay is almost certainly going to cause you
> > > > horrible grief at some point, so you absolutely want to know early if
> > > > your overlay is in a format your tool doesn't understand.
> > > 
> > > I'm not sure how we can achieve that without applying it once, and see
> > > if it fails. The obvious things are easy to detect (like a missing
> > > __overlay__ node), but some others really aren't (like a poorly
> > > formatted phandle, or one that overflows) without applying it
> > > entirely. And that seems difficult without malloc.
> > 
> > So, atomically applying either the whole overlay or nothing would be a
> > nice property, but it is indeed infeasibly difficult to achieve
> > without malloc().  Well.. we sort of could by making apply_overlay()
> > take an output buffer separate from the base tree, but that's not what
> > I'm suggesting.
> > 
> > I'm fine with the base tree being trashed with an incomplete
> > application when apply_overlay() reports failure.  WHat I'm not ok
> > with is *silent* failure.  If you ignore fragments you don't
> > understand, then - if the overlay uses features that aren't supported
> > by this version of the code - you'll end up with an incompletely
> > applied overlay while the apply_overlay() function *reports success*.
> > That is a recipe for disaster.
> 
> Ok, that makes sense. I'll return an error if the target is missing as
> well then.
> 
> But then, I think we fall back to the discussion you had with
> Pantelis: how do you identify an overlay node (that must have a
> target) and some other "metadata" node that shouldn't be applied (and
> will not have a target). In the first case, we need to report an error
> if it's missing. In the second, we should just ignore the node
> entirely.

Right.  I can see two obvious approaches:

     1. All (top-level) nodes named fragment@* are assumed to be
        overlay fragments.

     2. All top-evel nodes with a subnode named '__overlay__' are
        assumed to be overlay fragments

(2) differs from looking for target properties because whatever
target variants we add in future, they're still likely to want an
__overlay__ node.  Or at worst, we can add a dummy __overlay__ node to
them.     

> Would turning that code the other way around, and if it has an
> __overlay__ subnode, target or target-path is mandatory, and if not
> just ignore the node entirely, work for you?

I'd prefer to pick a single defining factor for the overlay fragments,
rather than a grab bag of options.

-- 
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 v2 6/6] libfdt: Add overlay application function

[David Gibson]

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

On Wed, Jul 13, 2016 at 10:12:16PM +0200, Maxime Ripard wrote:
> Hi David,
> 
> I'll fix your comments, but I have a bunch of questions, see below.
> 
> On Wed, Jul 13, 2016 at 12:31:20AM +1000, David Gibson wrote:
> > > +/**
> > > + * overlay_adjust_node_phandles - Offsets the phandles of a node
> > > + * @fdto: Device tree overlay blob
> > > + * @node: Offset of the node we want to adjust
> > > + * @delta: Offset to shift the phandles of
> > > + *
> > > + * overlay_adjust_node_phandles() adds a constant to all the phandles
> > > + * of a given node. This is mainly use as part of the overlay
> > > + * application process, when we want to update all the overlay
> > > + * phandles to not conflict with the overlays of the base device tree.
> > > + *
> > > + * returns:
> > > + *      0 on success
> > > + *      Negative error code on failure
> > > + */
> > > +static int overlay_adjust_node_phandles(void *fdto, int node,
> > > +					uint32_t delta)
> > > +{
> > > +	unsigned char found = 0;
> > 
> > Just use an int.  I should probably think about introducing bool to libfdt.
> > 
> > > +	int child;
> > > +	int ret;
> > > +
> > > +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> > > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > > +		return ret;
> > > +
> > > +	if (!ret)
> > > +		found = 1;
> > > +
> > > +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> > > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * If neither phandle nor linux,phandle have been found return
> > > +	 * an error.
> > > +	 */
> > > +	if (!found && !ret)
> > > +		return ret;
> > 
> > Clearer to return 0 here, since ret must equal 0.
> > 
> > Except.. it's not mandatory for a node to have a phandle, so this just
> > looks wrong, anyway.
> 
> Hmmm, that's true. I have no idea how that can possibly work
> though. It should break at the very first node, since the root node
> will not have one...
> 
> I'll look into this.

Ok.

> > > +
> > > +	fdt_for_each_subnode(child, fdto, node)
> > > +		overlay_adjust_node_phandles(fdto, child, delta);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * overlay_adjust_local_phandles - Adjust the phandles of a whole overlay
> > > + * @fdto: Device tree overlay blob
> > > + * @delta: Offset to shift the phandles of
> > > + *
> > > + * overlay_adjust_local_phandles() adds a constant to all the
> > > + * phandles of an overlay. This is mainly use as part of the overlay
> > > + * application process, when we want to update all the overlay
> > > + * phandles to not conflict with the overlays of the base device tree.
> > > + *
> > > + * returns:
> > > + *      0 on success
> > > + *      Negative error code on failure
> > > + */
> > > +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);
> > > +}
> > > +
> > > +/**
> > > + * overlay_update_local_node_references - Adjust the overlay references
> > > + * @fdto: Device tree overlay blob
> > > + * @tree_node: Node offset of the node to operate on
> > > + * @fixup_node: Node offset of the matching local fixups node
> > > + * @delta: Offset to shift the phandles of
> > > + *
> > > + * overlay_update_local_nodes_references() update the phandles
> > > + * pointing to a node within the device tree overlay by adding a
> > > + * constant delta.
> > > + *
> > > + * This is mainly used as part of a device tree application process,
> > > + * where you want the device tree overlays phandles to not conflict
> > > + * with the ones from the base device tree before merging them.
> > > + *
> > > + * returns:
> > > + *      0 on success
> > > + *      Negative error code on failure
> > > + */
> > > +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 unsigned char *fixup_val, *tree_val;
> > 
> > You might as well make fixup_val a u32 *, it should remove some casts
> > below.
> 
> Yes, but then, if it's poorly formatted, we might get unaligned
> accesses too..

No.  The flattening format guarantees that property values are 32-bit
aligned, so since the property consists of nothing but u32s they'll
all remain aligned.

The difference with applying the fixups is that the properties could
have formats that are a mixture of u32s and strings or other unaligned
data, so the locations to be fixed may not be aligned.

> And we can't really use memcpy here, since we don't
> know the size of the property at compile time.

Um.. what?

> 
> > > +		const char *name;
> > > +		int fixup_len;
> > > +		int tree_len;
> > > +		int i;
> > > +
> > > +		fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> > > +						  &name, &fixup_len);
> > > +		if (!fixup_val)
> > > +			return fixup_len;
> > > +
> > > +		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> > > +		if (!tree_val)
> > > +			return tree_len;
> > > +		for (i = 0; i < fixup_len; i += sizeof(uint32_t)) {
> > 
> > This could lead to an unsafe dereference if fixup_len is not a
> > multiple of 4 (i.e. badly formatted fixup properyt).  Another reason
> > to use a u32 *.
> 
> Then maybe the safest would be to simply check that the length is a
> multiple of 4, and return an error if it's not?

Yes, I think so.

> > > diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> > > index 6a2662b2abaf..054758298de9 100755
> > > --- a/tests/run_tests.sh
> > > +++ b/tests/run_tests.sh
> > > @@ -280,6 +280,14 @@ libfdt_tests () {
> > >      run_test get_alias aliases.dtb
> > >      run_test path_offset_aliases aliases.dtb
> > >  
> > > +    # Overlay tests
> > > +    echo "/dts-v1/; / {};" | $DTC -@ > /dev/null 2>&1
> > 
> >        ^^^^^
> > 
> > What's this about?
> 
> In current master tree, calling dtc with -@ will fail, since the
> support to compile the overlays is not upstream yet (afaik).
> 
> This is just to make sur that the dtc binary is actually able to
> compile something we can run our test on. Otherwise we would have
> failed tests, while the code might or might not work, which seems bad.

Ah, yes.  I was mixing up the dtc patches with the libfdt patches.

Hrm.. it would be nice if the libfdt testcases, or at least some of
the, didn't require the patched dtc.  It might be worth making
examples with manually constructed __symbols__ and __fixups__ for this
purpose.

-- 
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 v2 6/6] libfdt: Add overlay application function

[Phil Elwell]

On Thu, Jul 14, 2016 at 9:30 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Jul 13, 2016 at 09:37:57PM +0200, Maxime Ripard wrote:
>> On Thu, Jul 14, 2016 at 01:07:45AM +1000, David Gibson wrote:
>> > On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:
>> > > Hi David,
>> > >
>> > > On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
>> > > > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
>> > > > > On 11/07/2016 20:56, Maxime Ripard wrote:
>> > > > [snip]
>> > > >
>> > > > > > +static int overlay_merge(void *fdt, void *fdto)
>> > > > > > +{
>> > > > > > +   int fragment;
>> > > > > > +
>> > > > > > +   fdt_for_each_subnode(fragment, fdto, 0) {
>> > > > > > +           int overlay;
>> > > > > > +           int target;
>> > > > > > +           int ret;
>> > > > > > +
>> > > > > > +           target = overlay_get_target(fdt, fdto, fragment);
>> > > > > > +           if (target < 0)
>> > > > > > +                   continue;
>> > > > > > +
>> > > > > > +           overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
>> > > > > > +           if (overlay < 0)
>> > > > > > +                   return overlay;
>> > > >
>> > > > > Why does the absence of a target cause a fragment to be ignored but
>> > > > > the absence of an "__overlay__" property cause the merging to be
>> > > > > abandoned with an error? Can't we just ignore fragments that aren't
>> > > > > recognised?
>> > > >
>> > > > So, I had the same question.  But fragments we can't make sense MUST
>> > > > cause failures, and not be silently ignored.
>> > > >
>> > > > An incompletely applied overlay is almost certainly going to cause you
>> > > > horrible grief at some point, so you absolutely want to know early if
>> > > > your overlay is in a format your tool doesn't understand.
>> > >
>> > > I'm not sure how we can achieve that without applying it once, and see
>> > > if it fails. The obvious things are easy to detect (like a missing
>> > > __overlay__ node), but some others really aren't (like a poorly
>> > > formatted phandle, or one that overflows) without applying it
>> > > entirely. And that seems difficult without malloc.
>> >
>> > So, atomically applying either the whole overlay or nothing would be a
>> > nice property, but it is indeed infeasibly difficult to achieve
>> > without malloc().  Well.. we sort of could by making apply_overlay()
>> > take an output buffer separate from the base tree, but that's not what
>> > I'm suggesting.
>> >
>> > I'm fine with the base tree being trashed with an incomplete
>> > application when apply_overlay() reports failure.  WHat I'm not ok
>> > with is *silent* failure.  If you ignore fragments you don't
>> > understand, then - if the overlay uses features that aren't supported
>> > by this version of the code - you'll end up with an incompletely
>> > applied overlay while the apply_overlay() function *reports success*.
>> > That is a recipe for disaster.
>>
>> Ok, that makes sense. I'll return an error if the target is missing as
>> well then.
>>
>> But then, I think we fall back to the discussion you had with
>> Pantelis: how do you identify an overlay node (that must have a
>> target) and some other "metadata" node that shouldn't be applied (and
>> will not have a target). In the first case, we need to report an error
>> if it's missing. In the second, we should just ignore the node
>> entirely.
>
> Right.  I can see two obvious approaches:
>
>      1. All (top-level) nodes named fragment@* are assumed to be
>         overlay fragments.
>
>      2. All top-evel nodes with a subnode named '__overlay__' are
>         assumed to be overlay fragments
>
> (2) differs from looking for target properties because whatever
> target variants we add in future, they're still likely to want an
> __overlay__ node.  Or at worst, we can add a dummy __overlay__ node to
> them.
>
>> Would turning that code the other way around, and if it has an
>> __overlay__ subnode, target or target-path is mandatory, and if not
>> just ignore the node entirely, work for you?
>
> I'd prefer to pick a single defining factor for the overlay fragments,
> rather than a grab bag of options.

I think it's potentially dangerous using the presence of a particular
property to identify an overlay - what if the property name ("target",
say) was also the name of a label or property within one of the
fragments? It could then appear in the __symbols__ or __fixups__ node,
causing it to be treated as a potential overlay, but application will
then be halted with an error because it has no __overlay__ subnode.

If we make the presence of the __overlay__ subnode the decision point
(which is nicer anyway because it is a single test rather than two -
"target" and "target-path") then we avoid this hazard - __symbols__
and __fixups__ have no subnodes, and __local_fixups__ only has
top-level nodes as its subnodes, meaning __overlay__ will always be
one level further down.

Phil - Raspberry Pi

Re: [PATCH v2 6/6] libfdt: Add overlay application function

[David Gibson]

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

On Fri, Jul 15, 2016 at 10:18:04AM +0100, Phil Elwell wrote:
> On Thu, Jul 14, 2016 at 9:30 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Jul 13, 2016 at 09:37:57PM +0200, Maxime Ripard wrote:
> >> On Thu, Jul 14, 2016 at 01:07:45AM +1000, David Gibson wrote:
> >> > On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:
> >> > > Hi David,
> >> > >
> >> > > On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
> >> > > > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> >> > > > > On 11/07/2016 20:56, Maxime Ripard wrote:
> >> > > > [snip]
> >> > > >
> >> > > > > > +static int overlay_merge(void *fdt, void *fdto)
> >> > > > > > +{
> >> > > > > > +   int fragment;
> >> > > > > > +
> >> > > > > > +   fdt_for_each_subnode(fragment, fdto, 0) {
> >> > > > > > +           int overlay;
> >> > > > > > +           int target;
> >> > > > > > +           int ret;
> >> > > > > > +
> >> > > > > > +           target = overlay_get_target(fdt, fdto, fragment);
> >> > > > > > +           if (target < 0)
> >> > > > > > +                   continue;
> >> > > > > > +
> >> > > > > > +           overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> >> > > > > > +           if (overlay < 0)
> >> > > > > > +                   return overlay;
> >> > > >
> >> > > > > Why does the absence of a target cause a fragment to be ignored but
> >> > > > > the absence of an "__overlay__" property cause the merging to be
> >> > > > > abandoned with an error? Can't we just ignore fragments that aren't
> >> > > > > recognised?
> >> > > >
> >> > > > So, I had the same question.  But fragments we can't make sense MUST
> >> > > > cause failures, and not be silently ignored.
> >> > > >
> >> > > > An incompletely applied overlay is almost certainly going to cause you
> >> > > > horrible grief at some point, so you absolutely want to know early if
> >> > > > your overlay is in a format your tool doesn't understand.
> >> > >
> >> > > I'm not sure how we can achieve that without applying it once, and see
> >> > > if it fails. The obvious things are easy to detect (like a missing
> >> > > __overlay__ node), but some others really aren't (like a poorly
> >> > > formatted phandle, or one that overflows) without applying it
> >> > > entirely. And that seems difficult without malloc.
> >> >
> >> > So, atomically applying either the whole overlay or nothing would be a
> >> > nice property, but it is indeed infeasibly difficult to achieve
> >> > without malloc().  Well.. we sort of could by making apply_overlay()
> >> > take an output buffer separate from the base tree, but that's not what
> >> > I'm suggesting.
> >> >
> >> > I'm fine with the base tree being trashed with an incomplete
> >> > application when apply_overlay() reports failure.  WHat I'm not ok
> >> > with is *silent* failure.  If you ignore fragments you don't
> >> > understand, then - if the overlay uses features that aren't supported
> >> > by this version of the code - you'll end up with an incompletely
> >> > applied overlay while the apply_overlay() function *reports success*.
> >> > That is a recipe for disaster.
> >>
> >> Ok, that makes sense. I'll return an error if the target is missing as
> >> well then.
> >>
> >> But then, I think we fall back to the discussion you had with
> >> Pantelis: how do you identify an overlay node (that must have a
> >> target) and some other "metadata" node that shouldn't be applied (and
> >> will not have a target). In the first case, we need to report an error
> >> if it's missing. In the second, we should just ignore the node
> >> entirely.
> >
> > Right.  I can see two obvious approaches:
> >
> >      1. All (top-level) nodes named fragment@* are assumed to be
> >         overlay fragments.
> >
> >      2. All top-evel nodes with a subnode named '__overlay__' are
> >         assumed to be overlay fragments
> >
> > (2) differs from looking for target properties because whatever
> > target variants we add in future, they're still likely to want an
> > __overlay__ node.  Or at worst, we can add a dummy __overlay__ node to
> > them.
> >
> >> Would turning that code the other way around, and if it has an
> >> __overlay__ subnode, target or target-path is mandatory, and if not
> >> just ignore the node entirely, work for you?
> >
> > I'd prefer to pick a single defining factor for the overlay fragments,
> > rather than a grab bag of options.
> 
> I think it's potentially dangerous using the presence of a particular
> property to identify an overlay - what if the property name ("target",
> say) was also the name of a label or property within one of the
> fragments?

Yes, I agree.

> It could then appear in the __symbols__ or __fixups__ node,
> causing it to be treated as a potential overlay, but application will
> then be halted with an error because it has no __overlay__ subnode.

Indeed.

> If we make the presence of the __overlay__ subnode the decision point
> (which is nicer anyway because it is a single test rather than two -
> "target" and "target-path") then we avoid this hazard - __symbols__
> and __fixups__ have no subnodes, and __local_fixups__ only has
> top-level nodes as its subnodes, meaning __overlay__ will always be
> one level further down.

That last point is a little hairier than I'd like, but I think it's an
acceptably low risk in practice.

-- 
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 v2 6/6] libfdt: Add overlay application function

[Maxime Ripard]

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

On Fri, Jul 15, 2016 at 10:18:04AM +0100, Phil Elwell wrote:
> On Thu, Jul 14, 2016 at 9:30 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Jul 13, 2016 at 09:37:57PM +0200, Maxime Ripard wrote:
> >> On Thu, Jul 14, 2016 at 01:07:45AM +1000, David Gibson wrote:
> >> > On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:
> >> > > Hi David,
> >> > >
> >> > > On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
> >> > > > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> >> > > > > On 11/07/2016 20:56, Maxime Ripard wrote:
> >> > > > [snip]
> >> > > >
> >> > > > > > +static int overlay_merge(void *fdt, void *fdto)
> >> > > > > > +{
> >> > > > > > +   int fragment;
> >> > > > > > +
> >> > > > > > +   fdt_for_each_subnode(fragment, fdto, 0) {
> >> > > > > > +           int overlay;
> >> > > > > > +           int target;
> >> > > > > > +           int ret;
> >> > > > > > +
> >> > > > > > +           target = overlay_get_target(fdt, fdto, fragment);
> >> > > > > > +           if (target < 0)
> >> > > > > > +                   continue;
> >> > > > > > +
> >> > > > > > +           overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> >> > > > > > +           if (overlay < 0)
> >> > > > > > +                   return overlay;
> >> > > >
> >> > > > > Why does the absence of a target cause a fragment to be ignored but
> >> > > > > the absence of an "__overlay__" property cause the merging to be
> >> > > > > abandoned with an error? Can't we just ignore fragments that aren't
> >> > > > > recognised?
> >> > > >
> >> > > > So, I had the same question.  But fragments we can't make sense MUST
> >> > > > cause failures, and not be silently ignored.
> >> > > >
> >> > > > An incompletely applied overlay is almost certainly going to cause you
> >> > > > horrible grief at some point, so you absolutely want to know early if
> >> > > > your overlay is in a format your tool doesn't understand.
> >> > >
> >> > > I'm not sure how we can achieve that without applying it once, and see
> >> > > if it fails. The obvious things are easy to detect (like a missing
> >> > > __overlay__ node), but some others really aren't (like a poorly
> >> > > formatted phandle, or one that overflows) without applying it
> >> > > entirely. And that seems difficult without malloc.
> >> >
> >> > So, atomically applying either the whole overlay or nothing would be a
> >> > nice property, but it is indeed infeasibly difficult to achieve
> >> > without malloc().  Well.. we sort of could by making apply_overlay()
> >> > take an output buffer separate from the base tree, but that's not what
> >> > I'm suggesting.
> >> >
> >> > I'm fine with the base tree being trashed with an incomplete
> >> > application when apply_overlay() reports failure.  WHat I'm not ok
> >> > with is *silent* failure.  If you ignore fragments you don't
> >> > understand, then - if the overlay uses features that aren't supported
> >> > by this version of the code - you'll end up with an incompletely
> >> > applied overlay while the apply_overlay() function *reports success*.
> >> > That is a recipe for disaster.
> >>
> >> Ok, that makes sense. I'll return an error if the target is missing as
> >> well then.
> >>
> >> But then, I think we fall back to the discussion you had with
> >> Pantelis: how do you identify an overlay node (that must have a
> >> target) and some other "metadata" node that shouldn't be applied (and
> >> will not have a target). In the first case, we need to report an error
> >> if it's missing. In the second, we should just ignore the node
> >> entirely.
> >
> > Right.  I can see two obvious approaches:
> >
> >      1. All (top-level) nodes named fragment@* are assumed to be
> >         overlay fragments.
> >
> >      2. All top-evel nodes with a subnode named '__overlay__' are
> >         assumed to be overlay fragments
> >
> > (2) differs from looking for target properties because whatever
> > target variants we add in future, they're still likely to want an
> > __overlay__ node.  Or at worst, we can add a dummy __overlay__ node to
> > them.
> >
> >> Would turning that code the other way around, and if it has an
> >> __overlay__ subnode, target or target-path is mandatory, and if not
> >> just ignore the node entirely, work for you?
> >
> > I'd prefer to pick a single defining factor for the overlay fragments,
> > rather than a grab bag of options.
> 
> I think it's potentially dangerous using the presence of a particular
> property to identify an overlay - what if the property name ("target",
> say) was also the name of a label or property within one of the
> fragments? It could then appear in the __symbols__ or __fixups__ node,
> causing it to be treated as a potential overlay, but application will
> then be halted with an error because it has no __overlay__ subnode.

Ok, let's go for option 2 then.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 5/6] libfdt: Add fdt_setprop_inplace_namelen_partial

[Maxime Ripard]

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

Hi David,

On Tue, Jul 12, 2016 at 09:45:20PM +1000, David Gibson wrote:
> On Mon, Jul 11, 2016 at 09:56:22PM +0200, Maxime Ripard wrote:
> > Add a function to modify inplace only a portion of a property..
> > 
> > This is especially useful when the property is an array of values, and you
> > want to update one of them without changing the DT size.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> 
> Now it just needs some testcases..

Since fdt_setprop_inplace relies on it now, it already has some test
coverage.

However, I tried to add some testcases for this to the test_tree1, but
it was generating a lot of errors because it seems that not all tests
are run from test_tree1.dts and generated by some other mean I
couldn't identify. Could you point me to the right direction?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 5/6] libfdt: Add fdt_setprop_inplace_namelen_partial

[David Gibson]

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

On Mon, Jul 18, 2016 at 03:45:58PM +0200, Maxime Ripard wrote:
> Hi David,
> 
> On Tue, Jul 12, 2016 at 09:45:20PM +1000, David Gibson wrote:
> > On Mon, Jul 11, 2016 at 09:56:22PM +0200, Maxime Ripard wrote:
> > > Add a function to modify inplace only a portion of a property..
> > > 
> > > This is especially useful when the property is an array of values, and you
> > > want to update one of them without changing the DT size.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> > 
> > Now it just needs some testcases..
> 
> Since fdt_setprop_inplace relies on it now, it already has some test
> coverage.
> 
> However, I tried to add some testcases for this to the test_tree1, but
> it was generating a lot of errors because it seems that not all tests
> are run from test_tree1.dts and generated by some other mean I
> couldn't identify. Could you point me to the right direction?

I'm not quite sure what you mean.

test_tree1 is generated in a number of different ways - these are
cross-compared at various points as part of the tests.

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