[PATCH v3 0/3] Introducing Device Tree Overlays

[PATCH v3 0/3] Introducing Device Tree Overlays

[Pantelis Antoniou]

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 against mainline as of today, Nov 5 2013:
	be408cd3e1fef73e9408b196a79b9934697fe3b1 
	Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net

It relies on the following previously submitted patches/patchsets:

The DTC compiler patch required to get symbol resolution working

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

The I2C device registration patch

	"of: i2c: Export single device registration method"

And finally a patchset exporting various OF fixes

	"OF: Fixes in preperation of DT overlays"

Note that although this patchset allows DT overlay removal
on my preferred platform (omap), platform device registration
fails; another patchset that deals with this issue has been posted:

	"omap_device removal fixups"

I should also note that the /proc interface is an anachronism, but
it is the place where /proc/device-tree is also located, so it makes
sense to group /proc/device-tree & /proc/device-tree-overlay* together.
I am open to suggestions about where the generic interface should reside.
A suggestion has been made about configfs but I'd like to get this out
as a basis of discussion.

This low level interface should be used as a starting point for platforms
with discoverable hardware on the daughtercard level since they should use the
facilities provided to implement their own policy, dealing with things like
conflicts, high level resource allocation and so on.

Changes in V3:
 * Fixed of_free_overlay_info() missing static inline
 * Punting when handling PCI devices
 * Missing EXPORTs()
all by Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>

Changes in V2:
 * 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 (3):
  OF: Introduce Device Tree resolve support.
  OF: Introduce DT overlay support.
  DT: proc: Add runtime overlay interface in /proc

 .../devicetree/dynamic-resolution-notes.txt        |  25 +
 Documentation/devicetree/overlay-notes.txt         | 179 +++++
 drivers/of/Kconfig                                 |  19 +
 drivers/of/Makefile                                |   2 +
 drivers/of/overlay.c                               | 886 +++++++++++++++++++++
 drivers/of/resolver.c                              | 396 +++++++++
 fs/proc/proc_devtree.c                             | 278 +++++++
 include/linux/of.h                                 | 130 +++
 8 files changed, 1915 insertions(+)
 create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
 create mode 100644 Documentation/devicetree/overlay-notes.txt
 create mode 100644 drivers/of/overlay.c
 create mode 100644 drivers/of/resolver.c

-- 
1.7.12

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

[PATCH v3 1/3] 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 <panto@antoniou-consulting.com>
---
 .../devicetree/dynamic-resolution-notes.txt        |  25 ++
 drivers/of/Kconfig                                 |   9 +
 drivers/of/Makefile                                |   1 +
 drivers/of/resolver.c                              | 396 +++++++++++++++++++++
 include/linux/of.h                                 |  17 +
 5 files changed, 448 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 78cc760..2a00ae5 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -74,4 +74,13 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_RESOLVE
+	bool "OF Dynamic resolution support"
+	depends on OF
+	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 9bc6d8c..93da457 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 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_RESOLVE)  += resolver.o
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
new file mode 100644
index 0000000..dfbb51a
--- /dev/null
+++ b/drivers/of/resolver.c
@@ -0,0 +1,396 @@
+/*
+ * 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/of_fdt.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+
+/**
+ * Find a subtree's maximum phandle value.
+ */
+static phandle __of_get_tree_max_phandle(struct device_node *node,
+		phandle max_phandle)
+{
+	struct device_node *child;
+
+	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
+			node->phandle > max_phandle)
+		max_phandle = node->phandle;
+
+	__for_each_child_of_node(node, child)
+		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
+
+	return max_phandle;
+}
+
+/**
+ * Find live tree's maximum phandle value.
+ */
+static phandle of_get_tree_max_phandle(void)
+{
+	struct device_node *node;
+	phandle phandle;
+	unsigned long flags;
+
+	/* get root node */
+	node = of_find_node_by_path("/");
+	if (node == NULL)
+		return OF_PHANDLE_ILLEGAL;
+
+	/* now search recursively */
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	phandle = __of_get_tree_max_phandle(node, 0);
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	of_node_put(node);
+
+	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_cpu(*(uint32_t *)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 == NULL)
+		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 == NULL) {
+			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 == NULL) {
+				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 == NULL) {
+				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 == NULL) {
+				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 == NULL) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			phandle = be32_to_cpu(*(uint32_t *)
+					(sprop->value + offset));
+			*(uint32_t *)(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 == NULL ||
+			!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 == NULL &&
+				of_node_cmp(child->name, "__symbols__") == 0)
+			resolve_sym = child;
+
+		if (resolve_fix == NULL &&
+				of_node_cmp(child->name, "__fixups__") == 0)
+			resolve_fix = child;
+
+		/* both found, don't bother anymore */
+		if (resolve_sym != NULL && resolve_fix != NULL)
+			break;
+	}
+
+	/* we do allow for the case where no fixups are needed */
+	if (resolve_fix == NULL)
+		goto merge_sym;
+
+	/* we need to fixup, but no root symbols... */
+	if (root_sym == NULL)
+		return -EINVAL;
+
+	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 err_fail;
+		}
+
+		refnode = of_find_node_by_path(refpath);
+		if (refnode == NULL) {
+			pr_err("%s: Could not find node by path '%s'\n",
+					__func__, refpath);
+			err = -ENOENT;
+			goto err_fail;
+		}
+
+		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 == NULL) {
+			pr_err("%s: Could not copy value of '%s'\n",
+					__func__, rprop->name);
+			err = -ENOMEM;
+			goto err_fail;
+		}
+
+		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 == NULL) {
+				pr_err("%s: Illegal symbol "
+					"entry '%s' (1)\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				err = -EINVAL;
+				goto err_fail;
+			}
+			*s++ = '\0';
+
+			propstr = s;
+			s = strchr(s, ':');
+			if (s == NULL) {
+				pr_err("%s: Illegal symbol "
+					"entry '%s' (2)\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				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(resolve,
+					nodestr);
+			if (refnode == NULL) {
+				pr_err("%s: Could not find refnode '%s'\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			/* now find the property */
+			for_each_property_of_node(refnode, sprop) {
+				if (of_prop_cmp(sprop->name, propstr) == 0)
+					break;
+			}
+
+			if (sprop == NULL) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			*(uint32_t *)(sprop->value + offset) =
+				cpu_to_be32(phandle);
+		}
+
+		kfree(propval);
+	}
+
+merge_sym:
+
+	of_node_put(root_sym);
+
+	return 0;
+
+err_fail:
+
+	if (root_sym != NULL)
+		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 9d69bd2..22d42e5 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -721,4 +721,21 @@ static inline int of_multi_prop_cmp(const struct property *prop, const char *val
 
 #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

Re: [PATCH v3 1/3] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Fri,  8 Nov 2013 17:06:08 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> 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-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> ---
>  .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>  drivers/of/Kconfig                                 |   9 +
>  drivers/of/Makefile                                |   1 +
>  drivers/of/resolver.c                              | 396 +++++++++++++++++++++
>  include/linux/of.h                                 |  17 +
>  5 files changed, 448 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]

dt-object-internal.txt is in the DTC patch, not the kernel tree.

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

Missing footnote reference line for [1]?

> +
> +In sequence the resolver works by the following steps:
> +
> +1. Get the maximum device tree phandle value from the live tree + 1.

Is there a (realistic) worry about leaking phandle number space from
plugging/unplugging trees repeated addition/removal of overlays?

> +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 78cc760..2a00ae5 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -74,4 +74,13 @@ config OF_MTD
>  	depends on MTD
>  	def_bool y
>  
> +config OF_RESOLVE
> +	bool "OF Dynamic resolution support"
> +	depends on OF
> +	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 9bc6d8c..93da457 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  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_RESOLVE)  += resolver.o
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> new file mode 100644
> index 0000000..dfbb51a
> --- /dev/null
> +++ b/drivers/of/resolver.c
> @@ -0,0 +1,396 @@
> +/*
> + * Functions for dealing with DT resolution
> + *
> + * Copyright (C) 2012 Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> + * 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/of_fdt.h>
> +#include <linux/string.h>
> +#include <linux/ctype.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +
> +/**
> + * Find a subtree's maximum phandle value.
> + */
> +static phandle __of_get_tree_max_phandle(struct device_node *node,
> +		phandle max_phandle)
> +{
> +	struct device_node *child;
> +
> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
> +			node->phandle > max_phandle)
> +		max_phandle = node->phandle;
> +
> +	__for_each_child_of_node(node, child)
> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
> +
> +	return max_phandle;
> +}
> +
> +/**
> + * Find live tree's maximum phandle value.
> + */
> +static phandle of_get_tree_max_phandle(void)
> +{
> +	struct device_node *node;
> +	phandle phandle;
> +	unsigned long flags;
> +
> +	/* get root node */
> +	node = of_find_node_by_path("/");
> +	if (node == NULL)
> +		return OF_PHANDLE_ILLEGAL;
> +
> +	/* now search recursively */
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	phandle = __of_get_tree_max_phandle(node, 0);
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);

I don't see another user. What is the reason for the __ version of
of_get_tree_max_phandle?

> +
> +	of_node_put(node);
> +
> +	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_cpu(*(uint32_t *)prop->value);

Unnecessary cast if you use:
	phandle = be32_to_cpup(prop->value);

> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
> +			continue;

Isn't this an error condition? Should never have OF_PHANDLE_ILLEGAL in
the property here.

> +
> +		/* adjust */
> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);

*(__be32*)prop->value = ...

> +	}
> +
> +	/* 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 == NULL)
> +		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 == NULL) {
> +			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 == NULL) {
> +				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 == NULL) {
> +				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 == NULL) {
> +				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 == NULL) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail;
> +			}
> +
> +			phandle = be32_to_cpu(*(uint32_t *)
> +					(sprop->value + offset));
> +			*(uint32_t *)(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 == NULL ||
> +			!of_node_check_flag(resolve, OF_DETACHED)) {
> +		return -EINVAL;
> +	}
> +
> +	/* first we need to adjust the phandles */
> +	phandle_delta = of_get_tree_max_phandle() + 1;

Probably need to grab the devtree lock before doing the above, and not
release it until after the trees are merged. Otherwise there is the
potential of trying to merge two trees at once and getting phandle
conflicts.

Overall the patch looks pretty nice. I'm looking forward to hearing back
on the questions above.

g.

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

Re: [PATCH v3 1/3] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

On Nov 11, 2013, at 7:17 PM, Grant Likely wrote:

> On Fri,  8 Nov 2013 17:06:08 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> 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-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>> 
>> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
>> ---
>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>> drivers/of/Kconfig                                 |   9 +
>> drivers/of/Makefile                                |   1 +
>> drivers/of/resolver.c                              | 396 +++++++++++++++++++++
>> include/linux/of.h                                 |  17 +
>> 5 files changed, 448 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]
> 
> dt-object-internal.txt is in the DTC patch, not the kernel tree.
> 

Yes, good catch. I will fix the reference.

BTW, what about moving/copying some of the DTC docs in the kernel doc
directory? The dtc Documentation directory is missing from the kernel tree.


>> +
>> +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].
> 
> Missing footnote reference line for [1]?
> 

Yes.

>> +
>> +In sequence the resolver works by the following steps:
>> +
>> +1. Get the maximum device tree phandle value from the live tree + 1.
> 
> Is there a (realistic) worry about leaking phandle number space from
> plugging/unplugging trees repeated addition/removal of overlays?
> 

I think not. But doing it this way has the nice property of keeping all phandle
values the same each time you do a load-unload-load sequence. 

>> +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 78cc760..2a00ae5 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -74,4 +74,13 @@ config OF_MTD
>> 	depends on MTD
>> 	def_bool y
>> 
>> +config OF_RESOLVE
>> +	bool "OF Dynamic resolution support"
>> +	depends on OF
>> +	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 9bc6d8c..93da457 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>> 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_RESOLVE)  += resolver.o
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> new file mode 100644
>> index 0000000..dfbb51a
>> --- /dev/null
>> +++ b/drivers/of/resolver.c
>> @@ -0,0 +1,396 @@
>> +/*
>> + * Functions for dealing with DT resolution
>> + *
>> + * Copyright (C) 2012 Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
>> + * 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/of_fdt.h>
>> +#include <linux/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +
>> +/**
>> + * Find a subtree's maximum phandle value.
>> + */
>> +static phandle __of_get_tree_max_phandle(struct device_node *node,
>> +		phandle max_phandle)
>> +{
>> +	struct device_node *child;
>> +
>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
>> +			node->phandle > max_phandle)
>> +		max_phandle = node->phandle;
>> +
>> +	__for_each_child_of_node(node, child)
>> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
>> +
>> +	return max_phandle;
>> +}
>> +
>> +/**
>> + * Find live tree's maximum phandle value.
>> + */
>> +static phandle of_get_tree_max_phandle(void)
>> +{
>> +	struct device_node *node;
>> +	phandle phandle;
>> +	unsigned long flags;
>> +
>> +	/* get root node */
>> +	node = of_find_node_by_path("/");
>> +	if (node == NULL)
>> +		return OF_PHANDLE_ILLEGAL;
>> +
>> +	/* now search recursively */
>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	phandle = __of_get_tree_max_phandle(node, 0);
>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> 
> I don't see another user. What is the reason for the __ version of
> of_get_tree_max_phandle?
> 
>> +
>> +	of_node_put(node);
>> +
>> +	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_cpu(*(uint32_t *)prop->value);
> 
> Unnecessary cast if you use:
> 	phandle = be32_to_cpup(prop->value);

OK.

> 
>> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
>> +			continue;
> 
> Isn't this an error condition? Should never have OF_PHANDLE_ILLEGAL in
> the property here.

Hmm, I think so. I'll see if there's anything special there.

> 
>> +
>> +		/* adjust */
>> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
> 
> *(__be32*)prop->value = ...


It is the same for the compiler, but you're right. 
> 
>> +	}
>> +
>> +	/* 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 == NULL)
>> +		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 == NULL) {
>> +			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 == NULL) {
>> +				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 == NULL) {
>> +				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 == NULL) {
>> +				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 == NULL) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			phandle = be32_to_cpu(*(uint32_t *)
>> +					(sprop->value + offset));
>> +			*(uint32_t *)(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 == NULL ||
>> +			!of_node_check_flag(resolve, OF_DETACHED)) {
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* first we need to adjust the phandles */
>> +	phandle_delta = of_get_tree_max_phandle() + 1;
> 
> Probably need to grab the devtree lock before doing the above, and not
> release it until after the trees are merged. Otherwise there is the
> potential of trying to merge two trees at once and getting phandle
> conflicts.

No, because the device tree being passed it it guaranteed to be
in detached state, it is not part of the live device tree; 
the check in the beginning of the function makes sure.

When we apply the overlay the devtree lock is taken properly.

> 
> Overall the patch looks pretty nice. I'm looking forward to hearing back
> on the questions above.
> 

Thank you
> g.
> 

Regards

-- Pantelis

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

Re: [PATCH v3 1/3] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Tue, 12 Nov 2013 09:28:42 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> 
> On Nov 11, 2013, at 7:17 PM, Grant Likely wrote:
> 
> > On Fri,  8 Nov 2013 17:06:08 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >> 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-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> >> ---
> >> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
> >> drivers/of/Kconfig                                 |   9 +
> >> drivers/of/Makefile                                |   1 +
> >> drivers/of/resolver.c                              | 396 +++++++++++++++++++++
> >> include/linux/of.h                                 |  17 +
> >> 5 files changed, 448 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]
> > 
> > dt-object-internal.txt is in the DTC patch, not the kernel tree.
> > 
> 
> Yes, good catch. I will fix the reference.
> 
> BTW, what about moving/copying some of the DTC docs in the kernel doc
> directory? The dtc Documentation directory is missing from the kernel tree.
> 
> 
> >> +
> >> +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].
> > 
> > Missing footnote reference line for [1]?
> > 
> 
> Yes.
> 
> >> +
> >> +In sequence the resolver works by the following steps:
> >> +
> >> +1. Get the maximum device tree phandle value from the live tree + 1.
> > 
> > Is there a (realistic) worry about leaking phandle number space from
> > plugging/unplugging trees repeated addition/removal of overlays?
> > 
> 
> I think not. But doing it this way has the nice property of keeping all phandle
> values the same each time you do a load-unload-load sequence. 

It will break if there are two overlays "leapfrogging" each other on
loads/unloads. It may be a very outside corner case, but it is worth
thinking about.

> 
> >> +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 78cc760..2a00ae5 100644
> >> --- a/drivers/of/Kconfig
> >> +++ b/drivers/of/Kconfig
> >> @@ -74,4 +74,13 @@ config OF_MTD
> >> 	depends on MTD
> >> 	def_bool y
> >> 
> >> +config OF_RESOLVE
> >> +	bool "OF Dynamic resolution support"
> >> +	depends on OF
> >> +	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 9bc6d8c..93da457 100644
> >> --- a/drivers/of/Makefile
> >> +++ b/drivers/of/Makefile
> >> @@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
> >> 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_RESOLVE)  += resolver.o
> >> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> >> new file mode 100644
> >> index 0000000..dfbb51a
> >> --- /dev/null
> >> +++ b/drivers/of/resolver.c
> >> @@ -0,0 +1,396 @@
> >> +/*
> >> + * Functions for dealing with DT resolution
> >> + *
> >> + * Copyright (C) 2012 Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> >> + * 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/of_fdt.h>
> >> +#include <linux/string.h>
> >> +#include <linux/ctype.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/string.h>
> >> +#include <linux/slab.h>
> >> +
> >> +/**
> >> + * Find a subtree's maximum phandle value.
> >> + */
> >> +static phandle __of_get_tree_max_phandle(struct device_node *node,
> >> +		phandle max_phandle)
> >> +{
> >> +	struct device_node *child;
> >> +
> >> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
> >> +			node->phandle > max_phandle)
> >> +		max_phandle = node->phandle;
> >> +
> >> +	__for_each_child_of_node(node, child)
> >> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
> >> +
> >> +	return max_phandle;
> >> +}
> >> +
> >> +/**
> >> + * Find live tree's maximum phandle value.
> >> + */
> >> +static phandle of_get_tree_max_phandle(void)
> >> +{
> >> +	struct device_node *node;
> >> +	phandle phandle;
> >> +	unsigned long flags;
> >> +
> >> +	/* get root node */
> >> +	node = of_find_node_by_path("/");
> >> +	if (node == NULL)
> >> +		return OF_PHANDLE_ILLEGAL;
> >> +
> >> +	/* now search recursively */
> >> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> >> +	phandle = __of_get_tree_max_phandle(node, 0);
> >> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > 
> > I don't see another user. What is the reason for the __ version of
> > of_get_tree_max_phandle?
> > 
> >> +
> >> +	of_node_put(node);
> >> +
> >> +	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_cpu(*(uint32_t *)prop->value);
> > 
> > Unnecessary cast if you use:
> > 	phandle = be32_to_cpup(prop->value);
> 
> OK.
> 
> > 
> >> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
> >> +			continue;
> > 
> > Isn't this an error condition? Should never have OF_PHANDLE_ILLEGAL in
> > the property here.
> 
> Hmm, I think so. I'll see if there's anything special there.
> 
> > 
> >> +
> >> +		/* adjust */
> >> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
> > 
> > *(__be32*)prop->value = ...
> 
> 
> It is the same for the compiler, but you're right. 
> > 
> >> +	}
> >> +
> >> +	/* 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 == NULL)
> >> +		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 == NULL) {
> >> +			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 == NULL) {
> >> +				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 == NULL) {
> >> +				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 == NULL) {
> >> +				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 == NULL) {
> >> +				pr_err("%s: Could not find property '%s'\n",
> >> +					__func__, (char *)rprop->value);
> >> +				err = -ENOENT;
> >> +				goto err_fail;
> >> +			}
> >> +
> >> +			phandle = be32_to_cpu(*(uint32_t *)
> >> +					(sprop->value + offset));
> >> +			*(uint32_t *)(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 == NULL ||
> >> +			!of_node_check_flag(resolve, OF_DETACHED)) {
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/* first we need to adjust the phandles */
> >> +	phandle_delta = of_get_tree_max_phandle() + 1;
> > 
> > Probably need to grab the devtree lock before doing the above, and not
> > release it until after the trees are merged. Otherwise there is the
> > potential of trying to merge two trees at once and getting phandle
> > conflicts.
> 
> No, because the device tree being passed it it guaranteed to be
> in detached state, it is not part of the live device tree; 
> the check in the beginning of the function makes sure.
> 
> When we apply the overlay the devtree lock is taken properly.

That doesn't protect against getting duplicate phandle bases. You need
something to protect the range of phandles that the overlay trees will
want to use. The problem with the above code is that it calculates the
phandle base that it wants, but then goes and does a bunch of stuff
without a lock which allows another overlay to try and use the same
phandle range.

g.

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

Re: [PATCH v3 1/3] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi Grant,

On Nov 13, 2013, at 2:39 AM, Grant Likely wrote:

> On Tue, 12 Nov 2013 09:28:42 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> 
>> On Nov 11, 2013, at 7:17 PM, Grant Likely wrote:
>> 
>>> On Fri,  8 Nov 2013 17:06:08 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>>>> 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-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
>>>> ---
>>>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>>>> drivers/of/Kconfig                                 |   9 +
>>>> drivers/of/Makefile                                |   1 +
>>>> drivers/of/resolver.c                              | 396 +++++++++++++++++++++
>>>> include/linux/of.h                                 |  17 +
>>>> 5 files changed, 448 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]
>>> 
>>> dt-object-internal.txt is in the DTC patch, not the kernel tree.
>>> 
>> 
>> Yes, good catch. I will fix the reference.
>> 
>> BTW, what about moving/copying some of the DTC docs in the kernel doc
>> directory? The dtc Documentation directory is missing from the kernel tree.
>> 
>> 
>>>> +
>>>> +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].
>>> 
>>> Missing footnote reference line for [1]?
>>> 
>> 
>> Yes.
>> 
>>>> +
>>>> +In sequence the resolver works by the following steps:
>>>> +
>>>> +1. Get the maximum device tree phandle value from the live tree + 1.
>>> 
>>> Is there a (realistic) worry about leaking phandle number space from
>>> plugging/unplugging trees repeated addition/removal of overlays?
>>> 
>> 
>> I think not. But doing it this way has the nice property of keeping all phandle
>> values the same each time you do a load-unload-load sequence. 
> 
> It will break if there are two overlays "leapfrogging" each other on
> loads/unloads. It may be a very outside corner case, but it is worth
> thinking about.
> 

Normally that can't happen when using an overlay manager when these calls
are taken while holding it's lock.

But I see your point. I will rework it.

>> 
>>>> +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 78cc760..2a00ae5 100644
>>>> --- a/drivers/of/Kconfig
>>>> +++ b/drivers/of/Kconfig
>>>> @@ -74,4 +74,13 @@ config OF_MTD
>>>> 	depends on MTD
>>>> 	def_bool y
>>>> 
>>>> +config OF_RESOLVE
>>>> +	bool "OF Dynamic resolution support"
>>>> +	depends on OF
>>>> +	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 9bc6d8c..93da457 100644
>>>> --- a/drivers/of/Makefile
>>>> +++ b/drivers/of/Makefile
>>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>>>> 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_RESOLVE)  += resolver.o
>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>> new file mode 100644
>>>> index 0000000..dfbb51a
>>>> --- /dev/null
>>>> +++ b/drivers/of/resolver.c
>>>> @@ -0,0 +1,396 @@
>>>> +/*
>>>> + * Functions for dealing with DT resolution
>>>> + *
>>>> + * Copyright (C) 2012 Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
>>>> + * 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/of_fdt.h>
>>>> +#include <linux/string.h>
>>>> +#include <linux/ctype.h>
>>>> +#include <linux/errno.h>
>>>> +#include <linux/string.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +/**
>>>> + * Find a subtree's maximum phandle value.
>>>> + */
>>>> +static phandle __of_get_tree_max_phandle(struct device_node *node,
>>>> +		phandle max_phandle)
>>>> +{
>>>> +	struct device_node *child;
>>>> +
>>>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
>>>> +			node->phandle > max_phandle)
>>>> +		max_phandle = node->phandle;
>>>> +
>>>> +	__for_each_child_of_node(node, child)
>>>> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
>>>> +
>>>> +	return max_phandle;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Find live tree's maximum phandle value.
>>>> + */
>>>> +static phandle of_get_tree_max_phandle(void)
>>>> +{
>>>> +	struct device_node *node;
>>>> +	phandle phandle;
>>>> +	unsigned long flags;
>>>> +
>>>> +	/* get root node */
>>>> +	node = of_find_node_by_path("/");
>>>> +	if (node == NULL)
>>>> +		return OF_PHANDLE_ILLEGAL;
>>>> +
>>>> +	/* now search recursively */
>>>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>>>> +	phandle = __of_get_tree_max_phandle(node, 0);
>>>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>> 
>>> I don't see another user. What is the reason for the __ version of
>>> of_get_tree_max_phandle?
>>> 
>>>> +
>>>> +	of_node_put(node);
>>>> +
>>>> +	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_cpu(*(uint32_t *)prop->value);
>>> 
>>> Unnecessary cast if you use:
>>> 	phandle = be32_to_cpup(prop->value);
>> 
>> OK.
>> 
>>> 
>>>> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
>>>> +			continue;
>>> 
>>> Isn't this an error condition? Should never have OF_PHANDLE_ILLEGAL in
>>> the property here.
>> 
>> Hmm, I think so. I'll see if there's anything special there.
>> 
>>> 
>>>> +
>>>> +		/* adjust */
>>>> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>>> 
>>> *(__be32*)prop->value = ...
>> 
>> 
>> It is the same for the compiler, but you're right. 
>>> 
>>>> +	}
>>>> +
>>>> +	/* 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 == NULL)
>>>> +		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 == NULL) {
>>>> +			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 == NULL) {
>>>> +				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 == NULL) {
>>>> +				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 == NULL) {
>>>> +				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 == NULL) {
>>>> +				pr_err("%s: Could not find property '%s'\n",
>>>> +					__func__, (char *)rprop->value);
>>>> +				err = -ENOENT;
>>>> +				goto err_fail;
>>>> +			}
>>>> +
>>>> +			phandle = be32_to_cpu(*(uint32_t *)
>>>> +					(sprop->value + offset));
>>>> +			*(uint32_t *)(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 == NULL ||
>>>> +			!of_node_check_flag(resolve, OF_DETACHED)) {
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	/* first we need to adjust the phandles */
>>>> +	phandle_delta = of_get_tree_max_phandle() + 1;
>>> 
>>> Probably need to grab the devtree lock before doing the above, and not
>>> release it until after the trees are merged. Otherwise there is the
>>> potential of trying to merge two trees at once and getting phandle
>>> conflicts.
>> 
>> No, because the device tree being passed it it guaranteed to be
>> in detached state, it is not part of the live device tree; 
>> the check in the beginning of the function makes sure.
>> 
>> When we apply the overlay the devtree lock is taken properly.
> 
> That doesn't protect against getting duplicate phandle bases. You need
> something to protect the range of phandles that the overlay trees will
> want to use. The problem with the above code is that it calculates the
> phandle base that it wants, but then goes and does a bunch of stuff
> without a lock which allows another overlay to try and use the same
> phandle range.
> 

Yes, I see. I'll try to see how that can be reworked.

> g.
> 

Regards

-- Pantelis


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

[PATCH v3 2/3] 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.

Note that the I2C client devices are 'special', as in they're not platform
devices. They need to be registered with an I2C specific method.

In general I2C is just no good platform device citizen and needs
special casing.

PCI is another special case where PCI device insertion and removal
is implemented in the PCI subsystem.

Bug fixes & PCI support by Guenter Roeck <groeck@juniper.net>

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 Documentation/devicetree/overlay-notes.txt | 179 ++++++
 drivers/of/Kconfig                         |  10 +
 drivers/of/Makefile                        |   1 +
 drivers/of/overlay.c                       | 886 +++++++++++++++++++++++++++++
 include/linux/of.h                         | 113 ++++
 5 files changed, 1189 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..5289cbb
--- /dev/null
+++ b/Documentation/devicetree/overlay-notes.txt
@@ -0,0 +1,179 @@
+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>;	/* target 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. 
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 2a00ae5..c2d5596 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,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 93da457..ca466e4 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_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..7d14881
--- /dev/null
+++ b/drivers/of/overlay.c
@@ -0,0 +1,886 @@
+/*
+ * 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/i2c.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>
+
+/**
+ * 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);
+			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);
+
+				/* 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,
+		struct platform_device *pdev, struct i2c_client *client,
+		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 (de->pdev == NULL && pdev)
+		de->pdev = of_dev_get(pdev);
+	if (de->client == NULL && client)
+		de->client = i2c_use_client(client);
+	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 platform_device *pdev;
+	struct i2c_client *client;
+	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) */
+	pdev = NULL;
+	client = NULL;
+	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;
+		pdev = of_find_device_by_node(node);
+		client = of_find_i2c_device_by_node(node);
+		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;
+	}
+
+	if (state == 0) {
+		pdev = of_find_device_by_node(node);
+		client = of_find_i2c_device_by_node(node);
+	}
+
+	of_overlay_device_entry_entry_add(ovinfo, node, pdev, client,
+			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)
+{
+	struct i2c_adapter *adap = NULL;
+	struct i2c_client *client;
+	struct platform_device *pdev, *parent_pdev = NULL;
+	int state;
+	struct device_node *np;
+	bool is_pci = false;
+
+	np = de->np;
+	while (np && !is_pci) {
+		is_pci = np->type && !of_node_cmp(np->type, "pci");
+		np = np->parent;
+	}
+
+	if (is_pci) {
+		pr_debug("%s: Node %s is pci node, leaving it alone\n",
+			__func__, np->full_name);
+		return 0;
+	}
+
+	state = !!de->state ^ !!revert;
+
+	if (de->np && de->np->parent) {
+		pr_debug("%s: parent is %s\n",
+				__func__, de->np->parent->full_name);
+		adap = of_find_i2c_adapter_by_node(de->np->parent);
+		if (adap == NULL)
+			parent_pdev = of_find_device_by_node(de->np->parent);
+	}
+
+	if (state) {
+
+		/* try to see if it's an I2C client node */
+		if (adap == NULL) {
+
+			pr_debug("%s: creating new platform device "
+					"new_node='%s' %p\n",
+					__func__, de->np->full_name, de->np);
+
+			pdev = of_platform_device_create(de->np, NULL,
+					parent_pdev ? &parent_pdev->dev : NULL);
+			if (pdev == NULL) {
+				pr_warn("%s: Failed to create platform device "
+						"for '%s'\n",
+						__func__, de->np->full_name);
+			} else
+				de->pdev = pdev;
+
+		} else {
+
+			client = of_find_i2c_device_by_node(de->np);
+			if (client != NULL) {
+				/* bus already created the device; do nothing */
+				put_device(&client->dev);
+			} else {
+				pr_debug("%s: creating new i2c_client device "
+						"new_node='%s' %p\n",
+						__func__, de->np->full_name, de->np);
+
+				client = of_i2c_register_device(adap, de->np);
+
+				if (client == NULL) {
+					pr_warn("%s: Failed to create i2c client device "
+							"for '%s'\n",
+							__func__, de->np->full_name);
+				} else
+					de->client = client;
+			}
+		}
+
+	} else {
+
+		if (de->pdev) {
+			pr_debug("%s: removing pdev %s\n", __func__,
+					dev_name(&de->pdev->dev));
+			platform_device_unregister(de->pdev);
+			de->pdev = NULL;
+		}
+
+		if (de->client) {
+			pr_debug("%s: removing i2c client %s\n", __func__,
+					dev_name(&de->client->dev));
+			i2c_unregister_device(de->client);
+			de->client = NULL;
+		}
+	}
+
+	if (adap)
+		put_device(&adap->dev);
+
+	if (parent_pdev)
+		of_dev_put(parent_pdev);
+
+	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 platform 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;
+	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) {
+
+		ret = 0;
+		switch (le->action) {
+		case OF_RECONFIG_ATTACH_NODE:
+			pr_debug("Reverting ATTACH_NODE %s\n",
+					le->np->full_name);
+			ret = of_detach_node(le->np);
+			break;
+
+		case OF_RECONFIG_DETACH_NODE:
+			pr_debug("Reverting DETACH_NODE %s\n",
+					le->np->full_name);
+			ret = of_attach_node(le->np);
+			break;
+
+		case OF_RECONFIG_ADD_PROPERTY:
+			pr_debug("Reverting ADD_PROPERTY %s %s\n",
+					le->np->full_name, le->prop->name);
+			ret = of_remove_property(le->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",
+					le->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 = &le->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);
+				if (prop == NULL) {
+					pr_err("%s: Failed to copy property\n",
+							__func__);
+					break;
+				}
+			}
+
+			if (le->action == OF_RECONFIG_REMOVE_PROPERTY)
+				ret = of_add_property(le->np, prop);
+			else
+				ret = of_update_property(le->np, prop);
+			break;
+
+		default:
+			/* nothing */
+			break;
+		}
+
+		if (ret != 0)
+			pr_err("%s: revert on node %s Failed!\n",
+					__func__, le->np->full_name);
+
+		of_node_put(le->np);
+
+		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 iterare over the device entries
+ * and create/destroy the platform 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);
+		} else {
+			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;
+}
+
+/**
+ * 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;
+
+	ret = of_property_read_u32(info_node, "target", &val);
+	if (ret != 0)
+		goto err_fail;
+
+	ovinfo->target = of_find_node_by_phandle(val);
+	if (ovinfo->target == NULL)
+		goto err_fail;
+
+	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
+	if (ovinfo->overlay == 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 22d42e5..4e19013 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/i2c.h>
 
 #include <asm/byteorder.h>
 #include <asm/errno.h>
@@ -738,4 +739,116 @@ 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	- Holds an overlay device entry
+ * @node:	list_head for the device list
+ * @np:		device node pointer to the device node affected
+ * @pdev:	pointer to the platform device affected
+ * @client:	pointer to the I2C client device affected
+ * @state:	new device state
+ * @prevstate:	previous device state
+ *
+ * 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 platform devices according
+ * to the new state of the live tree.
+ */
+struct of_overlay_device_entry {
+	struct list_head node;
+	struct device_node *np;
+	struct platform_device *pdev;
+	struct i2c_client *client;
+	int prevstate;
+	int state;
+};
+
+/**
+ * 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);
+
+#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;
+}
+
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Guenter Roeck]

On Fri, Nov 08, 2013 at 05:06:09PM +0200, Pantelis Antoniou 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.
> 
> Note that the I2C client devices are 'special', as in they're not platform
> devices. They need to be registered with an I2C specific method.
> 
> In general I2C is just no good platform device citizen and needs
> special casing.
> 
> PCI is another special case where PCI device insertion and removal
> is implemented in the PCI subsystem.
> 
> Bug fixes & PCI support by Guenter Roeck <groeck@juniper.net>
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>

Hi Pantelis,

another nitpick:

.git/rebase-apply/patch:195: trailing whitespace.
end up filling in the fields of the of_overlay_info array.
warning: 1 line adds whitespace errors.

Guenter

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Grant Likely]

On Fri,  8 Nov 2013 17:06:09 +0200, Pantelis Antoniou <panto@antoniou-consulting.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.
> 
> Note that the I2C client devices are 'special', as in they're not platform
> devices. They need to be registered with an I2C specific method.
> 
> In general I2C is just no good platform device citizen and needs
> special casing.
> 
> PCI is another special case where PCI device insertion and removal
> is implemented in the PCI subsystem.

Actually. I2C isn't special in this regard; SPI, MDIO, PCI, USB are all
in the same boat. platform_device just happens to be able to make a few
assumptions. If anything, platform_device is the 'special' one. :-)

In all cases it must be the underlying subsystem that should be
responsible for creation/removal of devices described in the overlay.
Sometimes that will extend right down into the individual bus device
drivers, but it is better if that can be avoided.

> 
> Bug fixes & PCI support by Guenter Roeck <groeck@juniper.net>
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  Documentation/devicetree/overlay-notes.txt | 179 ++++++
>  drivers/of/Kconfig                         |  10 +
>  drivers/of/Makefile                        |   1 +
>  drivers/of/overlay.c                       | 886 +++++++++++++++++++++++++++++
>  include/linux/of.h                         | 113 ++++
>  5 files changed, 1189 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..5289cbb
> --- /dev/null
> +++ b/Documentation/devicetree/overlay-notes.txt
> @@ -0,0 +1,179 @@
> +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().

Make of_overlay_info a kobject and use a release method to free it when
all users go away. Freeing directly is a dangerous thing to do.

> +
> +/**
> + * 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>;	/* target 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 */
> +				...
> +			};

Are the node & property removals reversable? What about property
modifications? If they are not, then it would be better to not support
removals at all for now. Otherwise we'll end up with overlays that
cannot be removed and I don't want to inadvertently put users into that
situation.

> +		};
> +	}
> +	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. 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 2a00ae5..c2d5596 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,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 93da457..ca466e4 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_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..7d14881
> --- /dev/null
> +++ b/drivers/of/overlay.c
> @@ -0,0 +1,886 @@
> +/*
> + * 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/i2c.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>
> +
> +/**
> + * 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);
> +			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);
> +
> +				/* 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,
> +		struct platform_device *pdev, struct i2c_client *client,
> +		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 (de->pdev == NULL && pdev)
> +		de->pdev = of_dev_get(pdev);
> +	if (de->client == NULL && client)
> +		de->client = i2c_use_client(client);
> +	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 platform_device *pdev;
> +	struct i2c_client *client;
> +	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) */
> +	pdev = NULL;
> +	client = NULL;
> +	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;
> +		pdev = of_find_device_by_node(node);
> +		client = of_find_i2c_device_by_node(node);
> +		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;
> +	}
> +
> +	if (state == 0) {
> +		pdev = of_find_device_by_node(node);
> +		client = of_find_i2c_device_by_node(node);
> +	}
> +
> +	of_overlay_device_entry_entry_add(ovinfo, node, pdev, client,
> +			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)
> +{

I've got big problems here. This is entirely the wrong place to create
and delete devices. Each subsystem needs to create and remove its own
device types. To begin with, only the subsystem knows which busses are
appropriate for creating child devices.

Second, it is incorrect to create a platform_device for every single
node by default. Some nodes aren't devices at all. Just looking for a
compatible property isn't sufficient either.

The solution here is for each subsystem to have it's own notifier, and
only create a child device if the changed node has a parent the
subsystem already knows about (because it's already populated device
tree devices from that parent). This is also true for platform_devices.

Finally, sometimes the tips of the tree will get populated, but not
until after some parent nodes have been delt with. In that case the
creation of devices needs to be deferred back to the driver for the
parent bus.

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

Can overlays interact in bad ways? If overlay 1 is installed before
overlay 2, what happens if overlay 1 is removed?

Okay, my brain is tired. It's a complicated series and I don't see any
obvious bits that can be split off and be independently useful. I would
*really* like to see some test cases for this functionality. It's
complicated enough to make me quite nervous.... in fact I would really
like to see drivers/of/selftest.c become an early user of overlays. That
would make the DT selftests executable on any platform.

g.

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Grant,

On Nov 11, 2013, at 7:42 PM, Grant Likely wrote:

> On Fri,  8 Nov 2013 17:06:09 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> 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.
>> 
>> Note that the I2C client devices are 'special', as in they're not platform
>> devices. They need to be registered with an I2C specific method.
>> 
>> In general I2C is just no good platform device citizen and needs
>> special casing.
>> 
>> PCI is another special case where PCI device insertion and removal
>> is implemented in the PCI subsystem.
> 
> Actually. I2C isn't special in this regard; SPI, MDIO, PCI, USB are all
> in the same boat. platform_device just happens to be able to make a few
> assumptions. If anything, platform_device is the 'special' one. :-)
> 

I'm of the opinion that 'platform_device' shouldn't exist at all btw :)
Most of it's functionality can pretty easily be subsumed by device proper
and the world would be a better place :)
 
> In all cases it must be the underlying subsystem that should be
> responsible for creation/removal of devices described in the overlay.
> Sometimes that will extend right down into the individual bus device
> drivers, but it is better if that can be avoided.
> 

Yes, that's the right way to handle this, but we're talking about 
serious modification in almost every kind of device core.

Just getting the basic concept of overlays in is hard without having
to tackle something like this.

In the future, I'd be ecstatic if we could get a per device class
filter, but it's nothing we can fix right now.

IMHO of course.

>> 
>> Bug fixes & PCI support by Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>> 

[snip]

>> +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().
> 
> Make of_overlay_info a kobject and use a release method to free it when
> all users go away. Freeing directly is a dangerous thing to do.
> 

It's more dangerous than even that. We also have the mess with the way
unflattening works (properties point directly to binary blob area
with no way to free per property data). In general freeing/releasing
device tree nodes is tricky on a good day.

I agree we have to move to a kobject model eventually. 

>> +
>> +/**
>> + * 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>;	/* target 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 */
>> +				...
>> +			};
> 
> Are the node & property removals reversable? What about property
> modifications? If they are not, then it would be better to not support
> removals at all for now. Otherwise we'll end up with overlays that
> cannot be removed and I don't want to inadvertently put users into that
> situation.

Yes, everything is reversible. Doesn't mean that the drivers/driver core
can handle it. Using overlays is an excellent way to trigger bugs in
device removal as I've found out :)

> 
>> +		};
>> +	}
>> +	fragment@1 {	/* second child node */
>> +		...
>> +	};
>> +	/* more fragments follow */
>> +}

[snip]

>> +
>> +static int of_overlay_device_entry_change(struct of_overlay_info *ovinfo,
>> +		struct of_overlay_device_entry *de, int revert)
>> +{
> 
> I've got big problems here. This is entirely the wrong place to create
> and delete devices. Each subsystem needs to create and remove its own
> device types. To begin with, only the subsystem knows which busses are
> appropriate for creating child devices.
> 

It was the best I could under the circumstances, without having to touch
every subsystem.


> Second, it is incorrect to create a platform_device for every single
> node by default. Some nodes aren't devices at all. Just looking for a
> compatible property isn't sufficient either.
> 

Yep. 

> The solution here is for each subsystem to have it's own notifier, and
> only create a child device if the changed node has a parent the
> subsystem already knows about (because it's already populated device
> tree devices from that parent). This is also true for platform_devices.
> 
> Finally, sometimes the tips of the tree will get populated, but not
> until after some parent nodes have been delt with. In that case the
> creation of devices needs to be deferred back to the driver for the
> parent bus.
> 

Well, any solution that requires each and every subsystem to do dynamic
child insertions/removal is going to be hard sell.

What about something more gentle?

For example, I know that most of the cases fall into a couple of set
patterns for my case

a) Target is the ocp node (on chip peripheral node) and results in
the creation of platform devices under that node.

b) Target is one of the disabled devices under the ocp node
and the status property is modified, plus optional nodes are
added that result in the creation of more devices under the bus this
node controls. This is the case of enabled an I2C/SPI bus and inserting
children devices.

Perhaps the trick is having hooks about what to do in case certain
nodes are modified in a certain way, i.e. a notifier on the ocp node
would 'catch' those. The problem is a having a way to present this
information to the notifier in a way that's easy to be handled.

What do you think?

>> +
>> +/**
>> + * 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 platform 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;
>> +	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);
>> +	}
> 
> Can overlays interact in bad ways? If overlay 1 is installed before
> overlay 2, what happens if overlay 1 is removed?
> 

Yes, they can. It is not something easily fixed; the proper way would
be to calculate overlay intersection points and refuse to unload.

> Okay, my brain is tired. It's a complicated series and I don't see any
> obvious bits that can be split off and be independently useful. I would
> *really* like to see some test cases for this functionality. It's
> complicated enough to make me quite nervous.... in fact I would really
> like to see drivers/of/selftest.c become an early user of overlays. That
> would make the DT selftests executable on any platform.
> 

OK, I see your point. My brain is tired too, and I'm on travel too for
this week and the next. I'll see what I can come up with.

> g.
> 

Regards

-- Pantelis

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

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Grant Likely]

On Tue, 12 Nov 2013 10:30:37 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi Grant,
> 
> On Nov 11, 2013, at 7:42 PM, Grant Likely wrote:
> 
> > On Fri,  8 Nov 2013 17:06:09 +0200, Pantelis Antoniou <panto@antoniou-consulting.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.
> >> 
> >> Note that the I2C client devices are 'special', as in they're not platform
> >> devices. They need to be registered with an I2C specific method.
> >> 
> >> In general I2C is just no good platform device citizen and needs
> >> special casing.
> >> 
> >> PCI is another special case where PCI device insertion and removal
> >> is implemented in the PCI subsystem.
> > 
> > Actually. I2C isn't special in this regard; SPI, MDIO, PCI, USB are all
> > in the same boat. platform_device just happens to be able to make a few
> > assumptions. If anything, platform_device is the 'special' one. :-)
> > 
> 
> I'm of the opinion that 'platform_device' shouldn't exist at all btw :)
> Most of it's functionality can pretty easily be subsumed by device proper
> and the world would be a better place :)

I'm fine for merging some/all of the platform_device fields into struct
device. There are a few things, like resources, which would probably be
useful to have common on all struct device variants. However,
platform_device is far more about matching drivers to devices. Even if
all of platform_device went into struct device, there would still need
to be the platform_bus_type as the collection point for the device
drivers.

> > In all cases it must be the underlying subsystem that should be
> > responsible for creation/removal of devices described in the overlay.
> > Sometimes that will extend right down into the individual bus device
> > drivers, but it is better if that can be avoided.
> > 
> 
> Yes, that's the right way to handle this, but we're talking about 
> serious modification in almost every kind of device core.
> 
> Just getting the basic concept of overlays in is hard without having
> to tackle something like this.
> 
> In the future, I'd be ecstatic if we could get a per device class
> filter, but it's nothing we can fix right now.
> 
> IMHO of course.

It's fine to limit yourself to only platform_devices and i2c for now,
but that doesn't give license to do the wrong thing with the
implementation. The subsystem modifications really shouldn't be very
invasive. It will need a per-subsystem notifier callback to deal with
the add/remove events, and it will require the subsystem to maintain a
list of parent bus nodes that it cares about and should create children
for.

Doing it with a central function like this patch does is not a good
approach at all.

> 
> >> 
> >> Bug fixes & PCI support by Guenter Roeck <groeck@juniper.net>
> >> 
> 
> [snip]
> 
> >> +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().
> > 
> > Make of_overlay_info a kobject and use a release method to free it when
> > all users go away. Freeing directly is a dangerous thing to do.
> > 
> 
> It's more dangerous than even that. We also have the mess with the way
> unflattening works (properties point directly to binary blob area
> with no way to free per property data). In general freeing/releasing
> device tree nodes is tricky on a good day.

Repeating from my comment on the previous email, don't throw away the
blob. There is no need and it makes your job harder.

> 
> I agree we have to move to a kobject model eventually. 
> 
> >> +
> >> +/**
> >> + * 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>;	/* target 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 */
> >> +				...
> >> +			};
> > 
> > Are the node & property removals reversable? What about property
> > modifications? If they are not, then it would be better to not support
> > removals at all for now. Otherwise we'll end up with overlays that
> > cannot be removed and I don't want to inadvertently put users into that
> > situation.
> 
> Yes, everything is reversible. Doesn't mean that the drivers/driver core
> can handle it. Using overlays is an excellent way to trigger bugs in
> device removal as I've found out :)

:)

> 
> > 
> >> +		};
> >> +	}
> >> +	fragment@1 {	/* second child node */
> >> +		...
> >> +	};
> >> +	/* more fragments follow */
> >> +}
> 
> [snip]
> 
> >> +
> >> +static int of_overlay_device_entry_change(struct of_overlay_info *ovinfo,
> >> +		struct of_overlay_device_entry *de, int revert)
> >> +{
> > 
> > I've got big problems here. This is entirely the wrong place to create
> > and delete devices. Each subsystem needs to create and remove its own
> > device types. To begin with, only the subsystem knows which busses are
> > appropriate for creating child devices.
> > 
> 
> It was the best I could under the circumstances, without having to touch
> every subsystem.
> 
> 
> > Second, it is incorrect to create a platform_device for every single
> > node by default. Some nodes aren't devices at all. Just looking for a
> > compatible property isn't sufficient either.
> > 
> 
> Yep. 
> 
> > The solution here is for each subsystem to have it's own notifier, and
> > only create a child device if the changed node has a parent the
> > subsystem already knows about (because it's already populated device
> > tree devices from that parent). This is also true for platform_devices.
> > 
> > Finally, sometimes the tips of the tree will get populated, but not
> > until after some parent nodes have been delt with. In that case the
> > creation of devices needs to be deferred back to the driver for the
> > parent bus.
> > 
> 
> Well, any solution that requires each and every subsystem to do dynamic
> child insertions/removal is going to be hard sell.

They have to do it /anyway/! How many subsystems are we talking about?
SPI, I2C, platform_device (and amba_device). Maybe MDIO. To get to a
useful subset it isn't very many targets. SPI and I2C will probably be
receptive, so no problem there. The maintainer of drivers/of/platform.c
is friendly too. :-) Haven't asked about MDIO.

> 
> What about something more gentle?
> 
> For example, I know that most of the cases fall into a couple of set
> patterns for my case
> 
> a) Target is the ocp node (on chip peripheral node) and results in
> the creation of platform devices under that node.

No, I'm going to be hard-line on this. The code needs to be very
explicit about which nodes cause devices to be created for nodes. It
cannot be based on some arbitrary heuristics.

I'm not asking you to do something hard though. For the platform devices
I'd be fine with of_platform_populate() marking some kind of flag on
struct device_node that says children of it should be used to create
more platform_devices.

> b) Target is one of the disabled devices under the ocp node
> and the status property is modified, plus optional nodes are
> added that result in the creation of more devices under the bus this
> node controls. This is the case of enabled an I2C/SPI bus and inserting
> children devices.

Close, but again it has to match with what I've said above. The various
subsystems need to keep track of which nodes describe a subsystem bus
and only create devices on those nodes. It would be valuable to have a
simple library for keeping track of that information which is usable by
all subsystems, but it still needs to be under the control of the
subsystem.

> 
> Perhaps the trick is having hooks about what to do in case certain
> nodes are modified in a certain way, i.e. a notifier on the ocp node
> would 'catch' those. The problem is a having a way to present this
> information to the notifier in a way that's easy to be handled.
> 
> What do you think?

It would probably work to have a single notifier for each entire
subsystem instead of a separate notifier for each and every bus node.
Once a subsystem marks a node as interesting, it is quite simple for a
generic subsystem notifier callback to decide whether to create devices
based on it. If done right, it should work without any changes to the
SPI/I2C/etc bus drivers.

> 
> >> +
> >> +/**
> >> + * 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 platform 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;
> >> +	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);
> >> +	}
> > 
> > Can overlays interact in bad ways? If overlay 1 is installed before
> > overlay 2, what happens if overlay 1 is removed?
> > 
> 
> Yes, they can. It is not something easily fixed; the proper way would
> be to calculate overlay intersection points and refuse to unload.

I think this is important. If it cannot be solved immediately, then the
kernel should enforce overlays always get removed in the reverse order
that they were added. There may be use-cases that don't like it, but it
is safe.

> 
> > Okay, my brain is tired. It's a complicated series and I don't see any
> > obvious bits that can be split off and be independently useful. I would
> > *really* like to see some test cases for this functionality. It's
> > complicated enough to make me quite nervous.... in fact I would really
> > like to see drivers/of/selftest.c become an early user of overlays. That
> > would make the DT selftests executable on any platform.
> > 
> 
> OK, I see your point. My brain is tired too, and I'm on travel too for
> this week and the next. I'll see what I can come up with.

Enjoy your trip.

g.

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Grant,

On Nov 14, 2013, at 2:31 AM, Grant Likely wrote:

> On Tue, 12 Nov 2013 10:30:37 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> Hi Grant,
>> 
>> On Nov 11, 2013, at 7:42 PM, Grant Likely wrote:
>> 
>>> On Fri,  8 Nov 2013 17:06:09 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> 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.
>>>> 
>>>> Note that the I2C client devices are 'special', as in they're not platform
>>>> devices. They need to be registered with an I2C specific method.
>>>> 
>>>> In general I2C is just no good platform device citizen and needs
>>>> special casing.
>>>> 
>>>> PCI is another special case where PCI device insertion and removal
>>>> is implemented in the PCI subsystem.
>>> 
>>> Actually. I2C isn't special in this regard; SPI, MDIO, PCI, USB are all
>>> in the same boat. platform_device just happens to be able to make a few
>>> assumptions. If anything, platform_device is the 'special' one. :-)
>>> 
>> 
>> I'm of the opinion that 'platform_device' shouldn't exist at all btw :)
>> Most of it's functionality can pretty easily be subsumed by device proper
>> and the world would be a better place :)
> 
> I'm fine for merging some/all of the platform_device fields into struct
> device. There are a few things, like resources, which would probably be
> useful to have common on all struct device variants. However,
> platform_device is far more about matching drivers to devices. Even if
> all of platform_device went into struct device, there would still need
> to be the platform_bus_type as the collection point for the device
> drivers.
> 

We don't really need the resources structures on OF. That information is
present in OF format, which we can use to generate transient resources for
usage with the standard kernel interfaces.

BTW, last time I checked resource handling was broken on release.
There are a few patches I sent out fixing it but they were probably ignored.

>>> In all cases it must be the underlying subsystem that should be
>>> responsible for creation/removal of devices described in the overlay.
>>> Sometimes that will extend right down into the individual bus device
>>> drivers, but it is better if that can be avoided.
>>> 
>> 
>> Yes, that's the right way to handle this, but we're talking about 
>> serious modification in almost every kind of device core.
>> 
>> Just getting the basic concept of overlays in is hard without having
>> to tackle something like this.
>> 
>> In the future, I'd be ecstatic if we could get a per device class
>> filter, but it's nothing we can fix right now.
>> 
>> IMHO of course.
> 
> It's fine to limit yourself to only platform_devices and i2c for now,
> but that doesn't give license to do the wrong thing with the
> implementation. The subsystem modifications really shouldn't be very
> invasive. It will need a per-subsystem notifier callback to deal with
> the add/remove events, and it will require the subsystem to maintain a
> list of parent bus nodes that it cares about and should create children
> for.
> 
> Doing it with a central function like this patch does is not a good
> approach at all.
> 

This is indeed the cleaner approach... I'll take a look at what it takes
to do it; hope the changes needed are not anything major.

>> 
>>>> 
>>>> Bug fixes & PCI support by Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>>>> 
>> 
>> [snip]
>> 
>>>> +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().
>>> 
>>> Make of_overlay_info a kobject and use a release method to free it when
>>> all users go away. Freeing directly is a dangerous thing to do.
>>> 
>> 
>> It's more dangerous than even that. We also have the mess with the way
>> unflattening works (properties point directly to binary blob area
>> with no way to free per property data). In general freeing/releasing
>> device tree nodes is tricky on a good day.
> 
> Repeating from my comment on the previous email, don't throw away the
> blob. There is no need and it makes your job harder.
> 

OK.

>> 
>> I agree we have to move to a kobject model eventually. 
>> 
>>>> +
>>>> +/**
>>>> + * 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>;	/* target 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 */
>>>> +				...
>>>> +			};
>>> 
>>> Are the node & property removals reversable? What about property
>>> modifications? If they are not, then it would be better to not support
>>> removals at all for now. Otherwise we'll end up with overlays that
>>> cannot be removed and I don't want to inadvertently put users into that
>>> situation.
>> 
>> Yes, everything is reversible. Doesn't mean that the drivers/driver core
>> can handle it. Using overlays is an excellent way to trigger bugs in
>> device removal as I've found out :)
> 
> :)
> 
>> 
>>> 
>>>> +		};
>>>> +	}
>>>> +	fragment@1 {	/* second child node */
>>>> +		...
>>>> +	};
>>>> +	/* more fragments follow */
>>>> +}
>> 
>> [snip]
>> 
>>>> +
>>>> +static int of_overlay_device_entry_change(struct of_overlay_info *ovinfo,
>>>> +		struct of_overlay_device_entry *de, int revert)
>>>> +{
>>> 
>>> I've got big problems here. This is entirely the wrong place to create
>>> and delete devices. Each subsystem needs to create and remove its own
>>> device types. To begin with, only the subsystem knows which busses are
>>> appropriate for creating child devices.
>>> 
>> 
>> It was the best I could under the circumstances, without having to touch
>> every subsystem.
>> 
>> 
>>> Second, it is incorrect to create a platform_device for every single
>>> node by default. Some nodes aren't devices at all. Just looking for a
>>> compatible property isn't sufficient either.
>>> 
>> 
>> Yep. 
>> 
>>> The solution here is for each subsystem to have it's own notifier, and
>>> only create a child device if the changed node has a parent the
>>> subsystem already knows about (because it's already populated device
>>> tree devices from that parent). This is also true for platform_devices.
>>> 
>>> Finally, sometimes the tips of the tree will get populated, but not
>>> until after some parent nodes have been delt with. In that case the
>>> creation of devices needs to be deferred back to the driver for the
>>> parent bus.
>>> 
>> 
>> Well, any solution that requires each and every subsystem to do dynamic
>> child insertions/removal is going to be hard sell.
> 
> They have to do it /anyway/! How many subsystems are we talking about?
> SPI, I2C, platform_device (and amba_device). Maybe MDIO. To get to a
> useful subset it isn't very many targets. SPI and I2C will probably be
> receptive, so no problem there. The maintainer of drivers/of/platform.c
> is friendly too. :-) Haven't asked about MDIO.
> 

Famous last words :)

>> 
>> What about something more gentle?
>> 
>> For example, I know that most of the cases fall into a couple of set
>> patterns for my case
>> 
>> a) Target is the ocp node (on chip peripheral node) and results in
>> the creation of platform devices under that node.
> 
> No, I'm going to be hard-line on this. The code needs to be very
> explicit about which nodes cause devices to be created for nodes. It
> cannot be based on some arbitrary heuristics.
> 
> I'm not asking you to do something hard though. For the platform devices
> I'd be fine with of_platform_populate() marking some kind of flag on
> struct device_node that says children of it should be used to create
> more platform_devices.
> 
>> b) Target is one of the disabled devices under the ocp node
>> and the status property is modified, plus optional nodes are
>> added that result in the creation of more devices under the bus this
>> node controls. This is the case of enabled an I2C/SPI bus and inserting
>> children devices.
> 
> Close, but again it has to match with what I've said above. The various
> subsystems need to keep track of which nodes describe a subsystem bus
> and only create devices on those nodes. It would be valuable to have a
> simple library for keeping track of that information which is usable by
> all subsystems, but it still needs to be under the control of the
> subsystem.
> 
>> 
>> Perhaps the trick is having hooks about what to do in case certain
>> nodes are modified in a certain way, i.e. a notifier on the ocp node
>> would 'catch' those. The problem is a having a way to present this
>> information to the notifier in a way that's easy to be handled.
>> 
>> What do you think?
> 
> It would probably work to have a single notifier for each entire
> subsystem instead of a separate notifier for each and every bus node.
> Once a subsystem marks a node as interesting, it is quite simple for a
> generic subsystem notifier callback to decide whether to create devices
> based on it. If done right, it should work without any changes to the
> SPI/I2C/etc bus drivers.
> 

The proof is going to be in the code.

>> 
>>>> +
>>>> +/**
>>>> + * 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 platform 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;
>>>> +	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);
>>>> +	}
>>> 
>>> Can overlays interact in bad ways? If overlay 1 is installed before
>>> overlay 2, what happens if overlay 1 is removed?
>>> 
>> 
>> Yes, they can. It is not something easily fixed; the proper way would
>> be to calculate overlay intersection points and refuse to unload.
> 
> I think this is important. If it cannot be solved immediately, then the
> kernel should enforce overlays always get removed in the reverse order
> that they were added. There may be use-cases that don't like it, but it
> is safe.

OK, that makes sense.

We are not talking about a global overlay stack though, we're talking about
an overlay stack for overlays that overlap.

> 
>> 
>>> Okay, my brain is tired. It's a complicated series and I don't see any
>>> obvious bits that can be split off and be independently useful. I would
>>> *really* like to see some test cases for this functionality. It's
>>> complicated enough to make me quite nervous.... in fact I would really
>>> like to see drivers/of/selftest.c become an early user of overlays. That
>>> would make the DT selftests executable on any platform.
>>> 
>> 
>> OK, I see your point. My brain is tired too, and I'm on travel too for
>> this week and the next. I'll see what I can come up with.
> 
> Enjoy your trip.
> 

Thanks, although it's a work trip :)

> g.

Regards

-- Pantelis

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

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Grant Likely]

On Thu, 14 Nov 2013 11:01:35 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> On Nov 14, 2013, at 2:31 AM, Grant Likely wrote:
> > On Tue, 12 Nov 2013 10:30:37 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >> On Nov 11, 2013, at 7:42 PM, Grant Likely wrote:
> >>> On Fri,  8 Nov 2013 17:06:09 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >> I'm of the opinion that 'platform_device' shouldn't exist at all btw :)
> >> Most of it's functionality can pretty easily be subsumed by device proper
> >> and the world would be a better place :)
> > 
> > I'm fine for merging some/all of the platform_device fields into struct
> > device. There are a few things, like resources, which would probably be
> > useful to have common on all struct device variants. However,
> > platform_device is far more about matching drivers to devices. Even if
> > all of platform_device went into struct device, there would still need
> > to be the platform_bus_type as the collection point for the device
> > drivers.
> > 
> 
> We don't really need the resources structures on OF. That information is
> present in OF format, which we can use to generate transient resources for
> usage with the standard kernel interfaces.
> 
> BTW, last time I checked resource handling was broken on release.
> There are a few patches I sent out fixing it but they were probably ignored.

Please send them again. They probably got lost.

> >>> Can overlays interact in bad ways? If overlay 1 is installed before
> >>> overlay 2, what happens if overlay 1 is removed?
> >>> 
> >> 
> >> Yes, they can. It is not something easily fixed; the proper way would
> >> be to calculate overlay intersection points and refuse to unload.
> > 
> > I think this is important. If it cannot be solved immediately, then the
> > kernel should enforce overlays always get removed in the reverse order
> > that they were added. There may be use-cases that don't like it, but it
> > is safe.
> 
> OK, that makes sense.
> 
> We are not talking about a global overlay stack though, we're talking about
> an overlay stack for overlays that overlap.

I'm actually talking about a global overlay stack. Otherwise you've
still got the ever-increasing-phandles problem again.

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

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Grant,

On Nov 14, 2013, at 10:22 PM, Grant Likely wrote:

> On Thu, 14 Nov 2013 11:01:35 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> On Nov 14, 2013, at 2:31 AM, Grant Likely wrote:
>>> On Tue, 12 Nov 2013 10:30:37 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>>>> On Nov 11, 2013, at 7:42 PM, Grant Likely wrote:
>>>>> On Fri,  8 Nov 2013 17:06:09 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>>>> I'm of the opinion that 'platform_device' shouldn't exist at all btw :)
>>>> Most of it's functionality can pretty easily be subsumed by device proper
>>>> and the world would be a better place :)
>>> 
>>> I'm fine for merging some/all of the platform_device fields into struct
>>> device. There are a few things, like resources, which would probably be
>>> useful to have common on all struct device variants. However,
>>> platform_device is far more about matching drivers to devices. Even if
>>> all of platform_device went into struct device, there would still need
>>> to be the platform_bus_type as the collection point for the device
>>> drivers.
>>> 
>> 
>> We don't really need the resources structures on OF. That information is
>> present in OF format, which we can use to generate transient resources for
>> usage with the standard kernel interfaces.
>> 
>> BTW, last time I checked resource handling was broken on release.
>> There are a few patches I sent out fixing it but they were probably ignored.
> 
> Please send them again. They probably got lost.
> 
>>>>> Can overlays interact in bad ways? If overlay 1 is installed before
>>>>> overlay 2, what happens if overlay 1 is removed?
>>>>> 
>>>> 
>>>> Yes, they can. It is not something easily fixed; the proper way would
>>>> be to calculate overlay intersection points and refuse to unload.
>>> 
>>> I think this is important. If it cannot be solved immediately, then the
>>> kernel should enforce overlays always get removed in the reverse order
>>> that they were added. There may be use-cases that don't like it, but it
>>> is safe.
>> 
>> OK, that makes sense.
>> 
>> We are not talking about a global overlay stack though, we're talking about
>> an overlay stack for overlays that overlap.
> 
> I'm actually talking about a global overlay stack. Otherwise you've
> still got the ever-increasing-phandles problem again.
> 

A global overlay stack is easiest. Let's do that first.

There are use cases for multiple overlay stacks though. Take for instance the
case where you have multiple plugin connectors.

Each one can lead to creating a new platform device under ocp, but there is
no overlap.

Of course stackable expansion boards work just fine with a global overlay stack :)

> Cheers,
> g.

Regards

-- Pantelis

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

Re: [PATCH v3 2/3] OF: Introduce DT overlay support.

[Grant Likely]

On Fri, 15 Nov 2013 09:34:21 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> Hi Grant,
> 
> On Nov 14, 2013, at 10:22 PM, Grant Likely wrote:
> 
> > On Thu, 14 Nov 2013 11:01:35 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >> On Nov 14, 2013, at 2:31 AM, Grant Likely wrote:
> >>> On Tue, 12 Nov 2013 10:30:37 +0100, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >>>> On Nov 11, 2013, at 7:42 PM, Grant Likely wrote:
> >>>>> On Fri,  8 Nov 2013 17:06:09 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >>>> I'm of the opinion that 'platform_device' shouldn't exist at all btw :)
> >>>> Most of it's functionality can pretty easily be subsumed by device proper
> >>>> and the world would be a better place :)
> >>> 
> >>> I'm fine for merging some/all of the platform_device fields into struct
> >>> device. There are a few things, like resources, which would probably be
> >>> useful to have common on all struct device variants. However,
> >>> platform_device is far more about matching drivers to devices. Even if
> >>> all of platform_device went into struct device, there would still need
> >>> to be the platform_bus_type as the collection point for the device
> >>> drivers.
> >>> 
> >> 
> >> We don't really need the resources structures on OF. That information is
> >> present in OF format, which we can use to generate transient resources for
> >> usage with the standard kernel interfaces.
> >> 
> >> BTW, last time I checked resource handling was broken on release.
> >> There are a few patches I sent out fixing it but they were probably ignored.
> > 
> > Please send them again. They probably got lost.
> > 
> >>>>> Can overlays interact in bad ways? If overlay 1 is installed before
> >>>>> overlay 2, what happens if overlay 1 is removed?
> >>>>> 
> >>>> 
> >>>> Yes, they can. It is not something easily fixed; the proper way would
> >>>> be to calculate overlay intersection points and refuse to unload.
> >>> 
> >>> I think this is important. If it cannot be solved immediately, then the
> >>> kernel should enforce overlays always get removed in the reverse order
> >>> that they were added. There may be use-cases that don't like it, but it
> >>> is safe.
> >> 
> >> OK, that makes sense.
> >> 
> >> We are not talking about a global overlay stack though, we're talking about
> >> an overlay stack for overlays that overlap.
> > 
> > I'm actually talking about a global overlay stack. Otherwise you've
> > still got the ever-increasing-phandles problem again.
> > 
> 
> A global overlay stack is easiest. Let's do that first.
> 
> There are use cases for multiple overlay stacks though. Take for instance the
> case where you have multiple plugin connectors.
> 
> Each one can lead to creating a new platform device under ocp, but there is
> no overlap.
> 
> Of course stackable expansion boards work just fine with a global overlay stack :)

Agreed. baby steps...

g.

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

[PATCH v3 3/3] DT: proc: Add runtime overlay interface in /proc

[Pantelis Antoniou]

Add a runtime interface to /proc to enable generic device tree overlay
usage.

Two new /proc files are added:

/proc/device-tree-overlay & /proc/device-tree-overlay-status

/proc/device-tree-overlay accepts a stream of a device tree objects and
applies it to the running kernel's device tree.

	$ cat ~/BB-UART2-00A0.dtbo >device-tree-overlay
	overlay_proc_release: Applied #2 overlay segments @0

/proc/device-tree-overlay-status displays the the overlays added using
the /proc interface

	$ cat device-tree-overlay-status
	0: 861 bytes BB-UART2:00A0

The format of the status line is
	<ID>: <SIZE> bytes <part-number>:<version>

<ID> is the id of the overlay
<SIZE> is the size of the overlay in bytes
<part-number>, <version> are (optional) root level properties of the DTBO

You can remove an overlay by echoing the <ID> number of the overlay
precedded with a '-'

So
	$ echo "-0" >device-tree-overlay-status

Removes the overlay.

Note that this seldom works on most platforms since platform_device
removal is something that almost never works without extra patches.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 fs/proc/proc_devtree.c | 278 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 278 insertions(+)

diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index 106a835..48f7925 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -16,6 +16,9 @@
 #include <linux/slab.h>
 #include <asm/prom.h>
 #include <asm/uaccess.h>
+#include <linux/of_fdt.h>
+#include <linux/idr.h>
+
 #include "internal.h"
 
 static inline void set_node_proc_entry(struct device_node *np,
@@ -28,6 +31,275 @@ static inline void set_node_proc_entry(struct device_node *np,
 
 static struct proc_dir_entry *proc_device_tree;
 
+#if defined(CONFIG_OF_OVERLAY)
+static struct proc_dir_entry *proc_device_tree_overlay;
+static struct proc_dir_entry *proc_device_tree_overlay_status;
+static DEFINE_MUTEX(overlay_lock);
+static DEFINE_IDR(overlay_idr);
+
+struct proc_overlay_data {
+	void			*buf;
+	size_t			alloc;
+	size_t			size;
+
+	int 			id;
+	struct device_node	*overlay;
+	int			ovinfo_cnt;
+	struct of_overlay_info	*ovinfo;
+	unsigned int		failed : 1;
+	unsigned int		applied : 1;
+	unsigned int		removing : 1;
+};
+
+static int overlay_proc_open(struct inode *inode, struct file *file)
+{
+	struct proc_overlay_data *od;
+
+	od = kzalloc(sizeof(*od), GFP_KERNEL);
+	if (od == NULL)
+		return -ENOMEM;
+
+	od->id = -1;
+
+	/* save it */
+	file->private_data = od;
+
+	return 0;
+}
+
+static ssize_t overlay_proc_write(struct file *file, const char __user *buf, size_t size, loff_t *ppos)
+{
+	struct proc_overlay_data *od = file->private_data;
+	void *new_buf;
+
+	/* need to alloc? */
+	if (od->size + size > od->alloc) {
+
+		/* start at 256K at first */
+		if (od->alloc == 0)
+			od->alloc = SZ_256K / 2;
+
+		/* double buffer */
+		od->alloc <<= 1;
+		new_buf = kzalloc(od->alloc, GFP_KERNEL);
+		if (new_buf == NULL) {
+			pr_err("%s: failed to grow buffer\n", __func__);
+			od->failed = 1;
+			return -ENOMEM;
+		}
+
+		/* copy all we had previously */
+		memcpy(new_buf, od->buf, od->size);
+
+		/* free old buffer and assign new */
+		kfree(od->buf);
+		od->buf = new_buf;
+	}
+
+	if (unlikely(copy_from_user(od->buf + od->size, buf, size))) {
+		pr_err("%s: fault copying from userspace\n", __func__);
+		return -EFAULT;
+	}
+
+	od->size += size;
+	*ppos += size;
+
+	return size;
+}
+
+static int overlay_proc_release(struct inode *inode, struct file *file)
+{
+	struct proc_overlay_data *od = file->private_data;
+	int id;
+	int err = 0;
+
+	/* perfectly normal when not loading */
+	if (od == NULL)
+		return 0;
+
+	if (od->failed)
+		goto out_free;
+
+	of_fdt_unflatten_tree(od->buf, &od->overlay);
+	if (od->overlay == NULL) {
+		pr_err("%s: failed to unflatten tree\n", __func__);
+		err = -EINVAL;
+		goto out_free;
+	}
+	pr_debug("%s: unflattened OK\n", __func__);
+
+	/* mark it as detached */
+	of_node_set_flag(od->overlay, OF_DETACHED);
+
+	/* perform resolution */
+	err = of_resolve(od->overlay);
+	if (err != 0) {
+		pr_err("%s: Failed to resolve tree\n", __func__);
+		goto out_free;
+	}
+	pr_debug("%s: resolved OK\n", __func__);
+
+	/* now build an overlay info array */
+	err = of_build_overlay_info(od->overlay,
+			&od->ovinfo_cnt, &od->ovinfo);
+	if (err != 0) {
+		pr_err("%s: Failed to build overlay info\n", __func__);
+		goto out_free;
+	}
+
+	pr_debug("%s: built %d overlay segments\n", __func__,
+			od->ovinfo_cnt);
+
+	err = of_overlay(od->ovinfo_cnt, od->ovinfo);
+	if (err != 0) {
+		pr_err("%s: Failed to apply overlay\n", __func__);
+		goto out_free;
+	}
+
+	od->applied = 1;
+
+	mutex_lock(&overlay_lock);
+	idr_preload(GFP_KERNEL);
+	id = idr_alloc(&overlay_idr, od, 0, -1, GFP_KERNEL);
+	idr_preload_end();
+	mutex_unlock(&overlay_lock);
+
+	if (id < 0) {
+		err = id;
+		pr_err("%s: failed to get id for overlay\n", __func__);
+		goto out_free;
+	}
+	od->id = id;
+
+	pr_info("%s: Applied #%d overlay segments @%d\n", __func__,
+			od->ovinfo_cnt, od->id);
+
+	return 0;
+
+out_free:
+	if (od->id != -1)
+		idr_remove(&overlay_idr, od->id);
+	if (od->applied)
+		of_overlay_revert(od->ovinfo_cnt, od->ovinfo);
+	if (od->ovinfo)
+		of_free_overlay_info(od->ovinfo_cnt, od->ovinfo);
+	/* release memory */
+	kfree(od->buf);
+	kfree(od);
+
+	return 0;
+}
+
+static const struct file_operations overlay_proc_fops = {
+	.owner		= THIS_MODULE,
+	.open		= overlay_proc_open,
+	.write		= overlay_proc_write,
+	.release	= overlay_proc_release,
+};
+
+/*
+ * Supply data on a read from /proc/device-tree-overlay-status
+ */
+static int overlay_status_proc_show(struct seq_file *m, void *v)
+{
+	int err, id;
+	struct proc_overlay_data *od;
+	const char *part_number, *version;
+
+	rcu_read_lock();
+	idr_for_each_entry(&overlay_idr, od, id) {
+		seq_printf(m, "%d: %d bytes", id, od->size);
+		/* TODO Make this standardized? */
+		err = of_property_read_string(od->overlay, "part-number",
+					&part_number);
+		if (err != 0)
+			part_number = NULL;
+		err = of_property_read_string(od->overlay, "version",
+					&version);
+		if (err != 0)
+			version = NULL;
+		if (part_number) {
+			seq_printf(m, " %s", part_number);
+			if (version)
+				seq_printf(m, ":%s", version);
+		}
+		seq_printf(m, "\n");
+	}
+	rcu_read_unlock();
+
+	return 0;
+}
+
+static int overlay_status_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, overlay_status_proc_show, __PDE_DATA(inode));
+}
+
+static ssize_t overlay_status_proc_write(struct file *file, const char __user *buf,
+			     size_t size, loff_t *ppos)
+{
+	struct proc_overlay_data *od;
+	char buffer[PROC_NUMBUF + 1];
+	char *ptr;
+	int id, err, count;
+
+	memset(buffer, 0, sizeof(buffer));
+	count = size;
+	if (count > sizeof(buffer) - 1)
+		count = sizeof(buffer) - 1;
+
+	if (copy_from_user(buffer, buf, count)) {
+		err = -EFAULT;
+		goto out;
+	}
+
+	ptr = buffer;
+	if (*ptr != '-') {	/* only removal supported */
+		err = -EINVAL;
+		goto out;
+	}
+	ptr++;
+	err = kstrtoint(strstrip(ptr), 0, &id);
+	if (err)
+		goto out;
+
+	/* find it */
+	mutex_lock(&overlay_lock);
+	od = idr_find(&overlay_idr, id);
+	if (od == NULL) {
+		mutex_unlock(&overlay_lock);
+		err = -EINVAL;
+		goto out;
+	}
+	/* remove it */
+	idr_remove(&overlay_idr, id);
+	mutex_unlock(&overlay_lock);
+
+	err = of_overlay_revert(od->ovinfo_cnt, od->ovinfo);
+	if (err != 0) {
+		pr_err("%s: of_overlay_revert failed\n", __func__);
+		goto out;
+	}
+	/* release memory */
+	of_free_overlay_info(od->ovinfo_cnt, od->ovinfo);
+	kfree(od->buf);
+	kfree(od);
+
+	pr_info("%s: Removed overlay with id %d\n", __func__, id);
+out:
+	return err < 0 ? err : count;
+}
+
+static const struct file_operations overlay_status_proc_fops = {
+	.owner		= THIS_MODULE,
+	.open		= overlay_status_proc_open,
+	.read		= seq_read,
+	.write		= overlay_status_proc_write,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+#endif
+
 /*
  * Supply data on a read from /proc/device-tree/node/property.
  */
@@ -239,5 +511,11 @@ void __init proc_device_tree_init(void)
 		return;
 	}
 	proc_device_tree_add_node(root, proc_device_tree);
+#if defined(CONFIG_OF_OVERLAY)
+	proc_device_tree_overlay = proc_create_data("device-tree-overlay",
+			       S_IWUSR, NULL, &overlay_proc_fops, NULL);
+	proc_device_tree_overlay_status = proc_create_data("device-tree-overlay-status",
+			       S_IRUSR| S_IWUSR, NULL, &overlay_status_proc_fops, NULL);
+#endif
 	of_node_put(root);
 }
-- 
1.7.12

Re: [PATCH v3 3/3] DT: proc: Add runtime overlay interface in /proc

[Guenter Roeck]

On Fri, Nov 08, 2013 at 05:06:10PM +0200, Pantelis Antoniou wrote:
> Add a runtime interface to /proc to enable generic device tree overlay
> usage.
> 

Hi Pantelis,

> ---
>  fs/proc/proc_devtree.c | 278 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 278 insertions(+)
> 
[ ... ]

> +
> +		/* start at 256K at first */
> +		if (od->alloc == 0)
> +			od->alloc = SZ_256K / 2;
> +

I have to include <linux/sizes.h> to get this to build.

Also, even though /proc/device-tree-overlay is supposed to be write only, I get
this:

# cat /proc/device-tree-overlay
cat: [  651.973568] overlay_proc_release: failed to unflatten tree
/proc/device-tree-overlay: Input/output error

which seems to be a bit odd. I did not spend any time to track it down, though.

Otherwise, the code seems to be working well.

Thanks,
Guenter

Re: [PATCH v3 3/3] DT: proc: Add runtime overlay interface in /proc

[Ionut Nicu]

Hi,

On 08.11.2013 16:06, ext Pantelis Antoniou wrote:
> Add a runtime interface to /proc to enable generic device tree overlay
> usage.
> 

> +
> +		/* start at 256K at first */
> +		if (od->alloc == 0)
> +			od->alloc = SZ_256K / 2;
> +

Same problem as Guenter reported. I have to include <linux/sizes.h> or otherwise
the code will not build.

> +	rcu_read_lock();
> +	idr_for_each_entry(&overlay_idr, od, id) {
> +		seq_printf(m, "%d: %d bytes", id, od->size);

For od->size we should use "%zu", otherwise there will be a compilation warning.
At least I get one on mips64...

Otherwise, I've tested this and it works fine for me also.

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

Re: [PATCH v3 3/3] DT: proc: Add runtime overlay interface in /proc

[Pantelis Antoniou]

Hi Ionut,

On Nov 11, 2013, at 11:22 AM, Ionut Nicu wrote:

> Hi,
> 
> On 08.11.2013 16:06, ext Pantelis Antoniou wrote:
>> Add a runtime interface to /proc to enable generic device tree overlay
>> usage.
>> 
> 
>> +
>> +		/* start at 256K at first */
>> +		if (od->alloc == 0)
>> +			od->alloc = SZ_256K / 2;
>> +
> 
> Same problem as Guenter reported. I have to include <linux/sizes.h> or otherwise
> the code will not build.
> 

OK, I guess on my platform it gets implicitly included.

>> +	rcu_read_lock();
>> +	idr_for_each_entry(&overlay_idr, od, id) {
>> +		seq_printf(m, "%d: %d bytes", id, od->size);
> 
> For od->size we should use "%zu", otherwise there will be a compilation warning.
> At least I get one on mips64...
> 

Both of these will be included on the next revision of the patchset.

> Otherwise, I've tested this and it works fine for me also.
> 
> Thanks,
> Ionut

Regards

-- Pantelis

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

Re: [PATCH v3 3/3] DT: proc: Add runtime overlay interface in /proc

[Grant Likely]

On Fri,  8 Nov 2013 17:06:10 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> Add a runtime interface to /proc to enable generic device tree overlay
> usage.
> 
> Two new /proc files are added:
> 
> /proc/device-tree-overlay & /proc/device-tree-overlay-status
> 
> /proc/device-tree-overlay accepts a stream of a device tree objects and
> applies it to the running kernel's device tree.
> 
> 	$ cat ~/BB-UART2-00A0.dtbo >device-tree-overlay
> 	overlay_proc_release: Applied #2 overlay segments @0
> 
> /proc/device-tree-overlay-status displays the the overlays added using
> the /proc interface
> 
> 	$ cat device-tree-overlay-status
> 	0: 861 bytes BB-UART2:00A0
> 
> The format of the status line is
> 	<ID>: <SIZE> bytes <part-number>:<version>
> 
> <ID> is the id of the overlay
> <SIZE> is the size of the overlay in bytes
> <part-number>, <version> are (optional) root level properties of the DTBO
> 
> You can remove an overlay by echoing the <ID> number of the overlay
> precedded with a '-'
> 
> So
> 	$ echo "-0" >device-tree-overlay-status
> 
> Removes the overlay.
> 
> Note that this seldom works on most platforms since platform_device
> removal is something that almost never works without extra patches.
> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>

Why /proc? Did you consider using the firmware loading mechanism? While
I expressed concerned about the capebus approach, the loading of
overlays needs to remain under the control of either a driver or the
platform. By default it should not be possible to drop an arbitrary
overlay into /proc/device-tree-overlay and have things change in the
base tree. Along the same lines, I would expect for the device driver or
platform to be able to filter or limit the parts of the tree that are
allowed to be modified.

A side benefit of the firmware loader is that the kernel can obtain the
overlay on its own if needed at boot time without userspace involvement.

g.

> ---
>  fs/proc/proc_devtree.c | 278 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 278 insertions(+)
> 
> diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> index 106a835..48f7925 100644
> --- a/fs/proc/proc_devtree.c
> +++ b/fs/proc/proc_devtree.c
> @@ -16,6 +16,9 @@
>  #include <linux/slab.h>
>  #include <asm/prom.h>
>  #include <asm/uaccess.h>
> +#include <linux/of_fdt.h>
> +#include <linux/idr.h>
> +
>  #include "internal.h"
>  
>  static inline void set_node_proc_entry(struct device_node *np,
> @@ -28,6 +31,275 @@ static inline void set_node_proc_entry(struct device_node *np,
>  
>  static struct proc_dir_entry *proc_device_tree;
>  
> +#if defined(CONFIG_OF_OVERLAY)
> +static struct proc_dir_entry *proc_device_tree_overlay;
> +static struct proc_dir_entry *proc_device_tree_overlay_status;
> +static DEFINE_MUTEX(overlay_lock);
> +static DEFINE_IDR(overlay_idr);
> +
> +struct proc_overlay_data {
> +	void			*buf;
> +	size_t			alloc;
> +	size_t			size;
> +
> +	int 			id;
> +	struct device_node	*overlay;
> +	int			ovinfo_cnt;
> +	struct of_overlay_info	*ovinfo;
> +	unsigned int		failed : 1;
> +	unsigned int		applied : 1;
> +	unsigned int		removing : 1;
> +};
> +
> +static int overlay_proc_open(struct inode *inode, struct file *file)
> +{
> +	struct proc_overlay_data *od;
> +
> +	od = kzalloc(sizeof(*od), GFP_KERNEL);
> +	if (od == NULL)
> +		return -ENOMEM;
> +
> +	od->id = -1;
> +
> +	/* save it */
> +	file->private_data = od;
> +
> +	return 0;
> +}
> +
> +static ssize_t overlay_proc_write(struct file *file, const char __user *buf, size_t size, loff_t *ppos)
> +{
> +	struct proc_overlay_data *od = file->private_data;
> +	void *new_buf;
> +
> +	/* need to alloc? */
> +	if (od->size + size > od->alloc) {
> +
> +		/* start at 256K at first */
> +		if (od->alloc == 0)
> +			od->alloc = SZ_256K / 2;
> +
> +		/* double buffer */
> +		od->alloc <<= 1;
> +		new_buf = kzalloc(od->alloc, GFP_KERNEL);
> +		if (new_buf == NULL) {
> +			pr_err("%s: failed to grow buffer\n", __func__);
> +			od->failed = 1;
> +			return -ENOMEM;
> +		}
> +
> +		/* copy all we had previously */
> +		memcpy(new_buf, od->buf, od->size);
> +
> +		/* free old buffer and assign new */
> +		kfree(od->buf);
> +		od->buf = new_buf;
> +	}
> +
> +	if (unlikely(copy_from_user(od->buf + od->size, buf, size))) {
> +		pr_err("%s: fault copying from userspace\n", __func__);
> +		return -EFAULT;
> +	}
> +
> +	od->size += size;
> +	*ppos += size;
> +
> +	return size;
> +}
> +
> +static int overlay_proc_release(struct inode *inode, struct file *file)
> +{
> +	struct proc_overlay_data *od = file->private_data;
> +	int id;
> +	int err = 0;
> +
> +	/* perfectly normal when not loading */
> +	if (od == NULL)
> +		return 0;
> +
> +	if (od->failed)
> +		goto out_free;
> +
> +	of_fdt_unflatten_tree(od->buf, &od->overlay);
> +	if (od->overlay == NULL) {
> +		pr_err("%s: failed to unflatten tree\n", __func__);
> +		err = -EINVAL;
> +		goto out_free;
> +	}
> +	pr_debug("%s: unflattened OK\n", __func__);
> +
> +	/* mark it as detached */
> +	of_node_set_flag(od->overlay, OF_DETACHED);
> +
> +	/* perform resolution */
> +	err = of_resolve(od->overlay);
> +	if (err != 0) {
> +		pr_err("%s: Failed to resolve tree\n", __func__);
> +		goto out_free;
> +	}
> +	pr_debug("%s: resolved OK\n", __func__);
> +
> +	/* now build an overlay info array */
> +	err = of_build_overlay_info(od->overlay,
> +			&od->ovinfo_cnt, &od->ovinfo);
> +	if (err != 0) {
> +		pr_err("%s: Failed to build overlay info\n", __func__);
> +		goto out_free;
> +	}
> +
> +	pr_debug("%s: built %d overlay segments\n", __func__,
> +			od->ovinfo_cnt);
> +
> +	err = of_overlay(od->ovinfo_cnt, od->ovinfo);
> +	if (err != 0) {
> +		pr_err("%s: Failed to apply overlay\n", __func__);
> +		goto out_free;
> +	}
> +
> +	od->applied = 1;
> +
> +	mutex_lock(&overlay_lock);
> +	idr_preload(GFP_KERNEL);
> +	id = idr_alloc(&overlay_idr, od, 0, -1, GFP_KERNEL);
> +	idr_preload_end();
> +	mutex_unlock(&overlay_lock);
> +
> +	if (id < 0) {
> +		err = id;
> +		pr_err("%s: failed to get id for overlay\n", __func__);
> +		goto out_free;
> +	}
> +	od->id = id;
> +
> +	pr_info("%s: Applied #%d overlay segments @%d\n", __func__,
> +			od->ovinfo_cnt, od->id);
> +
> +	return 0;
> +
> +out_free:
> +	if (od->id != -1)
> +		idr_remove(&overlay_idr, od->id);
> +	if (od->applied)
> +		of_overlay_revert(od->ovinfo_cnt, od->ovinfo);
> +	if (od->ovinfo)
> +		of_free_overlay_info(od->ovinfo_cnt, od->ovinfo);
> +	/* release memory */
> +	kfree(od->buf);
> +	kfree(od);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations overlay_proc_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= overlay_proc_open,
> +	.write		= overlay_proc_write,
> +	.release	= overlay_proc_release,
> +};
> +
> +/*
> + * Supply data on a read from /proc/device-tree-overlay-status
> + */
> +static int overlay_status_proc_show(struct seq_file *m, void *v)
> +{
> +	int err, id;
> +	struct proc_overlay_data *od;
> +	const char *part_number, *version;
> +
> +	rcu_read_lock();
> +	idr_for_each_entry(&overlay_idr, od, id) {
> +		seq_printf(m, "%d: %d bytes", id, od->size);
> +		/* TODO Make this standardized? */
> +		err = of_property_read_string(od->overlay, "part-number",
> +					&part_number);
> +		if (err != 0)
> +			part_number = NULL;
> +		err = of_property_read_string(od->overlay, "version",
> +					&version);
> +		if (err != 0)
> +			version = NULL;
> +		if (part_number) {
> +			seq_printf(m, " %s", part_number);
> +			if (version)
> +				seq_printf(m, ":%s", version);
> +		}
> +		seq_printf(m, "\n");
> +	}
> +	rcu_read_unlock();
> +
> +	return 0;
> +}
> +
> +static int overlay_status_proc_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, overlay_status_proc_show, __PDE_DATA(inode));
> +}
> +
> +static ssize_t overlay_status_proc_write(struct file *file, const char __user *buf,
> +			     size_t size, loff_t *ppos)
> +{
> +	struct proc_overlay_data *od;
> +	char buffer[PROC_NUMBUF + 1];
> +	char *ptr;
> +	int id, err, count;
> +
> +	memset(buffer, 0, sizeof(buffer));
> +	count = size;
> +	if (count > sizeof(buffer) - 1)
> +		count = sizeof(buffer) - 1;
> +
> +	if (copy_from_user(buffer, buf, count)) {
> +		err = -EFAULT;
> +		goto out;
> +	}
> +
> +	ptr = buffer;
> +	if (*ptr != '-') {	/* only removal supported */
> +		err = -EINVAL;
> +		goto out;
> +	}
> +	ptr++;
> +	err = kstrtoint(strstrip(ptr), 0, &id);
> +	if (err)
> +		goto out;
> +
> +	/* find it */
> +	mutex_lock(&overlay_lock);
> +	od = idr_find(&overlay_idr, id);
> +	if (od == NULL) {
> +		mutex_unlock(&overlay_lock);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +	/* remove it */
> +	idr_remove(&overlay_idr, id);
> +	mutex_unlock(&overlay_lock);
> +
> +	err = of_overlay_revert(od->ovinfo_cnt, od->ovinfo);
> +	if (err != 0) {
> +		pr_err("%s: of_overlay_revert failed\n", __func__);
> +		goto out;
> +	}
> +	/* release memory */
> +	of_free_overlay_info(od->ovinfo_cnt, od->ovinfo);
> +	kfree(od->buf);
> +	kfree(od);
> +
> +	pr_info("%s: Removed overlay with id %d\n", __func__, id);
> +out:
> +	return err < 0 ? err : count;
> +}
> +
> +static const struct file_operations overlay_status_proc_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= overlay_status_proc_open,
> +	.read		= seq_read,
> +	.write		= overlay_status_proc_write,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};
> +#endif
> +
>  /*
>   * Supply data on a read from /proc/device-tree/node/property.
>   */
> @@ -239,5 +511,11 @@ void __init proc_device_tree_init(void)
>  		return;
>  	}
>  	proc_device_tree_add_node(root, proc_device_tree);
> +#if defined(CONFIG_OF_OVERLAY)
> +	proc_device_tree_overlay = proc_create_data("device-tree-overlay",
> +			       S_IWUSR, NULL, &overlay_proc_fops, NULL);
> +	proc_device_tree_overlay_status = proc_create_data("device-tree-overlay-status",
> +			       S_IRUSR| S_IWUSR, NULL, &overlay_status_proc_fops, NULL);
> +#endif
>  	of_node_put(root);
>  }
> -- 
> 1.7.12
> 

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

Re: [PATCH v3 3/3] DT: proc: Add runtime overlay interface in /proc

[Pantelis Antoniou]

Hi Grant,

On Nov 11, 2013, at 7:47 PM, Grant Likely wrote:

> On Fri,  8 Nov 2013 17:06:10 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> Add a runtime interface to /proc to enable generic device tree overlay
>> usage.
>> 
>> Two new /proc files are added:
>> 
>> /proc/device-tree-overlay & /proc/device-tree-overlay-status
>> 
>> /proc/device-tree-overlay accepts a stream of a device tree objects and
>> applies it to the running kernel's device tree.
>> 
>> 	$ cat ~/BB-UART2-00A0.dtbo >device-tree-overlay
>> 	overlay_proc_release: Applied #2 overlay segments @0
>> 
>> /proc/device-tree-overlay-status displays the the overlays added using
>> the /proc interface
>> 
>> 	$ cat device-tree-overlay-status
>> 	0: 861 bytes BB-UART2:00A0
>> 
>> The format of the status line is
>> 	<ID>: <SIZE> bytes <part-number>:<version>
>> 
>> <ID> is the id of the overlay
>> <SIZE> is the size of the overlay in bytes
>> <part-number>, <version> are (optional) root level properties of the DTBO
>> 
>> You can remove an overlay by echoing the <ID> number of the overlay
>> precedded with a '-'
>> 
>> So
>> 	$ echo "-0" >device-tree-overlay-status
>> 
>> Removes the overlay.
>> 
>> Note that this seldom works on most platforms since platform_device
>> removal is something that almost never works without extra patches.
>> 
>> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> 
> Why /proc? Did you consider using the firmware loading mechanism? While
> I expressed concerned about the capebus approach, the loading of
> overlays needs to remain under the control of either a driver or the
> platform. By default it should not be possible to drop an arbitrary
> overlay into /proc/device-tree-overlay and have things change in the
> base tree. Along the same lines, I would expect for the device driver or
> platform to be able to filter or limit the parts of the tree that are
> allowed to be modified.
> 

The firmware loading mechanism is the preferred way to handle it, and is
in fact what is used in practice by the whole cape manager mechanism.
But that's part of specific board support, and this is more like a general
way to use overlays, in any kind of DT enabled system.

I agree this is a) completely dangerous and b) /proc is a bad place to put
it. I put it here in order to get the ball rolling, about the proper place
to put device tree related interfaces.

The good thing about this interface is that it's always available, and
it is good for debugging (especially loading/unloading debugging).
 

> A side benefit of the firmware loader is that the kernel can obtain the
> overlay on its own if needed at boot time without userspace involvement.
> 

Yes, that is correct, and it is the way we use it on the beaglebone.
The root fs device is present on a overlay, which is built into the kernel's
firmware.


> g.
> 

Regards

-- Pantelis

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

Re: [PATCH v3 3/3] DT: proc: Add runtime overlay interface in /proc

[Matt Porter]

On Tue, Nov 12, 2013 at 09:40:38AM +0100, Pantelis Antoniou wrote:
> Hi Grant,
> 
> On Nov 11, 2013, at 7:47 PM, Grant Likely wrote:
> 
> > On Fri,  8 Nov 2013 17:06:10 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >> Add a runtime interface to /proc to enable generic device tree overlay
> >> usage.
> >> 
> >> Two new /proc files are added:
> >> 
> >> /proc/device-tree-overlay & /proc/device-tree-overlay-status
> >> 
> >> /proc/device-tree-overlay accepts a stream of a device tree objects and
> >> applies it to the running kernel's device tree.
> >> 
> >> 	$ cat ~/BB-UART2-00A0.dtbo >device-tree-overlay
> >> 	overlay_proc_release: Applied #2 overlay segments @0
> >> 
> >> /proc/device-tree-overlay-status displays the the overlays added using
> >> the /proc interface
> >> 
> >> 	$ cat device-tree-overlay-status
> >> 	0: 861 bytes BB-UART2:00A0
> >> 
> >> The format of the status line is
> >> 	<ID>: <SIZE> bytes <part-number>:<version>
> >> 
> >> <ID> is the id of the overlay
> >> <SIZE> is the size of the overlay in bytes
> >> <part-number>, <version> are (optional) root level properties of the DTBO
> >> 
> >> You can remove an overlay by echoing the <ID> number of the overlay
> >> precedded with a '-'
> >> 
> >> So
> >> 	$ echo "-0" >device-tree-overlay-status
> >> 
> >> Removes the overlay.
> >> 
> >> Note that this seldom works on most platforms since platform_device
> >> removal is something that almost never works without extra patches.
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> > 
> > Why /proc? Did you consider using the firmware loading mechanism? While
> > I expressed concerned about the capebus approach, the loading of
> > overlays needs to remain under the control of either a driver or the
> > platform. By default it should not be possible to drop an arbitrary
> > overlay into /proc/device-tree-overlay and have things change in the
> > base tree. Along the same lines, I would expect for the device driver or
> > platform to be able to filter or limit the parts of the tree that are
> > allowed to be modified.
> > 
> 
> The firmware loading mechanism is the preferred way to handle it, and is
> in fact what is used in practice by the whole cape manager mechanism.
> But that's part of specific board support, and this is more like a general
> way to use overlays, in any kind of DT enabled system.
> 
> I agree this is a) completely dangerous and b) /proc is a bad place to put
> it. I put it here in order to get the ball rolling, about the proper place
> to put device tree related interfaces.
> 
> The good thing about this interface is that it's always available, and
> it is good for debugging (especially loading/unloading debugging).

As was pointed out earlier in the thread, /proc just isn't the place to
add these configuration interfaces any longer.
>  
> 
> > A side benefit of the firmware loader is that the kernel can obtain the
> > overlay on its own if needed at boot time without userspace involvement.
> > 
> 
> Yes, that is correct, and it is the way we use it on the beaglebone.
> The root fs device is present on a overlay, which is built into the kernel's
> firmware.

Let's be clear that there are two use cases here

1) kernel driven overlay loading
  - root device configuration, other early devices, kernel-based h/w
    detection

2) userspace driven overlay loading
  - s/w defined h/w, hobbyist connecting stuff on a breadboard,
    userspace arduino support

Combining a configfs api for userspace configuration of the overlay to
load with the firmware loader interface would address both of these.

As Grant noted, you want the firmware loader support to handle the
standard kernel driver initiated case which you were doing with capebus
already. But for userspace we have configuration to hand to the kernel
in the form of the name of an overlay. This configuration should be
provided via configfs.

Instantiate an overlay:

	mkdir /config/dto/0

would create the following attributes:

        /config/dto/0/overlay
	/config/dto/0/status

Start a firmware load of an overlay:

	echo "foo.dtbo" > /config/dto/0/overlay
	[loads /lib/firmware/foo.dtbo]

Status of overlay:

	cat /config/dto/0/status
	0:  861 bytes FOO:BAR0

Remove an overlay:

	rmdir /config/dto/0

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