[PATCH v4 0/2] of: Add unit tests for applying overlays

[PATCH v4 0/2] of: Add unit tests for applying overlays

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

Existing overlay unit tests examine individual pieces of the overlay code.
The new tests target the entire process of applying an overlay.

Changes from v3:
  - make subject line of patch 1 more generic

Changes from v2:
  - of_private.h: move __unflatten_device_tree() outside
    #ifdef CONFIG_OF_UNITTEST
  - unittest.c: move overlay declaration macros outside
    #ifdef CONFIG_OF_OVERLAY
  - unittest.c: make overlay_info overlays[] static
  - unittest.c: remove extra blank line

Changes from v1:
  - Move overlay base dtb unflattening into unittest.c.  Call from fdt.c.
  - Clarify file and variable names, 'overlay_test_' instead of 'ot_'
  - Use proper naming convention for node names.
  - A few added comments.
  - Improve error messages in the new tests.

Frank Rowand (2):
  of: support dtc compiler flags for overlays
  of: Add unit tests for applying overlays

 drivers/of/fdt.c                                 |  14 +-
 drivers/of/of_private.h                          |  12 +
 drivers/of/unittest-data/Makefile                |  17 +-
 drivers/of/unittest-data/overlay.dts             |  53 ++++
 drivers/of/unittest-data/overlay_bad_phandle.dts |  20 ++
 drivers/of/unittest-data/overlay_base.dts        |  80 ++++++
 drivers/of/unittest.c                            | 317 +++++++++++++++++++++++
 scripts/Makefile.lib                             |   2 +
 8 files changed, 507 insertions(+), 8 deletions(-)
 create mode 100644 drivers/of/unittest-data/overlay.dts
 create mode 100644 drivers/of/unittest-data/overlay_bad_phandle.dts
 create mode 100644 drivers/of/unittest-data/overlay_base.dts

-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v4 1/2] of: per-file dtc compiler flags

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

The dtc compiler version that adds initial support was available
in 4.11-rc1.  Add the ability to set an additional dtc compiler
flag is needed by overlays.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 scripts/Makefile.lib | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 0a07f9014944..0bbec480d323 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -283,6 +283,8 @@ ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),)
 DTC_FLAGS += -Wno-unit_address_vs_reg
 endif
 
+DTC_FLAGS += $(DTC_FLAGS_$(basetarget))
+
 # Generate an assembly file to wrap the output of the device tree compiler
 quiet_cmd_dt_S_dtb= DTB     $@
 cmd_dt_S_dtb=						\
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v4 2/2] of: Add unit tests for applying overlays

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

Existing overlay unit tests examine individual pieces of the overlay
code.  The new tests target the entire process of applying an overlay.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

There are checkpatch warnings.  I have reviewed them and feel they
can be ignored.

 drivers/of/fdt.c                                 |  14 +-
 drivers/of/of_private.h                          |  12 +
 drivers/of/unittest-data/Makefile                |  17 +-
 drivers/of/unittest-data/overlay.dts             |  53 ++++
 drivers/of/unittest-data/overlay_bad_phandle.dts |  20 ++
 drivers/of/unittest-data/overlay_base.dts        |  80 ++++++
 drivers/of/unittest.c                            | 317 +++++++++++++++++++++++
 7 files changed, 505 insertions(+), 8 deletions(-)
 create mode 100644 drivers/of/unittest-data/overlay.dts
 create mode 100644 drivers/of/unittest-data/overlay_bad_phandle.dts
 create mode 100644 drivers/of/unittest-data/overlay_base.dts

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..e33f7818bc6c 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -31,6 +31,8 @@
 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>
 
+#include "of_private.h"
+
 /*
  * of_fdt_limit_memory - limit the number of regions in the /memory node
  * @limit: maximum entries
@@ -469,11 +471,11 @@ static int unflatten_dt_nodes(const void *blob,
  * Returns NULL on failure or the memory chunk containing the unflattened
  * device tree on success.
  */
-static void *__unflatten_device_tree(const void *blob,
-				     struct device_node *dad,
-				     struct device_node **mynodes,
-				     void *(*dt_alloc)(u64 size, u64 align),
-				     bool detached)
+void *__unflatten_device_tree(const void *blob,
+			      struct device_node *dad,
+			      struct device_node **mynodes,
+			      void *(*dt_alloc)(u64 size, u64 align),
+			      bool detached)
 {
 	int size;
 	void *mem;
@@ -1261,6 +1263,8 @@ void __init unflatten_device_tree(void)
 
 	/* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
 	of_alias_scan(early_init_dt_alloc_memory_arch);
+
+	unittest_unflatten_overlay_base();
 }
 
 /**
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 18bbb4517e25..de5c604f5cc4 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -55,6 +55,18 @@ static inline int of_property_notify(int action, struct device_node *np,
 }
 #endif /* CONFIG_OF_DYNAMIC */
 
+#ifdef CONFIG_OF_UNITTEST
+extern void __init unittest_unflatten_overlay_base(void);
+#else
+static inline void unittest_unflatten_overlay_base(void) {};
+#endif
+
+extern void *__unflatten_device_tree(const void *blob,
+			      struct device_node *dad,
+			      struct device_node **mynodes,
+			      void *(*dt_alloc)(u64 size, u64 align),
+			      bool detached);
+
 /**
  * General utilities for working with live trees.
  *
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 1ac5cc01d627..6e00a9c69e58 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -1,7 +1,18 @@
 obj-y += testcases.dtb.o
+obj-y += overlay.dtb.o
+obj-y += overlay_bad_phandle.dtb.o
+obj-y += overlay_base.dtb.o
 
 targets += testcases.dtb testcases.dtb.S
+targets += overlay.dtb overlay.dtb.S
+targets += overlay_bad_phandle.dtb overlay_bad_phandle.dtb.S
+targets += overlay_base.dtb overlay_base.dtb.S
 
-.SECONDARY: \
-	$(obj)/testcases.dtb.S \
-	$(obj)/testcases.dtb
+.PRECIOUS: \
+	$(obj)/%.dtb.S \
+	$(obj)/%.dtb
+
+# enable creation of __symbols__ node
+DTC_FLAGS_overlay := -@
+DTC_FLAGS_overlay_bad_phandle := -@
+DTC_FLAGS_overlay_base := -@
diff --git a/drivers/of/unittest-data/overlay.dts b/drivers/of/unittest-data/overlay.dts
new file mode 100644
index 000000000000..6cd7e6a0c13e
--- /dev/null
+++ b/drivers/of/unittest-data/overlay.dts
@@ -0,0 +1,53 @@
+/dts-v1/;
+/plugin/;
+
+/ {
+
+	fragment@0 {
+		target = <&electric_1>;
+
+		__overlay__ {
+			status = "ok";
+
+			hvac_2: hvac-large-1 {
+				compatible = "ot,hvac-large";
+				heat-range = < 40 75 >;
+				cool-range = < 65 80 >;
+			};
+		};
+	};
+
+	fragment@1 {
+		target = <&rides_1>;
+
+		__overlay__ {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			status = "ok";
+
+			ride@200 {
+				compatible = "ot,ferris-wheel";
+				reg = < 0x00000200 0x100 >;
+				hvac-provider = < &hvac_2 >;
+				hvac-thermostat = < 27 32 > ;
+				hvac-zones = < 12 5 >;
+				hvac-zone-names = "operator", "snack-bar";
+				spin-controller = < &spin_ctrl_1 3 >;
+				spin-rph = < 30 >;
+				gondolas = < 16 >;
+				gondola-capacity = < 6 >;
+			};
+		};
+	};
+
+	fragment@2 {
+		target = <&lights_2>;
+
+		__overlay__ {
+			status = "ok";
+			color = "purple", "white", "red", "green";
+			rate = < 3 256 >;
+		};
+	};
+
+};
diff --git a/drivers/of/unittest-data/overlay_bad_phandle.dts b/drivers/of/unittest-data/overlay_bad_phandle.dts
new file mode 100644
index 000000000000..270ee885a623
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_bad_phandle.dts
@@ -0,0 +1,20 @@
+/dts-v1/;
+/plugin/;
+
+/ {
+
+	fragment@0 {
+		target = <&electric_1>;
+
+		__overlay__ {
+
+			// This label should cause an error when the overlay
+			// is applied.  There is already a phandle value
+			// in the base tree for motor-1.
+			spin_ctrl_1_conflict: motor-1 {
+				accelerate = < 3 >;
+				decelerate = < 5 >;
+			};
+		};
+	};
+};
diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
new file mode 100644
index 000000000000..5566b27fb61a
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -0,0 +1,80 @@
+/dts-v1/;
+/plugin/;
+
+/*
+ * Base device tree that overlays will be applied against.
+ *
+ * Do not add any properties in node "/".
+ * Do not add any nodes other than "/testcase-data-2" in node "/".
+ * Do not add anything that would result in dtc creating node "/__fixups__".
+ * dtc will create nodes "/__symbols__" and "/__local_fixups__".
+ */
+
+/ {
+	testcase-data-2 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		electric_1: substation@100 {
+			compatible = "ot,big-volts-control";
+			reg = < 0x00000100 0x100 >;
+			status = "disabled";
+
+			hvac_1: hvac-medium-1 {
+				compatible = "ot,hvac-medium";
+				heat-range = < 50 75 >;
+				cool-range = < 60 80 >;
+			};
+
+			spin_ctrl_1: motor-1 {
+				compatible = "ot,ferris-wheel-motor";
+				spin = "clockwise";
+			};
+
+			spin_ctrl_2: motor-8 {
+				compatible = "ot,roller-coaster-motor";
+			};
+		};
+
+		rides_1: fairway-1 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "ot,rides";
+			status = "disabled";
+			orientation = < 127 >;
+
+			ride@100 {
+				compatible = "ot,roller-coaster";
+				reg = < 0x00000100 0x100 >;
+				hvac-provider = < &hvac_1 >;
+				hvac-thermostat = < 29 > ;
+				hvac-zones = < 14 >;
+				hvac-zone-names = "operator";
+				spin-controller = < &spin_ctrl_2 5 &spin_ctrl_2 7 >;
+				spin-controller-names = "track_1", "track_2";
+				queues = < 2 >;
+			};
+		};
+
+		lights_1: lights@30000 {
+			compatible = "ot,work-lights";
+			reg = < 0x00030000 0x1000 >;
+			status = "disabled";
+		};
+
+		lights_2: lights@40000 {
+			compatible = "ot,show-lights";
+			reg = < 0x00040000 0x1000 >;
+			status = "disabled";
+			rate = < 13 138 >;
+		};
+
+		retail_1: vending@50000 {
+			reg = < 0x00050000 0x1000 >;
+			compatible = "ot,tickets";
+			status = "disabled";
+		};
+
+	};
+};
+
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 62db55b97c10..12597ff8cfb0 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -8,6 +8,7 @@
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/hashtable.h>
+#include <linux/libfdt.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
@@ -1925,6 +1926,320 @@ static void __init of_unittest_overlay(void)
 static inline void __init of_unittest_overlay(void) { }
 #endif
 
+/*
+ * __dtb_ot_begin[] and __dtb_ot_end[] are created by cmd_dt_S_dtb
+ * in scripts/Makefile.lib
+ */
+
+#define OVERLAY_INFO_EXTERN(name) \
+	extern uint8_t __dtb_##name##_begin[]; \
+	extern uint8_t __dtb_##name##_end[]
+
+#define OVERLAY_INFO(name, expected) \
+{	.dtb_begin	 = __dtb_##name##_begin, \
+	.dtb_end	 = __dtb_##name##_end, \
+	.expected_result = expected, \
+}
+
+struct overlay_info {
+	uint8_t		   *dtb_begin;
+	uint8_t		   *dtb_end;
+	void		   *data;
+	struct device_node *np_overlay;
+	int		   expected_result;
+	int		   overlay_id;
+};
+
+OVERLAY_INFO_EXTERN(overlay_base);
+OVERLAY_INFO_EXTERN(overlay);
+OVERLAY_INFO_EXTERN(overlay_bad_phandle);
+
+#ifdef CONFIG_OF_OVERLAY
+
+/* order of entries is hard-coded into users of overlays[] */
+static struct overlay_info overlays[] = {
+	OVERLAY_INFO(overlay_base, -9999),
+	OVERLAY_INFO(overlay, 0),
+	OVERLAY_INFO(overlay_bad_phandle, -EINVAL),
+	{}
+};
+
+static struct device_node *overlay_base_root;
+
+/*
+ * Create base device tree for the overlay unittest.
+ *
+ * This is called from very early boot code.
+ *
+ * Do as much as possible the same way as done in __unflatten_device_tree
+ * and other early boot steps for the normal FDT so that the overlay base
+ * unflattened tree will have the same characteristics as the real tree
+ * (such as having memory allocated by the early allocator).  The goal
+ * is to test "the real thing" as much as possible, and test "test setup
+ * code" as little as possible.
+ *
+ * Have to stop before resolving phandles, because that uses kmalloc.
+ */
+void __init unittest_unflatten_overlay_base(void)
+{
+	struct overlay_info *info;
+	u32 data_size;
+	u32 size;
+
+	info = &overlays[0];
+
+	if (info->expected_result != -9999) {
+		pr_err("No dtb 'overlay_base' to attach\n");
+		return;
+	}
+
+	data_size = info->dtb_end - info->dtb_begin;
+	if (!data_size) {
+		pr_err("No dtb 'overlay_base' to attach\n");
+		return;
+	}
+
+	size = fdt_totalsize(info->dtb_begin);
+	if (size != data_size) {
+		pr_err("dtb 'overlay_base' header totalsize != actual size");
+		return;
+	}
+
+	info->data = early_init_dt_alloc_memory_arch(size,
+					     roundup_pow_of_two(FDT_V17_SIZE));
+	if (!info->data) {
+		pr_err("alloc for dtb 'overlay_base' failed");
+		return;
+	}
+
+	memcpy(info->data, info->dtb_begin, size);
+
+	__unflatten_device_tree(info->data, NULL, &info->np_overlay,
+				early_init_dt_alloc_memory_arch, true);
+	overlay_base_root = info->np_overlay;
+}
+
+/*
+ * The purpose of of_unittest_overlay_data_add is to add an
+ * overlay in the normal fashion.  This is a test of the whole
+ * picture, instead of testing individual elements.
+ *
+ * A secondary purpose is to be able to verify that the contents of
+ * /proc/device-tree/ contains the updated structure and values from
+ * the overlay.  That must be verified separately in user space.
+ *
+ * Return 0 on unexpected error.
+ */
+static int __init overlay_data_add(int onum)
+{
+	struct overlay_info *info;
+	int k;
+	int ret;
+	u32 size;
+	u32 size_from_header;
+
+	for (k = 0, info = overlays; info; info++, k++) {
+		if (k == onum)
+			break;
+	}
+	if (onum > k)
+		return 0;
+
+	size = info->dtb_end - info->dtb_begin;
+	if (!size) {
+		pr_err("no overlay to attach, %d\n", onum);
+		ret = 0;
+	}
+
+	size_from_header = fdt_totalsize(info->dtb_begin);
+	if (size_from_header != size) {
+		pr_err("overlay header totalsize != actual size, %d", onum);
+		return 0;
+	}
+
+	/*
+	 * Must create permanent copy of FDT because of_fdt_unflatten_tree()
+	 * will create pointers to the passed in FDT in the EDT.
+	 */
+	info->data = kmemdup(info->dtb_begin, size, GFP_KERNEL);
+	if (!info->data) {
+		pr_err("unable to allocate memory for data, %d\n", onum);
+		return 0;
+	}
+
+	of_fdt_unflatten_tree(info->data, NULL, &info->np_overlay);
+	if (!info->np_overlay) {
+		pr_err("unable to unflatten overlay, %d\n", onum);
+		ret = 0;
+		goto out_free_data;
+	}
+	of_node_set_flag(info->np_overlay, OF_DETACHED);
+
+	ret = of_resolve_phandles(info->np_overlay);
+	if (ret) {
+		pr_err("resolve ot phandles (ret=%d), %d\n", ret, onum);
+		goto out_free_np_overlay;
+	}
+
+	ret = of_overlay_create(info->np_overlay);
+	if (ret < 0) {
+		pr_err("of_overlay_create() (ret=%d), %d\n", ret, onum);
+		goto out_free_np_overlay;
+	} else {
+		info->overlay_id = ret;
+		ret = 0;
+	}
+
+	pr_debug("__dtb_overlay_begin applied, overlay id %d\n", ret);
+
+	goto out;
+
+out_free_np_overlay:
+	/*
+	 * info->np_overlay is the unflattened device tree
+	 * It has not been spliced into the live tree.
+	 */
+
+	/* todo: function to free unflattened device tree */
+
+out_free_data:
+	kfree(info->data);
+
+out:
+	return (ret == info->expected_result);
+}
+
+/*
+ * The purpose of of_unittest_overlay_high_level is to add an overlay
+ * in the normal fashion.  This is a test of the whole picture,
+ * instead of individual elements.
+ *
+ * The first part of the function is _not_ normal overlay usage; it is
+ * finishing splicing the base overlay device tree into the live tree.
+ */
+static __init void of_unittest_overlay_high_level(void)
+{
+	struct device_node *last_sibling;
+	struct device_node *np;
+	struct device_node *of_symbols;
+	struct device_node *overlay_base_symbols;
+	struct device_node **pprev;
+	struct property *prop;
+	int ret;
+
+	if (!overlay_base_root) {
+		unittest(0, "overlay_base_root not initialized\n");
+		return;
+	}
+
+	/*
+	 * Could not fixup phandles in unittest_unflatten_overlay_base()
+	 * because kmalloc() was not yet available.
+	 */
+	of_resolve_phandles(overlay_base_root);
+
+	/*
+	 * do not allow overlay_base to duplicate any node already in
+	 * tree, this greatly simplifies the code
+	 */
+
+	/*
+	 * remove overlay_base_root node "__local_fixups", after
+	 * being used by of_resolve_phandles()
+	 */
+	pprev = &overlay_base_root->child;
+	for (np = overlay_base_root->child; np; np = np->sibling) {
+		if (!of_node_cmp(np->name, "__local_fixups__")) {
+			*pprev = np->sibling;
+			break;
+		}
+		pprev = &np->sibling;
+	}
+
+	/* remove overlay_base_root node "__symbols__" if in live tree */
+	of_symbols = of_get_child_by_name(of_root, "__symbols__");
+	if (of_symbols) {
+		/* will have to graft properties from node into live tree */
+		pprev = &overlay_base_root->child;
+		for (np = overlay_base_root->child; np; np = np->sibling) {
+			if (!of_node_cmp(np->name, "__symbols__")) {
+				overlay_base_symbols = np;
+				*pprev = np->sibling;
+				break;
+			}
+			pprev = &np->sibling;
+		}
+	}
+
+	for (np = overlay_base_root->child; np; np = np->sibling) {
+		if (of_get_child_by_name(of_root, np->name)) {
+			unittest(0, "illegal node name in overlay_base %s",
+				np->name);
+			return;
+		}
+	}
+
+	/*
+	 * overlay 'overlay_base' is not allowed to have root
+	 * properties, so only need to splice nodes into main device tree.
+	 *
+	 * root node of *overlay_base_root will not be freed, it is lost
+	 * memory.
+	 */
+
+	for (np = overlay_base_root->child; np; np = np->sibling)
+		np->parent = of_root;
+
+	mutex_lock(&of_mutex);
+
+	for (np = of_root->child; np; np = np->sibling)
+		last_sibling = np;
+
+	if (last_sibling)
+		last_sibling->sibling = overlay_base_root->child;
+	else
+		of_root->child = overlay_base_root->child;
+
+	for_each_of_allnodes_from(overlay_base_root, np)
+		__of_attach_node_sysfs(np);
+
+	if (of_symbols) {
+		for_each_property_of_node(overlay_base_symbols, prop) {
+			ret = __of_add_property(of_symbols, prop);
+			if (ret) {
+				unittest(0,
+					 "duplicate property '%s' in overlay_base node __symbols__",
+					 prop->name);
+				return;
+			}
+			ret = __of_add_property_sysfs(of_symbols, prop);
+			if (ret) {
+				unittest(0,
+					 "unable to add property '%s' in overlay_base node __symbols__ to sysfs",
+					 prop->name);
+				return;
+			}
+		}
+	}
+
+	mutex_unlock(&of_mutex);
+
+
+	/* now do the normal overlay usage test */
+
+	unittest(overlay_data_add(1),
+		 "Adding overlay 'overlay' failed\n");
+
+	unittest(overlay_data_add(2),
+		 "Adding overlay 'overlay_bad_phandle' failed\n");
+}
+
+#else
+
+static inline __init void of_unittest_overlay_high_level(void) {}
+
+#endif
+
 static int __init of_unittest(void)
 {
 	struct device_node *np;
@@ -1962,6 +2277,8 @@ static int __init of_unittest(void)
 	/* Double check linkage after removing testcase data */
 	of_unittest_check_tree_linkage();
 
+	of_unittest_overlay_high_level();
+
 	pr_info("end of unittest - %i passed, %i failed\n",
 		unittest_results.passed, unittest_results.failed);
 
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH v4 1/2] of: per-file dtc compiler flags

[Masahiro Yamada]

2017-04-26 9:09 GMT+09:00  <frowand.list@gmail.com>:
> From: Frank Rowand <frank.rowand@sony.com>
>
> The dtc compiler version that adds initial support was available
> in 4.11-rc1.  Add the ability to set an additional dtc compiler
> flag is needed by overlays.
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---

Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>





-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 1/2] of: per-file dtc compiler flags

[Rob Herring]

On Tue, Apr 25, 2017 at 05:09:53PM -0700, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> The dtc compiler version that adds initial support was available
> in 4.11-rc1.  Add the ability to set an additional dtc compiler
> flag is needed by overlays.
> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  scripts/Makefile.lib | 2 ++
>  1 file changed, 2 insertions(+)

Applied.

Re: [PATCH v4 2/2] of: Add unit tests for applying overlays

[Rob Herring]

On Tue, Apr 25, 2017 at 05:09:54PM -0700, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Existing overlay unit tests examine individual pieces of the overlay
> code.  The new tests target the entire process of applying an overlay.
> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> 
> There are checkpatch warnings.  I have reviewed them and feel they
> can be ignored.
> 
>  drivers/of/fdt.c                                 |  14 +-
>  drivers/of/of_private.h                          |  12 +
>  drivers/of/unittest-data/Makefile                |  17 +-
>  drivers/of/unittest-data/overlay.dts             |  53 ++++
>  drivers/of/unittest-data/overlay_bad_phandle.dts |  20 ++
>  drivers/of/unittest-data/overlay_base.dts        |  80 ++++++
>  drivers/of/unittest.c                            | 317 +++++++++++++++++++++++
>  7 files changed, 505 insertions(+), 8 deletions(-)
>  create mode 100644 drivers/of/unittest-data/overlay.dts
>  create mode 100644 drivers/of/unittest-data/overlay_bad_phandle.dts
>  create mode 100644 drivers/of/unittest-data/overlay_base.dts

Applied.

Rob

Re: [PATCH v4 2/2] of: Add unit tests for applying overlays

[Geert Uytterhoeven]

Hi Frank,

On Wed, Apr 26, 2017 at 2:09 AM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> Existing overlay unit tests examine individual pieces of the overlay
> code.  The new tests target the entire process of applying an overlay.
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>
> There are checkpatch warnings.  I have reviewed them and feel they
> can be ignored.
>
>  drivers/of/fdt.c                                 |  14 +-
>  drivers/of/of_private.h                          |  12 +
>  drivers/of/unittest-data/Makefile                |  17 +-
>  drivers/of/unittest-data/overlay.dts             |  53 ++++
>  drivers/of/unittest-data/overlay_bad_phandle.dts |  20 ++
>  drivers/of/unittest-data/overlay_base.dts        |  80 ++++++
>  drivers/of/unittest.c                            | 317 +++++++++++++++++++++++
>  7 files changed, 505 insertions(+), 8 deletions(-)
>
>  create mode 100644 drivers/of/unittest-data/overlay.dts
>  create mode 100644 drivers/of/unittest-data/overlay_bad_phandle.dts
>  create mode 100644 drivers/of/unittest-data/overlay_base.dts

Shouldn't these be called .dtso instead of .dts?

> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile

> +# enable creation of __symbols__ node
> +DTC_FLAGS_overlay := -@
> +DTC_FLAGS_overlay_bad_phandle := -@
> +DTC_FLAGS_overlay_base := -@

This flag is needed for all DTs that will be involved with overlays.

Hence what about enabling this globally instead, cfr. "Enable DT symbols when"
CONFIG_OF_OVERLAY is used
("http://www.spinics.net/lists/devicetree/msg103363.html")?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/2] of: Add unit tests for applying overlays

[Frank Rowand]

On 04/28/17 04:25, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Wed, Apr 26, 2017 at 2:09 AM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Existing overlay unit tests examine individual pieces of the overlay
>> code.  The new tests target the entire process of applying an overlay.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> There are checkpatch warnings.  I have reviewed them and feel they
>> can be ignored.
>>
>>  drivers/of/fdt.c                                 |  14 +-
>>  drivers/of/of_private.h                          |  12 +
>>  drivers/of/unittest-data/Makefile                |  17 +-
>>  drivers/of/unittest-data/overlay.dts             |  53 ++++
>>  drivers/of/unittest-data/overlay_bad_phandle.dts |  20 ++
>>  drivers/of/unittest-data/overlay_base.dts        |  80 ++++++
>>  drivers/of/unittest.c                            | 317 +++++++++++++++++++++++
>>  7 files changed, 505 insertions(+), 8 deletions(-)
>>
>>  create mode 100644 drivers/of/unittest-data/overlay.dts
>>  create mode 100644 drivers/of/unittest-data/overlay_bad_phandle.dts
>>  create mode 100644 drivers/of/unittest-data/overlay_base.dts
> 
> Shouldn't these be called .dtso instead of .dts?

That is a good question.  I'm not worried about solving it this week for
this patch, because this could turn into a bikeshed and I can always
fix it with a patch if we decide to change.  But if we do want to change
the naming, I would like to make the decision in the next couple of
months.  I would like to see more progress on overlays in general
this summer, and plan to be working on them myself.

I _think_ there has been some discussion about source file naming on the
devicetree-compiler or devicetree list in the far distant past.  Or I
may just be mis-remembering.

As far as I know, the current dtc does not know any suffixes other than
.dts for source files.  Not that the compiler has to know, we can always
specify '-I dts'.


> 
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
> 
>> +# enable creation of __symbols__ node
>> +DTC_FLAGS_overlay := -@
>> +DTC_FLAGS_overlay_bad_phandle := -@
>> +DTC_FLAGS_overlay_base := -@
> 
> This flag is needed for all DTs that will be involved with overlays.
> 
> Hence what about enabling this globally instead, cfr. "Enable DT symbols when"
> CONFIG_OF_OVERLAY is used
> ("http://www.spinics.net/lists/devicetree/msg103363.html")?

And another really good question.

There are some issues.  I have thought about it enough to know there are issues,
but do not have a solution and do not think I know all the issues.  Some
possible issues (or perceived issues) are:

- The size of __symbols__ in an FDT (akd compile .dtb image) in either a kernel
  image or a bootloader if overlays are not actually needed on a given system
  (even if the system is physically capable of using overlays).

- The size of __symbols__ in kernel memory if overlays are not actually needed
  on a given system (even if the system is physically capable of using overlays.)
  This could be possibly be enabled/disabled by a boot command, even if
  __symbols__ is in the FDT.

- A base FDT might want to have __symbols__ included with the expectation that
  overlays will be used in the future.  (The FDT might be built for the boot
  loader, then be stable for many kernel releases.)

- Should the creation of __symbols__ be a global switch, or should it be
  controlled on a per dtb basis?  Or a combination of both?

Again, I'm not worried about an immediate, this week solution, but I would
like to make good progress on this in the next couple of months.

-Frank

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Re: [PATCH v4 2/2] of: Add unit tests for applying overlays

[Geert Uytterhoeven]

Hi Frank,

On Fri, Apr 28, 2017 at 5:24 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 04/28/17 04:25, Geert Uytterhoeven wrote:
>> On Wed, Apr 26, 2017 at 2:09 AM,  <frowand.list@gmail.com> wrote:
>>>  create mode 100644 drivers/of/unittest-data/overlay.dts
>>>  create mode 100644 drivers/of/unittest-data/overlay_bad_phandle.dts
>>>  create mode 100644 drivers/of/unittest-data/overlay_base.dts
>>
>> Shouldn't these be called .dtso instead of .dts?
>
> That is a good question.  I'm not worried about solving it this week for
> this patch, because this could turn into a bikeshed and I can always
> fix it with a patch if we decide to change.  But if we do want to change
> the naming, I would like to make the decision in the next couple of
> months.  I would like to see more progress on overlays in general
> this summer, and plan to be working on them myself.
>
> I _think_ there has been some discussion about source file naming on the
> devicetree-compiler or devicetree list in the far distant past.  Or I
> may just be mis-remembering.
>
> As far as I know, the current dtc does not know any suffixes other than
> .dts for source files.  Not that the compiler has to know, we can always
> specify '-I dts'.

That's not an obstacle, though. I've been compiling .dtso files into .dtbo
files for several years, cfr.
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/renesas-overlays

According to "make V=1", the kernel build system basically uses

    ./scripts/dtc/dtc -O dtb -o <file>.dtbo <file>.dtbo.dts.tmp

with <file>.dtbo.dts.tmp the preprocessed .dtso files.
And dtc copes fine with that.

>>> --- a/drivers/of/unittest-data/Makefile
>>> +++ b/drivers/of/unittest-data/Makefile
>>
>>> +# enable creation of __symbols__ node
>>> +DTC_FLAGS_overlay := -@
>>> +DTC_FLAGS_overlay_bad_phandle := -@
>>> +DTC_FLAGS_overlay_base := -@
>>
>> This flag is needed for all DTs that will be involved with overlays.
>>
>> Hence what about enabling this globally instead, cfr. "Enable DT symbols when"
>> CONFIG_OF_OVERLAY is used
>> ("http://www.spinics.net/lists/devicetree/msg103363.html")?
>
> And another really good question.
>
> There are some issues.  I have thought about it enough to know there are issues,
> but do not have a solution and do not think I know all the issues.  Some
> possible issues (or perceived issues) are:
>
> - The size of __symbols__ in an FDT (akd compile .dtb image) in either a kernel
>   image or a bootloader if overlays are not actually needed on a given system
>   (even if the system is physically capable of using overlays).

What do you mean with "physically capable of using overlays"?
The presence of expansion connectors, like Raspberry Pi and BeagleBone Black?

Even lacking such connectors, an overlay could add hardware description that
was just missing (forgotten, or lack of DT bindings) when the main DTB was
built.

I agree about the size, but you never know in advance if an overlay will
be used or needed later.

> - The size of __symbols__ in kernel memory if overlays are not actually needed
>   on a given system (even if the system is physically capable of using overlays.)
>   This could be possibly be enabled/disabled by a boot command, even if
>   __symbols__ is in the FDT.

Agreed.  Not allowing overlays is akin to not allowing to load (more) kernel
modules, and may increase security.

> - A base FDT might want to have __symbols__ included with the expectation that
>   overlays will be used in the future.  (The FDT might be built for the boot
>   loader, then be stable for many kernel releases.)
>
> - Should the creation of __symbols__ be a global switch, or should it be
>   controlled on a per dtb basis?  Or a combination of both?

So you want to decouple OF overlay support in the Linux kernel from
__symbols__ present in the DTBs built?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/2] of: Add unit tests for applying overlays

[Rob Herring]

On Tue, Apr 25, 2017 at 7:09 PM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> Existing overlay unit tests examine individual pieces of the overlay
> code.  The new tests target the entire process of applying an overlay.
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---

[...]

> @@ -1261,6 +1263,8 @@ void __init unflatten_device_tree(void)
>
>         /* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
>         of_alias_scan(early_init_dt_alloc_memory_arch);
> +
> +       unittest_unflatten_overlay_base();

This breaks on systems that don't boot with FDT and call
unflatten_device_tree, namely x86 and UML (UML needs a few hacks to
work) which was a feature of the unittest. Considering applying
overlays on x86 is something we'll want to support, the unittest
should support that case.

Rob

Re: [PATCH v4 2/2] of: Add unit tests for applying overlays

[Frank Rowand]

On 07/25/17 13:36, Rob Herring wrote:
> On Tue, Apr 25, 2017 at 7:09 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Existing overlay unit tests examine individual pieces of the overlay
>> code.  The new tests target the entire process of applying an overlay.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
> 
> [...]
> 
>> @@ -1261,6 +1263,8 @@ void __init unflatten_device_tree(void)
>>
>>         /* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
>>         of_alias_scan(early_init_dt_alloc_memory_arch);
>> +
>> +       unittest_unflatten_overlay_base();
> 
> This breaks on systems that don't boot with FDT and call
> unflatten_device_tree, namely x86 and UML (UML needs a few hacks to
> work) which was a feature of the unittest. Considering applying
> overlays on x86 is something we'll want to support, the unittest
> should support that case.
> 
> Rob
> 

I meant to reply when I read this...

I'll look at this, probably next week.  Thanks for pointing out
the issue.

-Frank

Re: [PATCH v4 2/2] of: Add unit tests for applying overlays

[Frank Rowand]

Hi Rob,

On 07/25/17 13:36, Rob Herring wrote:
> On Tue, Apr 25, 2017 at 7:09 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Existing overlay unit tests examine individual pieces of the overlay
>> code.  The new tests target the entire process of applying an overlay.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
> 
> [...]
> 
>> @@ -1261,6 +1263,8 @@ void __init unflatten_device_tree(void)
>>
>>         /* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
>>         of_alias_scan(early_init_dt_alloc_memory_arch);
>> +
>> +       unittest_unflatten_overlay_base();
> 
> This breaks on systems that don't boot with FDT and call
> unflatten_device_tree, namely x86 and UML (UML needs a few hacks to
> work) which was a feature of the unittest. Considering applying
> overlays on x86 is something we'll want to support, the unittest
> should support that case.
> 
> Rob

What is the symptom of the breakage?  Or what is causing breakage?

-Frank