[PATCH v4 0/8] Introducing (yet again) Device Tree Overlays

[PATCH v4 0/8] Introducing (yet again) Device Tree Overlays

[Pantelis Antoniou]

From: Pantelis Antoniou <panto@antoniou-consulting.com>

The following patchset introduces Device Tree overlays, a method
of dynamically altering the kernel's live Device Tree, along with
a generic interface to use it in a board agnostic manner.

It is dependent on Grant Likely's DT kobjectification patches located
in his tree as queued for -next.

It relies on the following previously submitted patches/patchsets:

* OF: Add [__]of_find_node_by_full_name
* OF: Utility helper functions for dynamic nodes

To compile overlays you need the DTC compiler patch

* "dtc: Dynamic symbols & fixup support (v2)"

Changes since V3:
* Added overlay self-tests.
* Fix bug in of_init_overlay_info (wrong sizeof)
* Platform bus handler handles parent_pdev == NULL
* of_resolve fixes according to comments by robh
  + changed if (foo == NULL) to if (!foo)
  + changed if (foo != NULL) to if (foo)
  + drivers/of/Kconfig added dep on OF && !SPARC
  + convert to using be32_to_cpup
  + u32 -> __be32 when modifying property values
  + cosmetic fixes

Changes since V2:
* Use of a configfs board agnostic overlay method
* Use of per bus handlers instead of hardcoded behaviour
* Optional target-path overlay target, which allows one to use standard
DTBs without resolution options.

Changes since V1:

* Removal of any bits related to a specific board (beaglebone).
* Introduced a platform agnostic interface using /proc/device-tree-overlay
* Various bug fixes related to i2c device handling have been squashed in.

Pantelis Antoniou (8):
  OF: Introduce Device Tree resolve support.
  OF: Introduce DT overlay support.
  OF: DT-Overlay configfs interface
  OF: platform: Add overlay bus handler
  of: i2c: Export single device registration method
  OF: i2c: Add overlay bus handler
  OF: spi: Add overlay bus handler
  OF: selftest: Add overlay self-test support.

 .../devicetree/dynamic-resolution-notes.txt        |  25 +
 Documentation/devicetree/overlay-notes.txt         | 187 +++++
 drivers/base/platform.c                            |  98 ++-
 drivers/i2c/i2c-core.c                             | 186 +++--
 drivers/of/Kconfig                                 |  24 +
 drivers/of/Makefile                                |   3 +
 drivers/of/configfs.c                              | 272 +++++++
 drivers/of/overlay.c                               | 895 +++++++++++++++++++++
 drivers/of/resolver.c                              | 368 +++++++++
 drivers/of/selftest.c                              | 368 +++++++++
 drivers/of/testcase-data/testcases.dtsi            |   1 +
 drivers/of/testcase-data/tests-overlay.dtsi        | 125 +++
 drivers/spi/spi.c                                  | 345 +++++---
 include/linux/i2c.h                                |  10 +
 include/linux/of.h                                 | 170 ++++
 15 files changed, 2925 insertions(+), 152 deletions(-)
 create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
 create mode 100644 Documentation/devicetree/overlay-notes.txt
 create mode 100644 drivers/of/configfs.c
 create mode 100644 drivers/of/overlay.c
 create mode 100644 drivers/of/resolver.c
 create mode 100644 drivers/of/testcase-data/tests-overlay.dtsi

-- 
1.7.12

[PATCH v4 1/8] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Introduce support for dynamic device tree resolution.
Using it, it is possible to prepare a device tree that's
been loaded on runtime to be modified and inserted at the kernel
live tree.

Export of of_resolve by Guenter Roeck <groeck@juniper.net>

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 .../devicetree/dynamic-resolution-notes.txt        |  25 ++
 drivers/of/Kconfig                                 |   9 +
 drivers/of/Makefile                                |   1 +
 drivers/of/resolver.c                              | 368 +++++++++++++++++++++
 include/linux/of.h                                 |  17 +
 5 files changed, 420 insertions(+)
 create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
 create mode 100644 drivers/of/resolver.c

diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
new file mode 100644
index 0000000..0b396c4
--- /dev/null
+++ b/Documentation/devicetree/dynamic-resolution-notes.txt
@@ -0,0 +1,25 @@
+Device Tree Dynamic Resolver Notes
+----------------------------------
+
+This document describes the implementation of the in-kernel
+Device Tree resolver, residing in drivers/of/resolver.c and is a
+companion document to Documentation/devicetree/dt-object-internal.txt[1]
+
+How the resolver works
+----------------------
+
+The resolver is given as an input an arbitrary tree compiled with the
+proper dtc option and having a /plugin/ tag. This generates the
+appropriate __fixups__ & __local_fixups__ nodes as described in [1].
+
+In sequence the resolver works by the following steps:
+
+1. Get the maximum device tree phandle value from the live tree + 1.
+2. Adjust all the local phandles of the tree to resolve by that amount.
+3. Using the __local__fixups__ node information adjust all local references
+   by the same amount.
+4. For each property in the __fixups__ node locate the node it references
+   in the live tree. This is the label used to tag the node.
+5. Retrieve the phandle of the target of the fixup.
+5. For each fixup in the property locate the node:property:offset location
+   and replace it with the phandle value.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 889005f..4d39c88 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -77,4 +77,13 @@ config OF_RESERVED_MEM
 	help
 	  Helpers to allow for reservation of memory regions
 
+config OF_RESOLVE
+	bool "OF Dynamic resolution support"
+	depends on OF && !SPARC
+	select OF_DYNAMIC
+	select OF_DEVICE
+	help
+	  Enable OF dynamic resolution support. This allows you to
+	  load Device Tree object fragments are run time.
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 26de2ed..c241e79 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
+obj-$(CONFIG_OF_RESOLVE)  += resolver.o
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
new file mode 100644
index 0000000..7c29b16
--- /dev/null
+++ b/drivers/of/resolver.c
@@ -0,0 +1,368 @@
+/*
+ * Functions for dealing with DT resolution
+ *
+ * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+
+/*
+ * Find live tree's maximum phandle value.
+ */
+static phandle of_get_tree_max_phandle(void)
+{
+	struct device_node *node;
+	phandle phandle;
+	unsigned long flags;
+
+	/* now search recursively */
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	phandle = 0;
+	for_each_of_allnodes(node) {
+		if (node->phandle != OF_PHANDLE_ILLEGAL &&
+				node->phandle > phandle)
+			phandle = node->phandle;
+	}
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	return phandle;
+}
+
+/*
+ * Adjust a subtree's phandle values by a given delta.
+ * Makes sure not to just adjust the device node's phandle value,
+ * but modify the phandle properties values as well.
+ */
+static void __of_adjust_tree_phandles(struct device_node *node,
+		int phandle_delta)
+{
+	struct device_node *child;
+	struct property *prop;
+	phandle phandle;
+
+	/* first adjust the node's phandle direct value */
+	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
+		node->phandle += phandle_delta;
+
+	/* now adjust phandle & linux,phandle values */
+	for_each_property_of_node(node, prop) {
+
+		/* only look for these two */
+		if (of_prop_cmp(prop->name, "phandle") != 0 &&
+		    of_prop_cmp(prop->name, "linux,phandle") != 0)
+			continue;
+
+		/* must be big enough */
+		if (prop->length < 4)
+			continue;
+
+		/* read phandle value */
+		phandle = be32_to_cpup(prop->value);
+		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
+			continue;
+
+		/* adjust */
+		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
+	}
+
+	/* now do the children recursively */
+	__for_each_child_of_node(node, child)
+		__of_adjust_tree_phandles(child, phandle_delta);
+}
+
+/*
+ * Adjust the local phandle references by the given phandle delta.
+ * Assumes the existances of a __local_fixups__ node at the root
+ * of the tree. Does not take any devtree locks so make sure you
+ * call this on a tree which is at the detached state.
+ */
+static int __of_adjust_tree_phandle_references(struct device_node *node,
+		int phandle_delta)
+{
+	phandle phandle;
+	struct device_node *refnode, *child;
+	struct property *rprop, *sprop;
+	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
+	int offset, propcurlen;
+	int err;
+
+	/* locate the symbols & fixups nodes on resolve */
+	__for_each_child_of_node(node, child)
+		if (of_node_cmp(child->name, "__local_fixups__") == 0)
+			break;
+
+	/* no local fixups */
+	if (!child)
+		return 0;
+
+	/* find the local fixups property */
+	for_each_property_of_node(child, rprop) {
+
+		/* skip properties added automatically */
+		if (of_prop_cmp(rprop->name, "name") == 0)
+			continue;
+
+		/* make a copy */
+		propval = kmalloc(rprop->length, GFP_KERNEL);
+		if (!propval) {
+			pr_err("%s: Could not copy value of '%s'\n",
+					__func__, rprop->name);
+			return -ENOMEM;
+		}
+		memcpy(propval, rprop->value, rprop->length);
+
+		propend = propval + rprop->length;
+		for (propcur = propval; propcur < propend;
+				propcur += propcurlen + 1) {
+
+			propcurlen = strlen(propcur);
+
+			nodestr = propcur;
+			s = strchr(propcur, ':');
+			if (!s) {
+				pr_err("%s: Illegal symbol entry '%s' (1)\n",
+					__func__, propcur);
+				err = -EINVAL;
+				goto err_fail;
+			}
+			*s++ = '\0';
+
+			propstr = s;
+			s = strchr(s, ':');
+			if (!s) {
+				pr_err("%s: Illegal symbol entry '%s' (2)\n",
+					__func__, (char *)rprop->value);
+				err = -EINVAL;
+				goto err_fail;
+			}
+
+			*s++ = '\0';
+			offset = simple_strtoul(s, NULL, 10);
+
+			/* look into the resolve node for the full path */
+			refnode = __of_find_node_by_full_name(node, nodestr);
+			if (!refnode) {
+				pr_warn("%s: Could not find refnode '%s'\n",
+					__func__, (char *)rprop->value);
+				continue;
+			}
+
+			/* now find the property */
+			for_each_property_of_node(refnode, sprop) {
+				if (of_prop_cmp(sprop->name, propstr) == 0)
+					break;
+			}
+
+			if (!sprop) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			phandle = be32_to_cpup(sprop->value + offset);
+			*(__be32 *)(sprop->value + offset) =
+				cpu_to_be32(phandle + phandle_delta);
+		}
+
+		kfree(propval);
+	}
+
+	return 0;
+
+err_fail:
+	kfree(propval);
+	return err;
+}
+
+/**
+ * of_resolve	- Resolve the given node against the live tree.
+ *
+ * @resolve:	Node to resolve
+ *
+ * Perform dynamic Device Tree resolution against the live tree
+ * to the given node to resolve. This depends on the live tree
+ * having a __symbols__ node, and the resolve node the __fixups__ &
+ * __local_fixups__ nodes (if needed).
+ * The result of the operation is a resolve node that it's contents
+ * are fit to be inserted or operate upon the live tree.
+ * Returns 0 on success or a negative error value on error.
+ */
+int of_resolve(struct device_node *resolve)
+{
+	struct device_node *child, *refnode;
+	struct device_node *root_sym, *resolve_sym, *resolve_fix;
+	struct property *rprop, *sprop;
+	const char *refpath;
+	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
+	int offset, propcurlen;
+	phandle phandle, phandle_delta;
+	int err;
+
+	/* the resolve node must exist, and be detached */
+	if (!resolve || !of_node_check_flag(resolve, OF_DETACHED))
+		return -EINVAL;
+
+	/* first we need to adjust the phandles */
+	phandle_delta = of_get_tree_max_phandle() + 1;
+	__of_adjust_tree_phandles(resolve, phandle_delta);
+	err = __of_adjust_tree_phandle_references(resolve, phandle_delta);
+	if (err != 0)
+		return err;
+
+	root_sym = NULL;
+	resolve_sym = NULL;
+	resolve_fix = NULL;
+
+	/* this may fail (if no fixups are required) */
+	root_sym = of_find_node_by_path("/__symbols__");
+
+	/* locate the symbols & fixups nodes on resolve */
+	__for_each_child_of_node(resolve, child) {
+
+		if (!resolve_sym &&
+				of_node_cmp(child->name, "__symbols__") == 0)
+			resolve_sym = child;
+
+		if (!resolve_fix &&
+				of_node_cmp(child->name, "__fixups__") == 0)
+			resolve_fix = child;
+
+		/* both found, don't bother anymore */
+		if (resolve_sym && resolve_fix)
+			break;
+	}
+
+	/* we do allow for the case where no fixups are needed */
+	if (!resolve_fix) {
+		err = 0;	/* no error */
+		goto out;
+	}
+
+	/* we need to fixup, but no root symbols... */
+	if (!root_sym) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	for_each_property_of_node(resolve_fix, rprop) {
+
+		/* skip properties added automatically */
+		if (of_prop_cmp(rprop->name, "name") == 0)
+			continue;
+
+		err = of_property_read_string(root_sym,
+				rprop->name, &refpath);
+		if (err != 0) {
+			pr_err("%s: Could not find symbol '%s'\n",
+					__func__, rprop->name);
+			goto out;
+		}
+
+		refnode = of_find_node_by_path(refpath);
+		if (!refnode) {
+			pr_err("%s: Could not find node by path '%s'\n",
+					__func__, refpath);
+			err = -ENOENT;
+			goto out;
+		}
+
+		phandle = refnode->phandle;
+		of_node_put(refnode);
+
+		pr_debug("%s: %s phandle is 0x%08x\n",
+				__func__, rprop->name, phandle);
+
+		/* make a copy */
+		propval = kmalloc(rprop->length, GFP_KERNEL);
+		if (!propval) {
+			pr_err("%s: Could not copy value of '%s'\n",
+					__func__, rprop->name);
+			err = -ENOMEM;
+			goto out;
+		}
+
+		memcpy(propval, rprop->value, rprop->length);
+
+		propend = propval + rprop->length;
+		for (propcur = propval; propcur < propend;
+				propcur += propcurlen + 1) {
+			propcurlen = strlen(propcur);
+
+			nodestr = propcur;
+			s = strchr(propcur, ':');
+			if (!s) {
+				pr_err("%s: Illegal symbol "
+					"entry '%s' (1)\n",
+					__func__, (char *)rprop->value);
+				err = -EINVAL;
+				goto err_fail_free;
+			}
+			*s++ = '\0';
+
+			propstr = s;
+			s = strchr(s, ':');
+			if (!s) {
+				pr_err("%s: Illegal symbol "
+					"entry '%s' (2)\n",
+					__func__, (char *)rprop->value);
+				err = -EINVAL;
+				goto err_fail_free;
+			}
+
+			*s++ = '\0';
+			offset = simple_strtoul(s, NULL, 10);
+
+			/* look into the resolve node for the full path */
+			refnode = __of_find_node_by_full_name(resolve,
+					nodestr);
+			if (!refnode) {
+				pr_err("%s: Could not find refnode '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail_free;
+			}
+
+			/* now find the property */
+			for_each_property_of_node(refnode, sprop) {
+				if (of_prop_cmp(sprop->name, propstr) == 0)
+					break;
+			}
+
+			if (!sprop) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail_free;
+			}
+
+			*(__be32 *)(sprop->value + offset) =
+				cpu_to_be32(phandle);
+		}
+
+		kfree(propval);
+	}
+
+err_fail_free:
+	kfree(propval);
+
+out:
+	/* NULL is handled by of_node_put as NOP */
+	of_node_put(root_sym);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(of_resolve);
diff --git a/include/linux/of.h b/include/linux/of.h
index 4dcec8f..3edb9b9 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -811,4 +811,21 @@ static inline struct device_node *__of_create_empty_node( const char *name,
 
 #endif	/* !CONFIG_OF */
 
+
+/* illegal phandle value (set when unresolved) */
+#define OF_PHANDLE_ILLEGAL	0xdeadbeef
+
+#ifdef CONFIG_OF_RESOLVE
+
+int of_resolve(struct device_node *resolve);
+
+#else
+
+static inline int of_resolve(struct device_node *resolve)
+{
+	return -ENOTSUPP;
+}
+
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

[PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Introduce DT overlay support.
Using this functionality it is possible to dynamically overlay a part of
the kernel's tree with another tree that's been dynamically loaded.
It is also possible to remove node and properties.

The creation/destruction of the devices is handled by calling in to
bus specific handlers which can deal with the peculiarities of each
device.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 Documentation/devicetree/overlay-notes.txt | 187 ++++++
 drivers/of/Kconfig                         |  10 +
 drivers/of/Makefile                        |   1 +
 drivers/of/overlay.c                       | 895 +++++++++++++++++++++++++++++
 include/linux/of.h                         | 153 +++++
 5 files changed, 1246 insertions(+)
 create mode 100644 Documentation/devicetree/overlay-notes.txt
 create mode 100644 drivers/of/overlay.c

diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
new file mode 100644
index 0000000..882d512
--- /dev/null
+++ b/Documentation/devicetree/overlay-notes.txt
@@ -0,0 +1,187 @@
+Device Tree Overlay Notes
+-------------------------
+
+This document describes the implementation of the in-kernel
+device tree overlay functionality residing in drivers/of/overlay.c and is a
+companion document to Documentation/devicetree/dt-object-internal.txt[1] &
+Documentation/devicetree/dynamic-resolution-notes.txt[2]
+
+How overlays work
+-----------------
+
+A Device Tree's overlay purpose is to modify the kernel's live tree, and
+have the modification affecting the state of the the kernel in a way that
+is reflecting the changes.
+Since the kernel mainly deals with devices, any new device node that result
+in an active device should have it created while if the device node is either
+disabled or removed all together, the affected device should be deregistered.
+
+Lets take an example where we have a foo board with the following base tree
+which is taken from [1].
+
+---- foo.dts -----------------------------------------------------------------
+	/* FOO platform */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+		}
+	};
+---- foo.dts -----------------------------------------------------------------
+
+The overlay bar.dts, when loaded (and resolved as described in [2]) should
+
+---- bar.dts -----------------------------------------------------------------
+/plugin/;	/* allow undefined label references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&ocp>;
+		__overlay__ {
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+---- bar.dts -----------------------------------------------------------------
+
+result in foo+bar.dts
+
+---- foo+bar.dts -------------------------------------------------------------
+	/* FOO platform + bar peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		}
+	};
+---- foo+bar.dts -------------------------------------------------------------
+
+As a result of the the overlay, a new device node (bar) has been created
+so a bar platform device will be registered and if a matching device driver
+is loaded the device will be created as expected.
+
+Overlay in-kernel API
+---------------------
+
+The steps typically required to get an overlay to work are as follows:
+
+1. Use of_build_overlay_info() to create an array of initialized and
+ready to use of_overlay_info structures.
+2. Call of_overlay() to apply the overlays declared in the array.
+3. If the overlay needs to be removed, call of_overlay_revert().
+4. Finally release the memory taken by the overlay info array by
+of_free_overlay_info().
+
+/**
+ * of_build_overlay_info	- Build an overlay info array
+ * @tree:	Device node containing all the overlays
+ * @cntp:	Pointer to where the overlay info count will be help
+ * @ovinfop:	Pointer to the pointer of an overlay info structure.
+ *
+ * Helper function that given a tree containing overlay information,
+ * allocates and builds an overlay info array containing it, ready
+ * for use using of_overlay.
+ *
+ * Returns 0 on success with the @cntp @ovinfop pointers valid,
+ * while on error a negative error value is returned.
+ */
+int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop);
+
+/**
+ * of_free_overlay_info	- Free an overlay info array
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to free
+ *
+ * Releases the memory of a previously allocate ovinfo array
+ * by of_build_overlay_info.
+ * Returns 0, or an error if the arguments are bogus.
+ */
+int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab);
+
+/**
+ * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Applies the overlays given, while handling all error conditions
+ * appropriately. Either the operation succeeds, or if it fails the
+ * live tree is reverted to the state before the attempt.
+ * Returns 0, or an error if the overlay attempt failed.
+ */
+int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
+
+/**
+ * of_overlay_revert	- Revert a previously applied overlay
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Revert a previous overlay. The state of the live tree
+ * is reverted to the one before the overlay.
+ * Returns 0, or an error if the overlay table is not given.
+ */
+int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
+
+Overlay DTS Format
+------------------
+
+The DTS of an overlay should have the following format:
+
+{
+	/* ignored properties by the overlay */
+
+	fragment@0 {	/* first child node */
+
+		target=<phandle>;	/* phandle target of the overlay */
+	or
+		target-path="/path";	/* target path of the overlay */
+	or
+		target-alias="alias";	/* target alias of the overlay */
+		__overlay__ {
+			property-a;	/* add property-a to the target */
+			-property-b;	/* remove property-b from target */
+			node-a {	/* add to an existing, or create a node-a */
+				...
+			};
+			-node-b {	/* remove an existing node-b */
+				...
+			};
+		};
+	}
+	fragment@1 {	/* second child node */
+		...
+	};
+	/* more fragments follow */
+}
+
+It should be noted that the DT overlay format described is the one expected
+by the of_build_overlay_info() function, which is a helper function. There
+is nothing stopping someone coming up with his own DTS format and that will
+end up filling in the fields of the of_overlay_info array.
+
+Using the non-phandle based target method allows one to use a base DT which does
+not contain a __symbols__ now, i.e. it was not compiled with the -@ option.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 4d39c88..cfb7ff8 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -86,4 +86,14 @@ config OF_RESOLVE
 	  Enable OF dynamic resolution support. This allows you to
 	  load Device Tree object fragments are run time.
 
+config OF_OVERLAY
+	bool "OF overlay support"
+	depends on OF
+	select OF_DYNAMIC
+	select OF_DEVICE
+	select OF_RESOLVE
+	help
+	  OpenFirmware overlay support. Allows you to modify on runtime the
+	  live tree using overlays.
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index c241e79..d2a6e0d 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
 obj-$(CONFIG_OF_RESOLVE)  += resolver.o
+obj-$(CONFIG_OF_OVERLAY) += overlay.o
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
new file mode 100644
index 0000000..1d4b884
--- /dev/null
+++ b/drivers/of/overlay.c
@@ -0,0 +1,895 @@
+/*
+ * Functions for working with device tree overlays
+ *
+ * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+#undef DEBUG
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+/* protect the handlers list */
+static DEFINE_MUTEX(of_handler_mutex);
+static struct list_head of_handler_list = LIST_HEAD_INIT(of_handler_list);
+
+int of_overlay_handler_register(struct of_overlay_handler *handler)
+{
+	/* guard against bad data */
+	if (!handler || !handler->name || !handler->ops ||
+		!handler->ops->create || !handler->ops->remove)
+		return -EINVAL;
+
+	mutex_lock(&of_handler_mutex);
+	list_add_tail(&handler->list, &of_handler_list);
+	mutex_unlock(&of_handler_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_overlay_handler_register);
+
+void of_overlay_handler_unregister(struct of_overlay_handler *handler)
+{
+	struct of_overlay_handler *curr;
+
+	mutex_lock(&of_handler_mutex);
+	list_for_each_entry(curr, &of_handler_list, list) {
+		if (handler == curr) {
+			list_del(&handler->list);
+			break;
+		}
+	}
+	mutex_unlock(&of_handler_mutex);
+}
+EXPORT_SYMBOL_GPL(of_overlay_handler_unregister);
+
+static int handler_create(struct of_overlay_device_entry *entry, int revert)
+{
+	struct of_overlay_handler *handler;
+	int ret;
+
+	mutex_lock(&of_handler_mutex);
+	list_for_each_entry(handler, &of_handler_list, list) {
+		ret = (*handler->ops->create)(entry, revert);
+		/* ENOTSUPP means try next */
+		if (ret == -ENOTSUPP)
+			continue;
+		/* anything else means something happened */
+		break;
+	}
+	mutex_unlock(&of_handler_mutex);
+
+	return ret;
+}
+
+static int handler_remove(struct of_overlay_device_entry *entry, int revert)
+{
+	struct of_overlay_handler *handler;
+	int ret;
+
+	mutex_lock(&of_handler_mutex);
+	list_for_each_entry(handler, &of_handler_list, list) {
+		ret = (*handler->ops->remove)(entry, revert);
+		/* ENOTSUPP means try next */
+		if (ret == -ENOTSUPP)
+			continue;
+		/* anything else means something happened */
+		break;
+	}
+	mutex_unlock(&of_handler_mutex);
+
+	return ret;
+}
+
+/*
+ * Apply a single overlay node recursively.
+ *
+ * Property or node names that start with '-' signal that
+ * the property/node is to be removed.
+ *
+ * All the property notifiers are appropriately called.
+ * Note that the in case of an error the target node is left
+ * in a inconsistent state. Error recovery should be performed
+ * by recording the modification using the of notifiers.
+ */
+static int of_overlay_apply_one(struct device_node *target,
+		const struct device_node *overlay)
+{
+	const char *pname, *cname;
+	struct device_node *child, *tchild;
+	struct property *prop, *propn, *tprop;
+	int remove;
+	char *full_name;
+	const char *suffix;
+	int ret;
+
+	/* sanity checks */
+	if (target == NULL || overlay == NULL)
+		return -EINVAL;
+
+	for_each_property_of_node(overlay, prop) {
+
+		/* don't touch, 'name' */
+		if (of_prop_cmp(prop->name, "name") == 0)
+			continue;
+
+		/* default is add */
+		remove = 0;
+		pname = prop->name;
+		if (*pname == '-') {	/* skip, - notes removal */
+			pname++;
+			remove = 1;
+			propn = NULL;
+		} else {
+			propn = __of_copy_property(prop, GFP_KERNEL,
+					OF_PROP_ALLOCALL);
+			if (propn == NULL)
+				return -ENOMEM;
+		}
+
+		tprop = of_find_property(target, pname, NULL);
+
+		/* found? */
+		if (tprop != NULL) {
+			if (propn != NULL)
+				ret = of_update_property(target, propn);
+			else
+				ret = of_remove_property(target, tprop);
+		} else {
+			if (propn != NULL)
+				ret = of_add_property(target, propn);
+			else
+				ret = 0;
+		}
+		if (ret != 0)
+			return ret;
+	}
+
+	__for_each_child_of_node(overlay, child) {
+
+		/* default is add */
+		remove = 0;
+		cname = child->name;
+		if (*cname == '-') {	/* skip, - notes removal */
+			cname++;
+			remove = 1;
+		}
+
+		/* special case for nodes with a suffix */
+		suffix = strrchr(child->full_name, '@');
+		if (suffix != NULL) {
+			cname = kbasename(child->full_name);
+			WARN_ON(cname == NULL);	/* sanity check */
+			if (cname == NULL)
+				continue;
+			if (*cname == '-')
+				cname++;
+		}
+
+		tchild = of_get_child_by_name(target, cname);
+		if (tchild != NULL) {
+
+			if (!remove) {
+
+				/* apply overlay recursively */
+				ret = of_overlay_apply_one(tchild, child);
+				of_node_put(tchild);
+
+				if (ret != 0)
+					return ret;
+
+			} else {
+
+				ret = of_detach_node(tchild);
+				of_node_put(tchild);
+			}
+
+		} else {
+
+			if (!remove) {
+				full_name = kasprintf(GFP_KERNEL, "%s/%s",
+						target->full_name, cname);
+				if (full_name == NULL)
+					return -ENOMEM;
+
+				/* create empty tree as a target */
+				tchild = __of_create_empty_node(cname,
+						child->type, full_name,
+						child->phandle, GFP_KERNEL,
+						OF_NODE_ALLOCALL);
+
+				/* free either way */
+				kfree(full_name);
+
+				if (tchild == NULL)
+					return -ENOMEM;
+
+				/* point to parent */
+				tchild->parent = target;
+
+				ret = of_attach_node(tchild);
+				if (ret != 0)
+					return ret;
+
+				/* apply the overlay */
+				ret = of_overlay_apply_one(tchild, child);
+				if (ret != 0) {
+					__of_free_tree(tchild);
+					return ret;
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Lookup an overlay device entry
+ */
+struct of_overlay_device_entry *of_overlay_device_entry_lookup(
+		struct of_overlay_info *ovinfo, struct device_node *node)
+{
+	struct of_overlay_device_entry *de;
+
+	/* no need for locks, we'de under the ovinfo->lock */
+	list_for_each_entry(de, &ovinfo->de_list, node) {
+		if (de->np == node)
+			return de;
+	}
+	return NULL;
+}
+
+/*
+ * Add an overlay log entry
+ */
+static int of_overlay_log_entry_entry_add(struct of_overlay_info *ovinfo,
+		unsigned long action, struct device_node *dn,
+		struct property *prop)
+{
+	struct of_overlay_log_entry *le;
+
+	/* check */
+	if (ovinfo == NULL || dn == NULL)
+		return -EINVAL;
+
+	le = kzalloc(sizeof(*le), GFP_KERNEL);
+	if (le == NULL) {
+		pr_err("%s: Failed to allocate\n", __func__);
+		return -ENOMEM;
+	}
+
+	/* get a reference to the node */
+	le->action = action;
+	le->np = of_node_get(dn);
+	le->prop = prop;
+
+	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
+		le->old_prop = of_find_property(dn, prop->name, NULL);
+
+	list_add_tail(&le->node, &ovinfo->le_list);
+
+	return 0;
+}
+
+/*
+ * Add an overlay device entry
+ */
+static void of_overlay_device_entry_entry_add(struct of_overlay_info *ovinfo,
+		struct device_node *node,
+		int prevstate, int state)
+{
+	struct of_overlay_device_entry *de;
+	int fresh;
+
+	/* check */
+	if (ovinfo == NULL)
+		return;
+
+	fresh = 0;
+	de = of_overlay_device_entry_lookup(ovinfo, node);
+	if (de == NULL) {
+		de = kzalloc(sizeof(*de), GFP_KERNEL);
+		if (de == NULL) {
+			pr_err("%s: Failed to allocate\n", __func__);
+			return;
+		}
+		fresh = 1;
+		de->prevstate = -1;
+	}
+
+	if (de->np == NULL)
+		de->np = of_node_get(node);
+	if (fresh)
+		de->prevstate = prevstate;
+	de->state = state;
+
+	if (fresh)
+		list_add_tail(&de->node, &ovinfo->de_list);
+}
+
+/*
+ * Overlay OF notifier
+ *
+ * Called every time there's a property/node modification
+ * Every modification causes a log entry addition, while
+ * any modification that causes a node's state to change
+ * from/to disabled to/from enabled causes a device entry
+ * addition.
+ */
+static int of_overlay_notify(struct notifier_block *nb,
+				unsigned long action, void *arg)
+{
+	struct of_overlay_info *ovinfo;
+	struct device_node *node;
+	struct property *prop, *sprop, *cprop;
+	struct of_prop_reconfig *pr;
+	struct device_node *tnode;
+	int depth;
+	int prevstate, state;
+	int err = 0;
+
+	ovinfo = container_of(nb, struct of_overlay_info, notifier);
+
+	/* prep vars */
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+	case OF_RECONFIG_DETACH_NODE:
+		node = arg;
+		if (node == NULL)
+			return notifier_from_errno(-EINVAL);
+		prop = NULL;
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		pr = arg;
+		if (pr == NULL)
+			return notifier_from_errno(-EINVAL);
+		node = pr->dn;
+		if (node == NULL)
+			return notifier_from_errno(-EINVAL);
+		prop = pr->prop;
+		if (prop == NULL)
+			return notifier_from_errno(-EINVAL);
+		break;
+	default:
+		return notifier_from_errno(0);
+	}
+
+	/* add to the log */
+	err = of_overlay_log_entry_entry_add(ovinfo, action, node, prop);
+	if (err != 0)
+		return notifier_from_errno(err);
+
+	/* come up with the device entry (if any) */
+	state = 0;
+	prevstate = 0;
+
+	/* determine the state the node will end up */
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		/* we demand that a compatible node is present */
+		state = of_find_property(node, "compatible", NULL) &&
+			of_device_is_available(node);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		prevstate = of_find_property(node, "compatible", NULL) &&
+			of_device_is_available(node);
+		state = 0;
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		/* either one cause a change in state */
+		if (strcmp(prop->name, "status") != 0 &&
+				strcmp(prop->name, "compatible") != 0)
+			return notifier_from_errno(0);
+
+		if (strcmp(prop->name, "status") == 0) {
+			/* status */
+			cprop = of_find_property(node, "compatible", NULL);
+			sprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
+				prop : NULL;
+		} else {
+			/* compatible */
+			sprop = of_find_property(node, "status", NULL);
+			cprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
+				prop : NULL;
+		}
+
+		prevstate = of_find_property(node, "compatible", NULL) &&
+			of_device_is_available(node);
+		state = cprop && cprop->length > 0 &&
+			    (!sprop || (sprop->length > 0 &&
+				(strcmp(sprop->value, "okay") == 0 ||
+				 strcmp(sprop->value, "ok") == 0)));
+		break;
+
+	default:
+		return notifier_from_errno(0);
+	}
+
+	/* find depth */
+	depth = 1;
+	tnode = node;
+	while (tnode != NULL && tnode != ovinfo->target) {
+		tnode = tnode->parent;
+		depth++;
+	}
+
+	/* respect overlay's maximum depth */
+	if (ovinfo->device_depth != 0 && depth > ovinfo->device_depth) {
+		pr_debug("OF: skipping device creation for node=%s depth=%d\n",
+				node->name, depth);
+		goto out;
+	}
+
+	of_overlay_device_entry_entry_add(ovinfo, node, prevstate, state);
+out:
+
+	return notifier_from_errno(err);
+}
+
+/*
+ * Prepare for the overlay, for now it just registers the
+ * notifier.
+ */
+static int of_overlay_prep_one(struct of_overlay_info *ovinfo)
+{
+	int err;
+
+	err = of_reconfig_notifier_register(&ovinfo->notifier);
+	if (err != 0) {
+		pr_err("%s: failed to register notifier for '%s'\n",
+			__func__, ovinfo->target->full_name);
+		return err;
+	}
+	return 0;
+}
+
+static int of_overlay_device_entry_change(struct of_overlay_info *ovinfo,
+		struct of_overlay_device_entry *de, int revert)
+{
+	int state;
+	int ret;
+
+	state = !!de->state ^ !!revert;
+
+	if (state)
+		ret = handler_create(de, revert);
+	else
+		ret = handler_remove(de, revert);
+
+	if (ret != 0 && ret != -ENOTSUPP)
+		pr_warn("%s: Failed to %s device "
+				"for node '%s'\n", __func__,
+				state ? "create" : "remove",
+				de->np->full_name);
+	return 0;
+}
+
+/*
+ * Revert one overlay
+ * Either due to an error, or due to normal overlay removal.
+ * Using the log entries, we revert any change to the live tree.
+ * In the same manner, using the device entries we enable/disable
+ * the devices appropriately.
+ */
+static void of_overlay_revert_one(struct of_overlay_info *ovinfo)
+{
+	struct of_overlay_device_entry *de, *den;
+	struct of_overlay_log_entry *le, *len;
+	struct property *prop, **propp;
+	struct device_node *np;
+	int ret;
+	unsigned long flags;
+
+	if (!ovinfo || !ovinfo->target || !ovinfo->overlay)
+		return;
+
+	pr_debug("%s: Reverting overlay on '%s'\n", __func__,
+			ovinfo->target->full_name);
+
+	/* overlay applied correctly, now create/destroy pdevs */
+	list_for_each_entry_safe_reverse(de, den, &ovinfo->de_list, node) {
+		of_overlay_device_entry_change(ovinfo, de, 1);
+		of_node_put(de->np);
+		list_del(&de->node);
+		kfree(de);
+	}
+
+	list_for_each_entry_safe_reverse(le, len, &ovinfo->le_list, node) {
+
+		/* get node and immediately put */
+		np = le->np;
+		of_node_put(le->np);
+		le->np = NULL;
+
+		ret = 0;
+		switch (le->action) {
+		case OF_RECONFIG_ATTACH_NODE:
+			pr_debug("Reverting ATTACH_NODE %s\n",
+					np->full_name);
+			ret = of_detach_node(np);
+			break;
+
+		case OF_RECONFIG_DETACH_NODE:
+			pr_debug("Reverting DETACH_NODE %s\n",
+					np->full_name);
+			ret = of_attach_node(np);
+			break;
+
+		case OF_RECONFIG_ADD_PROPERTY:
+			pr_debug("Reverting ADD_PROPERTY %s %s\n",
+					np->full_name, le->prop->name);
+			ret = of_remove_property(np, le->prop);
+			break;
+
+		case OF_RECONFIG_REMOVE_PROPERTY:
+		case OF_RECONFIG_UPDATE_PROPERTY:
+
+			pr_debug("Reverting %s_PROPERTY %s %s\n",
+				le->action == OF_RECONFIG_REMOVE_PROPERTY ?
+					"REMOVE" : "UPDATE",
+					np->full_name, le->prop->name);
+
+			/* property is possibly on deadprops (avoid alloc) */
+			raw_spin_lock_irqsave(&devtree_lock, flags);
+			prop = le->action == OF_RECONFIG_REMOVE_PROPERTY ?
+				le->prop : le->old_prop;
+			propp = &np->deadprops;
+			while (*propp != NULL) {
+				if (*propp == prop)
+					break;
+				propp = &(*propp)->next;
+			}
+			if (*propp != NULL) {
+				/* remove it from deadprops */
+				(*propp)->next = prop->next;
+				raw_spin_unlock_irqrestore(&devtree_lock,
+						flags);
+			} else {
+				raw_spin_unlock_irqrestore(&devtree_lock,
+						flags);
+				/* not found, just make a copy */
+				prop = __of_copy_property(prop, GFP_KERNEL,
+						OF_PROP_ALLOCALL);
+				if (prop == NULL) {
+					pr_err("%s: Failed to copy property\n",
+							__func__);
+					break;
+				}
+			}
+
+			if (le->action == OF_RECONFIG_REMOVE_PROPERTY)
+				ret = of_add_property(np, prop);
+			else
+				ret = of_update_property(np, prop);
+			break;
+
+		default:
+			/* nothing */
+			break;
+		}
+
+		if (ret != 0)
+			pr_err("%s: revert on node %s Failed!\n",
+					__func__, np->full_name);
+
+		list_del(&le->node);
+
+		kfree(le);
+	}
+}
+
+/*
+ * Perform the post overlay work.
+ *
+ * We unregister the notifier, and in the case on an error we
+ * revert the overlay.
+ * If the overlay applied correctly, we iterate over the device entries
+ * and create/destroy the devices appropriately.
+ */
+static int of_overlay_post_one(struct of_overlay_info *ovinfo, int err)
+{
+	struct of_overlay_device_entry *de, *den;
+
+	of_reconfig_notifier_unregister(&ovinfo->notifier);
+
+	if (err != 0) {
+		/* revert this (possible partially applied) overlay */
+		of_overlay_revert_one(ovinfo);
+		return 0;
+	}
+
+	/* overlay applied correctly, now create/destroy pdevs */
+	list_for_each_entry_safe(de, den, &ovinfo->de_list, node) {
+
+		/* no state change? just remove this entry */
+		if (de->prevstate == de->state) {
+			of_node_put(de->np);
+			list_del(&de->node);
+			kfree(de);
+			continue;
+		}
+
+		of_overlay_device_entry_change(ovinfo, de, 0);
+	}
+
+	return 0;
+}
+
+/**
+ * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Applies the overlays given, while handling all error conditions
+ * appropriately. Either the operation succeeds, or if it fails the
+ * live tree is reverted to the state before the attempt.
+ * Returns 0, or an error if the overlay attempt failed.
+ */
+int of_overlay(int count, struct of_overlay_info *ovinfo_tab)
+{
+	struct of_overlay_info *ovinfo;
+	int i, err;
+
+	if (!ovinfo_tab)
+		return -EINVAL;
+
+	/* first we apply the overlays atomically */
+	for (i = 0; i < count; i++) {
+
+		ovinfo = &ovinfo_tab[i];
+
+		mutex_lock(&ovinfo->lock);
+
+		err = of_overlay_prep_one(ovinfo);
+		if (err == 0)
+			err = of_overlay_apply_one(ovinfo->target,
+					ovinfo->overlay);
+		of_overlay_post_one(ovinfo, err);
+
+		mutex_unlock(&ovinfo->lock);
+
+		if (err != 0) {
+			pr_err("%s: overlay failed '%s'\n",
+				__func__, ovinfo->target->full_name);
+			goto err_fail;
+		}
+	}
+
+	return 0;
+
+err_fail:
+	while (--i >= 0) {
+		ovinfo = &ovinfo_tab[i];
+
+		mutex_lock(&ovinfo->lock);
+		of_overlay_revert_one(ovinfo);
+		mutex_unlock(&ovinfo->lock);
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(of_overlay);
+
+/**
+ * of_overlay_revert	- Revert a previously applied overlay
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Revert a previous overlay. The state of the live tree
+ * is reverted to the one before the overlay.
+ * Returns 0, or an error if the overlay table is not given.
+ */
+int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
+{
+	struct of_overlay_info *ovinfo;
+	int i;
+
+	if (!ovinfo_tab)
+		return -EINVAL;
+
+	/* revert the overlays in reverse */
+	for (i = count - 1; i >= 0; i--) {
+
+		ovinfo = &ovinfo_tab[i];
+
+		mutex_lock(&ovinfo->lock);
+		of_overlay_revert_one(ovinfo);
+		mutex_unlock(&ovinfo->lock);
+
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_overlay_revert);
+
+/**
+ * of_init_overlay_info	- Initialize a single of_overlay_info structure
+ * @ovinfo:	Pointer to the overlay info structure to initialize
+ *
+ * Initialize a single overlay info structure.
+ */
+void of_init_overlay_info(struct of_overlay_info *ovinfo)
+{
+	memset(ovinfo, 0, sizeof(*ovinfo));
+	mutex_init(&ovinfo->lock);
+	INIT_LIST_HEAD(&ovinfo->de_list);
+	INIT_LIST_HEAD(&ovinfo->le_list);
+
+	ovinfo->notifier.notifier_call = of_overlay_notify;
+}
+
+/*
+ * Find the target node using a number of different strategies
+ * in order of preference
+ *
+ * "target" property containing the phandle of the target
+ * "target-path" property containing the path of the target
+ *
+ */
+struct device_node *find_target_node(struct device_node *info_node)
+{
+	const char *path;
+	u32 val;
+	int ret;
+
+	/* first try to go by using the target as a phandle */
+	ret = of_property_read_u32(info_node, "target", &val);
+	if (ret == 0)
+		return of_find_node_by_phandle(val);
+
+	/* now try to locate by path */
+	ret = of_property_read_string(info_node, "target-path", &path);
+	if (ret == 0)
+		return of_find_node_by_path(path);
+
+	pr_err("%s: Failed to find target for node %p (%s)\n", __func__,
+			info_node, info_node->name);
+
+	return NULL;
+}
+
+/**
+ * of_fill_overlay_info	- Fill an overlay info structure
+ * @info_node:	Device node containing the overlay
+ * @ovinfo:	Pointer to the overlay info structure to fill
+ *
+ * Fills an overlay info structure with the overlay information
+ * from a device node. This device node must have a target property
+ * which contains a phandle of the overlay target node, and an
+ * __overlay__ child node which has the overlay contents.
+ * Both ovinfo->target & ovinfo->overlay have their references taken.
+ *
+ * Returns 0 on success, or a negative error value.
+ */
+int of_fill_overlay_info(struct device_node *info_node,
+		struct of_overlay_info *ovinfo)
+{
+	u32 val;
+	int ret;
+
+	if (!info_node || !ovinfo)
+		return -EINVAL;
+
+	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
+	if (ovinfo->overlay == NULL)
+		goto err_fail;
+
+	ovinfo->target = find_target_node(info_node);
+	if (ovinfo->target == NULL)
+		goto err_fail;
+
+	ret = of_property_read_u32(info_node, "depth", &val);
+	if (ret == 0)
+		ovinfo->device_depth = val;
+	else
+		ovinfo->device_depth = 0;
+
+	return 0;
+
+err_fail:
+	of_node_put(ovinfo->target);
+	of_node_put(ovinfo->overlay);
+
+	memset(ovinfo, 0, sizeof(*ovinfo));
+	return -EINVAL;
+}
+
+/**
+ * of_build_overlay_info	- Build an overlay info array
+ * @tree:	Device node containing all the overlays
+ * @cntp:	Pointer to where the overlay info count will be help
+ * @ovinfop:	Pointer to the pointer of an overlay info structure.
+ *
+ * Helper function that given a tree containing overlay information,
+ * allocates and builds an overlay info array containing it, ready
+ * for use using of_overlay.
+ *
+ * Returns 0 on success with the @cntp @ovinfop pointers valid,
+ * while on error a negative error value is returned.
+ */
+int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop)
+{
+	struct device_node *node;
+	struct of_overlay_info *ovinfo;
+	int cnt, err;
+
+	if (tree == NULL || cntp == NULL || ovinfop == NULL)
+		return -EINVAL;
+
+	/* worst case; every child is a node */
+	cnt = 0;
+	for_each_child_of_node(tree, node)
+		cnt++;
+
+	ovinfo = kzalloc(cnt * sizeof(*ovinfo), GFP_KERNEL);
+	if (ovinfo == NULL)
+		return -ENOMEM;
+
+	cnt = 0;
+	for_each_child_of_node(tree, node) {
+
+		of_init_overlay_info(&ovinfo[cnt]);
+		err = of_fill_overlay_info(node, &ovinfo[cnt]);
+		if (err == 0)
+			cnt++;
+	}
+
+	/* if nothing filled, return error */
+	if (cnt == 0) {
+		kfree(ovinfo);
+		return -ENODEV;
+	}
+
+	*cntp = cnt;
+	*ovinfop = ovinfo;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_build_overlay_info);
+
+/**
+ * of_free_overlay_info	- Free an overlay info array
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to free
+ *
+ * Releases the memory of a previously allocate ovinfo array
+ * by of_build_overlay_info.
+ * Returns 0, or an error if the arguments are bogus.
+ */
+int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab)
+{
+	struct of_overlay_info *ovinfo;
+	int i;
+
+	if (!ovinfo_tab || count < 0)
+		return -EINVAL;
+
+	/* do it in reverse */
+	for (i = count - 1; i >= 0; i--) {
+		ovinfo = &ovinfo_tab[i];
+
+		of_node_put(ovinfo->target);
+		of_node_put(ovinfo->overlay);
+	}
+	kfree(ovinfo_tab);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_free_overlay_info);
diff --git a/include/linux/of.h b/include/linux/of.h
index 3edb9b9..358f984 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -23,6 +23,7 @@
 #include <linux/spinlock.h>
 #include <linux/topology.h>
 #include <linux/notifier.h>
+#include <linux/list.h>
 
 #include <asm/byteorder.h>
 #include <asm/errno.h>
@@ -828,4 +829,156 @@ static inline int of_resolve(struct device_node *resolve)
 
 #endif
 
+/**
+ * Overlay support
+ */
+
+/**
+ * struct of_overlay_log_entry	- Holds a DT log entry
+ * @node:	list_head for the log list
+ * @action:	notifier action
+ * @np:		pointer to the device node affected
+ * @prop:	pointer to the property affected
+ * @old_prop:	hold a pointer to the original property
+ *
+ * Every modification of the device tree during application of the
+ * overlay is held in a list of of_overlay_log_entry structures.
+ * That way we can recover from a partial application, or we can
+ * revert the overlay properly.
+ */
+struct of_overlay_log_entry {
+	struct list_head node;
+	unsigned long action;
+	struct device_node *np;
+	struct property *prop;
+	struct property *old_prop;
+};
+
+struct of_overlay_device_entry;
+
+/**
+ * struct of_overlay_handler_ops	- Overlay device handler ops
+ * @create:	method to be called to create a device
+ * @remove:	method to be called to destroy a device
+ *
+ * Both these functions return 0 on success, ENOTSUPP if the
+ * device entry does not match, and an error code otherwise.
+ */
+struct of_overlay_handler_ops {
+	int (*create)(struct of_overlay_device_entry *entry, int revert);
+	int (*remove)(struct of_overlay_device_entry *entry, int revert);
+};
+
+/**
+ * struct of_overlay_handler	- Overlay device handler
+ * @list:	list links for all handlers
+ * @name:	name of this handler
+ * @ops:	ops member functions
+ *
+ * The handler is registered by each bus that supports
+ * dynamic creation/removal of devices
+ */
+struct of_overlay_handler {
+	struct list_head list;
+	const char *name;
+	const struct of_overlay_handler_ops *ops;
+};
+
+/**
+ * struct of_overlay_device_entry	- Holds an overlay device entry
+ * @node:	list_head for the device list
+ * @np:		device node pointer to the device node affected
+ * @state:	new device state
+ * @prevstate:	previous device state
+ * @priv:	private pointer for use by bus handlers
+ *
+ * When the overlay results in a device node's state to change this
+ * fact is recorded in a list of device entries. After the overlay
+ * is applied we can create/destroy the devices according
+ * to the new state of the live tree.
+ */
+struct of_overlay_device_entry {
+	struct list_head node;
+	struct device_node *np;
+	int prevstate;
+	int state;
+	void *priv;
+};
+
+/**
+ * struct of_overlay_info	- Holds a single overlay info
+ * @target:	target of the overlay operation
+ * @overlay:	pointer to the overlay contents node
+ * @lock:	Lock to hold when accessing the lists
+ * @le_list:	List of the overlay logs
+ * @de_list:	List of the overlay records
+ * @notifier:	of reconfiguration notifier
+ *
+ * Holds a single overlay state, including all the overlay logs &
+ * records.
+ */
+struct of_overlay_info {
+	struct device_node *target;
+	struct device_node *overlay;
+	struct mutex lock;
+	struct list_head le_list;
+	struct list_head de_list;
+	struct notifier_block notifier;
+	int device_depth;
+};
+
+#ifdef CONFIG_OF_OVERLAY
+
+int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
+int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
+
+int of_fill_overlay_info(struct device_node *info_node,
+		struct of_overlay_info *ovinfo);
+int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop);
+int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo);
+
+int of_overlay_handler_register(struct of_overlay_handler *handler);
+void of_overlay_handler_unregister(struct of_overlay_handler *handler);
+
+#else
+
+static inline int of_overlay(int count, struct of_overlay_info *ovinfo_tab)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_fill_overlay_info(struct device_node *info_node,
+		struct of_overlay_info *ovinfo)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_overlay_handler_register(struct of_overlay_handler *handler)
+{
+	return 0;
+}
+
+static inline void of_overlay_handler_unregister(struct of_overlay_handler *handler)
+{
+}
+
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

[PATCH v4 3/8] OF: DT-Overlay configfs interface

[Pantelis Antoniou]

Add a runtime interface to using configfs for generic device tree overlay
usage.

A device-tree configfs entry is created in /config/device-tree/overlays

To create an overlay you mkdir the directory and then echo the overlay
firmware file to the path property file.

	# mkdir /config/device-tree/overlays/foo
	# echo foo.dtbo >/config/device-tree/overlays/foo/path

The overlay file will be loaded using the standard firmware loader
and will be applied.

To remove it simply rmdir the directory.

	# rmdir /config/device-tree/overlays/foo

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/Kconfig    |   5 +
 drivers/of/Makefile   |   1 +
 drivers/of/configfs.c | 272 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 drivers/of/configfs.c

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index cfb7ff8..872e45e 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -77,6 +77,10 @@ config OF_RESERVED_MEM
 	help
 	  Helpers to allow for reservation of memory regions
 
+config OF_CONFIGFS
+	select CONFIGFS_FS
+	def_bool n
+
 config OF_RESOLVE
 	bool "OF Dynamic resolution support"
 	depends on OF && !SPARC
@@ -92,6 +96,7 @@ config OF_OVERLAY
 	select OF_DYNAMIC
 	select OF_DEVICE
 	select OF_RESOLVE
+	select OF_CONFIGFS
 	help
 	  OpenFirmware overlay support. Allows you to modify on runtime the
 	  live tree using overlays.
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index d2a6e0d..4efa17b 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
 obj-$(CONFIG_OF_RESOLVE)  += resolver.o
 obj-$(CONFIG_OF_OVERLAY) += overlay.o
+obj-$(CONFIG_OF_CONFIGFS) += configfs.o
diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c
new file mode 100644
index 0000000..a494643
--- /dev/null
+++ b/drivers/of/configfs.c
@@ -0,0 +1,272 @@
+/*
+ * Configfs entries for device-tree
+ *
+ * Copyright (C) 2013 - Pantelis Antoniou <panto@antoniou-consulting.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <linux/ctype.h>
+#include <linux/cpu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/proc_fs.h>
+#include <linux/configfs.h>
+#include <linux/types.h>
+#include <linux/stat.h>
+#include <linux/limits.h>
+#include <linux/file.h>
+#include <linux/vmalloc.h>
+#include <linux/firmware.h>
+
+#include "of_private.h"
+
+#ifdef CONFIG_OF_OVERLAY
+
+struct cfs_overlay_item {
+	struct config_item 	item;
+
+	char			path[PATH_MAX];
+
+	const struct firmware	*fw;
+	struct device_node	*overlay;
+	int			ovinfo_cnt;
+	struct of_overlay_info	*ovinfo;
+	unsigned int		applied : 1;
+};
+
+static inline struct cfs_overlay_item *to_cfs_overlay_item(struct config_item *item)
+{
+	return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
+}
+
+CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
+#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)	\
+struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
+	__CONFIGFS_ATTR(_name, _mode, _show, _store)
+#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)	\
+struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
+	__CONFIGFS_ATTR_RO(_name, _show)
+
+static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
+		char *page)
+{
+	return sprintf(page, "%s\n", overlay->path);
+}
+
+static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
+		const char *page, size_t count)
+{
+	const char *p = page;
+	char *s;
+	int err;
+
+	/* if it's set do not allow changes */
+	if (overlay->path[0] != '\0')
+		return -EPERM;
+
+	/* copy to path buffer (and make sure it's always zero terminated */
+	count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
+	overlay->path[sizeof(overlay->path) - 1] = '\0';
+
+	/* strip trailing newlines */
+	s = overlay->path + strlen(overlay->path);
+	while (s > overlay->path && *--s == '\n')
+		*s = '\0';
+
+	pr_debug("%s: path is '%s'\n", __func__, overlay->path);
+
+	err = request_firmware(&overlay->fw, overlay->path, NULL);
+	if (err != 0)
+		goto out_err;
+
+	/* unflatten the tree */
+	of_fdt_unflatten_tree((void *)overlay->fw->data, &overlay->overlay);
+	if (overlay->overlay == NULL) {
+		pr_err("%s: failed to unflatten tree\n", __func__);
+		err = -EINVAL;
+		goto out_err;
+	}
+	pr_debug("%s: unflattened OK\n", __func__);
+
+	/* mark it as detached */
+	of_node_set_flag(overlay->overlay, OF_DETACHED);
+
+	/* perform resolution */
+	err = of_resolve(overlay->overlay);
+	if (err != 0) {
+		pr_err("%s: Failed to resolve tree\n", __func__);
+		goto out_err;
+	}
+	pr_debug("%s: resolved OK\n", __func__);
+
+	/* now build an overlay info array */
+	err = of_build_overlay_info(overlay->overlay,
+			&overlay->ovinfo_cnt, &overlay->ovinfo);
+	if (err != 0) {
+		pr_err("%s: Failed to build overlay info\n", __func__);
+		goto out_err;
+	}
+
+	pr_debug("%s: built %d overlay segments\n", __func__,
+			overlay->ovinfo_cnt);
+
+	err = of_overlay(overlay->ovinfo_cnt, overlay->ovinfo);
+	if (err != 0) {
+		pr_err("%s: Failed to apply overlay\n", __func__);
+		goto out_err;
+	}
+
+	overlay->applied = 1;
+
+	pr_debug("%s: Applied #%d overlay segments\n", __func__,
+			overlay->ovinfo_cnt);
+
+	return count;
+
+out_err:
+	if (overlay->applied)
+		of_overlay_revert(overlay->ovinfo_cnt, overlay->ovinfo);
+	overlay->applied = 0;
+
+	if (overlay->ovinfo)
+		of_free_overlay_info(overlay->ovinfo_cnt, overlay->ovinfo);
+	overlay->ovinfo = NULL;
+	overlay->ovinfo_cnt = 0;
+
+	release_firmware(overlay->fw);
+	overlay->fw = NULL;
+
+	overlay->path[0] = '\0';
+	return err;
+}
+
+static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
+		char *page)
+{
+	return sprintf(page, "%s\n",
+			overlay->applied ? "applied" : "unapplied");
+}
+
+CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR, \
+		cfs_overlay_item_path_show, cfs_overlay_item_path_store);
+CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
+
+static struct configfs_attribute *cfs_overlay_attrs[] = {
+	&cfs_overlay_item_attr_path.attr,
+	&cfs_overlay_item_attr_status.attr,
+	NULL,
+};
+
+static void cfs_overlay_release(struct config_item *item)
+{
+	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
+
+	if (overlay->applied)
+		of_overlay_revert(overlay->ovinfo_cnt, overlay->ovinfo);
+	if (overlay->ovinfo)
+		of_free_overlay_info(overlay->ovinfo_cnt, overlay->ovinfo);
+	if (overlay->fw)
+		release_firmware(overlay->fw);
+	kfree(overlay);
+}
+
+CONFIGFS_ATTR_OPS(cfs_overlay_item);
+static struct configfs_item_operations cfs_overlay_item_ops = {
+	.release		= cfs_overlay_release,
+	.show_attribute		= cfs_overlay_item_attr_show,
+	.store_attribute	= cfs_overlay_item_attr_store,
+};
+
+static struct config_item_type cfs_overlay_type = {
+	.ct_item_ops	= &cfs_overlay_item_ops,
+	.ct_attrs	= cfs_overlay_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_item *cfs_overlay_group_make_item(struct config_group *group, const char *name)
+{
+	struct cfs_overlay_item *overlay;
+
+	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
+	if (!overlay)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
+	return &overlay->item;
+}
+
+static void cfs_overlay_group_drop_item(struct config_group *group, struct config_item *item)
+{
+	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
+
+	config_item_put(&overlay->item);
+}
+
+static struct configfs_group_operations overlays_ops = {
+	.make_item	= cfs_overlay_group_make_item,
+	.drop_item	= cfs_overlay_group_drop_item,
+};
+
+static struct config_item_type overlays_type = {
+	.ct_group_ops   = &overlays_ops,
+	.ct_owner       = THIS_MODULE,
+};
+
+#endif /* CONFIG_OF_OVERLAY */
+
+static struct configfs_group_operations of_cfs_ops = {
+	/* empty - we don't allow anything to be created */
+};
+
+static struct config_item_type of_cfs_type = {
+	.ct_group_ops   = &of_cfs_ops,
+	.ct_owner       = THIS_MODULE,
+};
+
+struct config_group of_cfs_overlay_group;
+
+struct config_group *of_cfs_def_groups[] = {
+#ifdef CONFIG_OF_OVERLAY
+	&of_cfs_overlay_group,
+#endif
+	NULL
+};
+
+static struct configfs_subsystem of_cfs_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "device-tree",
+			.ci_type = &of_cfs_type,
+		},
+		.default_groups = of_cfs_def_groups,
+	},
+	.su_mutex = __MUTEX_INITIALIZER(of_cfs_subsys.su_mutex),
+};
+
+static int __init of_cfs_init(void)
+{
+	int ret;
+
+	pr_info("%s\n", __func__);
+
+	config_group_init(&of_cfs_subsys.su_group);
+#ifdef CONFIG_OF_OVERLAY
+	config_group_init_type_name(&of_cfs_overlay_group, "overlays", &overlays_type);
+#endif
+
+	ret = configfs_register_subsystem(&of_cfs_subsys);
+	if (ret != 0) {
+		pr_err("%s: failed to register subsys\n", __func__);
+		goto out;
+	}
+	pr_info("%s: OK\n", __func__);
+out:
+	return ret;
+}
+late_initcall(of_cfs_init);
-- 
1.7.12

[PATCH v4 4/8] OF: platform: Add overlay bus handler

[Pantelis Antoniou]

Add the bus handler registration needed for performing overlays
containing platform devices.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/base/platform.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 3 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index e714709..bf5b429 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -922,6 +922,86 @@ struct bus_type platform_bus_type = {
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
+#ifdef CONFIG_OF_OVERLAY
+
+static int platform_handler_create(struct of_overlay_device_entry *de,
+		int revert)
+{
+	struct device_node *dn;
+	struct platform_device *pdev_parent, *pdev;
+
+	if (!de || !de->np)
+		return -ENOTSUPP;
+
+	dn = de->np;
+
+	/* verify that the parent is a bus */
+	if (!of_match_node(of_default_bus_match_table, dn->parent))
+		return -ENOTSUPP;
+
+	/* pdev_parent may be NULL when there is no bus platform device */
+	pdev_parent = of_find_device_by_node(dn->parent);
+	pdev = of_platform_device_create(dn, NULL,
+			pdev_parent ? &pdev_parent->dev : NULL);
+	of_dev_put(pdev_parent);
+
+	if (pdev == NULL) {
+		pr_err("%s: failed to create platform device "
+				"for '%s'\n",
+				__func__, dn->full_name);
+		/* of_platform_device_create tosses the real error code */
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int platform_handler_remove(struct of_overlay_device_entry *de,
+		int revert)
+{
+	struct device_node *dn;
+	struct platform_device *pdev;
+
+	if (!de || !de->np)
+		return -ENOTSUPP;
+
+	dn = de->np;
+
+	pdev = of_find_device_by_node(dn);
+	if (pdev == NULL)
+		return -ENOTSUPP;
+
+	/* unregister takes one ref away */
+	platform_device_unregister(pdev);
+
+	/* and put the reference of the find */
+	of_dev_put(pdev);
+
+	return 0;
+}
+
+static const struct of_overlay_handler_ops platform_handler_ops = {
+	.create	= platform_handler_create,
+	.remove = platform_handler_remove,
+};
+
+static struct of_overlay_handler platform_handler = {
+	.name = "platform",
+	.ops = &platform_handler_ops,
+};
+
+static int __init platform_bus_handler_register(void)
+{
+	return of_overlay_handler_register(&platform_handler);
+}
+
+#else
+static inline int platform_bus_handler_register(void)
+{
+	return 0;
+}
+#endif
+
 int __init platform_bus_init(void)
 {
 	int error;
@@ -930,10 +1010,22 @@ int __init platform_bus_init(void)
 
 	error = device_register(&platform_bus);
 	if (error)
-		return error;
-	error =  bus_register(&platform_bus_type);
+		goto err_out;
+
+	error = bus_register(&platform_bus_type);
 	if (error)
-		device_unregister(&platform_bus);
+		goto err_unreg_dev;
+
+	error = platform_bus_handler_register();
+	if (error)
+		goto err_unreg_bus;
+
+	return 0;
+err_unreg_bus:
+	bus_unregister(&platform_bus_type);
+err_unreg_dev:
+	device_unregister(&platform_bus);
+err_out:
 	return error;
 }
 
-- 
1.7.12

[PATCH v4 5/8] of: i2c: Export single device registration method

[Pantelis Antoniou]

From: Pantelis Antoniou <panto@antoniou-consulting.com>

Dynamically inserting i2c client device nodes requires the use
of a single device registration method. Rework and export it.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/i2c/i2c-core.c | 99 +++++++++++++++++++++++++++-----------------------
 include/linux/i2c.h    | 10 +++++
 2 files changed, 64 insertions(+), 45 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5fb80b8..aee3b99 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -49,6 +49,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/acpi.h>
 #include <asm/uaccess.h>
+#include <linux/err.h>
 
 #include "i2c-core.h"
 
@@ -982,63 +983,71 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter)
 /* OF support code */
 
 #if IS_ENABLED(CONFIG_OF)
-static void of_i2c_register_devices(struct i2c_adapter *adap)
+struct i2c_client *
+of_i2c_register_device(struct i2c_adapter *adap,
+		struct device_node *node)
 {
-	void *result;
-	struct device_node *node;
+	struct i2c_client *result;
+	struct i2c_board_info info = {};
+	struct dev_archdata dev_ad = {};
+	const __be32 *addr;
+	int len;
 
-	/* Only register child devices if the adapter has a node pointer set */
-	if (!adap->dev.of_node)
-		return;
+	dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
 
-	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
+	if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
+		dev_err(&adap->dev, "of_i2c: modalias failure on %s\n",
+			node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
 
-	for_each_available_child_of_node(adap->dev.of_node, node) {
-		struct i2c_board_info info = {};
-		struct dev_archdata dev_ad = {};
-		const __be32 *addr;
-		int len;
+	addr = of_get_property(node, "reg", &len);
+	if (!addr || (len < sizeof(int))) {
+		dev_err(&adap->dev, "of_i2c: invalid reg on %s\n",
+			node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
 
-		dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
+	info.addr = be32_to_cpup(addr);
+	if (info.addr > (1 << 10) - 1) {
+		dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
+			info.addr, node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
 
-		if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
-			dev_err(&adap->dev, "of_i2c: modalias failure on %s\n",
-				node->full_name);
-			continue;
-		}
+	info.irq = irq_of_parse_and_map(node, 0);
+	info.of_node = of_node_get(node);
+	info.archdata = &dev_ad;
 
-		addr = of_get_property(node, "reg", &len);
-		if (!addr || (len < sizeof(int))) {
-			dev_err(&adap->dev, "of_i2c: invalid reg on %s\n",
-				node->full_name);
-			continue;
-		}
+	if (of_get_property(node, "wakeup-source", NULL))
+		info.flags |= I2C_CLIENT_WAKE;
 
-		info.addr = be32_to_cpup(addr);
-		if (info.addr > (1 << 10) - 1) {
-			dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
-				info.addr, node->full_name);
-			continue;
-		}
+	request_module("%s%s", I2C_MODULE_PREFIX, info.type);
 
-		info.irq = irq_of_parse_and_map(node, 0);
-		info.of_node = of_node_get(node);
-		info.archdata = &dev_ad;
+	result = i2c_new_device(adap, &info);
+	if (result == NULL) {
+		dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
+			node->full_name);
+		of_node_put(node);
+		irq_dispose_mapping(info.irq);
+		return ERR_PTR(-EINVAL);
+	}
+	return result;
+}
+EXPORT_SYMBOL(of_i2c_register_device);
 
-		if (of_get_property(node, "wakeup-source", NULL))
-			info.flags |= I2C_CLIENT_WAKE;
+static void of_i2c_register_devices(struct i2c_adapter *adap)
+{
+	struct device_node *node;
 
-		request_module("%s%s", I2C_MODULE_PREFIX, info.type);
+	/* Only register child devices if the adapter has a node pointer set */
+	if (!adap->dev.of_node)
+		return;
 
-		result = i2c_new_device(adap, &info);
-		if (result == NULL) {
-			dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
-				node->full_name);
-			of_node_put(node);
-			irq_dispose_mapping(info.irq);
-			continue;
-		}
-	}
+	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
+
+	for_each_available_child_of_node(adap->dev.of_node, node)
+		of_i2c_register_device(adap, node);
 }
 
 static int of_dev_node_match(struct device *dev, void *data)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index deddeb8..a5802be 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -557,6 +557,9 @@ static inline int i2c_adapter_id(struct i2c_adapter *adap)
 #endif /* I2C */
 
 #if IS_ENABLED(CONFIG_OF)
+struct i2c_client *
+of_i2c_register_device(struct i2c_adapter *adap, struct device_node *node);
+
 /* must call put_device() when done with returned i2c_client device */
 extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
 
@@ -565,6 +568,13 @@ extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 
 #else
 
+static inline struct i2c_client *
+of_i2c_register_device(struct i2c_adapter *adap,
+		struct device_node *node)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 {
 	return NULL;
-- 
1.7.12

[PATCH v4 6/8] OF: i2c: Add overlay bus handler

[Pantelis Antoniou]

Add the bus handler registration needed for performing overlays
containing i2c devices.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/i2c/i2c-core.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index aee3b99..017fb03 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1082,10 +1082,89 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 	return i2c_verify_adapter(dev);
 }
 EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
+
+#ifdef CONFIG_OF_OVERLAY
+static int i2c_handler_create(struct of_overlay_device_entry *de,
+		int revert)
+{
+	struct device_node *dn;
+	struct i2c_adapter *adap;
+	struct i2c_client *client;
+
+	if (!de || !de->np)
+		return -ENOTSUPP;
+
+	dn = de->np;
+
+	adap = of_find_i2c_adapter_by_node(dn->parent);
+	if (adap == NULL)
+		return -ENOTSUPP;
+
+	client = of_i2c_register_device(adap, dn);
+	put_device(&adap->dev);
+
+	if (client == NULL) {
+		pr_err("%s: failed to create i2c client device "
+				"for '%s'\n",
+				__func__, dn->full_name);
+		/* of_i2c_device_create tosses the real error code */
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int i2c_handler_remove(struct of_overlay_device_entry *de,
+		int revert)
+{
+	struct device_node *dn;
+	struct i2c_client *client;
+
+	if (!de || !de->np)
+		return -ENOTSUPP;
+
+	dn = de->np;
+
+	client = of_find_i2c_device_by_node(dn);
+	if (client == NULL)
+		return -ENOTSUPP;
+
+	/* unregister takes one ref away */
+	i2c_unregister_device(client);
+
+	/* and put the reference of the find */
+	put_device(&client->dev);
+
+	return 0;
+}
+
+static const struct of_overlay_handler_ops i2c_handler_ops = {
+	.create	= i2c_handler_create,
+	.remove = i2c_handler_remove,
+};
+
+static struct of_overlay_handler i2c_handler = {
+	.name = "i2c",
+	.ops = &i2c_handler_ops,
+};
+
+static int __init i2c_bus_handler_register(void)
+{
+	return of_overlay_handler_register(&i2c_handler);
+}
+#endif
+
 #else
 static void of_i2c_register_devices(struct i2c_adapter *adap) { }
 #endif /* CONFIG_OF */
 
+#if !defined(CONFIG_OF) || !defined(CONFIG_OF_OVERLAY)
+static inline int i2c_bus_handler_register(void)
+{
+	return 0;
+}
+#endif
+
 /* ACPI support code */
 
 #if IS_ENABLED(CONFIG_ACPI)
@@ -1677,8 +1756,14 @@ static int __init i2c_init(void)
 	retval = i2c_add_driver(&dummy_driver);
 	if (retval)
 		goto class_err;
-	return 0;
 
+	retval = i2c_bus_handler_register();
+	if (retval)
+		goto bus_handler_err;
+
+	return 0;
+bus_handler_err:
+	i2c_del_driver(&dummy_driver);
 class_err:
 #ifdef CONFIG_I2C_COMPAT
 	class_compat_unregister(i2c_adapter_compat_class);
-- 
1.7.12

[PATCH v4 7/8] OF: spi: Add overlay bus handler

[Pantelis Antoniou]

Add the bus handler registration needed for performing overlays
containing spi devices.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/spi/spi.c | 345 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 242 insertions(+), 103 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4eb9bf0..dac5573 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1179,6 +1179,123 @@ err_init_queue:
 /*-------------------------------------------------------------------------*/
 
 #if defined(CONFIG_OF)
+
+static struct spi_device *
+of_register_spi_device(struct spi_master *master, struct device_node *node)
+{
+	struct spi_device *spi;
+	struct device_node *nc;
+	int err;
+	u32 value;
+
+	/* Alloc an spi_device */
+	spi = spi_alloc_device(master);
+	if (!spi) {
+		dev_err(&master->dev, "spi_device alloc error for %s\n",
+			nc->full_name);
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	/* Select device driver */
+	err = of_modalias_node(nc, spi->modalias,
+				sizeof(spi->modalias));
+	if (err) {
+		dev_err(&master->dev, "cannot find modalias for %s\n",
+			nc->full_name);
+		goto err_out;
+	}
+
+	/* Device address */
+	err = of_property_read_u32(nc, "reg", &value);
+	if (err) {
+		dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
+			nc->full_name, err);
+		goto err_out;
+	}
+	spi->chip_select = value;
+
+	/* Mode (clock phase/polarity/etc.) */
+	if (of_find_property(nc, "spi-cpha", NULL))
+		spi->mode |= SPI_CPHA;
+	if (of_find_property(nc, "spi-cpol", NULL))
+		spi->mode |= SPI_CPOL;
+	if (of_find_property(nc, "spi-cs-high", NULL))
+		spi->mode |= SPI_CS_HIGH;
+	if (of_find_property(nc, "spi-3wire", NULL))
+		spi->mode |= SPI_3WIRE;
+
+	/* Device DUAL/QUAD mode */
+	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
+		switch (value) {
+		case 1:
+			break;
+		case 2:
+			spi->mode |= SPI_TX_DUAL;
+			break;
+		case 4:
+			spi->mode |= SPI_TX_QUAD;
+			break;
+		default:
+			dev_err(&master->dev,
+				"spi-tx-bus-width %d not supported\n",
+				value);
+			err = -EINVAL;
+			goto err_out;
+		}
+	}
+
+	if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
+		switch (value) {
+		case 1:
+			break;
+		case 2:
+			spi->mode |= SPI_RX_DUAL;
+			break;
+		case 4:
+			spi->mode |= SPI_RX_QUAD;
+			break;
+		default:
+			dev_err(&master->dev,
+				"spi-rx-bus-width %d not supported\n",
+				value);
+			err = -EINVAL;
+			goto err_out;
+		}
+	}
+
+	/* Device speed */
+	err = of_property_read_u32(nc, "spi-max-frequency", &value);
+	if (err) {
+		dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
+			nc->full_name, err);
+		goto err_out;
+	}
+	spi->max_speed_hz = value;
+
+	/* IRQ */
+	spi->irq = irq_of_parse_and_map(nc, 0);
+
+	/* Store a pointer to the node in the device structure */
+	of_node_get(nc);
+	spi->dev.of_node = nc;
+
+	/* Register the new device */
+	request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias);
+	err = spi_add_device(spi);
+	if (err) {
+		dev_err(&master->dev, "spi_device register error %s\n",
+			nc->full_name);
+		goto err_out;
+	}
+
+	return spi;
+
+err_out:
+	spi_dev_put(spi);
+	return ERR_PTR(err);
+}
+
 /**
  * of_register_spi_devices() - Register child devices onto the SPI bus
  * @master:	Pointer to spi_master device
@@ -1188,124 +1305,140 @@ err_init_queue:
  */
 static void of_register_spi_devices(struct spi_master *master)
 {
-	struct spi_device *spi;
 	struct device_node *nc;
-	int rc;
-	u32 value;
 
 	if (!master->dev.of_node)
 		return;
 
-	for_each_available_child_of_node(master->dev.of_node, nc) {
-		/* Alloc an spi_device */
-		spi = spi_alloc_device(master);
-		if (!spi) {
-			dev_err(&master->dev, "spi_device alloc error for %s\n",
-				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
+	for_each_available_child_of_node(master->dev.of_node, nc)
+		of_register_spi_device(master, nc);
+}
 
-		/* Select device driver */
-		if (of_modalias_node(nc, spi->modalias,
-				     sizeof(spi->modalias)) < 0) {
-			dev_err(&master->dev, "cannot find modalias for %s\n",
-				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
 
-		/* Device address */
-		rc = of_property_read_u32(nc, "reg", &value);
-		if (rc) {
-			dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
-				nc->full_name, rc);
-			spi_dev_put(spi);
-			continue;
-		}
-		spi->chip_select = value;
-
-		/* Mode (clock phase/polarity/etc.) */
-		if (of_find_property(nc, "spi-cpha", NULL))
-			spi->mode |= SPI_CPHA;
-		if (of_find_property(nc, "spi-cpol", NULL))
-			spi->mode |= SPI_CPOL;
-		if (of_find_property(nc, "spi-cs-high", NULL))
-			spi->mode |= SPI_CS_HIGH;
-		if (of_find_property(nc, "spi-3wire", NULL))
-			spi->mode |= SPI_3WIRE;
-
-		/* Device DUAL/QUAD mode */
-		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
-			switch (value) {
-			case 1:
-				break;
-			case 2:
-				spi->mode |= SPI_TX_DUAL;
-				break;
-			case 4:
-				spi->mode |= SPI_TX_QUAD;
-				break;
-			default:
-				dev_err(&master->dev,
-					"spi-tx-bus-width %d not supported\n",
-					value);
-				spi_dev_put(spi);
-				continue;
-			}
-		}
+/* bah; the match functions differ just by const-ness */
+static int of_dev_node_match_const(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
 
-		if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
-			switch (value) {
-			case 1:
-				break;
-			case 2:
-				spi->mode |= SPI_RX_DUAL;
-				break;
-			case 4:
-				spi->mode |= SPI_RX_QUAD;
-				break;
-			default:
-				dev_err(&master->dev,
-					"spi-rx-bus-width %d not supported\n",
-					value);
-				spi_dev_put(spi);
-				continue;
-			}
-		}
+/* must call put_device() when done with returned spi_device device */
+struct spi_device *of_find_spi_device_by_node(struct device_node *node)
+{
+	struct device *dev;
 
-		/* Device speed */
-		rc = of_property_read_u32(nc, "spi-max-frequency", &value);
-		if (rc) {
-			dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
-				nc->full_name, rc);
-			spi_dev_put(spi);
-			continue;
-		}
-		spi->max_speed_hz = value;
-
-		/* IRQ */
-		spi->irq = irq_of_parse_and_map(nc, 0);
-
-		/* Store a pointer to the node in the device structure */
-		of_node_get(nc);
-		spi->dev.of_node = nc;
-
-		/* Register the new device */
-		request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias);
-		rc = spi_add_device(spi);
-		if (rc) {
-			dev_err(&master->dev, "spi_device register error %s\n",
-				nc->full_name);
-			spi_dev_put(spi);
-		}
+	dev = bus_find_device(&spi_bus_type, NULL, node,
+					 of_dev_node_match);
+	if (!dev)
+		return NULL;
 
+	return to_spi_device(dev);
+}
+EXPORT_SYMBOL(of_find_spi_device_by_node);
+
+/* forward decl */
+static struct class spi_master_class;
+
+/* the spi masters are not using spi_bus, so we have to find it some other way */
+struct spi_master *of_find_spi_master_by_node(struct device_node *node)
+{
+	struct device *dev;
+
+	dev = class_find_device(&spi_master_class, NULL, node,
+				of_dev_node_match_const);
+	if (!dev)
+		return NULL;
+
+	/* reference got in class_find_device */
+	return container_of(dev, struct spi_master, dev);
+}
+EXPORT_SYMBOL(of_find_spi_master_by_node);
+
+#ifdef CONFIG_OF_OVERLAY
+static int spi_handler_create(struct of_overlay_device_entry *de,
+		int revert)
+{
+	struct device_node *dn;
+	struct spi_master *master;
+	struct spi_device *spi;
+
+	if (!de || !de->np)
+		return -ENOTSUPP;
+
+	dn = de->np;
+
+	master = of_find_spi_master_by_node(dn->parent);
+	if (master == NULL)
+		return -ENOTSUPP;
+
+	spi = of_register_spi_device(master, dn);
+	put_device(&master->dev);
+
+	if (spi == NULL) {
+		pr_err("%s: failed to create spi device "
+				"for '%s'\n",
+				__func__, dn->full_name);
+		/* of_register_spi_device tosses the real error code */
+		return -EINVAL;
 	}
+
+	return 0;
 }
+
+static int spi_handler_remove(struct of_overlay_device_entry *de,
+		int revert)
+{
+	struct device_node *dn;
+	struct spi_device *spi;
+
+	if (!de || !de->np)
+		return -ENOTSUPP;
+
+	dn = de->np;
+
+	spi = of_find_spi_device_by_node(dn);
+	if (spi == NULL)
+		return -ENOTSUPP;
+
+	/* unregister takes one ref away */
+	spi_unregister_device(spi);
+
+	/* and put the reference of the find */
+	put_device(&spi->dev);
+
+	return 0;
+}
+
+static const struct of_overlay_handler_ops spi_handler_ops = {
+	.create	= spi_handler_create,
+	.remove = spi_handler_remove,
+};
+
+static struct of_overlay_handler spi_handler = {
+	.name = "spi",
+	.ops = &spi_handler_ops,
+};
+
+static int __init spi_bus_handler_register(void)
+{
+	return of_overlay_handler_register(&spi_handler);
+}
+#endif
+
 #else
 static void of_register_spi_devices(struct spi_master *master) { }
 #endif
 
+#if !defined(CONFIG_OF) || !defined(CONFIG_OF_OVERLAY)
+static inline int spi_bus_handler_register(void)
+{
+	return 0;
+}
+#endif
+
 #ifdef CONFIG_ACPI
 static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 {
@@ -2270,8 +2403,14 @@ static int __init spi_init(void)
 	status = class_register(&spi_master_class);
 	if (status < 0)
 		goto err2;
-	return 0;
 
+	status = spi_bus_handler_register();
+	if (status < 0)
+		goto err3;
+
+	return 0;
+err3:
+	class_unregister(&spi_master_class);
 err2:
 	bus_unregister(&spi_bus_type);
 err1:
-- 
1.7.12

[PATCH v4 8/8] OF: selftest: Add overlay self-test support.

[Pantelis Antoniou]

This patch adds overlay tests to the OF selftest.

It tests overlay device addition/removal and whether
the apply revert sequence is correct.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/selftest.c                       | 368 ++++++++++++++++++++++++++++
 drivers/of/testcase-data/testcases.dtsi     |   1 +
 drivers/of/testcase-data/tests-overlay.dtsi | 125 ++++++++++
 3 files changed, 494 insertions(+)
 create mode 100644 drivers/of/testcase-data/tests-overlay.dtsi

diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index ae44500..eeb29ff 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -14,6 +14,8 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
 
 static struct selftest_results {
 	int passed;
@@ -427,6 +429,371 @@ static void __init of_selftest_match_node(void)
 	}
 }
 
+#ifdef CONFIG_OF_OVERLAY
+
+static int selftest_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	if (np == NULL) {
+		dev_err(dev, "No OF data for device\n");
+		return -EINVAL;
+
+	}
+
+	dev_dbg(dev, "%s for node @%s\n", __func__, np->full_name);
+	return 0;
+}
+
+static int selftest_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	dev_dbg(dev, "%s for node @%s\n", __func__, np->full_name);
+	return 0;
+}
+
+static struct of_device_id selftest_match[] = {
+	{ .compatible = "selftest", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, altera_jtaguart_match);
+
+static struct platform_driver selftest_driver = {
+	.probe			= selftest_probe,
+	.remove			= selftest_remove,
+	.driver = {
+		.name		= "selftest",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(selftest_match),
+	},
+};
+
+/* get the platform device instantiated at the path */
+static struct platform_device *of_path_to_platform_device(const char *path)
+{
+	struct device_node *np;
+	struct platform_device *pdev;
+
+	np = of_find_node_by_path(path);
+	if (np == NULL)
+		return NULL;
+
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+
+	return pdev;
+}
+
+/* find out if a platform device exists at that path */
+static int of_path_platform_device_exists(const char *path)
+{
+	struct platform_device *pdev;
+
+	pdev = of_path_to_platform_device(path);
+	platform_device_put(pdev);
+	return pdev != NULL;
+}
+
+static const char *selftest_path(int nr)
+{
+	static char buf[256];
+
+	snprintf(buf, sizeof(buf) - 1,
+		"/testcase-data/overlay-node/test-bus/test-selftest%d", nr);
+	buf[sizeof(buf) - 1] = '\0';
+
+	return buf;
+}
+
+static const char *overlay_path(int nr)
+{
+	static char buf[256];
+
+	snprintf(buf, sizeof(buf) - 1,
+		"/testcase-data/overlay%d", nr);
+	buf[sizeof(buf) - 1] = '\0';
+
+	return buf;
+}
+
+static const char *bus_path = "/testcase-data/overlay-node/test-bus";
+
+static int of_selftest_apply_overlay(int selftest_nr, int overlay_nr,
+		int *ovcount_arg, struct of_overlay_info **ovinfo_arg)
+{
+	struct device_node *np = NULL;
+	int ret, ovcount_val, *ovcount;
+	struct of_overlay_info *ovinfo_val, **ovinfo;
+
+	if (ovcount_arg == NULL || ovinfo_arg == NULL) {
+		ovcount = &ovcount_val;
+		ovinfo = &ovinfo_val;
+	} else {
+		ovcount = ovcount_arg;
+		ovinfo = ovinfo_arg;
+	}
+
+	*ovcount = 0;
+	*ovinfo = NULL;
+
+	np = of_find_node_by_path(overlay_path(overlay_nr));
+	if (np == NULL) {
+		selftest(0, "could not find overlay node @\"%s\"\n",
+				overlay_path(overlay_nr));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = of_build_overlay_info(np, ovcount, ovinfo);
+	if (ret != 0) {
+		selftest(0, "could not build overlay from \"%s\"\n",
+				overlay_path(overlay_nr));
+		goto out;
+	}
+
+	ret = of_overlay(*ovcount, *ovinfo);
+	if (ret != 0) {
+		selftest(0, "could not apply overlay from \"%s\"\n",
+				overlay_path(overlay_nr));
+		goto out;
+	}
+
+	ret = 0;
+
+out:
+	/* free if no argument passed */
+	if (ovinfo == &ovinfo_val)
+		of_free_overlay_info(*ovcount, *ovinfo);
+	of_node_put(np);
+	return ret;
+}
+
+/* apply an overlay while checking before and after states */
+static int of_selftest_apply_overlay_check(int overlay_nr, int selftest_nr,
+		int before, int after)
+{
+	int ret;
+
+	/* selftest device must not be in before state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= before) {
+		selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!before ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	ret = of_selftest_apply_overlay(overlay_nr, selftest_nr, NULL, NULL);
+	if (ret != 0) {
+		/* of_selftest_apply_overlay already called selftest() */
+		return ret;
+	}
+
+	/* selftest device must be to set to after state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= after) {
+		selftest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!after ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* apply an overlay and then revert it while checking before, after states */
+static int of_selftest_apply_revert_overlay_check(int overlay_nr,
+		int selftest_nr, int before, int after)
+{
+	int ret, ovcount;
+	struct of_overlay_info *ovinfo;
+
+	/* selftest device must be in before state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= before) {
+		selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!before ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	/* apply the overlay */
+	ret = of_selftest_apply_overlay(overlay_nr, selftest_nr,
+			&ovcount, &ovinfo);
+	if (ret != 0) {
+		/* of_selftest_apply_overlay already called selftest() */
+		return ret;
+	}
+
+	/* selftest device must be in after state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= after) {
+		selftest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!after ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	ret = of_overlay_revert(ovcount, ovinfo);
+	if (ret != 0) {
+		selftest(0, "overlay @\"%s\" failed to revert @\"%s\"\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr));
+		return ret;
+	}
+
+	of_free_overlay_info(ovcount, ovinfo);
+
+	/* selftest device must be again in before state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= before) {
+		selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!before ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* test activation of device */
+static void of_selftest_overlay_0(void)
+{
+	int ret;
+
+	/* device should enable */
+	ret = of_selftest_apply_overlay_check(0, 0, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 0);
+}
+
+/* test deactivation of device */
+static void of_selftest_overlay_1(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_overlay_check(1, 1, 1, 0);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 1);
+}
+
+/* test activation of device */
+static void of_selftest_overlay_2(void)
+{
+	int ret;
+
+	/* device should enable */
+	ret = of_selftest_apply_overlay_check(2, 2, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 2);
+}
+
+/* test deactivation of device */
+static void of_selftest_overlay_3(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_overlay_check(3, 3, 1, 0);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 3);
+}
+
+/* test activation of a full device node */
+static void of_selftest_overlay_4(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_overlay_check(4, 4, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 4);
+}
+
+/* test overlay apply/revert sequence */
+static void of_selftest_overlay_5(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_revert_overlay_check(5, 5, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 5);
+}
+
+static void __init of_selftest_overlay(void)
+{
+	struct device_node *bus_np = NULL;
+	int ret;
+
+	ret = platform_driver_register(&selftest_driver);
+	if (ret != 0) {
+		selftest(0, "could not register selftest driver\n");
+		goto out;
+	}
+
+	bus_np = of_find_node_by_path(bus_path);
+	if (bus_np == NULL) {
+		selftest(0, "could not find bus_path \"%s\"\n", bus_path);
+		goto out;
+	}
+
+	ret = of_platform_populate(bus_np, of_default_bus_match_table,
+			NULL, NULL);
+	if (ret != 0) {
+		selftest(0, "could not populate bus @ \"%s\"\n", bus_path);
+		goto out;
+	}
+
+	if (!of_path_platform_device_exists(selftest_path(100))) {
+		selftest(0, "could not find selftest0 @ \"%s\"\n", selftest_path(100));
+		goto out;
+	}
+
+	if (of_path_platform_device_exists(selftest_path(101))) {
+		selftest(0, "selftest1 @ \"%s\" should not exist\n", selftest_path(101));
+		goto out;
+	}
+
+	selftest(1, "basic infrastructure of overlays passed");
+
+	/* tests in sequence */
+	of_selftest_overlay_0();
+	of_selftest_overlay_1();
+	of_selftest_overlay_2();
+	of_selftest_overlay_3();
+	of_selftest_overlay_4();
+	of_selftest_overlay_5();
+
+out:
+	of_node_put(bus_np);
+}
+
+#else
+static inline void __init of_selftest_overlay(void) { }
+#endif
+
 static int __init of_selftest(void)
 {
 	struct device_node *np;
@@ -445,6 +812,7 @@ static int __init of_selftest(void)
 	of_selftest_parse_interrupts();
 	of_selftest_parse_interrupts_extended();
 	of_selftest_match_node();
+	of_selftest_overlay();
 	pr_info("end of selftest - %i passed, %i failed\n",
 		selftest_results.passed, selftest_results.failed);
 	return 0;
diff --git a/drivers/of/testcase-data/testcases.dtsi b/drivers/of/testcase-data/testcases.dtsi
index 3a5b75a..6a9441e 100644
--- a/drivers/of/testcase-data/testcases.dtsi
+++ b/drivers/of/testcase-data/testcases.dtsi
@@ -1,3 +1,4 @@
 #include "tests-phandle.dtsi"
 #include "tests-interrupts.dtsi"
 #include "tests-match.dtsi"
+#include "tests-overlay.dtsi"
diff --git a/drivers/of/testcase-data/tests-overlay.dtsi b/drivers/of/testcase-data/tests-overlay.dtsi
new file mode 100644
index 0000000..9981b20
--- /dev/null
+++ b/drivers/of/testcase-data/tests-overlay.dtsi
@@ -0,0 +1,125 @@
+
+/ {
+	testcase-data {
+		overlay-node {
+
+			/* test bus */
+			selftestbus: test-bus {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				selftest100: test-selftest100 {
+					compatible = "selftest";
+					status = "okay";
+					reg = <100>;
+				};
+
+				selftest101: test-selftest101 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <101>;
+				};
+
+				selftest0: test-selftest0 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <0>;
+				};
+
+				selftest1: test-selftest1 {
+					compatible = "selftest";
+					status = "okay";
+					reg = <1>;
+				};
+
+				selftest2: test-selftest2 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <2>;
+				};
+
+				selftest3: test-selftest3 {
+					compatible = "selftest";
+					status = "okay";
+					reg = <3>;
+				};
+
+				selftest5: test-selftest5 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <5>;
+				};
+			};
+		};
+
+		/* test enable using absolute target path */
+		overlay0 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest0";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test disable using absolute target path */
+		overlay1 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest1";
+				__overlay__ {
+					status = "disabled";
+				};
+			};
+		};
+
+		/* test enable using label */
+		overlay2 {
+			fragment@0 {
+				target = <&selftest2>;
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test disable using label */
+		overlay3 {
+			fragment@0 {
+				target = <&selftest3>;
+				__overlay__ {
+					status = "disabled";
+				};
+			};
+		};
+
+		/* test insertion of a full node */
+		overlay4 {
+			fragment@0 {
+				target = <&selftestbus>;
+				__overlay__ {
+
+					/* suppress DTC warning */
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					test-selftest4 {
+						compatible = "selftest";
+						status = "okay";
+						reg = <4>;
+					};
+				};
+			};
+		};
+
+		/* test overlay apply revert */
+		overlay5 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest5";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+	};
+};
-- 
1.7.12

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Fri,  4 Apr 2014 15:43:55 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Introduce DT overlay support.
> Using this functionality it is possible to dynamically overlay a part of
> the kernel's tree with another tree that's been dynamically loaded.
> It is also possible to remove node and properties.
> 
> The creation/destruction of the devices is handled by calling in to
> bus specific handlers which can deal with the peculiarities of each
> device.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Hi Panto,

I spent a bunch of time yesterday going over this code and I have some
comments and questions. First off I must apologize. I think I completely
misunderstood the approach you are taking for putting the overlay data
into the tree. I had assumed that you were unflattening the overlay tree
and then rearranging the new device_nodes to insert them into the
existing tree, but it looks like you're leaving the overlay tree alone
and merely using it's nodes as templates when allocating new nodes that
will get inserted into the tree. Sorry for being confused on this. A few
of my opinions have probably changed now that I understand what you're
trying to do.

As I've said before, this is a complicated bit of code that touches into
the core of device tree behaviour, so I am being cautious. This patch
and the previous patch add a lot of functionality that is hard to review
all at once, and it conflates some concepts that I would like to be very
well defined. For example, this patch adds at least 3 distinct features:

- Revertable batch modifications to the live tree.
- New notification infrastructure for informing other code when changes
  take place
- Parsing a tree in the overlay format to create an of_overlay_info
  array.

The revertable batch feature should be a patch all on its own without
any of the change tracking or notification aside from what the core code
is already doing. That change could be applied independently even if I
still have issues with the notification or parsing code.

I would even split up the first feature into two steps: batch
modifications to the tree, and then adding the logging feature so a
batch modification can be reverted.

The notification infrastructure bothers me. It duplicates the
notification that the core DT code already performs. I do understand
that the current notifications don't do what you need them to because
you need it all deferred until the complete set of batch changes are
applied. However, instead of creating a new infrastructure, the existing
notifier should be reworked to be batch-change aware.

The parsing function is a specific user of the other two features. It
*may* be appropiate to be in a separate module, but it certainly should
be in a separate patch.

More generally I am concerned about whether or not overlays
will introduce corner cases that can never be handled correctly,
particularly in how multiple overlays will get handled. I want to see
very clear rules on what happens when multiple overlays are applied, and
then removed again. Is it possible to remove overlays out of order? If
so, what are the conditions that would not be allowed?

I'm also concerned about security. Turning on overlay support opens up
basically full access to the hardware of the system. It definitely needs
to be accessible only by root, but I think it goes farther than that.
When doing changes, there should be a mechanism to restrict which nodes
are allowed to be changed.

We also need to think about kexec. Kexec works by sucking the live tree
out of the kernel and creating a .dtb from it to pass to the new kernel.
What will the rules be when kexecing? Do all the overlays need to be
removed, or does the kernel get the tree with all the overlays applied
(in which case none of the overlays can be removed on the other side of
kexec).

More comments in-line...

> ---
>  Documentation/devicetree/overlay-notes.txt | 187 ++++++
>  drivers/of/Kconfig                         |  10 +
>  drivers/of/Makefile                        |   1 +
>  drivers/of/overlay.c                       | 895 +++++++++++++++++++++++++++++
>  include/linux/of.h                         | 153 +++++
>  5 files changed, 1246 insertions(+)
>  create mode 100644 Documentation/devicetree/overlay-notes.txt
>  create mode 100644 drivers/of/overlay.c
> 
> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
> new file mode 100644
> index 0000000..882d512
> --- /dev/null
> +++ b/Documentation/devicetree/overlay-notes.txt
> @@ -0,0 +1,187 @@
> +Device Tree Overlay Notes
> +-------------------------
> +
> +This document describes the implementation of the in-kernel
> +device tree overlay functionality residing in drivers/of/overlay.c and is a
> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
> +
> +How overlays work
> +-----------------
> +
> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
> +have the modification affecting the state of the the kernel in a way that
> +is reflecting the changes.
> +Since the kernel mainly deals with devices, any new device node that result
> +in an active device should have it created while if the device node is either
> +disabled or removed all together, the affected device should be deregistered.
> +
> +Lets take an example where we have a foo board with the following base tree
> +which is taken from [1].
> +
> +---- foo.dts -----------------------------------------------------------------
> +	/* FOO platform */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +		}
> +	};
> +---- foo.dts -----------------------------------------------------------------
> +
> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
> +
> +---- bar.dts -----------------------------------------------------------------
> +/plugin/;	/* allow undefined label references and record them */
> +/ {
> +	....	/* various properties for loader use; i.e. part id etc. */
> +	fragment@0 {
> +		target = <&ocp>;
> +		__overlay__ {
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +};
> +---- bar.dts -----------------------------------------------------------------
> +
> +result in foo+bar.dts
> +
> +---- foo+bar.dts -------------------------------------------------------------
> +	/* FOO platform + bar peripheral */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		}
> +	};
> +---- foo+bar.dts -------------------------------------------------------------
> +
> +As a result of the the overlay, a new device node (bar) has been created
> +so a bar platform device will be registered and if a matching device driver
> +is loaded the device will be created as expected.
> +
> +Overlay in-kernel API
> +---------------------
> +
> +The steps typically required to get an overlay to work are as follows:
> +
> +1. Use of_build_overlay_info() to create an array of initialized and
> +ready to use of_overlay_info structures.
> +2. Call of_overlay() to apply the overlays declared in the array.
> +3. If the overlay needs to be removed, call of_overlay_revert().
> +4. Finally release the memory taken by the overlay info array by
> +of_free_overlay_info().
> +
> +/**
> + * of_build_overlay_info	- Build an overlay info array
> + * @tree:	Device node containing all the overlays
> + * @cntp:	Pointer to where the overlay info count will be help
> + * @ovinfop:	Pointer to the pointer of an overlay info structure.
> + *
> + * Helper function that given a tree containing overlay information,
> + * allocates and builds an overlay info array containing it, ready
> + * for use using of_overlay.
> + *
> + * Returns 0 on success with the @cntp @ovinfop pointers valid,
> + * while on error a negative error value is returned.
> + */

Nit: Don't duplicate the function documentation in this file. It will
end up being out-of-date.

> +int of_build_overlay_info(struct device_node *tree,
> +		int *cntp, struct of_overlay_info **ovinfop);
> +
> +/**
> + * of_free_overlay_info	- Free an overlay info array
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to free
> + *
> + * Releases the memory of a previously allocate ovinfo array
> + * by of_build_overlay_info.
> + * Returns 0, or an error if the arguments are bogus.
> + */
> +int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab);
> +
> +/**
> + * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to apply
> + *
> + * Applies the overlays given, while handling all error conditions
> + * appropriately. Either the operation succeeds, or if it fails the
> + * live tree is reverted to the state before the attempt.
> + * Returns 0, or an error if the overlay attempt failed.
> + */
> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
> +
> +/**
> + * of_overlay_revert	- Revert a previously applied overlay
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to apply
> + *
> + * Revert a previous overlay. The state of the live tree
> + * is reverted to the one before the overlay.
> + * Returns 0, or an error if the overlay table is not given.
> + */
> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
> +
> +Overlay DTS Format
> +------------------
> +
> +The DTS of an overlay should have the following format:
> +
> +{
> +	/* ignored properties by the overlay */
> +
> +	fragment@0 {	/* first child node */
> +
> +		target=<phandle>;	/* phandle target of the overlay */
> +	or
> +		target-path="/path";	/* target path of the overlay */
> +	or
> +		target-alias="alias";	/* target alias of the overlay */


Documentation needs to be updated. target-alias doesn't exist in the
code anymore.

> +		__overlay__ {
> +			property-a;	/* add property-a to the target */
> +			-property-b;	/* remove property-b from target */
> +			node-a {	/* add to an existing, or create a node-a */
> +				...
> +			};
> +			-node-b {	/* remove an existing node-b */
> +				...
> +			};
> +		};

Instead of the '-' operator at the beginning of the property or node
name to indicate removal (a name starting with '-' is still a valid
name), can there be a separate __remove__ container node?

It may also be safer to have a __add__ operator that only adds the node
if there isn't already a node or property of that name.

> +	}
> +	fragment@1 {	/* second child node */
> +		...
> +	};
> +	/* more fragments follow */
> +}
> +
> +It should be noted that the DT overlay format described is the one expected
> +by the of_build_overlay_info() function, which is a helper function. There
> +is nothing stopping someone coming up with his own DTS format and that will
> +end up filling in the fields of the of_overlay_info array.
> +
> +Using the non-phandle based target method allows one to use a base DT which does
> +not contain a __symbols__ now, i.e. it was not compiled with the -@ option.

sp: s/now/node/

Is it the base DT that needs the __symbols__ node, or the overlay tree?
I had thought it was the overlay tree that contained the __symbols__
node. Regardless, this is the first mention in this file of __symbols__.
It would be good to discuss briefly how it works.

> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 4d39c88..cfb7ff8 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -86,4 +86,14 @@ config OF_RESOLVE
>  	  Enable OF dynamic resolution support. This allows you to
>  	  load Device Tree object fragments are run time.
>  
> +config OF_OVERLAY
> +	bool "OF overlay support"
> +	depends on OF
> +	select OF_DYNAMIC
> +	select OF_DEVICE
> +	select OF_RESOLVE
> +	help
> +	  OpenFirmware overlay support. Allows you to modify on runtime the
> +	  live tree using overlays.

Should not be a user-visable option. Drivers using it should select it
or otherwise cause it to be enabled.

> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c241e79..d2a6e0d 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>  obj-$(CONFIG_OF_MTD)	+= of_mtd.o
>  obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
>  obj-$(CONFIG_OF_RESOLVE)  += resolver.o
> +obj-$(CONFIG_OF_OVERLAY) += overlay.o
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> new file mode 100644
> index 0000000..1d4b884
> --- /dev/null
> +++ b/drivers/of/overlay.c
> @@ -0,0 +1,895 @@
> +/*
> + * Functions for working with device tree overlays
> + *
> + * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
> + * Copyright (C) 2012 Texas Instruments Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + */
> +#undef DEBUG
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/string.h>
> +#include <linux/ctype.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +
> +/* protect the handlers list */
> +static DEFINE_MUTEX(of_handler_mutex);
> +static struct list_head of_handler_list = LIST_HEAD_INIT(of_handler_list);
> +
> +int of_overlay_handler_register(struct of_overlay_handler *handler)
> +{
> +	/* guard against bad data */
> +	if (!handler || !handler->name || !handler->ops ||
> +		!handler->ops->create || !handler->ops->remove)
> +		return -EINVAL;
> +
> +	mutex_lock(&of_handler_mutex);
> +	list_add_tail(&handler->list, &of_handler_list);
> +	mutex_unlock(&of_handler_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_handler_register);
> +
> +void of_overlay_handler_unregister(struct of_overlay_handler *handler)
> +{
> +	struct of_overlay_handler *curr;
> +
> +	mutex_lock(&of_handler_mutex);
> +	list_for_each_entry(curr, &of_handler_list, list) {
> +		if (handler == curr) {
> +			list_del(&handler->list);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&of_handler_mutex);
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_handler_unregister);
> +
> +static int handler_create(struct of_overlay_device_entry *entry, int revert)
> +{
> +	struct of_overlay_handler *handler;
> +	int ret;
> +
> +	mutex_lock(&of_handler_mutex);
> +	list_for_each_entry(handler, &of_handler_list, list) {
> +		ret = (*handler->ops->create)(entry, revert);
> +		/* ENOTSUPP means try next */
> +		if (ret == -ENOTSUPP)
> +			continue;
> +		/* anything else means something happened */
> +		break;
> +	}
> +	mutex_unlock(&of_handler_mutex);
> +
> +	return ret;
> +}
> +
> +static int handler_remove(struct of_overlay_device_entry *entry, int revert)
> +{
> +	struct of_overlay_handler *handler;
> +	int ret;
> +
> +	mutex_lock(&of_handler_mutex);
> +	list_for_each_entry(handler, &of_handler_list, list) {
> +		ret = (*handler->ops->remove)(entry, revert);
> +		/* ENOTSUPP means try next */
> +		if (ret == -ENOTSUPP)
> +			continue;
> +		/* anything else means something happened */
> +		break;
> +	}
> +	mutex_unlock(&of_handler_mutex);
> +
> +	return ret;
> +}
> +
> +/*
> + * Apply a single overlay node recursively.
> + *
> + * Property or node names that start with '-' signal that
> + * the property/node is to be removed.
> + *
> + * All the property notifiers are appropriately called.
> + * Note that the in case of an error the target node is left
> + * in a inconsistent state. Error recovery should be performed
> + * by recording the modification using the of notifiers.
> + */
> +static int of_overlay_apply_one(struct device_node *target,
> +		const struct device_node *overlay)
> +{
> +	const char *pname, *cname;
> +	struct device_node *child, *tchild;
> +	struct property *prop, *propn, *tprop;
> +	int remove;
> +	char *full_name;
> +	const char *suffix;
> +	int ret;
> +
> +	/* sanity checks */
> +	if (target == NULL || overlay == NULL)
> +		return -EINVAL;
> +
> +	for_each_property_of_node(overlay, prop) {
> +
> +		/* don't touch, 'name' */
> +		if (of_prop_cmp(prop->name, "name") == 0)
> +			continue;
> +
> +		/* default is add */
> +		remove = 0;
> +		pname = prop->name;
> +		if (*pname == '-') {	/* skip, - notes removal */
> +			pname++;
> +			remove = 1;
> +			propn = NULL;
> +		} else {
> +			propn = __of_copy_property(prop, GFP_KERNEL,
> +					OF_PROP_ALLOCALL);
> +			if (propn == NULL)
> +				return -ENOMEM;
> +		}
> +
> +		tprop = of_find_property(target, pname, NULL);
> +
> +		/* found? */
> +		if (tprop != NULL) {
> +			if (propn != NULL)
> +				ret = of_update_property(target, propn);
> +			else
> +				ret = of_remove_property(target, tprop);
> +		} else {
> +			if (propn != NULL)
> +				ret = of_add_property(target, propn);
> +			else
> +				ret = 0;
> +		}
> +		if (ret != 0)
> +			return ret;
> +	}
> +
> +	__for_each_child_of_node(overlay, child) {
> +
> +		/* default is add */
> +		remove = 0;
> +		cname = child->name;
> +		if (*cname == '-') {	/* skip, - notes removal */
> +			cname++;
> +			remove = 1;
> +		}
> +
> +		/* special case for nodes with a suffix */
> +		suffix = strrchr(child->full_name, '@');
> +		if (suffix != NULL) {
> +			cname = kbasename(child->full_name);
> +			WARN_ON(cname == NULL);	/* sanity check */
> +			if (cname == NULL)
> +				continue;
> +			if (*cname == '-')
> +				cname++;
> +		}
> +
> +		tchild = of_get_child_by_name(target, cname);
> +		if (tchild != NULL) {
> +
> +			if (!remove) {
> +
> +				/* apply overlay recursively */
> +				ret = of_overlay_apply_one(tchild, child);
> +				of_node_put(tchild);
> +
> +				if (ret != 0)
> +					return ret;
> +
> +			} else {
> +
> +				ret = of_detach_node(tchild);
> +				of_node_put(tchild);
> +			}
> +
> +		} else {
> +
> +			if (!remove) {
> +				full_name = kasprintf(GFP_KERNEL, "%s/%s",
> +						target->full_name, cname);
> +				if (full_name == NULL)
> +					return -ENOMEM;
> +
> +				/* create empty tree as a target */
> +				tchild = __of_create_empty_node(cname,
> +						child->type, full_name,
> +						child->phandle, GFP_KERNEL,
> +						OF_NODE_ALLOCALL);
> +
> +				/* free either way */
> +				kfree(full_name);
> +
> +				if (tchild == NULL)
> +					return -ENOMEM;
> +
> +				/* point to parent */
> +				tchild->parent = target;
> +
> +				ret = of_attach_node(tchild);
> +				if (ret != 0)
> +					return ret;
> +
> +				/* apply the overlay */
> +				ret = of_overlay_apply_one(tchild, child);
> +				if (ret != 0) {
> +					__of_free_tree(tchild);
> +					return ret;
> +				}
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Lookup an overlay device entry
> + */
> +struct of_overlay_device_entry *of_overlay_device_entry_lookup(
> +		struct of_overlay_info *ovinfo, struct device_node *node)
> +{
> +	struct of_overlay_device_entry *de;
> +
> +	/* no need for locks, we'de under the ovinfo->lock */
> +	list_for_each_entry(de, &ovinfo->de_list, node) {
> +		if (de->np == node)
> +			return de;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * Add an overlay log entry
> + */
> +static int of_overlay_log_entry_entry_add(struct of_overlay_info *ovinfo,
> +		unsigned long action, struct device_node *dn,
> +		struct property *prop)
> +{
> +	struct of_overlay_log_entry *le;
> +
> +	/* check */
> +	if (ovinfo == NULL || dn == NULL)
> +		return -EINVAL;
> +
> +	le = kzalloc(sizeof(*le), GFP_KERNEL);
> +	if (le == NULL) {
> +		pr_err("%s: Failed to allocate\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	/* get a reference to the node */
> +	le->action = action;
> +	le->np = of_node_get(dn);
> +	le->prop = prop;
> +
> +	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
> +		le->old_prop = of_find_property(dn, prop->name, NULL);
> +
> +	list_add_tail(&le->node, &ovinfo->le_list);
> +
> +	return 0;
> +}
> +
> +/*
> + * Add an overlay device entry
> + */
> +static void of_overlay_device_entry_entry_add(struct of_overlay_info *ovinfo,
> +		struct device_node *node,
> +		int prevstate, int state)
> +{
> +	struct of_overlay_device_entry *de;
> +	int fresh;
> +
> +	/* check */
> +	if (ovinfo == NULL)
> +		return;
> +
> +	fresh = 0;
> +	de = of_overlay_device_entry_lookup(ovinfo, node);
> +	if (de == NULL) {
> +		de = kzalloc(sizeof(*de), GFP_KERNEL);
> +		if (de == NULL) {
> +			pr_err("%s: Failed to allocate\n", __func__);
> +			return;
> +		}
> +		fresh = 1;
> +		de->prevstate = -1;
> +	}
> +
> +	if (de->np == NULL)
> +		de->np = of_node_get(node);
> +	if (fresh)
> +		de->prevstate = prevstate;
> +	de->state = state;
> +
> +	if (fresh)
> +		list_add_tail(&de->node, &ovinfo->de_list);
> +}
> +
> +/*
> + * Overlay OF notifier
> + *
> + * Called every time there's a property/node modification
> + * Every modification causes a log entry addition, while
> + * any modification that causes a node's state to change
> + * from/to disabled to/from enabled causes a device entry
> + * addition.

I'm not convinced on the filter fo when notifications are sent. I think
it is still appropriate to send notifications on any node or property
change, and that it should be merged with the existing notifier code.

It seems a very odd structure that the existing notifier is being used
to call into this notifier
> + */
> +static int of_overlay_notify(struct notifier_block *nb,
> +				unsigned long action, void *arg)
> +{
> +	struct of_overlay_info *ovinfo;
> +	struct device_node *node;
> +	struct property *prop, *sprop, *cprop;
> +	struct of_prop_reconfig *pr;
> +	struct device_node *tnode;
> +	int depth;
> +	int prevstate, state;
> +	int err = 0;
> +
> +	ovinfo = container_of(nb, struct of_overlay_info, notifier);
> +
> +	/* prep vars */
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +	case OF_RECONFIG_DETACH_NODE:
> +		node = arg;
> +		if (node == NULL)
> +			return notifier_from_errno(-EINVAL);
> +		prop = NULL;
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		pr = arg;
> +		if (pr == NULL)
> +			return notifier_from_errno(-EINVAL);
> +		node = pr->dn;
> +		if (node == NULL)
> +			return notifier_from_errno(-EINVAL);
> +		prop = pr->prop;
> +		if (prop == NULL)
> +			return notifier_from_errno(-EINVAL);

None of the above error conditions will ever be true. The dt notifier code
already ensures that it only issues notifications when it has pointers
to the things that are being modified.

> +		break;
> +	default:
> +		return notifier_from_errno(0);
> +	}
> +
> +	/* add to the log */
> +	err = of_overlay_log_entry_entry_add(ovinfo, action, node, prop);
> +	if (err != 0)
> +		return notifier_from_errno(err);

It seems odd to add the log entry from within a notifier callback rather
than handling it directly in the code that initiated the modification.
How do you know that the modification entry actually came from the batch
that is being applied? If to overlays were progressing in parallel then
it will all get interleaved and will be completely broken.

> +
> +	/* come up with the device entry (if any) */
> +	state = 0;
> +	prevstate = 0;
> +
> +	/* determine the state the node will end up */
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		/* we demand that a compatible node is present */
> +		state = of_find_property(node, "compatible", NULL) &&
> +			of_device_is_available(node);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		prevstate = of_find_property(node, "compatible", NULL) &&
> +			of_device_is_available(node);
> +		state = 0;
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		/* either one cause a change in state */
> +		if (strcmp(prop->name, "status") != 0 &&
> +				strcmp(prop->name, "compatible") != 0)
> +			return notifier_from_errno(0);
> +
> +		if (strcmp(prop->name, "status") == 0) {
> +			/* status */
> +			cprop = of_find_property(node, "compatible", NULL);
> +			sprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
> +				prop : NULL;
> +		} else {
> +			/* compatible */
> +			sprop = of_find_property(node, "status", NULL);
> +			cprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
> +				prop : NULL;
> +		}
> +
> +		prevstate = of_find_property(node, "compatible", NULL) &&
> +			of_device_is_available(node);
> +		state = cprop && cprop->length > 0 &&
> +			    (!sprop || (sprop->length > 0 &&
> +				(strcmp(sprop->value, "okay") == 0 ||
> +				 strcmp(sprop->value, "ok") == 0)));
> +		break;
> +
> +	default:
> +		return notifier_from_errno(0);
> +	}
> +
> +	/* find depth */
> +	depth = 1;
> +	tnode = node;
> +	while (tnode != NULL && tnode != ovinfo->target) {
> +		tnode = tnode->parent;
> +		depth++;
> +	}
> +
> +	/* respect overlay's maximum depth */
> +	if (ovinfo->device_depth != 0 && depth > ovinfo->device_depth) {
> +		pr_debug("OF: skipping device creation for node=%s depth=%d\n",
> +				node->name, depth);
> +		goto out;
> +	}

What is the purpose of the maximum depth?

> +
> +	of_overlay_device_entry_entry_add(ovinfo, node, prevstate, state);
> +out:
> +
> +	return notifier_from_errno(err);
> +}
> +
> +/*
> + * Prepare for the overlay, for now it just registers the
> + * notifier.
> + */
> +static int of_overlay_prep_one(struct of_overlay_info *ovinfo)
> +{
> +	int err;
> +
> +	err = of_reconfig_notifier_register(&ovinfo->notifier);
> +	if (err != 0) {
> +		pr_err("%s: failed to register notifier for '%s'\n",
> +			__func__, ovinfo->target->full_name);
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static int of_overlay_device_entry_change(struct of_overlay_info *ovinfo,
> +		struct of_overlay_device_entry *de, int revert)
> +{
> +	int state;
> +	int ret;
> +
> +	state = !!de->state ^ !!revert;
> +
> +	if (state)
> +		ret = handler_create(de, revert);
> +	else
> +		ret = handler_remove(de, revert);
> +
> +	if (ret != 0 && ret != -ENOTSUPP)
> +		pr_warn("%s: Failed to %s device "
> +				"for node '%s'\n", __func__,
> +				state ? "create" : "remove",
> +				de->np->full_name);
> +	return 0;
> +}
> +
> +/*
> + * Revert one overlay
> + * Either due to an error, or due to normal overlay removal.
> + * Using the log entries, we revert any change to the live tree.
> + * In the same manner, using the device entries we enable/disable
> + * the devices appropriately.
> + */
> +static void of_overlay_revert_one(struct of_overlay_info *ovinfo)
> +{
> +	struct of_overlay_device_entry *de, *den;
> +	struct of_overlay_log_entry *le, *len;
> +	struct property *prop, **propp;
> +	struct device_node *np;
> +	int ret;
> +	unsigned long flags;
> +
> +	if (!ovinfo || !ovinfo->target || !ovinfo->overlay)
> +		return;
> +
> +	pr_debug("%s: Reverting overlay on '%s'\n", __func__,
> +			ovinfo->target->full_name);
> +
> +	/* overlay applied correctly, now create/destroy pdevs */
> +	list_for_each_entry_safe_reverse(de, den, &ovinfo->de_list, node) {
> +		of_overlay_device_entry_change(ovinfo, de, 1);
> +		of_node_put(de->np);
> +		list_del(&de->node);
> +		kfree(de);
> +	}
> +
> +	list_for_each_entry_safe_reverse(le, len, &ovinfo->le_list, node) {
> +
> +		/* get node and immediately put */
> +		np = le->np;
> +		of_node_put(le->np);
> +		le->np = NULL;
> +
> +		ret = 0;
> +		switch (le->action) {
> +		case OF_RECONFIG_ATTACH_NODE:
> +			pr_debug("Reverting ATTACH_NODE %s\n",
> +					np->full_name);
> +			ret = of_detach_node(np);
> +			break;
> +
> +		case OF_RECONFIG_DETACH_NODE:
> +			pr_debug("Reverting DETACH_NODE %s\n",
> +					np->full_name);
> +			ret = of_attach_node(np);
> +			break;
> +
> +		case OF_RECONFIG_ADD_PROPERTY:
> +			pr_debug("Reverting ADD_PROPERTY %s %s\n",
> +					np->full_name, le->prop->name);
> +			ret = of_remove_property(np, le->prop);
> +			break;
> +
> +		case OF_RECONFIG_REMOVE_PROPERTY:
> +		case OF_RECONFIG_UPDATE_PROPERTY:
> +
> +			pr_debug("Reverting %s_PROPERTY %s %s\n",
> +				le->action == OF_RECONFIG_REMOVE_PROPERTY ?
> +					"REMOVE" : "UPDATE",
> +					np->full_name, le->prop->name);
> +
> +			/* property is possibly on deadprops (avoid alloc) */
> +			raw_spin_lock_irqsave(&devtree_lock, flags);
> +			prop = le->action == OF_RECONFIG_REMOVE_PROPERTY ?
> +				le->prop : le->old_prop;
> +			propp = &np->deadprops;
> +			while (*propp != NULL) {
> +				if (*propp == prop)
> +					break;
> +				propp = &(*propp)->next;
> +			}
> +			if (*propp != NULL) {
> +				/* remove it from deadprops */
> +				(*propp)->next = prop->next;
> +				raw_spin_unlock_irqrestore(&devtree_lock,
> +						flags);
> +			} else {
> +				raw_spin_unlock_irqrestore(&devtree_lock,
> +						flags);
> +				/* not found, just make a copy */
> +				prop = __of_copy_property(prop, GFP_KERNEL,
> +						OF_PROP_ALLOCALL);
> +				if (prop == NULL) {
> +					pr_err("%s: Failed to copy property\n",
> +							__func__);
> +					break;
> +				}
> +			}
> +
> +			if (le->action == OF_RECONFIG_REMOVE_PROPERTY)
> +				ret = of_add_property(np, prop);
> +			else
> +				ret = of_update_property(np, prop);
> +			break;
> +
> +		default:
> +			/* nothing */
> +			break;
> +		}
> +
> +		if (ret != 0)
> +			pr_err("%s: revert on node %s Failed!\n",
> +					__func__, np->full_name);
> +
> +		list_del(&le->node);
> +
> +		kfree(le);
> +	}
> +}
> +
> +/*
> + * Perform the post overlay work.
> + *
> + * We unregister the notifier, and in the case on an error we
> + * revert the overlay.
> + * If the overlay applied correctly, we iterate over the device entries
> + * and create/destroy the devices appropriately.
> + */
> +static int of_overlay_post_one(struct of_overlay_info *ovinfo, int err)
> +{
> +	struct of_overlay_device_entry *de, *den;
> +
> +	of_reconfig_notifier_unregister(&ovinfo->notifier);
> +
> +	if (err != 0) {
> +		/* revert this (possible partially applied) overlay */
> +		of_overlay_revert_one(ovinfo);
> +		return 0;
> +	}
> +
> +	/* overlay applied correctly, now create/destroy pdevs */
> +	list_for_each_entry_safe(de, den, &ovinfo->de_list, node) {
> +
> +		/* no state change? just remove this entry */
> +		if (de->prevstate == de->state) {
> +			of_node_put(de->np);
> +			list_del(&de->node);
> +			kfree(de);
> +			continue;
> +		}
> +
> +		of_overlay_device_entry_change(ovinfo, de, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to apply
> + *
> + * Applies the overlays given, while handling all error conditions
> + * appropriately. Either the operation succeeds, or if it fails the
> + * live tree is reverted to the state before the attempt.
> + * Returns 0, or an error if the overlay attempt failed.
> + */
> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab)

overlay fragments don't live in isolation. There needs to be a top level
of_overlay container structure that holds the list/array of of_overlay_info
structures and can hold any other data that may be required for
reverting the overlay, such as the modification log.

> +{
> +	struct of_overlay_info *ovinfo;
> +	int i, err;
> +

This entire function needs to be protected by a global mutex. At no
point in time should multiple overlays get processed concurrently.

I see that each individual of_overlay_info structure has a mutex, but I
don't understand why the locking it that granular. The entire overlay
should be a single block and only one overlay should ever be processed
at a time. What is the reason for locking per of_overlay_info?

> +	if (!ovinfo_tab)
> +		return -EINVAL;
> +
> +	/* first we apply the overlays atomically */
> +	for (i = 0; i < count; i++) {
> +
> +		ovinfo = &ovinfo_tab[i];
> +
> +		mutex_lock(&ovinfo->lock);
> +
> +		err = of_overlay_prep_one(ovinfo);
> +		if (err == 0)
> +			err = of_overlay_apply_one(ovinfo->target,
> +					ovinfo->overlay);
> +		of_overlay_post_one(ovinfo, err);
> +
> +		mutex_unlock(&ovinfo->lock);
> +
> +		if (err != 0) {
> +			pr_err("%s: overlay failed '%s'\n",
> +				__func__, ovinfo->target->full_name);
> +			goto err_fail;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_fail:
> +	while (--i >= 0) {
> +		ovinfo = &ovinfo_tab[i];
> +
> +		mutex_lock(&ovinfo->lock);
> +		of_overlay_revert_one(ovinfo);
> +		mutex_unlock(&ovinfo->lock);
> +	}
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(of_overlay);
> +
> +/**
> + * of_overlay_revert	- Revert a previously applied overlay
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to apply
> + *
> + * Revert a previous overlay. The state of the live tree
> + * is reverted to the one before the overlay.
> + * Returns 0, or an error if the overlay table is not given.
> + */
> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
> +{
> +	struct of_overlay_info *ovinfo;
> +	int i;
> +
> +	if (!ovinfo_tab)
> +		return -EINVAL;
> +
> +	/* revert the overlays in reverse */
> +	for (i = count - 1; i >= 0; i--) {
> +
> +		ovinfo = &ovinfo_tab[i];
> +
> +		mutex_lock(&ovinfo->lock);
> +		of_overlay_revert_one(ovinfo);
> +		mutex_unlock(&ovinfo->lock);
> +
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_revert);
> +
> +/**
> + * of_init_overlay_info	- Initialize a single of_overlay_info structure
> + * @ovinfo:	Pointer to the overlay info structure to initialize
> + *
> + * Initialize a single overlay info structure.
> + */
> +void of_init_overlay_info(struct of_overlay_info *ovinfo)
> +{
> +	memset(ovinfo, 0, sizeof(*ovinfo));
> +	mutex_init(&ovinfo->lock);
> +	INIT_LIST_HEAD(&ovinfo->de_list);
> +	INIT_LIST_HEAD(&ovinfo->le_list);
> +
> +	ovinfo->notifier.notifier_call = of_overlay_notify;
> +}
> +
> +/*
> + * Find the target node using a number of different strategies
> + * in order of preference
> + *
> + * "target" property containing the phandle of the target
> + * "target-path" property containing the path of the target
> + *
> + */
> +struct device_node *find_target_node(struct device_node *info_node)
> +{
> +	const char *path;
> +	u32 val;
> +	int ret;
> +
> +	/* first try to go by using the target as a phandle */
> +	ret = of_property_read_u32(info_node, "target", &val);
> +	if (ret == 0)
> +		return of_find_node_by_phandle(val);
> +
> +	/* now try to locate by path */
> +	ret = of_property_read_string(info_node, "target-path", &path);
> +	if (ret == 0)
> +		return of_find_node_by_path(path);
> +
> +	pr_err("%s: Failed to find target for node %p (%s)\n", __func__,
> +			info_node, info_node->name);
> +
> +	return NULL;
> +}
> +
> +/**
> + * of_fill_overlay_info	- Fill an overlay info structure
> + * @info_node:	Device node containing the overlay
> + * @ovinfo:	Pointer to the overlay info structure to fill
> + *
> + * Fills an overlay info structure with the overlay information
> + * from a device node. This device node must have a target property
> + * which contains a phandle of the overlay target node, and an
> + * __overlay__ child node which has the overlay contents.
> + * Both ovinfo->target & ovinfo->overlay have their references taken.
> + *
> + * Returns 0 on success, or a negative error value.
> + */
> +int of_fill_overlay_info(struct device_node *info_node,
> +		struct of_overlay_info *ovinfo)
> +{
> +	u32 val;
> +	int ret;
> +
> +	if (!info_node || !ovinfo)
> +		return -EINVAL;
> +
> +	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
> +	if (ovinfo->overlay == NULL)
> +		goto err_fail;
> +
> +	ovinfo->target = find_target_node(info_node);
> +	if (ovinfo->target == NULL)
> +		goto err_fail;
> +
> +	ret = of_property_read_u32(info_node, "depth", &val);
> +	if (ret == 0)
> +		ovinfo->device_depth = val;
> +	else
> +		ovinfo->device_depth = 0;
> +
> +	return 0;
> +
> +err_fail:
> +	of_node_put(ovinfo->target);
> +	of_node_put(ovinfo->overlay);
> +
> +	memset(ovinfo, 0, sizeof(*ovinfo));
> +	return -EINVAL;
> +}
> +
> +/**
> + * of_build_overlay_info	- Build an overlay info array
> + * @tree:	Device node containing all the overlays
> + * @cntp:	Pointer to where the overlay info count will be help
> + * @ovinfop:	Pointer to the pointer of an overlay info structure.
> + *
> + * Helper function that given a tree containing overlay information,
> + * allocates and builds an overlay info array containing it, ready
> + * for use using of_overlay.
> + *
> + * Returns 0 on success with the @cntp @ovinfop pointers valid,
> + * while on error a negative error value is returned.
> + */
> +int of_build_overlay_info(struct device_node *tree,
> +		int *cntp, struct of_overlay_info **ovinfop)
> +{
> +	struct device_node *node;
> +	struct of_overlay_info *ovinfo;
> +	int cnt, err;
> +
> +	if (tree == NULL || cntp == NULL || ovinfop == NULL)
> +		return -EINVAL;
> +
> +	/* worst case; every child is a node */
> +	cnt = 0;
> +	for_each_child_of_node(tree, node)
> +		cnt++;
> +
> +	ovinfo = kzalloc(cnt * sizeof(*ovinfo), GFP_KERNEL);
> +	if (ovinfo == NULL)
> +		return -ENOMEM;
> +
> +	cnt = 0;
> +	for_each_child_of_node(tree, node) {
> +
> +		of_init_overlay_info(&ovinfo[cnt]);
> +		err = of_fill_overlay_info(node, &ovinfo[cnt]);
> +		if (err == 0)
> +			cnt++;
> +	}
> +
> +	/* if nothing filled, return error */
> +	if (cnt == 0) {
> +		kfree(ovinfo);
> +		return -ENODEV;
> +	}
> +
> +	*cntp = cnt;
> +	*ovinfop = ovinfo;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_build_overlay_info);
> +
> +/**
> + * of_free_overlay_info	- Free an overlay info array
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to free
> + *
> + * Releases the memory of a previously allocate ovinfo array
> + * by of_build_overlay_info.
> + * Returns 0, or an error if the arguments are bogus.
> + */
> +int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab)
> +{
> +	struct of_overlay_info *ovinfo;
> +	int i;
> +
> +	if (!ovinfo_tab || count < 0)
> +		return -EINVAL;
> +
> +	/* do it in reverse */
> +	for (i = count - 1; i >= 0; i--) {
> +		ovinfo = &ovinfo_tab[i];
> +
> +		of_node_put(ovinfo->target);
> +		of_node_put(ovinfo->overlay);
> +	}
> +	kfree(ovinfo_tab);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_free_overlay_info);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 3edb9b9..358f984 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -23,6 +23,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/topology.h>
>  #include <linux/notifier.h>
> +#include <linux/list.h>
>  
>  #include <asm/byteorder.h>
>  #include <asm/errno.h>
> @@ -828,4 +829,156 @@ static inline int of_resolve(struct device_node *resolve)
>  
>  #endif
>  
> +/**
> + * Overlay support
> + */
> +
> +/**
> + * struct of_overlay_log_entry	- Holds a DT log entry
> + * @node:	list_head for the log list
> + * @action:	notifier action
> + * @np:		pointer to the device node affected
> + * @prop:	pointer to the property affected
> + * @old_prop:	hold a pointer to the original property
> + *
> + * Every modification of the device tree during application of the
> + * overlay is held in a list of of_overlay_log_entry structures.
> + * That way we can recover from a partial application, or we can
> + * revert the overlay properly.
> + */
> +struct of_overlay_log_entry {
> +	struct list_head node;
> +	unsigned long action;
> +	struct device_node *np;
> +	struct property *prop;
> +	struct property *old_prop;
> +};
> +
> +struct of_overlay_device_entry;
> +
> +/**
> + * struct of_overlay_handler_ops	- Overlay device handler ops
> + * @create:	method to be called to create a device
> + * @remove:	method to be called to destroy a device
> + *
> + * Both these functions return 0 on success, ENOTSUPP if the
> + * device entry does not match, and an error code otherwise.
> + */
> +struct of_overlay_handler_ops {
> +	int (*create)(struct of_overlay_device_entry *entry, int revert);
> +	int (*remove)(struct of_overlay_device_entry *entry, int revert);
> +};
> +
> +/**
> + * struct of_overlay_handler	- Overlay device handler
> + * @list:	list links for all handlers
> + * @name:	name of this handler
> + * @ops:	ops member functions
> + *
> + * The handler is registered by each bus that supports
> + * dynamic creation/removal of devices
> + */
> +struct of_overlay_handler {
> +	struct list_head list;
> +	const char *name;
> +	const struct of_overlay_handler_ops *ops;
> +};
> +
> +/**
> + * struct of_overlay_device_entry	- Holds an overlay device entry
> + * @node:	list_head for the device list
> + * @np:		device node pointer to the device node affected
> + * @state:	new device state
> + * @prevstate:	previous device state
> + * @priv:	private pointer for use by bus handlers
> + *
> + * When the overlay results in a device node's state to change this
> + * fact is recorded in a list of device entries. After the overlay
> + * is applied we can create/destroy the devices according
> + * to the new state of the live tree.
> + */
> +struct of_overlay_device_entry {
> +	struct list_head node;
> +	struct device_node *np;
> +	int prevstate;
> +	int state;
> +	void *priv;
> +};
> +
> +/**
> + * struct of_overlay_info	- Holds a single overlay info
> + * @target:	target of the overlay operation
> + * @overlay:	pointer to the overlay contents node
> + * @lock:	Lock to hold when accessing the lists
> + * @le_list:	List of the overlay logs
> + * @de_list:	List of the overlay records
> + * @notifier:	of reconfiguration notifier
> + *
> + * Holds a single overlay state, including all the overlay logs &
> + * records.
> + */
> +struct of_overlay_info {
> +	struct device_node *target;
> +	struct device_node *overlay;
> +	struct mutex lock;
> +	struct list_head le_list;
> +	struct list_head de_list;
> +	struct notifier_block notifier;
> +	int device_depth;
> +};
> +
> +#ifdef CONFIG_OF_OVERLAY
> +
> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
> +
> +int of_fill_overlay_info(struct device_node *info_node,
> +		struct of_overlay_info *ovinfo);
> +int of_build_overlay_info(struct device_node *tree,
> +		int *cntp, struct of_overlay_info **ovinfop);
> +int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo);
> +
> +int of_overlay_handler_register(struct of_overlay_handler *handler);
> +void of_overlay_handler_unregister(struct of_overlay_handler *handler);
> +
> +#else
> +
> +static inline int of_overlay(int count, struct of_overlay_info *ovinfo_tab)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int of_fill_overlay_info(struct device_node *info_node,
> +		struct of_overlay_info *ovinfo)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int of_build_overlay_info(struct device_node *tree,
> +		int *cntp, struct of_overlay_info **ovinfop)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int of_overlay_handler_register(struct of_overlay_handler *handler)
> +{
> +	return 0;
> +}
> +
> +static inline void of_overlay_handler_unregister(struct of_overlay_handler *handler)
> +{
> +}
> +
> +#endif
> +
>  #endif /* _LINUX_OF_H */
> -- 
> 1.7.12
> 

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Michael Stickel]

Hi Grant,

Am 14.05.2014 12:08, schrieb Grant Likely:
> More generally I am concerned about whether or not overlays
> will introduce corner cases that can never be handled correctly,
> particularly in how multiple overlays will get handled. I want to see
> very clear rules on what happens when multiple overlays are applied, and
> then removed again. Is it possible to remove overlays out of order? If
> so, what are the conditions that would not be allowed?

Yes, it is possible that an overlay depends on another.

The problem is not, that an overlay is removed other overlays depend on,
but that nodes of an overlay may depend on the to-be-removed overlay and
the resulting devicetree can become inconsistent.


I have an SPI Bus with two slaves. The second slave is used only on one
of our boards. That is why we split the overlays the following way:

xxxx_spi1.dts:
  Pinmux for SPI-Bus and activation of spi-controller.
  Pinmux for CS0 and definition of first slave.

xxxx_spi1_cs1:
  Pinmux for CS1 and definition of second slave.

When the overlay for the bus is removed, the overlays for the second
slave does not make any sense anymore.

It is even worse in a scenario we have with a test board.
One of the slaves is an spi-io-controller with a few bitbanging i2c
masters. In an extreme case, each component is defined in a separate
overlay and only the overlay with the master is removed. I know, that
this is completely sick. The devices are removed cleanly because of the
device dependency.

Michael

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Geert Uytterhoeven]

On Wed, May 14, 2014 at 12:08 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
>> +config OF_OVERLAY
>> +     bool "OF overlay support"
>> +     depends on OF
>> +     select OF_DYNAMIC
>> +     select OF_DEVICE
>> +     select OF_RESOLVE
>> +     help
>> +       OpenFirmware overlay support. Allows you to modify on runtime the
>> +       live tree using overlays.
>
> Should not be a user-visable option. Drivers using it should select it

Why not? It's up to the (final) user to use it, or not.

> or otherwise cause it to be enabled.

Why should this be driver-specific? Shouldn't this just work with all drivers?

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/8] OF: Introduce DT overlay support.

[Guenter Roeck]

On 05/14/2014 06:03 AM, Geert Uytterhoeven wrote:
> On Wed, May 14, 2014 at 12:08 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>>> +config OF_OVERLAY
>>> +     bool "OF overlay support"
>>> +     depends on OF
>>> +     select OF_DYNAMIC
>>> +     select OF_DEVICE
>>> +     select OF_RESOLVE
>>> +     help
>>> +       OpenFirmware overlay support. Allows you to modify on runtime the
>>> +       live tree using overlays.
>>
>> Should not be a user-visable option. Drivers using it should select it
>
> Why not? It's up to the (final) user to use it, or not.
>
>> or otherwise cause it to be enabled.
>
> Why should this be driver-specific? Shouldn't this just work with all drivers?
>

It does once enabled.

I think what Grant refers to is that there has to be a driver which implements
the actual overlay insertion and removal (ie calls the new API functions),
and that the Kconfig option for this driver should select OF_OVERLAY
instead of depending on it.

Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Wed, 14 May 2014 15:03:35 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, May 14, 2014 at 12:08 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
> >> +config OF_OVERLAY
> >> +     bool "OF overlay support"
> >> +     depends on OF
> >> +     select OF_DYNAMIC
> >> +     select OF_DEVICE
> >> +     select OF_RESOLVE
> >> +     help
> >> +       OpenFirmware overlay support. Allows you to modify on runtime the
> >> +       live tree using overlays.
> >
> > Should not be a user-visable option. Drivers using it should select it
> 
> Why not? It's up to the (final) user to use it, or not.
> 
> > or otherwise cause it to be enabled.
> 
> Why should this be driver-specific? Shouldn't this just work with all drivers?

This patch is just the infrastructure. The actual user-visible option
is in another patch.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Wed, 14 May 2014 14:11:52 +0200, Michael Stickel <ms@mycable.de> wrote:
> Hi Grant,
> 
> Am 14.05.2014 12:08, schrieb Grant Likely:
> > More generally I am concerned about whether or not overlays
> > will introduce corner cases that can never be handled correctly,
> > particularly in how multiple overlays will get handled. I want to see
> > very clear rules on what happens when multiple overlays are applied, and
> > then removed again. Is it possible to remove overlays out of order? If
> > so, what are the conditions that would not be allowed?
> 
> Yes, it is possible that an overlay depends on another.
> 
> The problem is not, that an overlay is removed other overlays depend on,
> but that nodes of an overlay may depend on the to-be-removed overlay and
> the resulting devicetree can become inconsistent.

So what should the rule be then? It sounds to me that it should be a
hard and fast rule for overlays to always be removed in-order. If two
overlays are applied, and the first one needs to be removed again, then
that forces a removal of the second. The code needs to enforce it too.

The question can be revisited if someone can find a way to validate
overlays do not conflict.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Guenter Roeck]

On Wed, May 14, 2014 at 04:49:07PM +0100, Grant Likely wrote:
> On Wed, 14 May 2014 14:11:52 +0200, Michael Stickel <ms@mycable.de> wrote:
> > Hi Grant,
> > 
> > Am 14.05.2014 12:08, schrieb Grant Likely:
> > > More generally I am concerned about whether or not overlays
> > > will introduce corner cases that can never be handled correctly,
> > > particularly in how multiple overlays will get handled. I want to see
> > > very clear rules on what happens when multiple overlays are applied, and
> > > then removed again. Is it possible to remove overlays out of order? If
> > > so, what are the conditions that would not be allowed?
> > 
> > Yes, it is possible that an overlay depends on another.
> > 
> > The problem is not, that an overlay is removed other overlays depend on,
> > but that nodes of an overlay may depend on the to-be-removed overlay and
> > the resulting devicetree can become inconsistent.
> 
> So what should the rule be then? It sounds to me that it should be a
> hard and fast rule for overlays to always be removed in-order. If two
> overlays are applied, and the first one needs to be removed again, then
> that forces a removal of the second. The code needs to enforce it too.
> 
> The question can be revisited if someone can find a way to validate
> overlays do not conflict.
> 
We'll need to find a way to determine if overlays are nested. Only nested
overlays must be removed in order. Otherwise the entire concept falls apart.
In our case, overlays are used for hot plugged cards. Obviously there can
and will be more than one of those cards in the system, and they can
and will be inserted and removed in any order.

Maybe a nesting level counter in each property would do it ? Or a reference
pointing to overlay specific objects / data ?

Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Grant,

On May 14, 2014, at 3:08 AM, Grant Likely wrote:

> On Fri,  4 Apr 2014 15:43:55 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>> Introduce DT overlay support.
>> Using this functionality it is possible to dynamically overlay a part of
>> the kernel's tree with another tree that's been dynamically loaded.
>> It is also possible to remove node and properties.
>> 
>> The creation/destruction of the devices is handled by calling in to
>> bus specific handlers which can deal with the peculiarities of each
>> device.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> 
> Hi Panto,
> 
> I spent a bunch of time yesterday going over this code and I have some
> comments and questions. First off I must apologize. I think I completely
> misunderstood the approach you are taking for putting the overlay data
> into the tree. I had assumed that you were unflattening the overlay tree
> and then rearranging the new device_nodes to insert them into the
> existing tree, but it looks like you're leaving the overlay tree alone
> and merely using it's nodes as templates when allocating new nodes that
> will get inserted into the tree. Sorry for being confused on this. A few
> of my opinions have probably changed now that I understand what you're
> trying to do.
> 

No worries. It is a complicated subject after all. Let me reply with 
what we discussed on IRC, so it's recorded for posterity.

> As I've said before, this is a complicated bit of code that touches into
> the core of device tree behaviour, so I am being cautious. This patch
> and the previous patch add a lot of functionality that is hard to review
> all at once, and it conflates some concepts that I would like to be very
> well defined. For example, this patch adds at least 3 distinct features:
> 
> - Revertable batch modifications to the live tree.
> - New notification infrastructure for informing other code when changes
>  take place
> - Parsing a tree in the overlay format to create an of_overlay_info
>  array.
> 
> The revertable batch feature should be a patch all on its own without
> any of the change tracking or notification aside from what the core code
> is already doing. That change could be applied independently even if I
> still have issues with the notification or parsing code.
> 
> I would even split up the first feature into two steps: batch
> modifications to the tree, and then adding the logging feature so a
> batch modification can be reverted.
> 

OK.

> The notification infrastructure bothers me. It duplicates the
> notification that the core DT code already performs. I do understand
> that the current notifications don't do what you need them to because
> you need it all deferred until the complete set of batch changes are
> applied. However, instead of creating a new infrastructure, the existing
> notifier should be reworked to be batch-change aware.
> 

If I understood correctly, you're asking of rolling in this per-bus notifier
mechanism in the standard DT notifier infrastructure already in place.
I can't be absolutely sure about the details right now, but seems possible.

I don't know if the kernel notifier framework will be unmodified, but I hope so.
 
> The parsing function is a specific user of the other two features. It
> *may* be appropiate to be in a separate module, but it certainly should
> be in a separate patch.
> 

OK

> More generally I am concerned about whether or not overlays
> will introduce corner cases that can never be handled correctly,
> particularly in how multiple overlays will get handled. I want to see
> very clear rules on what happens when multiple overlays are applied, and
> then removed again. Is it possible to remove overlays out of order? If
> so, what are the conditions that would not be allowed?
> 

As mentioned by Guenter, a single stack of overlays is no good; we need to
support an arbitrary stacking method. One way to implement it would be to
add an overlay use counter which we will increase whenever a node is 'touched'
by an overlay. Removal is only going to be permitted if the counters of a
subtree are all less or equal to 1. The equal to 0 is to account for non-overlay
node attachements.

> I'm also concerned about security. Turning on overlay support opens up
> basically full access to the hardware of the system. It definitely needs
> to be accessible only by root, but I think it goes farther than that.
> When doing changes, there should be a mechanism to restrict which nodes
> are allowed to be changed.
> 

It's hard for me to think of all the security implications. For starters this
is obviously for root only (or in-kernel users).

We can implement some kind of attribute like 'mutable' that we will use to adorn
a subtree that permits overlay application.

> We also need to think about kexec. Kexec works by sucking the live tree
> out of the kernel and creating a .dtb from it to pass to the new kernel.
> What will the rules be when kexecing? Do all the overlays need to be
> removed, or does the kernel get the tree with all the overlays applied
> (in which case none of the overlays can be removed on the other side of
> kexec).
> 

We can add a sysfs attribute that configures whether overlays are reverted before
kexec or not. I can't really tell which is the correct option, so let's allow the
policy up to user-space.

> More comments in-line...
> 
>> ---
>> Documentation/devicetree/overlay-notes.txt | 187 ++++++
>> drivers/of/Kconfig                         |  10 +
>> drivers/of/Makefile                        |   1 +
>> drivers/of/overlay.c                       | 895 +++++++++++++++++++++++++++++
>> include/linux/of.h                         | 153 +++++
>> 5 files changed, 1246 insertions(+)
>> create mode 100644 Documentation/devicetree/overlay-notes.txt
>> create mode 100644 drivers/of/overlay.c
>> 
>> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
>> new file mode 100644
>> index 0000000..882d512
>> --- /dev/null
>> +++ b/Documentation/devicetree/overlay-notes.txt
>> @@ -0,0 +1,187 @@
>> +Device Tree Overlay Notes
>> +-------------------------
>> +
>> +This document describes the implementation of the in-kernel
>> +device tree overlay functionality residing in drivers/of/overlay.c and is a
>> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
>> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
>> +
>> +How overlays work
>> +-----------------
>> +
>> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
>> +have the modification affecting the state of the the kernel in a way that
>> +is reflecting the changes.
>> +Since the kernel mainly deals with devices, any new device node that result
>> +in an active device should have it created while if the device node is either
>> +disabled or removed all together, the affected device should be deregistered.
>> +
>> +Lets take an example where we have a foo board with the following base tree
>> +which is taken from [1].
>> +
>> +---- foo.dts -----------------------------------------------------------------
>> +	/* FOO platform */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +		}
>> +	};
>> +---- foo.dts -----------------------------------------------------------------
>> +
>> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
>> +
>> +---- bar.dts -----------------------------------------------------------------
>> +/plugin/;	/* allow undefined label references and record them */
>> +/ {
>> +	....	/* various properties for loader use; i.e. part id etc. */
>> +	fragment@0 {
>> +		target = <&ocp>;
>> +		__overlay__ {
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		};
>> +	};
>> +};
>> +---- bar.dts -----------------------------------------------------------------
>> +
>> +result in foo+bar.dts
>> +
>> +---- foo+bar.dts -------------------------------------------------------------
>> +	/* FOO platform + bar peripheral */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		}
>> +	};
>> +---- foo+bar.dts -------------------------------------------------------------
>> +
>> +As a result of the the overlay, a new device node (bar) has been created
>> +so a bar platform device will be registered and if a matching device driver
>> +is loaded the device will be created as expected.
>> +
>> +Overlay in-kernel API
>> +---------------------
>> +
>> +The steps typically required to get an overlay to work are as follows:
>> +
>> +1. Use of_build_overlay_info() to create an array of initialized and
>> +ready to use of_overlay_info structures.
>> +2. Call of_overlay() to apply the overlays declared in the array.
>> +3. If the overlay needs to be removed, call of_overlay_revert().
>> +4. Finally release the memory taken by the overlay info array by
>> +of_free_overlay_info().
>> +
>> +/**
>> + * of_build_overlay_info	- Build an overlay info array
>> + * @tree:	Device node containing all the overlays
>> + * @cntp:	Pointer to where the overlay info count will be help
>> + * @ovinfop:	Pointer to the pointer of an overlay info structure.
>> + *
>> + * Helper function that given a tree containing overlay information,
>> + * allocates and builds an overlay info array containing it, ready
>> + * for use using of_overlay.
>> + *
>> + * Returns 0 on success with the @cntp @ovinfop pointers valid,
>> + * while on error a negative error value is returned.
>> + */
> 
> Nit: Don't duplicate the function documentation in this file. It will
> end up being out-of-date.
> 

OK.

>> +int of_build_overlay_info(struct device_node *tree,
>> +		int *cntp, struct of_overlay_info **ovinfop);
>> +
>> +/**
>> + * of_free_overlay_info	- Free an overlay info array
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to free
>> + *
>> + * Releases the memory of a previously allocate ovinfo array
>> + * by of_build_overlay_info.
>> + * Returns 0, or an error if the arguments are bogus.
>> + */
>> +int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab);
>> +
>> +/**
>> + * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to apply
>> + *
>> + * Applies the overlays given, while handling all error conditions
>> + * appropriately. Either the operation succeeds, or if it fails the
>> + * live tree is reverted to the state before the attempt.
>> + * Returns 0, or an error if the overlay attempt failed.
>> + */
>> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
>> +
>> +/**
>> + * of_overlay_revert	- Revert a previously applied overlay
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to apply
>> + *
>> + * Revert a previous overlay. The state of the live tree
>> + * is reverted to the one before the overlay.
>> + * Returns 0, or an error if the overlay table is not given.
>> + */
>> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
>> +
>> +Overlay DTS Format
>> +------------------
>> +
>> +The DTS of an overlay should have the following format:
>> +
>> +{
>> +	/* ignored properties by the overlay */
>> +
>> +	fragment@0 {	/* first child node */
>> +
>> +		target=<phandle>;	/* phandle target of the overlay */
>> +	or
>> +		target-path="/path";	/* target path of the overlay */
>> +	or
>> +		target-alias="alias";	/* target alias of the overlay */
> 
> 
> Documentation needs to be updated. target-alias doesn't exist in the
> code anymore.

Yes, missed that.

> 
>> +		__overlay__ {
>> +			property-a;	/* add property-a to the target */
>> +			-property-b;	/* remove property-b from target */
>> +			node-a {	/* add to an existing, or create a node-a */
>> +				...
>> +			};
>> +			-node-b {	/* remove an existing node-b */
>> +				...
>> +			};
>> +		};
> 
> Instead of the '-' operator at the beginning of the property or node
> name to indicate removal (a name starting with '-' is still a valid
> name), can there be a separate __remove__ container node?
> 
> It may also be safer to have a __add__ operator that only adds the node
> if there isn't already a node or property of that name.
> 

Probably. I'll have to see how it can be done.

>> +	}
>> +	fragment@1 {	/* second child node */
>> +		...
>> +	};
>> +	/* more fragments follow */
>> +}
>> +
>> +It should be noted that the DT overlay format described is the one expected
>> +by the of_build_overlay_info() function, which is a helper function. There
>> +is nothing stopping someone coming up with his own DTS format and that will
>> +end up filling in the fields of the of_overlay_info array.
>> +
>> +Using the non-phandle based target method allows one to use a base DT which does
>> +not contain a __symbols__ now, i.e. it was not compiled with the -@ option.
> 
> sp: s/now/node/
> 

OK.

> Is it the base DT that needs the __symbols__ node, or the overlay tree?
> I had thought it was the overlay tree that contained the __symbols__
> node. Regardless, this is the first mention in this file of __symbols__.
> It would be good to discuss briefly how it works.
> 

The __symbols__ usage is explained in the resolve patch.
Since target-path has been added the base DT no longer needs a __symbols__ node.

>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 4d39c88..cfb7ff8 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -86,4 +86,14 @@ config OF_RESOLVE
>> 	  Enable OF dynamic resolution support. This allows you to
>> 	  load Device Tree object fragments are run time.
>> 
>> +config OF_OVERLAY
>> +	bool "OF overlay support"
>> +	depends on OF
>> +	select OF_DYNAMIC
>> +	select OF_DEVICE
>> +	select OF_RESOLVE
>> +	help
>> +	  OpenFirmware overlay support. Allows you to modify on runtime the
>> +	  live tree using overlays.
> 
> Should not be a user-visable option. Drivers using it should select it
> or otherwise cause it to be enabled.

Hmm. I don't know; if I let it up to drivers, platform devices will select it, in turn
making it always selected for 99.9% of the platforms out there.

Some people might not want to incur the code size penalty.

> 
>> +
>> endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index c241e79..d2a6e0d 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -11,3 +11,4 @@ obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>> obj-$(CONFIG_OF_MTD)	+= of_mtd.o
>> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
>> obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>> +obj-$(CONFIG_OF_OVERLAY) += overlay.o
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> new file mode 100644
>> index 0000000..1d4b884
>> --- /dev/null
>> +++ b/drivers/of/overlay.c
>> @@ -0,0 +1,895 @@
>> +/*
>> + * Functions for working with device tree overlays
>> + *
>> + * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
>> + * Copyright (C) 2012 Texas Instruments Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + */
>> +#undef DEBUG
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +
>> +/* protect the handlers list */
>> +static DEFINE_MUTEX(of_handler_mutex);
>> +static struct list_head of_handler_list = LIST_HEAD_INIT(of_handler_list);
>> +
>> +int of_overlay_handler_register(struct of_overlay_handler *handler)
>> +{
>> +	/* guard against bad data */
>> +	if (!handler || !handler->name || !handler->ops ||
>> +		!handler->ops->create || !handler->ops->remove)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&of_handler_mutex);
>> +	list_add_tail(&handler->list, &of_handler_list);
>> +	mutex_unlock(&of_handler_mutex);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_overlay_handler_register);
>> +
>> +void of_overlay_handler_unregister(struct of_overlay_handler *handler)
>> +{
>> +	struct of_overlay_handler *curr;
>> +
>> +	mutex_lock(&of_handler_mutex);
>> +	list_for_each_entry(curr, &of_handler_list, list) {
>> +		if (handler == curr) {
>> +			list_del(&handler->list);
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&of_handler_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(of_overlay_handler_unregister);
>> +
>> +static int handler_create(struct of_overlay_device_entry *entry, int revert)
>> +{
>> +	struct of_overlay_handler *handler;
>> +	int ret;
>> +
>> +	mutex_lock(&of_handler_mutex);
>> +	list_for_each_entry(handler, &of_handler_list, list) {
>> +		ret = (*handler->ops->create)(entry, revert);
>> +		/* ENOTSUPP means try next */
>> +		if (ret == -ENOTSUPP)
>> +			continue;
>> +		/* anything else means something happened */
>> +		break;
>> +	}
>> +	mutex_unlock(&of_handler_mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static int handler_remove(struct of_overlay_device_entry *entry, int revert)
>> +{
>> +	struct of_overlay_handler *handler;
>> +	int ret;
>> +
>> +	mutex_lock(&of_handler_mutex);
>> +	list_for_each_entry(handler, &of_handler_list, list) {
>> +		ret = (*handler->ops->remove)(entry, revert);
>> +		/* ENOTSUPP means try next */
>> +		if (ret == -ENOTSUPP)
>> +			continue;
>> +		/* anything else means something happened */
>> +		break;
>> +	}
>> +	mutex_unlock(&of_handler_mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Apply a single overlay node recursively.
>> + *
>> + * Property or node names that start with '-' signal that
>> + * the property/node is to be removed.
>> + *
>> + * All the property notifiers are appropriately called.
>> + * Note that the in case of an error the target node is left
>> + * in a inconsistent state. Error recovery should be performed
>> + * by recording the modification using the of notifiers.
>> + */
>> +static int of_overlay_apply_one(struct device_node *target,
>> +		const struct device_node *overlay)
>> +{
>> +	const char *pname, *cname;
>> +	struct device_node *child, *tchild;
>> +	struct property *prop, *propn, *tprop;
>> +	int remove;
>> +	char *full_name;
>> +	const char *suffix;
>> +	int ret;
>> +
>> +	/* sanity checks */
>> +	if (target == NULL || overlay == NULL)
>> +		return -EINVAL;
>> +
>> +	for_each_property_of_node(overlay, prop) {
>> +
>> +		/* don't touch, 'name' */
>> +		if (of_prop_cmp(prop->name, "name") == 0)
>> +			continue;
>> +
>> +		/* default is add */
>> +		remove = 0;
>> +		pname = prop->name;
>> +		if (*pname == '-') {	/* skip, - notes removal */
>> +			pname++;
>> +			remove = 1;
>> +			propn = NULL;
>> +		} else {
>> +			propn = __of_copy_property(prop, GFP_KERNEL,
>> +					OF_PROP_ALLOCALL);
>> +			if (propn == NULL)
>> +				return -ENOMEM;
>> +		}
>> +
>> +		tprop = of_find_property(target, pname, NULL);
>> +
>> +		/* found? */
>> +		if (tprop != NULL) {
>> +			if (propn != NULL)
>> +				ret = of_update_property(target, propn);
>> +			else
>> +				ret = of_remove_property(target, tprop);
>> +		} else {
>> +			if (propn != NULL)
>> +				ret = of_add_property(target, propn);
>> +			else
>> +				ret = 0;
>> +		}
>> +		if (ret != 0)
>> +			return ret;
>> +	}
>> +
>> +	__for_each_child_of_node(overlay, child) {
>> +
>> +		/* default is add */
>> +		remove = 0;
>> +		cname = child->name;
>> +		if (*cname == '-') {	/* skip, - notes removal */
>> +			cname++;
>> +			remove = 1;
>> +		}
>> +
>> +		/* special case for nodes with a suffix */
>> +		suffix = strrchr(child->full_name, '@');
>> +		if (suffix != NULL) {
>> +			cname = kbasename(child->full_name);
>> +			WARN_ON(cname == NULL);	/* sanity check */
>> +			if (cname == NULL)
>> +				continue;
>> +			if (*cname == '-')
>> +				cname++;
>> +		}
>> +
>> +		tchild = of_get_child_by_name(target, cname);
>> +		if (tchild != NULL) {
>> +
>> +			if (!remove) {
>> +
>> +				/* apply overlay recursively */
>> +				ret = of_overlay_apply_one(tchild, child);
>> +				of_node_put(tchild);
>> +
>> +				if (ret != 0)
>> +					return ret;
>> +
>> +			} else {
>> +
>> +				ret = of_detach_node(tchild);
>> +				of_node_put(tchild);
>> +			}
>> +
>> +		} else {
>> +
>> +			if (!remove) {
>> +				full_name = kasprintf(GFP_KERNEL, "%s/%s",
>> +						target->full_name, cname);
>> +				if (full_name == NULL)
>> +					return -ENOMEM;
>> +
>> +				/* create empty tree as a target */
>> +				tchild = __of_create_empty_node(cname,
>> +						child->type, full_name,
>> +						child->phandle, GFP_KERNEL,
>> +						OF_NODE_ALLOCALL);
>> +
>> +				/* free either way */
>> +				kfree(full_name);
>> +
>> +				if (tchild == NULL)
>> +					return -ENOMEM;
>> +
>> +				/* point to parent */
>> +				tchild->parent = target;
>> +
>> +				ret = of_attach_node(tchild);
>> +				if (ret != 0)
>> +					return ret;
>> +
>> +				/* apply the overlay */
>> +				ret = of_overlay_apply_one(tchild, child);
>> +				if (ret != 0) {
>> +					__of_free_tree(tchild);
>> +					return ret;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Lookup an overlay device entry
>> + */
>> +struct of_overlay_device_entry *of_overlay_device_entry_lookup(
>> +		struct of_overlay_info *ovinfo, struct device_node *node)
>> +{
>> +	struct of_overlay_device_entry *de;
>> +
>> +	/* no need for locks, we'de under the ovinfo->lock */
>> +	list_for_each_entry(de, &ovinfo->de_list, node) {
>> +		if (de->np == node)
>> +			return de;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * Add an overlay log entry
>> + */
>> +static int of_overlay_log_entry_entry_add(struct of_overlay_info *ovinfo,
>> +		unsigned long action, struct device_node *dn,
>> +		struct property *prop)
>> +{
>> +	struct of_overlay_log_entry *le;
>> +
>> +	/* check */
>> +	if (ovinfo == NULL || dn == NULL)
>> +		return -EINVAL;
>> +
>> +	le = kzalloc(sizeof(*le), GFP_KERNEL);
>> +	if (le == NULL) {
>> +		pr_err("%s: Failed to allocate\n", __func__);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* get a reference to the node */
>> +	le->action = action;
>> +	le->np = of_node_get(dn);
>> +	le->prop = prop;
>> +
>> +	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
>> +		le->old_prop = of_find_property(dn, prop->name, NULL);
>> +
>> +	list_add_tail(&le->node, &ovinfo->le_list);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Add an overlay device entry
>> + */
>> +static void of_overlay_device_entry_entry_add(struct of_overlay_info *ovinfo,
>> +		struct device_node *node,
>> +		int prevstate, int state)
>> +{
>> +	struct of_overlay_device_entry *de;
>> +	int fresh;
>> +
>> +	/* check */
>> +	if (ovinfo == NULL)
>> +		return;
>> +
>> +	fresh = 0;
>> +	de = of_overlay_device_entry_lookup(ovinfo, node);
>> +	if (de == NULL) {
>> +		de = kzalloc(sizeof(*de), GFP_KERNEL);
>> +		if (de == NULL) {
>> +			pr_err("%s: Failed to allocate\n", __func__);
>> +			return;
>> +		}
>> +		fresh = 1;
>> +		de->prevstate = -1;
>> +	}
>> +
>> +	if (de->np == NULL)
>> +		de->np = of_node_get(node);
>> +	if (fresh)
>> +		de->prevstate = prevstate;
>> +	de->state = state;
>> +
>> +	if (fresh)
>> +		list_add_tail(&de->node, &ovinfo->de_list);
>> +}
>> +
>> +/*
>> + * Overlay OF notifier
>> + *
>> + * Called every time there's a property/node modification
>> + * Every modification causes a log entry addition, while
>> + * any modification that causes a node's state to change
>> + * from/to disabled to/from enabled causes a device entry
>> + * addition.
> 
> I'm not convinced on the filter fo when notifications are sent. I think
> it is still appropriate to send notifications on any node or property
> change, and that it should be merged with the existing notifier code.
> 
> It seems a very odd structure that the existing notifier is being used
> to call into this notifier

Yep; there is some history behind that. The original implementation didn't
use notifiers at all, it used it's own hook in the DT code. Then it was
suggested to use notifiers, and...

>> + */
>> +static int of_overlay_notify(struct notifier_block *nb,
>> +				unsigned long action, void *arg)
>> +{
>> +	struct of_overlay_info *ovinfo;
>> +	struct device_node *node;
>> +	struct property *prop, *sprop, *cprop;
>> +	struct of_prop_reconfig *pr;
>> +	struct device_node *tnode;
>> +	int depth;
>> +	int prevstate, state;
>> +	int err = 0;
>> +
>> +	ovinfo = container_of(nb, struct of_overlay_info, notifier);
>> +
>> +	/* prep vars */
>> +	switch (action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		node = arg;
>> +		if (node == NULL)
>> +			return notifier_from_errno(-EINVAL);
>> +		prop = NULL;
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		pr = arg;
>> +		if (pr == NULL)
>> +			return notifier_from_errno(-EINVAL);
>> +		node = pr->dn;
>> +		if (node == NULL)
>> +			return notifier_from_errno(-EINVAL);
>> +		prop = pr->prop;
>> +		if (prop == NULL)
>> +			return notifier_from_errno(-EINVAL);
> 
> None of the above error conditions will ever be true. The dt notifier code
> already ensures that it only issues notifications when it has pointers
> to the things that are being modified.

OK

> 
>> +		break;
>> +	default:
>> +		return notifier_from_errno(0);
>> +	}
>> +
>> +	/* add to the log */
>> +	err = of_overlay_log_entry_entry_add(ovinfo, action, node, prop);
>> +	if (err != 0)
>> +		return notifier_from_errno(err);
> 
> It seems odd to add the log entry from within a notifier callback rather
> than handling it directly in the code that initiated the modification.
> How do you know that the modification entry actually came from the batch
> that is being applied? If to overlays were progressing in parallel then
> it will all get interleaved and will be completely broken.

At the moment, and by design a single overlay is being applied each time.

> 
>> +
>> +	/* come up with the device entry (if any) */
>> +	state = 0;
>> +	prevstate = 0;
>> +
>> +	/* determine the state the node will end up */
>> +	switch (action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		/* we demand that a compatible node is present */
>> +		state = of_find_property(node, "compatible", NULL) &&
>> +			of_device_is_available(node);
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		prevstate = of_find_property(node, "compatible", NULL) &&
>> +			of_device_is_available(node);
>> +		state = 0;
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		/* either one cause a change in state */
>> +		if (strcmp(prop->name, "status") != 0 &&
>> +				strcmp(prop->name, "compatible") != 0)
>> +			return notifier_from_errno(0);
>> +
>> +		if (strcmp(prop->name, "status") == 0) {
>> +			/* status */
>> +			cprop = of_find_property(node, "compatible", NULL);
>> +			sprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
>> +				prop : NULL;
>> +		} else {
>> +			/* compatible */
>> +			sprop = of_find_property(node, "status", NULL);
>> +			cprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
>> +				prop : NULL;
>> +		}
>> +
>> +		prevstate = of_find_property(node, "compatible", NULL) &&
>> +			of_device_is_available(node);
>> +		state = cprop && cprop->length > 0 &&
>> +			    (!sprop || (sprop->length > 0 &&
>> +				(strcmp(sprop->value, "okay") == 0 ||
>> +				 strcmp(sprop->value, "ok") == 0)));
>> +		break;
>> +
>> +	default:
>> +		return notifier_from_errno(0);
>> +	}
>> +
>> +	/* find depth */
>> +	depth = 1;
>> +	tnode = node;
>> +	while (tnode != NULL && tnode != ovinfo->target) {
>> +		tnode = tnode->parent;
>> +		depth++;
>> +	}
>> +
>> +	/* respect overlay's maximum depth */
>> +	if (ovinfo->device_depth != 0 && depth > ovinfo->device_depth) {
>> +		pr_debug("OF: skipping device creation for node=%s depth=%d\n",
>> +				node->name, depth);
>> +		goto out;
>> +	}
> 
> What is the purpose of the maximum depth?
> 

This is a remnant that's no longer required now that we have bus notifiers.
Will remove.

>> +
>> +	of_overlay_device_entry_entry_add(ovinfo, node, prevstate, state);
>> +out:
>> +
>> +	return notifier_from_errno(err);
>> +}
>> +
>> +/*
>> + * Prepare for the overlay, for now it just registers the
>> + * notifier.
>> + */
>> +static int of_overlay_prep_one(struct of_overlay_info *ovinfo)
>> +{
>> +	int err;
>> +
>> +	err = of_reconfig_notifier_register(&ovinfo->notifier);
>> +	if (err != 0) {
>> +		pr_err("%s: failed to register notifier for '%s'\n",
>> +			__func__, ovinfo->target->full_name);
>> +		return err;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int of_overlay_device_entry_change(struct of_overlay_info *ovinfo,
>> +		struct of_overlay_device_entry *de, int revert)
>> +{
>> +	int state;
>> +	int ret;
>> +
>> +	state = !!de->state ^ !!revert;
>> +
>> +	if (state)
>> +		ret = handler_create(de, revert);
>> +	else
>> +		ret = handler_remove(de, revert);
>> +
>> +	if (ret != 0 && ret != -ENOTSUPP)
>> +		pr_warn("%s: Failed to %s device "
>> +				"for node '%s'\n", __func__,
>> +				state ? "create" : "remove",
>> +				de->np->full_name);
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Revert one overlay
>> + * Either due to an error, or due to normal overlay removal.
>> + * Using the log entries, we revert any change to the live tree.
>> + * In the same manner, using the device entries we enable/disable
>> + * the devices appropriately.
>> + */
>> +static void of_overlay_revert_one(struct of_overlay_info *ovinfo)
>> +{
>> +	struct of_overlay_device_entry *de, *den;
>> +	struct of_overlay_log_entry *le, *len;
>> +	struct property *prop, **propp;
>> +	struct device_node *np;
>> +	int ret;
>> +	unsigned long flags;
>> +
>> +	if (!ovinfo || !ovinfo->target || !ovinfo->overlay)
>> +		return;
>> +
>> +	pr_debug("%s: Reverting overlay on '%s'\n", __func__,
>> +			ovinfo->target->full_name);
>> +
>> +	/* overlay applied correctly, now create/destroy pdevs */
>> +	list_for_each_entry_safe_reverse(de, den, &ovinfo->de_list, node) {
>> +		of_overlay_device_entry_change(ovinfo, de, 1);
>> +		of_node_put(de->np);
>> +		list_del(&de->node);
>> +		kfree(de);
>> +	}
>> +
>> +	list_for_each_entry_safe_reverse(le, len, &ovinfo->le_list, node) {
>> +
>> +		/* get node and immediately put */
>> +		np = le->np;
>> +		of_node_put(le->np);
>> +		le->np = NULL;
>> +
>> +		ret = 0;
>> +		switch (le->action) {
>> +		case OF_RECONFIG_ATTACH_NODE:
>> +			pr_debug("Reverting ATTACH_NODE %s\n",
>> +					np->full_name);
>> +			ret = of_detach_node(np);
>> +			break;
>> +
>> +		case OF_RECONFIG_DETACH_NODE:
>> +			pr_debug("Reverting DETACH_NODE %s\n",
>> +					np->full_name);
>> +			ret = of_attach_node(np);
>> +			break;
>> +
>> +		case OF_RECONFIG_ADD_PROPERTY:
>> +			pr_debug("Reverting ADD_PROPERTY %s %s\n",
>> +					np->full_name, le->prop->name);
>> +			ret = of_remove_property(np, le->prop);
>> +			break;
>> +
>> +		case OF_RECONFIG_REMOVE_PROPERTY:
>> +		case OF_RECONFIG_UPDATE_PROPERTY:
>> +
>> +			pr_debug("Reverting %s_PROPERTY %s %s\n",
>> +				le->action == OF_RECONFIG_REMOVE_PROPERTY ?
>> +					"REMOVE" : "UPDATE",
>> +					np->full_name, le->prop->name);
>> +
>> +			/* property is possibly on deadprops (avoid alloc) */
>> +			raw_spin_lock_irqsave(&devtree_lock, flags);
>> +			prop = le->action == OF_RECONFIG_REMOVE_PROPERTY ?
>> +				le->prop : le->old_prop;
>> +			propp = &np->deadprops;
>> +			while (*propp != NULL) {
>> +				if (*propp == prop)
>> +					break;
>> +				propp = &(*propp)->next;
>> +			}
>> +			if (*propp != NULL) {
>> +				/* remove it from deadprops */
>> +				(*propp)->next = prop->next;
>> +				raw_spin_unlock_irqrestore(&devtree_lock,
>> +						flags);
>> +			} else {
>> +				raw_spin_unlock_irqrestore(&devtree_lock,
>> +						flags);
>> +				/* not found, just make a copy */
>> +				prop = __of_copy_property(prop, GFP_KERNEL,
>> +						OF_PROP_ALLOCALL);
>> +				if (prop == NULL) {
>> +					pr_err("%s: Failed to copy property\n",
>> +							__func__);
>> +					break;
>> +				}
>> +			}
>> +
>> +			if (le->action == OF_RECONFIG_REMOVE_PROPERTY)
>> +				ret = of_add_property(np, prop);
>> +			else
>> +				ret = of_update_property(np, prop);
>> +			break;
>> +
>> +		default:
>> +			/* nothing */
>> +			break;
>> +		}
>> +
>> +		if (ret != 0)
>> +			pr_err("%s: revert on node %s Failed!\n",
>> +					__func__, np->full_name);
>> +
>> +		list_del(&le->node);
>> +
>> +		kfree(le);
>> +	}
>> +}
>> +
>> +/*
>> + * Perform the post overlay work.
>> + *
>> + * We unregister the notifier, and in the case on an error we
>> + * revert the overlay.
>> + * If the overlay applied correctly, we iterate over the device entries
>> + * and create/destroy the devices appropriately.
>> + */
>> +static int of_overlay_post_one(struct of_overlay_info *ovinfo, int err)
>> +{
>> +	struct of_overlay_device_entry *de, *den;
>> +
>> +	of_reconfig_notifier_unregister(&ovinfo->notifier);
>> +
>> +	if (err != 0) {
>> +		/* revert this (possible partially applied) overlay */
>> +		of_overlay_revert_one(ovinfo);
>> +		return 0;
>> +	}
>> +
>> +	/* overlay applied correctly, now create/destroy pdevs */
>> +	list_for_each_entry_safe(de, den, &ovinfo->de_list, node) {
>> +
>> +		/* no state change? just remove this entry */
>> +		if (de->prevstate == de->state) {
>> +			of_node_put(de->np);
>> +			list_del(&de->node);
>> +			kfree(de);
>> +			continue;
>> +		}
>> +
>> +		of_overlay_device_entry_change(ovinfo, de, 0);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to apply
>> + *
>> + * Applies the overlays given, while handling all error conditions
>> + * appropriately. Either the operation succeeds, or if it fails the
>> + * live tree is reverted to the state before the attempt.
>> + * Returns 0, or an error if the overlay attempt failed.
>> + */
>> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab)
> 
> overlay fragments don't live in isolation. There needs to be a top level
> of_overlay container structure that holds the list/array of of_overlay_info
> structures and can hold any other data that may be required for
> reverting the overlay, such as the modification log.

OK

> 
>> +{
>> +	struct of_overlay_info *ovinfo;
>> +	int i, err;
>> +
> 
> This entire function needs to be protected by a global mutex. At no
> point in time should multiple overlays get processed concurrently.
> 
> I see that each individual of_overlay_info structure has a mutex, but I
> don't understand why the locking it that granular. The entire overlay
> should be a single block and only one overlay should ever be processed
> at a time. What is the reason for locking per of_overlay_info?

To protect against simultaneous removal, but you're right it need to be 
batched to a higher level structure.

> 
>> +	if (!ovinfo_tab)
>> +		return -EINVAL;
>> +
>> +	/* first we apply the overlays atomically */
>> +	for (i = 0; i < count; i++) {
>> +
>> +		ovinfo = &ovinfo_tab[i];
>> +
>> +		mutex_lock(&ovinfo->lock);
>> +
>> +		err = of_overlay_prep_one(ovinfo);
>> +		if (err == 0)
>> +			err = of_overlay_apply_one(ovinfo->target,
>> +					ovinfo->overlay);
>> +		of_overlay_post_one(ovinfo, err);
>> +
>> +		mutex_unlock(&ovinfo->lock);
>> +
>> +		if (err != 0) {
>> +			pr_err("%s: overlay failed '%s'\n",
>> +				__func__, ovinfo->target->full_name);
>> +			goto err_fail;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +err_fail:
>> +	while (--i >= 0) {
>> +		ovinfo = &ovinfo_tab[i];
>> +
>> +		mutex_lock(&ovinfo->lock);
>> +		of_overlay_revert_one(ovinfo);
>> +		mutex_unlock(&ovinfo->lock);
>> +	}
>> +
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(of_overlay);
>> +
>> +/**
>> + * of_overlay_revert	- Revert a previously applied overlay
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to apply
>> + *
>> + * Revert a previous overlay. The state of the live tree
>> + * is reverted to the one before the overlay.
>> + * Returns 0, or an error if the overlay table is not given.
>> + */
>> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
>> +{
>> +	struct of_overlay_info *ovinfo;
>> +	int i;
>> +
>> +	if (!ovinfo_tab)
>> +		return -EINVAL;
>> +
>> +	/* revert the overlays in reverse */
>> +	for (i = count - 1; i >= 0; i--) {
>> +
>> +		ovinfo = &ovinfo_tab[i];
>> +
>> +		mutex_lock(&ovinfo->lock);
>> +		of_overlay_revert_one(ovinfo);
>> +		mutex_unlock(&ovinfo->lock);
>> +
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_overlay_revert);
>> +
>> +/**
>> + * of_init_overlay_info	- Initialize a single of_overlay_info structure
>> + * @ovinfo:	Pointer to the overlay info structure to initialize
>> + *
>> + * Initialize a single overlay info structure.
>> + */
>> +void of_init_overlay_info(struct of_overlay_info *ovinfo)
>> +{
>> +	memset(ovinfo, 0, sizeof(*ovinfo));
>> +	mutex_init(&ovinfo->lock);
>> +	INIT_LIST_HEAD(&ovinfo->de_list);
>> +	INIT_LIST_HEAD(&ovinfo->le_list);
>> +
>> +	ovinfo->notifier.notifier_call = of_overlay_notify;
>> +}
>> +
>> +/*
>> + * Find the target node using a number of different strategies
>> + * in order of preference
>> + *
>> + * "target" property containing the phandle of the target
>> + * "target-path" property containing the path of the target
>> + *
>> + */
>> +struct device_node *find_target_node(struct device_node *info_node)
>> +{
>> +	const char *path;
>> +	u32 val;
>> +	int ret;
>> +
>> +	/* first try to go by using the target as a phandle */
>> +	ret = of_property_read_u32(info_node, "target", &val);
>> +	if (ret == 0)
>> +		return of_find_node_by_phandle(val);
>> +
>> +	/* now try to locate by path */
>> +	ret = of_property_read_string(info_node, "target-path", &path);
>> +	if (ret == 0)
>> +		return of_find_node_by_path(path);
>> +
>> +	pr_err("%s: Failed to find target for node %p (%s)\n", __func__,
>> +			info_node, info_node->name);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * of_fill_overlay_info	- Fill an overlay info structure
>> + * @info_node:	Device node containing the overlay
>> + * @ovinfo:	Pointer to the overlay info structure to fill
>> + *
>> + * Fills an overlay info structure with the overlay information
>> + * from a device node. This device node must have a target property
>> + * which contains a phandle of the overlay target node, and an
>> + * __overlay__ child node which has the overlay contents.
>> + * Both ovinfo->target & ovinfo->overlay have their references taken.
>> + *
>> + * Returns 0 on success, or a negative error value.
>> + */
>> +int of_fill_overlay_info(struct device_node *info_node,
>> +		struct of_overlay_info *ovinfo)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	if (!info_node || !ovinfo)
>> +		return -EINVAL;
>> +
>> +	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
>> +	if (ovinfo->overlay == NULL)
>> +		goto err_fail;
>> +
>> +	ovinfo->target = find_target_node(info_node);
>> +	if (ovinfo->target == NULL)
>> +		goto err_fail;
>> +
>> +	ret = of_property_read_u32(info_node, "depth", &val);
>> +	if (ret == 0)
>> +		ovinfo->device_depth = val;
>> +	else
>> +		ovinfo->device_depth = 0;
>> +
>> +	return 0;
>> +
>> +err_fail:
>> +	of_node_put(ovinfo->target);
>> +	of_node_put(ovinfo->overlay);
>> +
>> +	memset(ovinfo, 0, sizeof(*ovinfo));
>> +	return -EINVAL;
>> +}
>> +
>> +/**
>> + * of_build_overlay_info	- Build an overlay info array
>> + * @tree:	Device node containing all the overlays
>> + * @cntp:	Pointer to where the overlay info count will be help
>> + * @ovinfop:	Pointer to the pointer of an overlay info structure.
>> + *
>> + * Helper function that given a tree containing overlay information,
>> + * allocates and builds an overlay info array containing it, ready
>> + * for use using of_overlay.
>> + *
>> + * Returns 0 on success with the @cntp @ovinfop pointers valid,
>> + * while on error a negative error value is returned.
>> + */
>> +int of_build_overlay_info(struct device_node *tree,
>> +		int *cntp, struct of_overlay_info **ovinfop)
>> +{
>> +	struct device_node *node;
>> +	struct of_overlay_info *ovinfo;
>> +	int cnt, err;
>> +
>> +	if (tree == NULL || cntp == NULL || ovinfop == NULL)
>> +		return -EINVAL;
>> +
>> +	/* worst case; every child is a node */
>> +	cnt = 0;
>> +	for_each_child_of_node(tree, node)
>> +		cnt++;
>> +
>> +	ovinfo = kzalloc(cnt * sizeof(*ovinfo), GFP_KERNEL);
>> +	if (ovinfo == NULL)
>> +		return -ENOMEM;
>> +
>> +	cnt = 0;
>> +	for_each_child_of_node(tree, node) {
>> +
>> +		of_init_overlay_info(&ovinfo[cnt]);
>> +		err = of_fill_overlay_info(node, &ovinfo[cnt]);
>> +		if (err == 0)
>> +			cnt++;
>> +	}
>> +
>> +	/* if nothing filled, return error */
>> +	if (cnt == 0) {
>> +		kfree(ovinfo);
>> +		return -ENODEV;
>> +	}
>> +
>> +	*cntp = cnt;
>> +	*ovinfop = ovinfo;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_build_overlay_info);
>> +
>> +/**
>> + * of_free_overlay_info	- Free an overlay info array
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to free
>> + *
>> + * Releases the memory of a previously allocate ovinfo array
>> + * by of_build_overlay_info.
>> + * Returns 0, or an error if the arguments are bogus.
>> + */
>> +int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab)
>> +{
>> +	struct of_overlay_info *ovinfo;
>> +	int i;
>> +
>> +	if (!ovinfo_tab || count < 0)
>> +		return -EINVAL;
>> +
>> +	/* do it in reverse */
>> +	for (i = count - 1; i >= 0; i--) {
>> +		ovinfo = &ovinfo_tab[i];
>> +
>> +		of_node_put(ovinfo->target);
>> +		of_node_put(ovinfo->overlay);
>> +	}
>> +	kfree(ovinfo_tab);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_free_overlay_info);
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 3edb9b9..358f984 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -23,6 +23,7 @@
>> #include <linux/spinlock.h>
>> #include <linux/topology.h>
>> #include <linux/notifier.h>
>> +#include <linux/list.h>
>> 
>> #include <asm/byteorder.h>
>> #include <asm/errno.h>
>> @@ -828,4 +829,156 @@ static inline int of_resolve(struct device_node *resolve)
>> 
>> #endif
>> 
>> +/**
>> + * Overlay support
>> + */
>> +
>> +/**
>> + * struct of_overlay_log_entry	- Holds a DT log entry
>> + * @node:	list_head for the log list
>> + * @action:	notifier action
>> + * @np:		pointer to the device node affected
>> + * @prop:	pointer to the property affected
>> + * @old_prop:	hold a pointer to the original property
>> + *
>> + * Every modification of the device tree during application of the
>> + * overlay is held in a list of of_overlay_log_entry structures.
>> + * That way we can recover from a partial application, or we can
>> + * revert the overlay properly.
>> + */
>> +struct of_overlay_log_entry {
>> +	struct list_head node;
>> +	unsigned long action;
>> +	struct device_node *np;
>> +	struct property *prop;
>> +	struct property *old_prop;
>> +};
>> +
>> +struct of_overlay_device_entry;
>> +
>> +/**
>> + * struct of_overlay_handler_ops	- Overlay device handler ops
>> + * @create:	method to be called to create a device
>> + * @remove:	method to be called to destroy a device
>> + *
>> + * Both these functions return 0 on success, ENOTSUPP if the
>> + * device entry does not match, and an error code otherwise.
>> + */
>> +struct of_overlay_handler_ops {
>> +	int (*create)(struct of_overlay_device_entry *entry, int revert);
>> +	int (*remove)(struct of_overlay_device_entry *entry, int revert);
>> +};
>> +
>> +/**
>> + * struct of_overlay_handler	- Overlay device handler
>> + * @list:	list links for all handlers
>> + * @name:	name of this handler
>> + * @ops:	ops member functions
>> + *
>> + * The handler is registered by each bus that supports
>> + * dynamic creation/removal of devices
>> + */
>> +struct of_overlay_handler {
>> +	struct list_head list;
>> +	const char *name;
>> +	const struct of_overlay_handler_ops *ops;
>> +};
>> +
>> +/**
>> + * struct of_overlay_device_entry	- Holds an overlay device entry
>> + * @node:	list_head for the device list
>> + * @np:		device node pointer to the device node affected
>> + * @state:	new device state
>> + * @prevstate:	previous device state
>> + * @priv:	private pointer for use by bus handlers
>> + *
>> + * When the overlay results in a device node's state to change this
>> + * fact is recorded in a list of device entries. After the overlay
>> + * is applied we can create/destroy the devices according
>> + * to the new state of the live tree.
>> + */
>> +struct of_overlay_device_entry {
>> +	struct list_head node;
>> +	struct device_node *np;
>> +	int prevstate;
>> +	int state;
>> +	void *priv;
>> +};
>> +
>> +/**
>> + * struct of_overlay_info	- Holds a single overlay info
>> + * @target:	target of the overlay operation
>> + * @overlay:	pointer to the overlay contents node
>> + * @lock:	Lock to hold when accessing the lists
>> + * @le_list:	List of the overlay logs
>> + * @de_list:	List of the overlay records
>> + * @notifier:	of reconfiguration notifier
>> + *
>> + * Holds a single overlay state, including all the overlay logs &
>> + * records.
>> + */
>> +struct of_overlay_info {
>> +	struct device_node *target;
>> +	struct device_node *overlay;
>> +	struct mutex lock;
>> +	struct list_head le_list;
>> +	struct list_head de_list;
>> +	struct notifier_block notifier;
>> +	int device_depth;
>> +};
>> +
>> +#ifdef CONFIG_OF_OVERLAY
>> +
>> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
>> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
>> +
>> +int of_fill_overlay_info(struct device_node *info_node,
>> +		struct of_overlay_info *ovinfo);
>> +int of_build_overlay_info(struct device_node *tree,
>> +		int *cntp, struct of_overlay_info **ovinfop);
>> +int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo);
>> +
>> +int of_overlay_handler_register(struct of_overlay_handler *handler);
>> +void of_overlay_handler_unregister(struct of_overlay_handler *handler);
>> +
>> +#else
>> +
>> +static inline int of_overlay(int count, struct of_overlay_info *ovinfo_tab)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +static inline int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +static inline int of_fill_overlay_info(struct device_node *info_node,
>> +		struct of_overlay_info *ovinfo)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +static inline int of_build_overlay_info(struct device_node *tree,
>> +		int *cntp, struct of_overlay_info **ovinfop)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +static inline int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +static inline int of_overlay_handler_register(struct of_overlay_handler *handler)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void of_overlay_handler_unregister(struct of_overlay_handler *handler)
>> +{
>> +}
>> +
>> +#endif
>> +
>> #endif /* _LINUX_OF_H */
>> -- 
>> 1.7.12
>> 
> 

Regards

-- Pantelis

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Michael,

On May 14, 2014, at 5:11 AM, Michael Stickel wrote:

> Hi Grant,
> 
> Am 14.05.2014 12:08, schrieb Grant Likely:
>> More generally I am concerned about whether or not overlays
>> will introduce corner cases that can never be handled correctly,
>> particularly in how multiple overlays will get handled. I want to see
>> very clear rules on what happens when multiple overlays are applied, and
>> then removed again. Is it possible to remove overlays out of order? If
>> so, what are the conditions that would not be allowed?
> 
> Yes, it is possible that an overlay depends on another.
> 
> The problem is not, that an overlay is removed other overlays depend on,
> but that nodes of an overlay may depend on the to-be-removed overlay and
> the resulting devicetree can become inconsistent.
> 
> 
> I have an SPI Bus with two slaves. The second slave is used only on one
> of our boards. That is why we split the overlays the following way:
> 
> xxxx_spi1.dts:
>  Pinmux for SPI-Bus and activation of spi-controller.
>  Pinmux for CS0 and definition of first slave.
> 
> xxxx_spi1_cs1:
>  Pinmux for CS1 and definition of second slave.
> 
> When the overlay for the bus is removed, the overlays for the second
> slave does not make any sense anymore.
> 
> It is even worse in a scenario we have with a test board.
> One of the slaves is an spi-io-controller with a few bitbanging i2c
> masters. In an extreme case, each component is defined in a separate
> overlay and only the overlay with the master is removed. I know, that
> this is completely sick. The devices are removed cleanly because of the
> device dependency.
> 

Well, shouldn't you be reverting the overlays in reverse sequence?

As I see it, when proper subtree tracking is implemented this use case 
(of removing #0 before #1) will not be allowed.

What is your ideal scenario for this use case?

> Michael
> 

Regards

-- Pantelis

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Guenter,

On May 14, 2014, at 6:18 AM, Guenter Roeck wrote:

> On 05/14/2014 06:03 AM, Geert Uytterhoeven wrote:
>> On Wed, May 14, 2014 at 12:08 PM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>>> +config OF_OVERLAY
>>>> +     bool "OF overlay support"
>>>> +     depends on OF
>>>> +     select OF_DYNAMIC
>>>> +     select OF_DEVICE
>>>> +     select OF_RESOLVE
>>>> +     help
>>>> +       OpenFirmware overlay support. Allows you to modify on runtime the
>>>> +       live tree using overlays.
>>> 
>>> Should not be a user-visable option. Drivers using it should select it
>> 
>> Why not? It's up to the (final) user to use it, or not.
>> 
>>> or otherwise cause it to be enabled.
>> 
>> Why should this be driver-specific? Shouldn't this just work with all drivers?
>> 
> 
> It does once enabled.
> 
> I think what Grant refers to is that there has to be a driver which implements
> the actual overlay insertion and removal (ie calls the new API functions),
> and that the Kconfig option for this driver should select OF_OVERLAY
> instead of depending on it.
> 

That makes sense.

> Guenter
> 

Regards

-- Pantelis

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Geert Uytterhoeven]

Hi Pantelis,

On Thu, May 15, 2014 at 9:12 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
>> We also need to think about kexec. Kexec works by sucking the live tree
>> out of the kernel and creating a .dtb from it to pass to the new kernel.
>> What will the rules be when kexecing? Do all the overlays need to be
>> removed, or does the kernel get the tree with all the overlays applied
>> (in which case none of the overlays can be removed on the other side of
>> kexec).
>
> We can add a sysfs attribute that configures whether overlays are reverted before
> kexec or not. I can't really tell which is the correct option, so let's allow the
> policy up to user-space.

Kexec'ing into a new kernel doesn't change the hardware, so IMHO the
in-kernel DT should not change.

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/8] OF: Introduce DT overlay support.

[Grant Likely]

Hi Pantelis,

Thanks for writing this up. A few responses below...

On Thu, 15 May 2014 00:12:17 -0700, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> On May 14, 2014, at 3:08 AM, Grant Likely wrote:
> > On Fri,  4 Apr 2014 15:43:55 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> > The notification infrastructure bothers me. It duplicates the
> > notification that the core DT code already performs. I do understand
> > that the current notifications don't do what you need them to because
> > you need it all deferred until the complete set of batch changes are
> > applied. However, instead of creating a new infrastructure, the existing
> > notifier should be reworked to be batch-change aware.
> > 
> 
> If I understood correctly, you're asking of rolling in this per-bus notifier
> mechanism in the standard DT notifier infrastructure already in place.
> I can't be absolutely sure about the details right now, but seems possible.
> 
> I don't know if the kernel notifier framework will be unmodified, but I hope so.

It should be. It will need to be the dt code that buffers up the
notification events to be played out after the batch of changes has been
applied. That shouldn't have any impact on core notifier framework.

[...]
> > Is it the base DT that needs the __symbols__ node, or the overlay tree?
> > I had thought it was the overlay tree that contained the __symbols__
> > node. Regardless, this is the first mention in this file of __symbols__.
> > It would be good to discuss briefly how it works.
> > 
> 
> The __symbols__ usage is explained in the resolve patch.
> Since target-path has been added the base DT no longer needs a __symbols__ node.

Can the target-phandle method be removed entirely then?

> >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >> index 4d39c88..cfb7ff8 100644
> >> --- a/drivers/of/Kconfig
> >> +++ b/drivers/of/Kconfig
> >> @@ -86,4 +86,14 @@ config OF_RESOLVE
> >> 	  Enable OF dynamic resolution support. This allows you to
> >> 	  load Device Tree object fragments are run time.
> >> 
> >> +config OF_OVERLAY
> >> +	bool "OF overlay support"
> >> +	depends on OF
> >> +	select OF_DYNAMIC
> >> +	select OF_DEVICE
> >> +	select OF_RESOLVE
> >> +	help
> >> +	  OpenFirmware overlay support. Allows you to modify on runtime the
> >> +	  live tree using overlays.
> > 
> > Should not be a user-visable option. Drivers using it should select it
> > or otherwise cause it to be enabled.
> 
> Hmm. I don't know; if I let it up to drivers, platform devices will select it, in turn
> making it always selected for 99.9% of the platforms out there.
> 
> Some people might not want to incur the code size penalty.

The only code ever selecting this function would be code that actually
calls the overlay functions.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Thu, 15 May 2014 09:20:24 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Pantelis,
> 
> On Thu, May 15, 2014 at 9:12 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
> >> We also need to think about kexec. Kexec works by sucking the live tree
> >> out of the kernel and creating a .dtb from it to pass to the new kernel.
> >> What will the rules be when kexecing? Do all the overlays need to be
> >> removed, or does the kernel get the tree with all the overlays applied
> >> (in which case none of the overlays can be removed on the other side of
> >> kexec).
> >
> > We can add a sysfs attribute that configures whether overlays are reverted before
> > kexec or not. I can't really tell which is the correct option, so let's allow the
> > policy up to user-space.
> 
> Kexec'ing into a new kernel doesn't change the hardware, so IMHO the
> in-kernel DT should not change.

Conceptually though, if overlays are applied then the new kernel has a
different tree from the old one. If the overlay is hotpluggable, then
after the kexec, the new kernel would no longer be able to unplug the
overlay if it uses the current state of the tree instead of pulling the
overlays off first.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Geert Uytterhoeven]

Hi Grant,

On Fri, May 16, 2014 at 12:58 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Thu, 15 May 2014 09:20:24 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, May 15, 2014 at 9:12 AM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>> >> We also need to think about kexec. Kexec works by sucking the live tree
>> >> out of the kernel and creating a .dtb from it to pass to the new kernel.
>> >> What will the rules be when kexecing? Do all the overlays need to be
>> >> removed, or does the kernel get the tree with all the overlays applied
>> >> (in which case none of the overlays can be removed on the other side of
>> >> kexec).
>> >
>> > We can add a sysfs attribute that configures whether overlays are reverted before
>> > kexec or not. I can't really tell which is the correct option, so let's allow the
>> > policy up to user-space.
>>
>> Kexec'ing into a new kernel doesn't change the hardware, so IMHO the
>> in-kernel DT should not change.
>
> Conceptually though, if overlays are applied then the new kernel has a
> different tree from the old one. If the overlay is hotpluggable, then
> after the kexec, the new kernel would no longer be able to unplug the
> overlay if it uses the current state of the tree instead of pulling the
> overlays off first.

Which brings me to another question...

Why has the overlay system been designed for plugging and unpluging whole
overlays?
That means the kernel has to remember the full stack, causing issues with
e.g. kexec.

Why not allowing the addition of removal of subtrees of the full device
tree?
This is similar to other hotpluggable subsystems (which are not necessarily
DT-based), like PCI Express. That way the kernel can pass a
DT-representation of the actual current device tree to a kexec'ed kernel.

I missed the initial design discussions, so forgive me if this has been
beaten to death before.

Thanks for your answers!

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/8] OF: Introduce DT overlay support.

[Grant Likely]

On Fri, 16 May 2014 13:52:42 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Grant,
> 
> On Fri, May 16, 2014 at 12:58 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
> > On Thu, 15 May 2014 09:20:24 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Thu, May 15, 2014 at 9:12 AM, Pantelis Antoniou
> >> <pantelis.antoniou@konsulko.com> wrote:
> >> >> We also need to think about kexec. Kexec works by sucking the live tree
> >> >> out of the kernel and creating a .dtb from it to pass to the new kernel.
> >> >> What will the rules be when kexecing? Do all the overlays need to be
> >> >> removed, or does the kernel get the tree with all the overlays applied
> >> >> (in which case none of the overlays can be removed on the other side of
> >> >> kexec).
> >> >
> >> > We can add a sysfs attribute that configures whether overlays are reverted before
> >> > kexec or not. I can't really tell which is the correct option, so let's allow the
> >> > policy up to user-space.
> >>
> >> Kexec'ing into a new kernel doesn't change the hardware, so IMHO the
> >> in-kernel DT should not change.
> >
> > Conceptually though, if overlays are applied then the new kernel has a
> > different tree from the old one. If the overlay is hotpluggable, then
> > after the kexec, the new kernel would no longer be able to unplug the
> > overlay if it uses the current state of the tree instead of pulling the
> > overlays off first.
> 
> Which brings me to another question...
> 
> Why has the overlay system been designed for plugging and unpluging whole
> overlays?
> That means the kernel has to remember the full stack, causing issues with
> e.g. kexec.

Mostly so that drivers don't see any difference in the livetree data
structure. It also means that userspace sees a single representation of
the hardware at any given time.

> Why not allowing the addition of removal of subtrees of the full device
> tree?

Overlays is more than just a subtree. A single overlay can make
manipulations of multiple subtrees that should be handled as logically
atomic.

> This is similar to other hotpluggable subsystems (which are not necessarily
> DT-based), like PCI Express. That way the kernel can pass a
> DT-representation of the actual current device tree to a kexec'ed kernel.

I'm not following you argument. Hardware hotplug systems like PCIe don't
manipulate the firmware data. The kernel detects the new device and
populates the Linux device model directly. Firmware provided data (ACPI
or FDT) isn't involved.

> 
> I missed the initial design discussions, so forgive me if this has been
> beaten to death before.

It's a good question. An alternative would be to keep the overlay tree
as a separate data structure and figure out how to make the core code
reference the overlay when iterating over nodes and properties. I don't
know how complex it would be to do that. We would definitely need to
adjust the data structure a bit, but that isn't an insurmountable
barrier.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Geert Uytterhoeven]

Hi Grant,

On Tue, May 20, 2014 at 7:50 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> Why has the overlay system been designed for plugging and unpluging whole
>> overlays?
>> That means the kernel has to remember the full stack, causing issues with
>> e.g. kexec.
>
> Mostly so that drivers don't see any difference in the livetree data
> structure. It also means that userspace sees a single representation of
> the hardware at any given time.

Sorry, I don't follow the argument about the "single representation of the
hardware".

>> Why not allowing the addition of removal of subtrees of the full device
>> tree?
>
> Overlays is more than just a subtree. A single overlay can make
> manipulations of multiple subtrees that should be handled as logically
> atomic.

Sure, it's more complicated due to the atomicity of multiple changes.

>> This is similar to other hotpluggable subsystems (which are not necessarily
>> DT-based), like PCI Express. That way the kernel can pass a
>> DT-representation of the actual current device tree to a kexec'ed kernel.
>
> I'm not following you argument. Hardware hotplug systems like PCIe don't
> manipulate the firmware data. The kernel detects the new device and
> populates the Linux device model directly. Firmware provided data (ACPI
> or FDT) isn't involved.

I mean the kernel doesn't remember the exact order in a stack, to reverse
operations. E.g. I can add hotplug a PCIe bridge with multiple devices
behind it, and unplug a single device later. It's still one subtree, though.

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/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Grant,

On May 20, 2014, at 8:50 AM, Grant Likely wrote:

> On Fri, 16 May 2014 13:52:42 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Grant,
>> 
>> On Fri, May 16, 2014 at 12:58 PM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>> On Thu, 15 May 2014 09:20:24 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Thu, May 15, 2014 at 9:12 AM, Pantelis Antoniou
>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>>> We also need to think about kexec. Kexec works by sucking the live tree
>>>>>> out of the kernel and creating a .dtb from it to pass to the new kernel.
>>>>>> What will the rules be when kexecing? Do all the overlays need to be
>>>>>> removed, or does the kernel get the tree with all the overlays applied
>>>>>> (in which case none of the overlays can be removed on the other side of
>>>>>> kexec).
>>>>> 
>>>>> We can add a sysfs attribute that configures whether overlays are reverted before
>>>>> kexec or not. I can't really tell which is the correct option, so let's allow the
>>>>> policy up to user-space.
>>>> 
>>>> Kexec'ing into a new kernel doesn't change the hardware, so IMHO the
>>>> in-kernel DT should not change.
>>> 
>>> Conceptually though, if overlays are applied then the new kernel has a
>>> different tree from the old one. If the overlay is hotpluggable, then
>>> after the kexec, the new kernel would no longer be able to unplug the
>>> overlay if it uses the current state of the tree instead of pulling the
>>> overlays off first.
>> 
>> Which brings me to another question...
>> 
>> Why has the overlay system been designed for plugging and unpluging whole
>> overlays?
>> That means the kernel has to remember the full stack, causing issues with
>> e.g. kexec.
> 
> Mostly so that drivers don't see any difference in the livetree data
> structure. It also means that userspace sees a single representation of
> the hardware at any given time.
> 

Yes. No change in semantics what-so-ever, when using the base DTS or an overlay.
In a nutshell, you should be able to copy the overlay contents on the base DTS
with equivalent results.

>> Why not allowing the addition of removal of subtrees of the full device
>> tree?
> 
> Overlays is more than just a subtree. A single overlay can make
> manipulations of multiple subtrees that should be handled as logically
> atomic.
> 

Right again. Drivers/subsystems are nowhere near ready to handle non-atomic
changes in their properties.

>> This is similar to other hotpluggable subsystems (which are not necessarily
>> DT-based), like PCI Express. That way the kernel can pass a
>> DT-representation of the actual current device tree to a kexec'ed kernel.
> 
> I'm not following you argument. Hardware hotplug systems like PCIe don't
> manipulate the firmware data. The kernel detects the new device and
> populates the Linux device model directly. Firmware provided data (ACPI
> or FDT) isn't involved.
> 

Hotplugged busses in theory do not need DT. In practice... that will come
later.

>> 
>> I missed the initial design discussions, so forgive me if this has been
>> beaten to death before.
> 
> It's a good question. An alternative would be to keep the overlay tree
> as a separate data structure and figure out how to make the core code
> reference the overlay when iterating over nodes and properties. I don't
> know how complex it would be to do that. We would definitely need to
> adjust the data structure a bit, but that isn't an insurmountable
> barrier.
> 

The changes required would be mind-boggingly complex. At least with the
overlay when you're done applying the changes you're done. Think about
the cases where multiple overlays are present. You'll have to iterate over
each and every one of them.
 
> g.
> 

Regards

-- Pantelis

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Tue, 20 May 2014 09:38:49 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Grant,
> 
> On Tue, May 20, 2014 at 7:50 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> Why has the overlay system been designed for plugging and unpluging whole
> >> overlays?
> >> That means the kernel has to remember the full stack, causing issues with
> >> e.g. kexec.
> >
> > Mostly so that drivers don't see any difference in the livetree data
> > structure. It also means that userspace sees a single representation of
> > the hardware at any given time.
> 
> Sorry, I don't follow the argument about the "single representation of the
> hardware".

Er, s/of the hardware/of the tree/. Right now the overlay design
modifies the live tree which at the same time modifies the tree
representation in /sys/firmware/devicetree. If the design was changed to
keep the overlay logically separate, then I would think we want to
expose that information to usespace also. In fact, I think we would need
to for usecases like kexec.

> 
> >> Why not allowing the addition of removal of subtrees of the full device
> >> tree?
> >
> > Overlays is more than just a subtree. A single overlay can make
> > manipulations of multiple subtrees that should be handled as logically
> > atomic.
> 
> Sure, it's more complicated due to the atomicity of multiple changes.
> 
> >> This is similar to other hotpluggable subsystems (which are not necessarily
> >> DT-based), like PCI Express. That way the kernel can pass a
> >> DT-representation of the actual current device tree to a kexec'ed kernel.
> >
> > I'm not following you argument. Hardware hotplug systems like PCIe don't
> > manipulate the firmware data. The kernel detects the new device and
> > populates the Linux device model directly. Firmware provided data (ACPI
> > or FDT) isn't involved.
> 
> I mean the kernel doesn't remember the exact order in a stack, to reverse
> operations. E.g. I can add hotplug a PCIe bridge with multiple devices
> behind it, and unplug a single device later. It's still one subtree, though.

The problem comes when one overlay adds a node or configuration that is
depended on by the second overlay, but there isn't a direct reference by
the second overlay into the first.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Geert Uytterhoeven]

Hi Grant,

On Mon, May 26, 2014 at 12:48 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Tue, 20 May 2014 09:38:49 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Tue, May 20, 2014 at 7:50 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> >> Why has the overlay system been designed for plugging and unpluging whole
>> >> overlays?
>> >> That means the kernel has to remember the full stack, causing issues with
>> >> e.g. kexec.
>> >
>> > Mostly so that drivers don't see any difference in the livetree data
>> > structure. It also means that userspace sees a single representation of
>> > the hardware at any given time.
>>
>> Sorry, I don't follow the argument about the "single representation of the
>> hardware".
>
> Er, s/of the hardware/of the tree/. Right now the overlay design
> modifies the live tree which at the same time modifies the tree
> representation in /sys/firmware/devicetree. If the design was changed to
> keep the overlay logically separate, then I would think we want to
> expose that information to usespace also. In fact, I think we would need
> to for usecases like kexec.

OK, so it does modify the real tree, and doesn't keep the actual overlays.

I was under the impression the overlay stack was also kept in memory, to allow
reversal, so there was a misunderstanding.

Hence for kexec, the tree in /sys/firmware/devicetree can just be passed
to the new kernel, as that's the current representation of the hardware?

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/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Geert,

On May 26, 2014, at 1:57 PM, Geert Uytterhoeven wrote:

> Hi Grant,
> 
> On Mon, May 26, 2014 at 12:48 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> On Tue, 20 May 2014 09:38:49 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, May 20, 2014 at 7:50 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>>> Why has the overlay system been designed for plugging and unpluging whole
>>>>> overlays?
>>>>> That means the kernel has to remember the full stack, causing issues with
>>>>> e.g. kexec.
>>>> 
>>>> Mostly so that drivers don't see any difference in the livetree data
>>>> structure. It also means that userspace sees a single representation of
>>>> the hardware at any given time.
>>> 
>>> Sorry, I don't follow the argument about the "single representation of the
>>> hardware".
>> 
>> Er, s/of the hardware/of the tree/. Right now the overlay design
>> modifies the live tree which at the same time modifies the tree
>> representation in /sys/firmware/devicetree. If the design was changed to
>> keep the overlay logically separate, then I would think we want to
>> expose that information to usespace also. In fact, I think we would need
>> to for usecases like kexec.
> 
> OK, so it does modify the real tree, and doesn't keep the actual overlays.
> 

It modifies the actual tree, and it keeps a log of each modification made to the
tree so that it will be able to revert the changes made.

> I was under the impression the overlay stack was also kept in memory, to allow
> reversal, so there was a misunderstanding.
> 

Yep.

> Hence for kexec, the tree in /sys/firmware/devicetree can just be passed
> to the new kernel, as that's the current representation of the hardware?
> 

Exactly.

> Gr{oetje,eeting}s,
> 
>                        Geert
> 

Regards

-- Pantelis

> --
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Mon, 26 May 2014 12:57:32 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Grant,
> 
> On Mon, May 26, 2014 at 12:48 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
> > On Tue, 20 May 2014 09:38:49 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Tue, May 20, 2014 at 7:50 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> >> Why has the overlay system been designed for plugging and unpluging whole
> >> >> overlays?
> >> >> That means the kernel has to remember the full stack, causing issues with
> >> >> e.g. kexec.
> >> >
> >> > Mostly so that drivers don't see any difference in the livetree data
> >> > structure. It also means that userspace sees a single representation of
> >> > the hardware at any given time.
> >>
> >> Sorry, I don't follow the argument about the "single representation of the
> >> hardware".
> >
> > Er, s/of the hardware/of the tree/. Right now the overlay design
> > modifies the live tree which at the same time modifies the tree
> > representation in /sys/firmware/devicetree. If the design was changed to
> > keep the overlay logically separate, then I would think we want to
> > expose that information to usespace also. In fact, I think we would need
> > to for usecases like kexec.
> 
> OK, so it does modify the real tree, and doesn't keep the actual overlays.
> 
> I was under the impression the overlay stack was also kept in memory, to allow
> reversal, so there was a misunderstanding.
> 
> Hence for kexec, the tree in /sys/firmware/devicetree can just be passed
> to the new kernel, as that's the current representation of the hardware?

Heeheehee. We're back where we started. The original question is whether
or not that is a valid approach. If the overlay represents something
that can be hot plugged/unplugged, then passing it through to the second
kernel would be the wrong thing to do. If it was a permenant addition,
then it probably doesn't need to be removed.

We do actually keep the overlay info in memory for the purpose of
removal exactly so we can support hot unbinding of devices and drivers
that make use of overlays.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Grant,

On May 26, 2014, at 2:23 PM, Grant Likely wrote:

> On Mon, 26 May 2014 12:57:32 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Grant,
>> 
>> On Mon, May 26, 2014 at 12:48 PM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>> On Tue, 20 May 2014 09:38:49 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Tue, May 20, 2014 at 7:50 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>>>> Why has the overlay system been designed for plugging and unpluging whole
>>>>>> overlays?
>>>>>> That means the kernel has to remember the full stack, causing issues with
>>>>>> e.g. kexec.
>>>>> 
>>>>> Mostly so that drivers don't see any difference in the livetree data
>>>>> structure. It also means that userspace sees a single representation of
>>>>> the hardware at any given time.
>>>> 
>>>> Sorry, I don't follow the argument about the "single representation of the
>>>> hardware".
>>> 
>>> Er, s/of the hardware/of the tree/. Right now the overlay design
>>> modifies the live tree which at the same time modifies the tree
>>> representation in /sys/firmware/devicetree. If the design was changed to
>>> keep the overlay logically separate, then I would think we want to
>>> expose that information to usespace also. In fact, I think we would need
>>> to for usecases like kexec.
>> 
>> OK, so it does modify the real tree, and doesn't keep the actual overlays.
>> 
>> I was under the impression the overlay stack was also kept in memory, to allow
>> reversal, so there was a misunderstanding.
>> 
>> Hence for kexec, the tree in /sys/firmware/devicetree can just be passed
>> to the new kernel, as that's the current representation of the hardware?
> 
> Heeheehee. We're back where we started. The original question is whether
> or not that is a valid approach. If the overlay represents something
> that can be hot plugged/unplugged, then passing it through to the second
> kernel would be the wrong thing to do. If it was a permenant addition,
> then it probably doesn't need to be removed.
> 
> We do actually keep the overlay info in memory for the purpose of
> removal exactly so we can support hot unbinding of devices and drivers
> that make use of overlays.
> 

We can support either method. I am not feeling any wiser about which one should be
the default TBH, so what about exporting a property and let the platform 
figure out which is more appropriate?

> g.
> 

Regards

-- Pantelis

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

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Sebastian Reichel]

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

Hi,

On Mon, May 26, 2014 at 02:55:37PM +0300, Pantelis Antoniou wrote:
> On May 26, 2014, at 2:23 PM, Grant Likely wrote:
> > On Mon, 26 May 2014 12:57:32 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Heeheehee. We're back where we started. The original question is whether
> > or not that is a valid approach. If the overlay represents something
> > that can be hot plugged/unplugged, then passing it through to the second
> > kernel would be the wrong thing to do. If it was a permenant addition,
> > then it probably doesn't need to be removed.
> > 
> > We do actually keep the overlay info in memory for the purpose of
> > removal exactly so we can support hot unbinding of devices and drivers
> > that make use of overlays.
> 
> We can support either method. I am not feeling any wiser about which one should be
> the default TBH, so what about exporting a property and let the platform 
> figure out which is more appropriate?

What about supporting "negative" overlays (so an overlay, that
removes DT entries)? That way one could reverse apply an overlay.
All the dependency stuff would basically be the users problem.  The
kernel only checks if it can apply an overlay (and return some error
code if it can't). This this code is needed anyway to check the
input from userspace.

As a result the overlay handling would basically have the same
behaviour as diff and patch :)

P.S.: Sorry if this has already been suggested. I have only read
mails from PATCHv4.

-- Sebastian

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

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Guenter Roeck]

On 05/26/2014 08:09 AM, Sebastian Reichel wrote:
> Hi,
>
> On Mon, May 26, 2014 at 02:55:37PM +0300, Pantelis Antoniou wrote:
>> On May 26, 2014, at 2:23 PM, Grant Likely wrote:
>>> On Mon, 26 May 2014 12:57:32 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> Heeheehee. We're back where we started. The original question is whether
>>> or not that is a valid approach. If the overlay represents something
>>> that can be hot plugged/unplugged, then passing it through to the second
>>> kernel would be the wrong thing to do. If it was a permenant addition,
>>> then it probably doesn't need to be removed.
>>>
>>> We do actually keep the overlay info in memory for the purpose of
>>> removal exactly so we can support hot unbinding of devices and drivers
>>> that make use of overlays.
>>
>> We can support either method. I am not feeling any wiser about which one should be
>> the default TBH, so what about exporting a property and let the platform
>> figure out which is more appropriate?
>
> What about supporting "negative" overlays (so an overlay, that
> removes DT entries)? That way one could reverse apply an overlay.
> All the dependency stuff would basically be the users problem.  The
> kernel only checks if it can apply an overlay (and return some error
> code if it can't). This this code is needed anyway to check the
> input from userspace.
>

Does that mean that I would need to describe such a negative overlay
for each overlay to be able to get it removed ?

This would introduce an endless source of problems with bad "reverse"
overlay descriptions. Sure, that would "be the users problem",
but I don't think that would make it better.

Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Sebastian,

On May 26, 2014, at 6:09 PM, Sebastian Reichel wrote:

> Hi,
> 
> On Mon, May 26, 2014 at 02:55:37PM +0300, Pantelis Antoniou wrote:
>> On May 26, 2014, at 2:23 PM, Grant Likely wrote:
>>> On Mon, 26 May 2014 12:57:32 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> Heeheehee. We're back where we started. The original question is whether
>>> or not that is a valid approach. If the overlay represents something
>>> that can be hot plugged/unplugged, then passing it through to the second
>>> kernel would be the wrong thing to do. If it was a permenant addition,
>>> then it probably doesn't need to be removed.
>>> 
>>> We do actually keep the overlay info in memory for the purpose of
>>> removal exactly so we can support hot unbinding of devices and drivers
>>> that make use of overlays.
>> 
>> We can support either method. I am not feeling any wiser about which one should be
>> the default TBH, so what about exporting a property and let the platform 
>> figure out which is more appropriate?
> 
> What about supporting "negative" overlays (so an overlay, that
> removes DT entries)? That way one could reverse apply an overlay.
> All the dependency stuff would basically be the users problem.  The
> kernel only checks if it can apply an overlay (and return some error
> code if it can't). This this code is needed anyway to check the
> input from userspace.
> 
> As a result the overlay handling would basically have the same
> behaviour as diff and patch :)
> 

This is already supported; nodes and properties can be prefixed with a minus
sign '-' and operate as expected :)

i.e. "-property;" removes a property and "-node { };" removes a node. 

> P.S.: Sorry if this has already been suggested. I have only read
> mails from PATCHv4.
> 
> -- Sebastian

Regards

-- Pantelis

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Mon, 26 May 2014 14:55:37 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
> 
> On May 26, 2014, at 2:23 PM, Grant Likely wrote:
> 
> > On Mon, 26 May 2014 12:57:32 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> Hi Grant,
> >> 
> >> On Mon, May 26, 2014 at 12:48 PM, Grant Likely
> >> <grant.likely@secretlab.ca> wrote:
> >>> On Tue, 20 May 2014 09:38:49 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>> On Tue, May 20, 2014 at 7:50 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> >>>>>> Why has the overlay system been designed for plugging and unpluging whole
> >>>>>> overlays?
> >>>>>> That means the kernel has to remember the full stack, causing issues with
> >>>>>> e.g. kexec.
> >>>>> 
> >>>>> Mostly so that drivers don't see any difference in the livetree data
> >>>>> structure. It also means that userspace sees a single representation of
> >>>>> the hardware at any given time.
> >>>> 
> >>>> Sorry, I don't follow the argument about the "single representation of the
> >>>> hardware".
> >>> 
> >>> Er, s/of the hardware/of the tree/. Right now the overlay design
> >>> modifies the live tree which at the same time modifies the tree
> >>> representation in /sys/firmware/devicetree. If the design was changed to
> >>> keep the overlay logically separate, then I would think we want to
> >>> expose that information to usespace also. In fact, I think we would need
> >>> to for usecases like kexec.
> >> 
> >> OK, so it does modify the real tree, and doesn't keep the actual overlays.
> >> 
> >> I was under the impression the overlay stack was also kept in memory, to allow
> >> reversal, so there was a misunderstanding.
> >> 
> >> Hence for kexec, the tree in /sys/firmware/devicetree can just be passed
> >> to the new kernel, as that's the current representation of the hardware?
> > 
> > Heeheehee. We're back where we started. The original question is whether
> > or not that is a valid approach. If the overlay represents something
> > that can be hot plugged/unplugged, then passing it through to the second
> > kernel would be the wrong thing to do. If it was a permenant addition,
> > then it probably doesn't need to be removed.
> > 
> > We do actually keep the overlay info in memory for the purpose of
> > removal exactly so we can support hot unbinding of devices and drivers
> > that make use of overlays.
> > 
> 
> We can support either method. I am not feeling any wiser about which one should be
> the default TBH, so what about exporting a property and let the platform 
> figure out which is more appropriate?

After thinking about it more, I think it is very likely that removing
all the overlays is the correct thing to do in the kexec use-case. When
kexec-ing, it makes sense that we'd want the exact same behaviour from
the kexec'ed kernel. That means we want the device drivers to do the
same thing including loading whatever overlays they depend on.

If the flattened tree was left applied, then the behaviour becomes
different.

I say always remove the overlays unless explicitly told not to, but I'm
struggling to come up with use cases where keeping them applied is
desirable.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Geert Uytterhoeven]

Hi Grant,

On Mon, May 26, 2014 at 11:33 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> After thinking about it more, I think it is very likely that removing
> all the overlays is the correct thing to do in the kexec use-case. When
> kexec-ing, it makes sense that we'd want the exact same behaviour from
> the kexec'ed kernel. That means we want the device drivers to do the
> same thing including loading whatever overlays they depend on.

Are the device drivers loading the overlays?
That sounds a bit backwards to me.

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/8] OF: Introduce DT overlay support.

[Sebastian Reichel]

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

Hi,

On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
> After thinking about it more, I think it is very likely that removing
> all the overlays is the correct thing to do in the kexec use-case. When
> kexec-ing, it makes sense that we'd want the exact same behaviour from
> the kexec'ed kernel. That means we want the device drivers to do the
> same thing including loading whatever overlays they depend on.
> 
> If the flattened tree was left applied, then the behaviour becomes
> different.
> 
> I say always remove the overlays unless explicitly told not to, but I'm
> struggling to come up with use cases where keeping them applied is
> desirable.

I would assume, that I want them applied in most cases. DT describes
the hardware. If I kexec into a new kernel I change software, not
hardware.

Maybe I'm missing the main purpose of the feature. I currently see
two useful usecases for DT overlays:

1. The dtb the kernel is booted with cannot be changed for some
   reason, but the board has additional hardware attached (e.g.
   the user added a sensor on the i2c bus)
2. The hardware is changed on the fly (e.g. the user flashed the
   FPGA part of a zynq processor), sensors on i2c bus, ...

In both cases the kernel should be booted with the additional
overlay information IMHO. Though for the second case it should
be possible to remove the "programmed" hardware information
somehow.

-- Sebastian

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

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Sebastian Reichel]

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

Hi,

On Mon, May 26, 2014 at 08:14:08AM -0700, Guenter Roeck wrote:
> On 05/26/2014 08:09 AM, Sebastian Reichel wrote:
> >On Mon, May 26, 2014 at 02:55:37PM +0300, Pantelis Antoniou wrote:
> >>On May 26, 2014, at 2:23 PM, Grant Likely wrote:
> >>>On Mon, 26 May 2014 12:57:32 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>Heeheehee. We're back where we started. The original question is whether
> >>>or not that is a valid approach. If the overlay represents something
> >>>that can be hot plugged/unplugged, then passing it through to the second
> >>>kernel would be the wrong thing to do. If it was a permenant addition,
> >>>then it probably doesn't need to be removed.
> >>>
> >>>We do actually keep the overlay info in memory for the purpose of
> >>>removal exactly so we can support hot unbinding of devices and drivers
> >>>that make use of overlays.
> >>
> >>We can support either method. I am not feeling any wiser about which one should be
> >>the default TBH, so what about exporting a property and let the platform
> >>figure out which is more appropriate?
> >
> >What about supporting "negative" overlays (so an overlay, that
> >removes DT entries)? That way one could reverse apply an overlay.
> >All the dependency stuff would basically be the users problem.  The
> >kernel only checks if it can apply an overlay (and return some error
> >code if it can't). This this code is needed anyway to check the
> >input from userspace.
> >
> 
> Does that mean that I would need to describe such a negative overlay
> for each overlay to be able to get it removed ?
> 
> This would introduce an endless source of problems with bad "reverse"
> overlay descriptions. Sure, that would "be the users problem",
> but I don't think that would make it better.

I was thinking about supporting something like "patch --reverse". So
you can try to undo the overlay by reverse applying it and you can
do it in arbitrary order.

Note: The dependency check must be done for all overlays coming from
userspace, so that's not a problem __here__. The reverse method can
"simply" reverse the overlay patch and apply it like a normal
overlay from userspace (and thus using the same dependency checks).

-- Sebastian

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

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Guenter Roeck]

On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
> Hi,
>
> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
>> After thinking about it more, I think it is very likely that removing
>> all the overlays is the correct thing to do in the kexec use-case. When
>> kexec-ing, it makes sense that we'd want the exact same behaviour from
>> the kexec'ed kernel. That means we want the device drivers to do the
>> same thing including loading whatever overlays they depend on.
>>
>> If the flattened tree was left applied, then the behaviour becomes
>> different.
>>
>> I say always remove the overlays unless explicitly told not to, but I'm
>> struggling to come up with use cases where keeping them applied is
>> desirable.
>
> I would assume, that I want them applied in most cases. DT describes
> the hardware. If I kexec into a new kernel I change software, not
> hardware.
>
> Maybe I'm missing the main purpose of the feature. I currently see
> two useful usecases for DT overlays:
>
> 1. The dtb the kernel is booted with cannot be changed for some
>     reason, but the board has additional hardware attached (e.g.
>     the user added a sensor on the i2c bus)
> 2. The hardware is changed on the fly (e.g. the user flashed the
>     FPGA part of a zynq processor), sensors on i2c bus, ...
>
> In both cases the kernel should be booted with the additional
> overlay information IMHO. Though for the second case it should
> be possible to remove the "programmed" hardware information
> somehow.
>

3. Some hot-plug device or card is inserted or removed.

I would argue that the kernel should _not_ be booted with the overlay in place.
Otherwise the code handling overlays would have to have special handling
for the restart case, which is much more complex than just to re-insert
the overlay when it is determined that the device or card is still there.

Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Guenter Roeck]

On 05/26/2014 02:44 PM, Geert Uytterhoeven wrote:
> Hi Grant,
>
> On Mon, May 26, 2014 at 11:33 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> After thinking about it more, I think it is very likely that removing
>> all the overlays is the correct thing to do in the kexec use-case. When
>> kexec-ing, it makes sense that we'd want the exact same behaviour from
>> the kexec'ed kernel. That means we want the device drivers to do the
>> same thing including loading whatever overlays they depend on.
>
> Are the device drivers loading the overlays?
> That sounds a bit backwards to me.
>

Depends on what you mean with 'device driver'. In our case we have a driver
dedicated to handle card status. Quite similar to what the connector subsystem
does but with significantly more functionality (including devicetree overlay
management).

Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Sebastian Reichel]

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

On Mon, May 26, 2014 at 04:42:44PM -0700, Guenter Roeck wrote:
> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
> >On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
> >>After thinking about it more, I think it is very likely that removing
> >>all the overlays is the correct thing to do in the kexec use-case. When
> >>kexec-ing, it makes sense that we'd want the exact same behaviour from
> >>the kexec'ed kernel. That means we want the device drivers to do the
> >>same thing including loading whatever overlays they depend on.
> >>
> >>If the flattened tree was left applied, then the behaviour becomes
> >>different.
> >>
> >>I say always remove the overlays unless explicitly told not to, but I'm
> >>struggling to come up with use cases where keeping them applied is
> >>desirable.
> >
> >I would assume, that I want them applied in most cases. DT describes
> >the hardware. If I kexec into a new kernel I change software, not
> >hardware.
> >
> >Maybe I'm missing the main purpose of the feature. I currently see
> >two useful usecases for DT overlays:
> >
> >1. The dtb the kernel is booted with cannot be changed for some
> >    reason, but the board has additional hardware attached (e.g.
> >    the user added a sensor on the i2c bus)
> >2. The hardware is changed on the fly (e.g. the user flashed the
> >    FPGA part of a zynq processor), sensors on i2c bus, ...
> >
> >In both cases the kernel should be booted with the additional
> >overlay information IMHO. Though for the second case it should
> >be possible to remove the "programmed" hardware information
> >somehow.
> >
> 
> 3. Some hot-plug device or card is inserted or removed.

Can you give a more specific example? I guess most hot-plug
devices are connected to busses, which are not described via
DT, but support auto-identification (USB, PCI, ...)

> I would argue that the kernel should _not_ be booted with the
> overlay in place.

well the device is still attached to the system when you kexec
into the new kernel, isn't it?

> Otherwise the code handling overlays would have to have special
> handling for the restart case, which is much more complex than
> just to re-insert the overlay when it is determined that the
> device or card is still there.

I assume, that the kernel cannot auto-detect the attached hardware.
Otherwise we don't need the DT entries, but can simply scan the bus.
So the restart case (or restart + kexec case if kexec behaves like a
restart) means, that userspace needs to provide the information
about device existence.

Removing the overlay is like dropping information supplied from the
user. Not something, which should be done carelessly.

-- Sebastian

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

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Guenter Roeck]

On 05/26/2014 05:32 PM, Sebastian Reichel wrote:
> On Mon, May 26, 2014 at 04:42:44PM -0700, Guenter Roeck wrote:
>> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
>>> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
>>>> After thinking about it more, I think it is very likely that removing
>>>> all the overlays is the correct thing to do in the kexec use-case. When
>>>> kexec-ing, it makes sense that we'd want the exact same behaviour from
>>>> the kexec'ed kernel. That means we want the device drivers to do the
>>>> same thing including loading whatever overlays they depend on.
>>>>
>>>> If the flattened tree was left applied, then the behaviour becomes
>>>> different.
>>>>
>>>> I say always remove the overlays unless explicitly told not to, but I'm
>>>> struggling to come up with use cases where keeping them applied is
>>>> desirable.
>>>
>>> I would assume, that I want them applied in most cases. DT describes
>>> the hardware. If I kexec into a new kernel I change software, not
>>> hardware.
>>>
>>> Maybe I'm missing the main purpose of the feature. I currently see
>>> two useful usecases for DT overlays:
>>>
>>> 1. The dtb the kernel is booted with cannot be changed for some
>>>     reason, but the board has additional hardware attached (e.g.
>>>     the user added a sensor on the i2c bus)
>>> 2. The hardware is changed on the fly (e.g. the user flashed the
>>>     FPGA part of a zynq processor), sensors on i2c bus, ...
>>>
>>> In both cases the kernel should be booted with the additional
>>> overlay information IMHO. Though for the second case it should
>>> be possible to remove the "programmed" hardware information
>>> somehow.
>>>
>>
>> 3. Some hot-plug device or card is inserted or removed.
>
> Can you give a more specific example? I guess most hot-plug
> devices are connected to busses, which are not described via
> DT, but support auto-identification (USB, PCI, ...)
>
The card interface provides i2c and pcie busses, plus a number of
gpio pins. Both I2C devices and PCIe devices depend on the inserted
card type. There may be PCIe switches on some cards, or just PCIe
devices on others. Auto-identification of PCIe devices does not help,
as the device configuration depends on the card type inserted.
A typical example is that the card may include a multi-function
FPGA device with LED, GPIO, and I2C bus master functionality,
and the specific configuration depends on the card type (even though
the FPGA is the same). Other cards may include a PCIe switch with
a number of ASICs connected to it. PCIe switch configuration
depends on the card type, not on the PCIe switch type.

>> I would argue that the kernel should _not_ be booted with the
>> overlay in place.
>
> well the device is still attached to the system when you kexec
> into the new kernel, isn't it?
>
So ? the code executed is ultimately the same, if the kernel is booted
from rommon or from kexec. In one case we'll have to insert the overlay,
in the other case not.

>> Otherwise the code handling overlays would have to have special
>> handling for the restart case, which is much more complex than
>> just to re-insert the overlay when it is determined that the
>> device or card is still there.
>
> I assume, that the kernel cannot auto-detect the attached hardware.

Correct.

> Otherwise we don't need the DT entries, but can simply scan the bus.
> So the restart case (or restart + kexec case if kexec behaves like a
> restart) means, that userspace needs to provide the information
> about device existence.
>
Exactly the same informatin we get if the kernel is booted from ROMMON
or BIOS. No difference here.

> Removing the overlay is like dropping information supplied from the
> user. Not something, which should be done carelessly.
>

In our case it is done when the card is removed.

Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Mon, 26 May 2014 23:44:41 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Grant,
> 
> On Mon, May 26, 2014 at 11:33 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
> > After thinking about it more, I think it is very likely that removing
> > all the overlays is the correct thing to do in the kexec use-case. When
> > kexec-ing, it makes sense that we'd want the exact same behaviour from
> > the kexec'ed kernel. That means we want the device drivers to do the
> > same thing including loading whatever overlays they depend on.
> 
> Are the device drivers loading the overlays?
> That sounds a bit backwards to me.

In a lot of cases, yes. For example, the beaglebone capebus driver would
be responsible for identifying and requesting or loading the correct
overlay to describe the hardware.

Having userspace provide an arbitrary overlay is only one of the use
cases.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Mon, 26 May 2014 16:42:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
> > Hi,
> >
> > On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
> >> After thinking about it more, I think it is very likely that removing
> >> all the overlays is the correct thing to do in the kexec use-case. When
> >> kexec-ing, it makes sense that we'd want the exact same behaviour from
> >> the kexec'ed kernel. That means we want the device drivers to do the
> >> same thing including loading whatever overlays they depend on.
> >>
> >> If the flattened tree was left applied, then the behaviour becomes
> >> different.
> >>
> >> I say always remove the overlays unless explicitly told not to, but I'm
> >> struggling to come up with use cases where keeping them applied is
> >> desirable.
> >
> > I would assume, that I want them applied in most cases. DT describes
> > the hardware. If I kexec into a new kernel I change software, not
> > hardware.
> >
> > Maybe I'm missing the main purpose of the feature. I currently see
> > two useful usecases for DT overlays:
> >
> > 1. The dtb the kernel is booted with cannot be changed for some
> >     reason, but the board has additional hardware attached (e.g.
> >     the user added a sensor on the i2c bus)
> > 2. The hardware is changed on the fly (e.g. the user flashed the
> >     FPGA part of a zynq processor), sensors on i2c bus, ...
> >
> > In both cases the kernel should be booted with the additional
> > overlay information IMHO. Though for the second case it should
> > be possible to remove the "programmed" hardware information
> > somehow.
> >
> 
> 3. Some hot-plug device or card is inserted or removed.
> 
> I would argue that the kernel should _not_ be booted with the overlay in place.
> Otherwise the code handling overlays would have to have special handling
> for the restart case, which is much more complex than just to re-insert
> the overlay when it is determined that the device or card is still there.

Exactly.

g.

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Grant,

On May 27, 2014, at 3:12 PM, Grant Likely wrote:

> On Mon, 26 May 2014 16:42:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
>>> Hi,
>>> 
>>> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
>>>> After thinking about it more, I think it is very likely that removing
>>>> all the overlays is the correct thing to do in the kexec use-case. When
>>>> kexec-ing, it makes sense that we'd want the exact same behaviour from
>>>> the kexec'ed kernel. That means we want the device drivers to do the
>>>> same thing including loading whatever overlays they depend on.
>>>> 
>>>> If the flattened tree was left applied, then the behaviour becomes
>>>> different.
>>>> 
>>>> I say always remove the overlays unless explicitly told not to, but I'm
>>>> struggling to come up with use cases where keeping them applied is
>>>> desirable.
>>> 
>>> I would assume, that I want them applied in most cases. DT describes
>>> the hardware. If I kexec into a new kernel I change software, not
>>> hardware.
>>> 
>>> Maybe I'm missing the main purpose of the feature. I currently see
>>> two useful usecases for DT overlays:
>>> 
>>> 1. The dtb the kernel is booted with cannot be changed for some
>>>    reason, but the board has additional hardware attached (e.g.
>>>    the user added a sensor on the i2c bus)
>>> 2. The hardware is changed on the fly (e.g. the user flashed the
>>>    FPGA part of a zynq processor), sensors on i2c bus, ...
>>> 
>>> In both cases the kernel should be booted with the additional
>>> overlay information IMHO. Though for the second case it should
>>> be possible to remove the "programmed" hardware information
>>> somehow.
>>> 
>> 
>> 3. Some hot-plug device or card is inserted or removed.
>> 
>> I would argue that the kernel should _not_ be booted with the overlay in place.
>> Otherwise the code handling overlays would have to have special handling
>> for the restart case, which is much more complex than just to re-insert
>> the overlay when it is determined that the device or card is still there.
> 
> Exactly.
> 

Looks like we are levitating to the 'remove overlays on kexec' approach.
Is that correct?

> g.
> 

Regards

-- Pantelis

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Guenter Roeck]

On Tue, May 27, 2014 at 03:24:35PM +0300, Pantelis Antoniou wrote:
> Hi Grant,
> 
> On May 27, 2014, at 3:12 PM, Grant Likely wrote:
> 
> > On Mon, 26 May 2014 16:42:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> >> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
> >>> Hi,
> >>> 
> >>> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
> >>>> After thinking about it more, I think it is very likely that removing
> >>>> all the overlays is the correct thing to do in the kexec use-case. When
> >>>> kexec-ing, it makes sense that we'd want the exact same behaviour from
> >>>> the kexec'ed kernel. That means we want the device drivers to do the
> >>>> same thing including loading whatever overlays they depend on.
> >>>> 
> >>>> If the flattened tree was left applied, then the behaviour becomes
> >>>> different.
> >>>> 
> >>>> I say always remove the overlays unless explicitly told not to, but I'm
> >>>> struggling to come up with use cases where keeping them applied is
> >>>> desirable.
> >>> 
> >>> I would assume, that I want them applied in most cases. DT describes
> >>> the hardware. If I kexec into a new kernel I change software, not
> >>> hardware.
> >>> 
> >>> Maybe I'm missing the main purpose of the feature. I currently see
> >>> two useful usecases for DT overlays:
> >>> 
> >>> 1. The dtb the kernel is booted with cannot be changed for some
> >>>    reason, but the board has additional hardware attached (e.g.
> >>>    the user added a sensor on the i2c bus)
> >>> 2. The hardware is changed on the fly (e.g. the user flashed the
> >>>    FPGA part of a zynq processor), sensors on i2c bus, ...
> >>> 
> >>> In both cases the kernel should be booted with the additional
> >>> overlay information IMHO. Though for the second case it should
> >>> be possible to remove the "programmed" hardware information
> >>> somehow.
> >>> 
> >> 
> >> 3. Some hot-plug device or card is inserted or removed.
> >> 
> >> I would argue that the kernel should _not_ be booted with the overlay in place.
> >> Otherwise the code handling overlays would have to have special handling
> >> for the restart case, which is much more complex than just to re-insert
> >> the overlay when it is determined that the device or card is still there.
> > 
> > Exactly.
> > 
> 
> Looks like we are levitating to the 'remove overlays on kexec' approach.
> Is that correct?
> 

Let's just assume for a minute that this is not the case, and that loaded
overlays are passed on.

This would be an interesting challenge for the overlay manager, as it would
have to handle a number of startup conditions. After all, it can not take
it as granted that the hardware state did not change after it was stopped
in the old kernel, and before it was started in the new kernel.
- overlay loaded, but hardware/device no longer present
  -> unload overlay
- overlay loaded, but different hardware present
  -> unload old overlay, load new one
- overlay not loaded, hardware present
  -> load overlay
- overlay loaded and matches hardware
  -> do nothing

In comparison, its task would be quite straightforward if loaded overlays
are not passed on to the new kernel.
- If hardware is present, load overlay

Ultimately, I seem to be missing something, as I don't really see the benefit
of passing on the loaded overlay(s) to the new kernel. Activation time, maybe,
for the most common case (overlay loaded and matches hardware) ?

Concern though is the other cases, with a mismatch between HW and loaded
overlays. I am not sure if it is even possible to ensure that there are
no race conditions if the devicetree is outdated at system startup. Sure,
that is unlikely to happen in 'normal' operating conditions, but that only
means that it _will_ happen a few hours after the first customer starts
playing with the system.

Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Guenter,

On May 27, 2014, at 6:21 PM, Guenter Roeck wrote:

> On Tue, May 27, 2014 at 03:24:35PM +0300, Pantelis Antoniou wrote:
>> Hi Grant,
>> 
>> On May 27, 2014, at 3:12 PM, Grant Likely wrote:
>> 
>>> On Mon, 26 May 2014 16:42:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
>>>>> Hi,
>>>>> 
>>>>> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
>>>>>> After thinking about it more, I think it is very likely that removing
>>>>>> all the overlays is the correct thing to do in the kexec use-case. When
>>>>>> kexec-ing, it makes sense that we'd want the exact same behaviour from
>>>>>> the kexec'ed kernel. That means we want the device drivers to do the
>>>>>> same thing including loading whatever overlays they depend on.
>>>>>> 
>>>>>> If the flattened tree was left applied, then the behaviour becomes
>>>>>> different.
>>>>>> 
>>>>>> I say always remove the overlays unless explicitly told not to, but I'm
>>>>>> struggling to come up with use cases where keeping them applied is
>>>>>> desirable.
>>>>> 
>>>>> I would assume, that I want them applied in most cases. DT describes
>>>>> the hardware. If I kexec into a new kernel I change software, not
>>>>> hardware.
>>>>> 
>>>>> Maybe I'm missing the main purpose of the feature. I currently see
>>>>> two useful usecases for DT overlays:
>>>>> 
>>>>> 1. The dtb the kernel is booted with cannot be changed for some
>>>>>   reason, but the board has additional hardware attached (e.g.
>>>>>   the user added a sensor on the i2c bus)
>>>>> 2. The hardware is changed on the fly (e.g. the user flashed the
>>>>>   FPGA part of a zynq processor), sensors on i2c bus, ...
>>>>> 
>>>>> In both cases the kernel should be booted with the additional
>>>>> overlay information IMHO. Though for the second case it should
>>>>> be possible to remove the "programmed" hardware information
>>>>> somehow.
>>>>> 
>>>> 
>>>> 3. Some hot-plug device or card is inserted or removed.
>>>> 
>>>> I would argue that the kernel should _not_ be booted with the overlay in place.
>>>> Otherwise the code handling overlays would have to have special handling
>>>> for the restart case, which is much more complex than just to re-insert
>>>> the overlay when it is determined that the device or card is still there.
>>> 
>>> Exactly.
>>> 
>> 
>> Looks like we are levitating to the 'remove overlays on kexec' approach.
>> Is that correct?
>> 
> 
> Let's just assume for a minute that this is not the case, and that loaded
> overlays are passed on.
> 
> This would be an interesting challenge for the overlay manager, as it would
> have to handle a number of startup conditions. After all, it can not take
> it as granted that the hardware state did not change after it was stopped
> in the old kernel, and before it was started in the new kernel.
> - overlay loaded, but hardware/device no longer present
>  -> unload overlay
> - overlay loaded, but different hardware present
>  -> unload old overlay, load new one
> - overlay not loaded, hardware present
>  -> load overlay
> - overlay loaded and matches hardware
>  -> do nothing
> 
> In comparison, its task would be quite straightforward if loaded overlays
> are not passed on to the new kernel.
> - If hardware is present, load overlay
> 

Yeah, exactly. The only case where having the applied overlays present on the 
kexec-ed kernel is either some kind of virtualization environment, or an fpga
that takes an awful lot amount of time to re-initializa.

> Ultimately, I seem to be missing something, as I don't really see the benefit
> of passing on the loaded overlay(s) to the new kernel. Activation time, maybe,
> for the most common case (overlay loaded and matches hardware) ?
> 
> Concern though is the other cases, with a mismatch between HW and loaded
> overlays. I am not sure if it is even possible to ensure that there are
> no race conditions if the devicetree is outdated at system startup. Sure,
> that is unlikely to happen in 'normal' operating conditions, but that only
> means that it _will_ happen a few hours after the first customer starts
> playing with the system.
> 

I concur. Accidents will happen, and if it's not a tightly controlled system
breakage is guaranteed.

> Guenter

Regards

-- Pantelis

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

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Geert Uytterhoeven]

Hi Günther,

On Tue, May 27, 2014 at 5:21 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, May 27, 2014 at 03:24:35PM +0300, Pantelis Antoniou wrote:
>> On May 27, 2014, at 3:12 PM, Grant Likely wrote:
>> > On Mon, 26 May 2014 16:42:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
>> >>> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
>> >>>> After thinking about it more, I think it is very likely that removing
>> >>>> all the overlays is the correct thing to do in the kexec use-case. When
>> >>>> kexec-ing, it makes sense that we'd want the exact same behaviour from
>> >>>> the kexec'ed kernel. That means we want the device drivers to do the
>> >>>> same thing including loading whatever overlays they depend on.
>> >>>>
>> >>>> If the flattened tree was left applied, then the behaviour becomes
>> >>>> different.
>> >>>>
>> >>>> I say always remove the overlays unless explicitly told not to, but I'm
>> >>>> struggling to come up with use cases where keeping them applied is
>> >>>> desirable.
>> >>>
>> >>> I would assume, that I want them applied in most cases. DT describes
>> >>> the hardware. If I kexec into a new kernel I change software, not
>> >>> hardware.
>> >>>
>> >>> Maybe I'm missing the main purpose of the feature. I currently see
>> >>> two useful usecases for DT overlays:
>> >>>
>> >>> 1. The dtb the kernel is booted with cannot be changed for some
>> >>>    reason, but the board has additional hardware attached (e.g.
>> >>>    the user added a sensor on the i2c bus)
>> >>> 2. The hardware is changed on the fly (e.g. the user flashed the
>> >>>    FPGA part of a zynq processor), sensors on i2c bus, ...
>> >>>
>> >>> In both cases the kernel should be booted with the additional
>> >>> overlay information IMHO. Though for the second case it should
>> >>> be possible to remove the "programmed" hardware information
>> >>> somehow.
>> >>>
>> >>
>> >> 3. Some hot-plug device or card is inserted or removed.
>> >>
>> >> I would argue that the kernel should _not_ be booted with the overlay in place.
>> >> Otherwise the code handling overlays would have to have special handling
>> >> for the restart case, which is much more complex than just to re-insert
>> >> the overlay when it is determined that the device or card is still there.
>> >
>> > Exactly.
>> >
>>
>> Looks like we are levitating to the 'remove overlays on kexec' approach.
>> Is that correct?
>>
>
> Let's just assume for a minute that this is not the case, and that loaded
> overlays are passed on.
>
> This would be an interesting challenge for the overlay manager, as it would
> have to handle a number of startup conditions. After all, it can not take
> it as granted that the hardware state did not change after it was stopped
> in the old kernel, and before it was started in the new kernel.
> - overlay loaded, but hardware/device no longer present
>   -> unload overlay
> - overlay loaded, but different hardware present
>   -> unload old overlay, load new one
> - overlay not loaded, hardware present
>   -> load overlay
> - overlay loaded and matches hardware
>   -> do nothing
>
> In comparison, its task would be quite straightforward if loaded overlays
> are not passed on to the new kernel.
> - If hardware is present, load overlay
>
> Ultimately, I seem to be missing something, as I don't really see the benefit
> of passing on the loaded overlay(s) to the new kernel. Activation time, maybe,
> for the most common case (overlay loaded and matches hardware) ?

All of the above assume you're using a system with overlays and an overlay
manager. I.e. you have add-on devices together with an overlay DT.
One way to handle this is via auto-detection of capes, that identify themselves,
so the overlay manager knows which overlay to load.

I'm trying to look at it in a more generic way: you add hardware which is not
in the DT, so the DT must be modified (in some way or another) to match the
hardware.

If you run kexec, the added hardware is still there. In the absence of a system
with auto-detection of capes, the newly booted kernel cannot know that
hardware has been added, compared to a pristine system. If it's in the
DT passed through kexec, everything will work.

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/8] OF: Introduce DT overlay support.

[Guenter Roeck]

Hi Geert,

On Tue, May 27, 2014 at 07:52:57PM +0200, Geert Uytterhoeven wrote:
> Hi Günther,
> 
> On Tue, May 27, 2014 at 5:21 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Tue, May 27, 2014 at 03:24:35PM +0300, Pantelis Antoniou wrote:
> >> On May 27, 2014, at 3:12 PM, Grant Likely wrote:
> >> > On Mon, 26 May 2014 16:42:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> >> >> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
> >> >>> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
> >> >>>> After thinking about it more, I think it is very likely that removing
> >> >>>> all the overlays is the correct thing to do in the kexec use-case. When
> >> >>>> kexec-ing, it makes sense that we'd want the exact same behaviour from
> >> >>>> the kexec'ed kernel. That means we want the device drivers to do the
> >> >>>> same thing including loading whatever overlays they depend on.
> >> >>>>
> >> >>>> If the flattened tree was left applied, then the behaviour becomes
> >> >>>> different.
> >> >>>>
> >> >>>> I say always remove the overlays unless explicitly told not to, but I'm
> >> >>>> struggling to come up with use cases where keeping them applied is
> >> >>>> desirable.
> >> >>>
> >> >>> I would assume, that I want them applied in most cases. DT describes
> >> >>> the hardware. If I kexec into a new kernel I change software, not
> >> >>> hardware.
> >> >>>
> >> >>> Maybe I'm missing the main purpose of the feature. I currently see
> >> >>> two useful usecases for DT overlays:
> >> >>>
> >> >>> 1. The dtb the kernel is booted with cannot be changed for some
> >> >>>    reason, but the board has additional hardware attached (e.g.
> >> >>>    the user added a sensor on the i2c bus)
> >> >>> 2. The hardware is changed on the fly (e.g. the user flashed the
> >> >>>    FPGA part of a zynq processor), sensors on i2c bus, ...
> >> >>>
> >> >>> In both cases the kernel should be booted with the additional
> >> >>> overlay information IMHO. Though for the second case it should
> >> >>> be possible to remove the "programmed" hardware information
> >> >>> somehow.
> >> >>>
> >> >>
> >> >> 3. Some hot-plug device or card is inserted or removed.
> >> >>
> >> >> I would argue that the kernel should _not_ be booted with the overlay in place.
> >> >> Otherwise the code handling overlays would have to have special handling
> >> >> for the restart case, which is much more complex than just to re-insert
> >> >> the overlay when it is determined that the device or card is still there.
> >> >
> >> > Exactly.
> >> >
> >>
> >> Looks like we are levitating to the 'remove overlays on kexec' approach.
> >> Is that correct?
> >>
> >
> > Let's just assume for a minute that this is not the case, and that loaded
> > overlays are passed on.
> >
> > This would be an interesting challenge for the overlay manager, as it would
> > have to handle a number of startup conditions. After all, it can not take
> > it as granted that the hardware state did not change after it was stopped
> > in the old kernel, and before it was started in the new kernel.
> > - overlay loaded, but hardware/device no longer present
> >   -> unload overlay
> > - overlay loaded, but different hardware present
> >   -> unload old overlay, load new one
> > - overlay not loaded, hardware present
> >   -> load overlay
> > - overlay loaded and matches hardware
> >   -> do nothing
> >
> > In comparison, its task would be quite straightforward if loaded overlays
> > are not passed on to the new kernel.
> > - If hardware is present, load overlay
> >
> > Ultimately, I seem to be missing something, as I don't really see the benefit
> > of passing on the loaded overlay(s) to the new kernel. Activation time, maybe,
> > for the most common case (overlay loaded and matches hardware) ?
> 
> All of the above assume you're using a system with overlays and an overlay
> manager. I.e. you have add-on devices together with an overlay DT.
> One way to handle this is via auto-detection of capes, that identify themselves,
> so the overlay manager knows which overlay to load.
> 
Possibly auto-detected (and in our case that is so), but not necessarily.
A user space overlay manager, which keeps state in user space, would be possible
as well.

> I'm trying to look at it in a more generic way: you add hardware which is not
> in the DT, so the DT must be modified (in some way or another) to match the
> hardware.
> 
> If you run kexec, the added hardware is still there. In the absence of a system
> with auto-detection of capes, the newly booted kernel cannot know that
> hardware has been added, compared to a pristine system. If it's in the
> DT passed through kexec, everything will work.
> 
Yes, but only for that case. You'd still have the problem that you'll need 
to keep the state somewhere to be able to remove the pre-loaded overlay(s) later
on. If you don't have hardware auto-detection, I would assume that to be in user
space. No matter if the system auto-detects hardware or not, the same mechanism
could be used: If, on startup, the hardware status (no matter if kept in some
file or auto-detected) indicates that an overlay should be loaded, load it.

If, for some reason, the overlay manager can not auto-detect hardware but keeps
state in the kernel, it may make sense to have that state passed to the new kernel.
However, that should be part of the overlay manager implementation, and not be
dictated by the core overlay mechanism.

Thanks,
Guenter

Re: [PATCH v4 2/8] OF: Introduce DT overlay support.

[Grant Likely]

On Tue, 27 May 2014 15:24:35 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
> 
> On May 27, 2014, at 3:12 PM, Grant Likely wrote:
> 
> > On Mon, 26 May 2014 16:42:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> >> On 05/26/2014 03:36 PM, Sebastian Reichel wrote:
> >>> Hi,
> >>> 
> >>> On Mon, May 26, 2014 at 10:33:03PM +0100, Grant Likely wrote:
> >>>> After thinking about it more, I think it is very likely that removing
> >>>> all the overlays is the correct thing to do in the kexec use-case. When
> >>>> kexec-ing, it makes sense that we'd want the exact same behaviour from
> >>>> the kexec'ed kernel. That means we want the device drivers to do the
> >>>> same thing including loading whatever overlays they depend on.
> >>>> 
> >>>> If the flattened tree was left applied, then the behaviour becomes
> >>>> different.
> >>>> 
> >>>> I say always remove the overlays unless explicitly told not to, but I'm
> >>>> struggling to come up with use cases where keeping them applied is
> >>>> desirable.
> >>> 
> >>> I would assume, that I want them applied in most cases. DT describes
> >>> the hardware. If I kexec into a new kernel I change software, not
> >>> hardware.
> >>> 
> >>> Maybe I'm missing the main purpose of the feature. I currently see
> >>> two useful usecases for DT overlays:
> >>> 
> >>> 1. The dtb the kernel is booted with cannot be changed for some
> >>>    reason, but the board has additional hardware attached (e.g.
> >>>    the user added a sensor on the i2c bus)
> >>> 2. The hardware is changed on the fly (e.g. the user flashed the
> >>>    FPGA part of a zynq processor), sensors on i2c bus, ...
> >>> 
> >>> In both cases the kernel should be booted with the additional
> >>> overlay information IMHO. Though for the second case it should
> >>> be possible to remove the "programmed" hardware information
> >>> somehow.
> >>> 
> >> 
> >> 3. Some hot-plug device or card is inserted or removed.
> >> 
> >> I would argue that the kernel should _not_ be booted with the overlay in place.
> >> Otherwise the code handling overlays would have to have special handling
> >> for the restart case, which is much more complex than just to re-insert
> >> the overlay when it is determined that the device or card is still there.
> > 
> > Exactly.
> > 
> 
> Looks like we are levitating to the 'remove overlays on kexec' approach.
> Is that correct?

Yes. What form that takes is yet to be decided. Kexec traditionally has
worked by userspace extracting the device tree from /proc/device-tree,
getting a kernel image into memory, and then doing the kexec call. It
wouldn't be very friendly to have kexec force all the overlays to be
removed while the kernel is still running.

We could solve this by making kexec use the original dtb blob instead of
the livetree. Otherwise we'd need an interface for userspace to extract
the original tree, or have the information to unwind the overlays. Not
exactly simple.

g.