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

[PATCH v3 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 libfdt v2:
  - Added test cases for the overlays that don't require overlay
    support in dtc
  - Only treat as fragments the top-level subnodes with an __overlay__
    subnode
  - Fixed kerneldoc for subnodes and property iterators
  - Improve kerneldoc for fdt_get_max_phandle
  - Make sure the fixup properties length is a multiple of 4 bytes
  - Improve the error checking on a few places
  - Improve a few variables names and types

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            | 634 ++++++++++++++++++++++++++++++++++++++++
 libfdt/fdt_ro.c                 |  26 ++
 libfdt/fdt_wip.c                |  29 +-
 libfdt/libfdt.h                 | 126 ++++++++
 libfdt/libfdt_env.h             |   1 +
 tests/.gitignore                |   2 +
 tests/Makefile.tests            |   4 +-
 tests/get_phandle.c             |   6 +
 tests/overlay.c                 | 232 +++++++++++++++
 tests/overlay_base.dts          |  21 ++
 tests/overlay_overlay_dtc.dts   |  85 ++++++
 tests/overlay_overlay_nodtc.dts |  82 ++++++
 tests/property_iterate.c        |  97 ++++++
 tests/property_iterate.dts      |  24 ++
 tests/run_tests.sh              |  16 +
 tests/subnode_iterate.c         |   8 +-
 17 files changed, 1383 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_dtc.dts
 create mode 100644 tests/overlay_overlay_nodtc.dts
 create mode 100644 tests/property_iterate.c
 create mode 100644 tests/property_iterate.dts

-- 
2.9.0

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

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 36222fd4a6f4..70ec39c7104c 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -168,6 +168,33 @@ 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(node, fdt, parent) {
+ *		if ((node < 0) && (node != -FDT_ERR_NOT_FOUND)) {
+ *			Error handling
+ *		}
+ *
+ *		Use node
+ *		...
+ *	}
+ *
+ * Note that this is implemented as a macro and @node is used as
+ * iterator in the loop. The parent variable 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

--
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 v3 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            | 26 +++++++++++++
 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, 152 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 70ec39c7104c..cb2f080c81d4 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -457,6 +457,32 @@ 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(property, fdt, node) {
+ *		if ((property < 0) && (property != -FDT_ERR_NOT_FOUND)) {
+ *			Error handling
+ *		}
+ *
+ *		Use property
+ *		...
+ *	}
+ *
+ * Note that this is implemented as a macro and property is used as
+ * iterator in the loop. The node variable 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 v3 3/6] libfdt: Add max phandle retrieval function

[Maxime Ripard]

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

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 libfdt/fdt_ro.c     | 26 ++++++++++++++++++++++++++
 libfdt/libfdt.h     | 15 +++++++++++++++
 tests/get_phandle.c |  6 ++++++
 3 files changed, 47 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 cb2f080c81d4..1a14bc16b3d7 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -285,6 +285,21 @@ 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. This will ignore badly formatted phandles, or phandles
+ * with a value of 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();
 }
-- 
2.9.0

[PATCH v3 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.

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
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 1a14bc16b3d7..e504467a6249 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -627,6 +627,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

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

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@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 e504467a6249..177fe68febce 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1077,6 +1077,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

[PATCH v3 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            | 634 ++++++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h                 |  30 ++
 libfdt/libfdt_env.h             |   1 +
 tests/.gitignore                |   1 +
 tests/Makefile.tests            |   3 +-
 tests/overlay.c                 | 232 +++++++++++++++
 tests/overlay_base.dts          |  21 ++
 tests/overlay_overlay_dtc.dts   |  85 ++++++
 tests/overlay_overlay_nodtc.dts |  82 ++++++
 tests/run_tests.sh              |  13 +
 11 files changed, 1102 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_dtc.dts
 create mode 100644 tests/overlay_overlay_nodtc.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..236618f34e83
--- /dev/null
+++ b/libfdt/fdt_overlay.c
@@ -0,0 +1,634 @@
+#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 ((len != sizeof(*val)) || (*val == (uint32_t)-1))
+		return (uint32_t)-1;
+
+	return fdt32_to_cpu(*val);
+}
+
+/**
+ * overlay_get_target - retrieves the offset of a fragment's target
+ * @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 offset 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;
+	if (adj_val == (uint32_t)-1)
+		return -FDT_ERR_BADPHANDLE;
+
+	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)
+{
+	int child;
+	int ret;
+
+	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
+	if (ret && ret != -FDT_ERR_NOTFOUND)
+		return ret;
+
+	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
+	if (ret && ret != -FDT_ERR_NOTFOUND)
+		return ret;
+
+	fdt_for_each_subnode(child, fdto, node) {
+		ret = overlay_adjust_node_phandles(fdto, child, delta);
+		if (ret)
+			return ret;
+	}
+
+	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 *tree_val;
+		const uint32_t *fixup_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;
+
+		if (fixup_len % sizeof(uint32_t))
+			return -FDT_ERR_BADSTRUCTURE;
+
+		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
+		if (!tree_val)
+			return tree_len;
+
+		if (tree_len % sizeof(uint32_t))
+			return -FDT_ERR_BADSTRUCTURE;
+
+		for (i = 0; i < (fixup_len / sizeof(uint32_t)); i++) {
+			uint32_t adj_val, index;
+
+			index = *(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 __fixups__ 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) {
+		if (len == -FDT_ERR_NOTFOUND)
+			return -FDT_ERR_INTERNAL;
+
+		return len;
+	}
+
+	do {
+		const char *prop_string = value;
+		const char *path, *name;
+		uint32_t prop_len = strnlen(value, len);
+		uint32_t path_len, name_len;
+		char *sep, *endptr;
+		int index;
+		int ret;
+
+		path = prop_string;
+		sep = memchr(prop_string, ':', prop_len);
+		if (!sep || (*sep != ':'))
+			return -FDT_ERR_BADSTRUCTURE;
+
+		path_len = sep - path;
+		if (path_len == prop_len)
+			return -FDT_ERR_BADSTRUCTURE;
+
+		name = sep + 1;
+		sep = memchr(name, ':', prop_len);
+		if (!sep || *sep != ':')
+			return -FDT_ERR_BADSTRUCTURE;
+
+		name_len = sep - name;
+		if ((path_len + 1 + name_len) == prop_len)
+			return -FDT_ERR_BADSTRUCTURE;
+
+		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 - Merges a node 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
+ * @node: Node offset in the overlay holding the changes to merge
+ *
+ * overlay_apply_node() merges a node 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 node)
+{
+	int property;
+	int subnode;
+
+	fdt_for_each_property_offset(property, fdto, node) {
+		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(subnode, fdto, node) {
+		const char *name = fdt_get_name(fdto, subnode, 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, subnode);
+		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;
+
+		/*
+		 * Each fragments will have an __overlay__ node. If
+		 * they don't, it's not supposed to be merged
+		 */
+		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+		if (overlay < 0)
+			continue;
+
+		target = overlay_get_target(fdt, fdto, fragment);
+		if (target < 0)
+			return target;
+
+		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 177fe68febce..d8035aee3dfe 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1750,6 +1750,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..45d4ff3a3d3f
--- /dev/null
+++ b/tests/overlay.c
@@ -0,0 +1,232 @@
+/*
+ * 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);
+	CHECK(node_off < 0);
+
+	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);
+	CHECK(node_off < 0);
+
+	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(void *fdt)
+{
+	return check_getprop_null_by_name(fdt, "/test-node/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, "/test-node/new-local-node");
+	CHECK(off < 0);
+
+	local_phandle = fdt_get_phandle(fdt, off);
+	CHECK(!local_phandle);
+
+	CHECK(fdt_getprop_u32_by_index(fdt, "/test-node",
+				       "test-several-phandle",
+				       0, &val));
+	CHECK(val != local_phandle);
+
+	CHECK(fdt_getprop_u32_by_index(fdt, "/test-node",
+				       "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, "/test-node/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-node",
+				       "test-phandle", 0, &val));
+	CHECK(test_phandle != val);
+
+	CHECK(fdt_getprop_u32_by_index(fdt, "/test-node",
+				       "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(fdt_base);
+	fdt_overlay_add_subnode_property(fdt_base);
+
+	/*
+	 * If the base tree has a __symbols__ node, do the tests that
+	 * are only successful with a proper phandle support, and thus
+	 * dtc -@
+	 */
+	if (fdt_path_offset(fdt_base, "/__symbols__") >= 0) {
+		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_dtc.dts b/tests/overlay_overlay_dtc.dts
new file mode 100644
index 000000000000..30d2362f746b
--- /dev/null
+++ b/tests/overlay_overlay_dtc.dts
@@ -0,0 +1,85 @@
+/*
+ * 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;
+			};
+		};
+	};
+
+	fragment@5 {
+		target = <&test>;
+
+		__overlay__ {
+			local: new-local-node {
+				new-property;
+			};
+		};
+	};
+
+	fragment@6 {
+		target = <&test>;
+
+		__overlay__ {
+			test-phandle = <&test>, <&local>;
+		};
+	};
+
+	fragment@7 {
+		target = <&test>;
+
+		__overlay__ {
+			test-several-phandle = <&local>, <&local>;
+		};
+	};
+
+	fragment@8 {
+		target = <&test>;
+
+		__overlay__ {
+			sub-test-node {
+				new-sub-test-property;
+			};
+		};
+	};
+};
diff --git a/tests/overlay_overlay_nodtc.dts b/tests/overlay_overlay_nodtc.dts
new file mode 100644
index 000000000000..e8d0f96d889c
--- /dev/null
+++ b/tests/overlay_overlay_nodtc.dts
@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+	fragment@0 {
+		target-path = "/test-node";
+
+		__overlay__ {
+			test-int-property = <43>;
+		};
+	};
+
+	/* Test that we can replace a string by a longer one */
+	fragment@1 {
+		target-path = "/test-node";
+
+		__overlay__ {
+			test-str-property = "foobar";
+		};
+	};
+
+	/* Test that we add a new property */
+	fragment@2 {
+		target-path = "/test-node";
+
+		__overlay__ {
+			test-str-property-2 = "foobar2";
+		};
+	};
+
+	fragment@3 {
+		target-path = "/test-node";
+
+		__overlay__ {
+			new-node {
+				new-property;
+			};
+		};
+	};
+
+	fragment@4 {
+		target-path = "/";
+
+		__overlay__ {
+			local: new-local-node {
+				new-property;
+			};
+		};
+	};
+
+	fragment@5 {
+		target-path = "/";
+
+		__overlay__ {
+			test-several-phandle = <&local>, <&local>;
+		};
+	};
+
+	fragment@6 {
+		target-path = "/test-node";
+
+		__overlay__ {
+			sub-test-node {
+				new-sub-test-property;
+			};
+		};
+	};
+
+	__local_fixups__ {
+		fragment@5 {
+			__overlay__ {
+				test-several-phandle = <0 4>;
+			};
+		};
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 6a2662b2abaf..7fcb34fa839a 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -280,6 +280,19 @@ libfdt_tests () {
     run_test get_alias aliases.dtb
     run_test path_offset_aliases aliases.dtb
 
+    # Overlay tests that don't require overlay support in dtc
+    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_nodtc.dts
+    run_test overlay overlay_base.dtb overlay_overlay.dtb
+
+    # Overlay tests that requires overlay support in dtc
+    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_dtc.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 v3 1/6] libfdt: Add a subnodes iterator macro

[David Gibson]

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

On Wed, Jul 20, 2016 at 04:20:39PM +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>

Some nits in the comment..

> ---
>  libfdt/libfdt.h         | 27 +++++++++++++++++++++++++++
>  tests/subnode_iterate.c |  8 ++------
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 36222fd4a6f4..70ec39c7104c 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -168,6 +168,33 @@ 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 generally go first.

> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_subnode(node, fdt, parent) {
> + *		if ((node < 0) && (node != -FDT_ERR_NOT_FOUND)) {
> + *			Error handling
> + *		}

This error handling needs to go *after* the for block.

> + *
> + *		Use node
> + *		...
> + *	}
> + *
> + * Note that this is implemented as a macro and @node is used as
> + * iterator in the loop. The parent variable be constant or even a
                                               ^ insert "may"

> + * literal.
> + *
> + * @node:	child node (int)

Probably worth noting it as (int, lvalue).

> + * @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 v3 2/6] libfdt: Add iterator over properties

[David Gibson]

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

On Wed, Jul 20, 2016 at 04:20:40PM +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>
> ---
>  libfdt/libfdt.h            | 26 +++++++++++++
>  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, 152 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 70ec39c7104c..cb2f080c81d4 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -457,6 +457,32 @@ 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

Name here doesn't match the actual macro name.

> + * @property_offset:	property offset (int)

Note (int, lvalue).

> + * @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(property, fdt, node) {
> + *		if ((property < 0) && (property != -FDT_ERR_NOT_FOUND)) {
> + *			Error handling
> + *		}

Error handling needs to go after the for block.

> + *
> + *		Use property
> + *		...
> + *	}
> + *
> + * Note that this is implemented as a macro and property is used as
> + * iterator in the loop. The node variable 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
>  

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

[David Gibson]

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

On Wed, Jul 20, 2016 at 04:20:44PM +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            | 634 ++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h                 |  30 ++
>  libfdt/libfdt_env.h             |   1 +
>  tests/.gitignore                |   1 +
>  tests/Makefile.tests            |   3 +-
>  tests/overlay.c                 | 232 +++++++++++++++
>  tests/overlay_base.dts          |  21 ++
>  tests/overlay_overlay_dtc.dts   |  85 ++++++
>  tests/overlay_overlay_nodtc.dts |  82 ++++++
>  tests/run_tests.sh              |  13 +
>  11 files changed, 1102 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_dtc.dts
>  create mode 100644 tests/overlay_overlay_nodtc.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..236618f34e83
> --- /dev/null
> +++ b/libfdt/fdt_overlay.c
> @@ -0,0 +1,634 @@
> +#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 ((len != sizeof(*val)) || (*val == (uint32_t)-1))
> +		return (uint32_t)-1;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +/**
> + * overlay_get_target - retrieves the offset of a fragment's target
> + * @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 offset 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;
> +	if (adj_val == (uint32_t)-1)
> +		return -FDT_ERR_BADPHANDLE;
> +
> +	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)
> +{
> +	int child;
> +	int ret;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;
> +
> +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> +	if (ret && ret != -FDT_ERR_NOTFOUND)
> +		return ret;
> +
> +	fdt_for_each_subnode(child, fdto, node) {
> +		ret = overlay_adjust_node_phandles(fdto, child, delta);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	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 *tree_val;

AFAICT there's no particular need for this to be unsigned.

> +		const uint32_t *fixup_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;
> +
> +		if (fixup_len % sizeof(uint32_t))
> +			return -FDT_ERR_BADSTRUCTURE;
> +
> +		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> +		if (!tree_val)
> +			return tree_len;
> +
> +		if (tree_len % sizeof(uint32_t))
> +			return -FDT_ERR_BADSTRUCTURE;

This test is incorrect - the fixup property has to be an array of
u32s, but the target of the fixup could have any format.

> +		for (i = 0; i < (fixup_len / sizeof(uint32_t)); i++) {
> +			uint32_t adj_val, index;
> +
> +			index = *(fixup_val + i);

I think fixup_val[i] would be clearer.

> +			index = fdt32_to_cpu(index);

And shorted, so these lines can be merged together.

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

I'd prefer sizeof(adj_val).

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

Should probably return prop_len here - you could get an error like
BADSTRUCTURE instead of 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 __fixups__ 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) {
> +		if (len == -FDT_ERR_NOTFOUND)
> +			return -FDT_ERR_INTERNAL;
> +
> +		return len;
> +	}
> +
> +	do {
> +		const char *prop_string = value;
> +		const char *path, *name;
> +		uint32_t prop_len = strnlen(value, len);

prop_len is a bad name, since it could well be less than the length of
the whole property.

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

As mentioned on some of the other patches in the series, I think we
want a new error code for bad fixup / overlay information.

> +
> +		path_len = sep - path;
> +		if (path_len == prop_len)
> +			return -FDT_ERR_BADSTRUCTURE;

I'm pretty sure this is impossible if sep != NULL.

> +		name = sep + 1;

But I think the case you actually need to test for is path_len ==
(prop_len - 1), that will occur when : is the last character.

> +		sep = memchr(name, ':', prop_len);
> +		if (!sep || *sep != ':')
> +			return -FDT_ERR_BADSTRUCTURE;

This still isn't quite safe.  If the property has no \0, and : is the
last character in it, you'll access beyond the end of the property
here.  It's probably easier if you just fail early if there is no \0 -
that's probably easier if you use memchr(\0) instead of strnlen().

> +
> +		name_len = sep - name;
> +		if ((path_len + 1 + name_len) == prop_len)
> +			return -FDT_ERR_BADSTRUCTURE;

Again, off-by-one in this test, I think.  Since there are so many
tricky edge cases here, it might be worth making testcases for them.

> +		index = strtoul(sep + 1, &endptr, 10)

And strtoul() is definitely unsafe since you haven't verified that
there's a terminating \0.

> +		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 - Merges a node 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
> + * @node: Node offset in the overlay holding the changes to merge
> + *
> + * overlay_apply_node() merges a node 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 node)
> +{
> +	int property;
> +	int subnode;
> +
> +	fdt_for_each_property_offset(property, fdto, node) {
> +		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(subnode, fdto, node) {
> +		const char *name = fdt_get_name(fdto, subnode, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(fdt, target, name);
> +		if (nnode == -FDT_ERR_EXISTS)
> +			nnode = fdt_subnode_offset(fdt, target, name);

Strictly speaking a NOTFOUND error here shold probably be turned into
INTERNAL, as above, since if add_subnode() returns EXISTS,
subnode_offset should not return NOTFOUND.  Not a big deal though.

> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = overlay_apply_node(fdt, nnode, fdto, subnode);
> +		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;
> +
> +		/*
> +		 * Each fragments will have an __overlay__ node. If
> +		 * they don't, it's not supposed to be merged
> +		 */
> +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +		if (overlay < 0)

Only NOTFOUND should result in a continue, other errors should be
reported.

> +			continue;
> +
> +		target = overlay_get_target(fdt, fdto, fragment);
> +		if (target < 0)
> +			return target;
> +
> +		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;

Since phandles can't have value 0, I don't think you need the +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 177fe68febce..d8035aee3dfe 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1750,6 +1750,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..45d4ff3a3d3f
> --- /dev/null
> +++ b/tests/overlay.c
> @@ -0,0 +1,232 @@
> +/*
> + * 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;

I'd prefer you don't use RC_PASS here - that's intended for the
program exit code, not as a general function return code.  Literal 0
is fine here.

> +}
> +
> +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);
> +	CHECK(node_off < 0);
> +
> +	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);
> +	CHECK(node_off < 0);
> +
> +	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(void *fdt)
> +{
> +	return check_getprop_null_by_name(fdt, "/test-node/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, "/test-node/new-local-node");
> +	CHECK(off < 0);
> +
> +	local_phandle = fdt_get_phandle(fdt, off);
> +	CHECK(!local_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/test-node",
> +				       "test-several-phandle",
> +				       0, &val));
> +	CHECK(val != local_phandle);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/test-node",
> +				       "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, "/test-node/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-node",
> +				       "test-phandle", 0, &val));
> +	CHECK(test_phandle != val);
> +
> +	CHECK(fdt_getprop_u32_by_index(fdt, "/test-node",
> +				       "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(fdt_base);
> +	fdt_overlay_add_subnode_property(fdt_base);
> +
> +	/*
> +	 * If the base tree has a __symbols__ node, do the tests that
> +	 * are only successful with a proper phandle support, and thus
> +	 * dtc -@
> +	 */
> +	if (fdt_path_offset(fdt_base, "/__symbols__") >= 0) {
> +		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_dtc.dts b/tests/overlay_overlay_dtc.dts
> new file mode 100644
> index 000000000000..30d2362f746b
> --- /dev/null
> +++ b/tests/overlay_overlay_dtc.dts
> @@ -0,0 +1,85 @@
> +/*
> + * 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;
> +			};
> +		};
> +	};
> +
> +	fragment@5 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			local: new-local-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	fragment@6 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-phandle = <&test>, <&local>;
> +		};
> +	};
> +
> +	fragment@7 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-several-phandle = <&local>, <&local>;
> +		};
> +	};
> +
> +	fragment@8 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			sub-test-node {
> +				new-sub-test-property;
> +			};
> +		};
> +	};
> +};
> diff --git a/tests/overlay_overlay_nodtc.dts b/tests/overlay_overlay_nodtc.dts
> new file mode 100644
> index 000000000000..e8d0f96d889c
> --- /dev/null
> +++ b/tests/overlay_overlay_nodtc.dts
> @@ -0,0 +1,82 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +	fragment@0 {
> +		target-path = "/test-node";
> +
> +		__overlay__ {
> +			test-int-property = <43>;
> +		};
> +	};
> +
> +	/* Test that we can replace a string by a longer one */
> +	fragment@1 {
> +		target-path = "/test-node";
> +
> +		__overlay__ {
> +			test-str-property = "foobar";
> +		};
> +	};
> +
> +	/* Test that we add a new property */
> +	fragment@2 {
> +		target-path = "/test-node";
> +
> +		__overlay__ {
> +			test-str-property-2 = "foobar2";
> +		};
> +	};
> +
> +	fragment@3 {
> +		target-path = "/test-node";
> +
> +		__overlay__ {
> +			new-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	fragment@4 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			local: new-local-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	fragment@5 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			test-several-phandle = <&local>, <&local>;
> +		};
> +	};
> +
> +	fragment@6 {
> +		target-path = "/test-node";
> +
> +		__overlay__ {
> +			sub-test-node {
> +				new-sub-test-property;
> +			};
> +		};
> +	};
> +
> +	__local_fixups__ {
> +		fragment@5 {
> +			__overlay__ {
> +				test-several-phandle = <0 4>;
> +			};
> +		};
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 6a2662b2abaf..7fcb34fa839a 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -280,6 +280,19 @@ libfdt_tests () {
>      run_test get_alias aliases.dtb
>      run_test path_offset_aliases aliases.dtb
>  
> +    # Overlay tests that don't require overlay support in dtc
> +    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_nodtc.dts
> +    run_test overlay overlay_base.dtb overlay_overlay.dtb
> +
> +    # Overlay tests that requires overlay support in dtc
> +    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_dtc.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 v3 6/6] libfdt: Add overlay application function

[Maxime Ripard]

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

Hi David,

On Mon, Jul 25, 2016 at 12:29:08AM +1000, David Gibson wrote:
> > +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) {
> > +		if (len == -FDT_ERR_NOTFOUND)
> > +			return -FDT_ERR_INTERNAL;
> > +
> > +		return len;
> > +	}
> > +
> > +	do {
> > +		const char *prop_string = value;
> > +		const char *path, *name;
> > +		uint32_t prop_len = strnlen(value, len);
> 
> prop_len is a bad name, since it could well be less than the length of
> the whole property.
> 
> > +		uint32_t path_len, name_len;
> > +		char *sep, *endptr;
> > +		int index;
> > +		int ret;
> > +
> > +		path = prop_string;
> > +		sep = memchr(prop_string, ':', prop_len);
> > +		if (!sep || (*sep != ':'))
> > +			return -FDT_ERR_BADSTRUCTURE;
> 
> As mentioned on some of the other patches in the series, I think we
> want a new error code for bad fixup / overlay information.
> 
> > +
> > +		path_len = sep - path;
> > +		if (path_len == prop_len)
> > +			return -FDT_ERR_BADSTRUCTURE;
> 
> I'm pretty sure this is impossible if sep != NULL.
> 
> > +		name = sep + 1;
> 
> But I think the case you actually need to test for is path_len ==
> (prop_len - 1), that will occur when : is the last character.
> 
> > +		sep = memchr(name, ':', prop_len);
> > +		if (!sep || *sep != ':')
> > +			return -FDT_ERR_BADSTRUCTURE;
> 
> This still isn't quite safe.  If the property has no \0, and : is the
> last character in it, you'll access beyond the end of the property
> here.  It's probably easier if you just fail early if there is no \0 -
> that's probably easier if you use memchr(\0) instead of strnlen().
> 
> > +
> > +		name_len = sep - name;
> > +		if ((path_len + 1 + name_len) == prop_len)
> > +			return -FDT_ERR_BADSTRUCTURE;
> 
> Again, off-by-one in this test, I think.  Since there are so many
> tricky edge cases here, it might be worth making testcases for them.

What do you want here? That we move the parsing code out of that loop,
make it public and put the prototype in libfdt_internal, or that we
craft some DT that would outline all the possible issues with that
function, and just test the return code of fdt_overlay_apply?

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

[David Gibson]

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

On Mon, Jul 25, 2016 at 08:20:41PM +0200, Maxime Ripard wrote:
> Hi David,
> 
> On Mon, Jul 25, 2016 at 12:29:08AM +1000, David Gibson wrote:
> > > +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) {
> > > +		if (len == -FDT_ERR_NOTFOUND)
> > > +			return -FDT_ERR_INTERNAL;
> > > +
> > > +		return len;
> > > +	}
> > > +
> > > +	do {
> > > +		const char *prop_string = value;
> > > +		const char *path, *name;
> > > +		uint32_t prop_len = strnlen(value, len);
> > 
> > prop_len is a bad name, since it could well be less than the length of
> > the whole property.
> > 
> > > +		uint32_t path_len, name_len;
> > > +		char *sep, *endptr;
> > > +		int index;
> > > +		int ret;
> > > +
> > > +		path = prop_string;
> > > +		sep = memchr(prop_string, ':', prop_len);
> > > +		if (!sep || (*sep != ':'))
> > > +			return -FDT_ERR_BADSTRUCTURE;
> > 
> > As mentioned on some of the other patches in the series, I think we
> > want a new error code for bad fixup / overlay information.
> > 
> > > +
> > > +		path_len = sep - path;
> > > +		if (path_len == prop_len)
> > > +			return -FDT_ERR_BADSTRUCTURE;
> > 
> > I'm pretty sure this is impossible if sep != NULL.
> > 
> > > +		name = sep + 1;
> > 
> > But I think the case you actually need to test for is path_len ==
> > (prop_len - 1), that will occur when : is the last character.
> > 
> > > +		sep = memchr(name, ':', prop_len);
> > > +		if (!sep || *sep != ':')
> > > +			return -FDT_ERR_BADSTRUCTURE;
> > 
> > This still isn't quite safe.  If the property has no \0, and : is the
> > last character in it, you'll access beyond the end of the property
> > here.  It's probably easier if you just fail early if there is no \0 -
> > that's probably easier if you use memchr(\0) instead of strnlen().
> > 
> > > +
> > > +		name_len = sep - name;
> > > +		if ((path_len + 1 + name_len) == prop_len)
> > > +			return -FDT_ERR_BADSTRUCTURE;
> > 
> > Again, off-by-one in this test, I think.  Since there are so many
> > tricky edge cases here, it might be worth making testcases for them.
> 
> What do you want here? That we move the parsing code out of that loop,
> make it public and put the prototype in libfdt_internal, or that we
> craft some DT that would outline all the possible issues with that
> function, and just test the return code of fdt_overlay_apply?

I was thinking crafted DTs.  But I've realized that doing usefully is
pretty tricky, since overruns probably won't cause an immediate
problem most of the time.  I guess they'd trip errors under valgrind
at least (as long you make sure there's at least one byte's alignment
gap until the next tag).

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

[Maxime Ripard]

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

On Tue, Jul 26, 2016 at 10:18:59AM +1000, David Gibson wrote:
> > > Again, off-by-one in this test, I think.  Since there are so many
> > > tricky edge cases here, it might be worth making testcases for them.
> > 
> > What do you want here? That we move the parsing code out of that loop,
> > make it public and put the prototype in libfdt_internal, or that we
> > craft some DT that would outline all the possible issues with that
> > function, and just test the return code of fdt_overlay_apply?
> 
> I was thinking crafted DTs.  But I've realized that doing usefully is
> pretty tricky, since overruns probably won't cause an immediate
> problem most of the time.  I guess they'd trip errors under valgrind
> at least (as long you make sure there's at least one byte's alignment
> gap until the next tag).

Is that a general comment?

I'm still not quite sure what you expect from me. Do you want to test
just that function, or only the fixup parsing function? Should I run
valgrind, or are you expecting it to be done later?

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

[David Gibson]

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

On Tue, Jul 26, 2016 at 08:16:59AM +0200, Maxime Ripard wrote:
> On Tue, Jul 26, 2016 at 10:18:59AM +1000, David Gibson wrote:
> > > > Again, off-by-one in this test, I think.  Since there are so many
> > > > tricky edge cases here, it might be worth making testcases for them.
> > > 
> > > What do you want here? That we move the parsing code out of that loop,
> > > make it public and put the prototype in libfdt_internal, or that we
> > > craft some DT that would outline all the possible issues with that
> > > function, and just test the return code of fdt_overlay_apply?
> > 
> > I was thinking crafted DTs.  But I've realized that doing usefully is
> > pretty tricky, since overruns probably won't cause an immediate
> > problem most of the time.  I guess they'd trip errors under valgrind
> > at least (as long you make sure there's at least one byte's alignment
> > gap until the next tag).
> 
> Is that a general comment?
> 
> I'm still not quite sure what you expect from me. Do you want to test
> just that function, or only the fixup parsing function? Should I run
> valgrind, or are you expecting it to be done later?

Sorry, I wasn't very clear.

You don't necessarily need to do anything.  But the fact that we've
been through several iterations with problems in the edge cases of
parsing here suggests that this is tricky and fragile, so tests would
be a good idea if possible.

Because these are for cases with bad input, you wouldn't need to check
for any particular results - just that the functions don't crash or
otherwise do bad things when given the bad input.  It's probably
easier to construct tests which will have bad behaviour that valgrind
can detect than ones which have bad behaviour which is more obvious.

The whole testsuite can be run under valgrind with "make checkm".
It's a good idea to run that every so often - I don't do it routinely
because of course it's much, much slower than running the testsuite
normally.

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