[PATCH v3 0/5] OF: Fixes in preperation of DT overlays

[PATCH v3 0/5] OF: Fixes in preperation of DT overlays

[Pantelis Antoniou]

This patchset introduces a number of fixes that are required
for the subsequent patches that add DT overlays support.

Most of them are trivial, adding small bits that are missing,
or exporting functions that were private before.

Changes in V3:
 * EXPORT_SYMBOL() of_property_notify

Changes in V2:
 * Reorded patchset so that bisect works

Pantelis Antoniou (5):
  OF: Introduce device tree node flag helpers.
  OF: Clear detach flag on attach
  OF: export of_property_notify
  OF: Export all DT proc update functions
  OF: Introduce utility helper functions

 drivers/of/Makefile |   2 +-
 drivers/of/base.c   |  78 ++++++++--------
 drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h  | 119 ++++++++++++++++++++++++
 4 files changed, 414 insertions(+), 38 deletions(-)
 create mode 100644 drivers/of/util.c

-- 
1.7.12

[PATCH v3 1/5] OF: Introduce device tree node flag helpers.

[Pantelis Antoniou]

Helper functions for working with device node flags.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 include/linux/of.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index f95aee3..786c4f6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -114,6 +114,26 @@ static inline void of_node_set_flag(struct device_node *n, unsigned long flag)
 	set_bit(flag, &n->_flags);
 }
 
+static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
+{
+	clear_bit(flag, &n->_flags);
+}
+
+static inline int of_property_check_flag(struct property *p, unsigned long flag)
+{
+	return test_bit(flag, &p->_flags);
+}
+
+static inline void of_property_set_flag(struct property *p, unsigned long flag)
+{
+	set_bit(flag, &p->_flags);
+}
+
+static inline void of_property_clear_flag(struct property *p, unsigned long flag)
+{
+	clear_bit(flag, &p->_flags);
+}
+
 extern struct device_node *of_find_all_nodes(struct device_node *prev);
 
 /*
-- 
1.7.12

[PATCH v3 2/5] OF: Clear detach flag on attach

[Pantelis Antoniou]

When attaching a node always clear the detach flag. Without this change
the sequence detach, attach fails.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7d4c70f..ca10916 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
 	np->allnext = of_allnodes;
 	np->parent->child = np;
 	of_allnodes = np;
+	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	of_add_proc_dt_entry(np);
-- 
1.7.12

[PATCH v3 3/5] OF: export of_property_notify

[Pantelis Antoniou]

of_property_notify can be utilized by other users too, export it.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c  |  9 ++-------
 include/linux/of.h | 11 +++++++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ca10916..6603795 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1424,7 +1424,7 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
 #if defined(CONFIG_OF_DYNAMIC)
-static int of_property_notify(int action, struct device_node *np,
+int of_property_notify(int action, struct device_node *np,
 			      struct property *prop)
 {
 	struct of_prop_reconfig pr;
@@ -1433,12 +1433,7 @@ static int of_property_notify(int action, struct device_node *np,
 	pr.prop = prop;
 	return of_reconfig_notify(action, &pr);
 }
-#else
-static int of_property_notify(int action, struct device_node *np,
-			      struct property *prop)
-{
-	return 0;
-}
+EXPORT_SYMBOL(of_property_notify);
 #endif
 
 /**
diff --git a/include/linux/of.h b/include/linux/of.h
index 786c4f6..6fec255 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -307,6 +307,17 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+#if defined(CONFIG_OF_DYNAMIC)
+extern int of_property_notify(int action, struct device_node *np,
+			      struct property *prop);
+#else
+static inline int of_property_notify(int action, struct device_node *np,
+			      struct property *prop)
+{
+	return 0;
+}
+#endif
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
-- 
1.7.12

[PATCH v3 4/5] OF: Export all DT proc update functions

[Pantelis Antoniou]

There are other users for the proc DT functions.
Export them.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c  | 70 ++++++++++++++++++++++++++++++------------------------
 include/linux/of.h | 29 ++++++++++++++++++++++
 2 files changed, 68 insertions(+), 31 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6603795..9c4afb5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1436,6 +1436,40 @@ int of_property_notify(int action, struct device_node *np,
 EXPORT_SYMBOL(of_property_notify);
 #endif
 
+#ifdef CONFIG_PROC_DEVICETREE
+
+void of_add_proc_dt_entry(struct device_node *dn)
+{
+	struct proc_dir_entry *ent;
+
+	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
+	if (ent)
+		proc_device_tree_add_node(dn, ent);
+}
+
+void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop)
+{
+	if (np && prop && np->pde)
+		proc_device_tree_add_prop(np->pde, prop);
+}
+
+void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop)
+{
+	if (np && prop && np->pde)
+		proc_device_tree_remove_prop(np->pde, prop);
+}
+
+void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop)
+{
+	if (np && newprop && oldprop && np->pde)
+		proc_device_tree_update_prop(np->pde, newprop, oldprop);
+}
+
+#endif /* CONFIG_PROC_DEVICETREE */
+
 /**
  * of_add_property - Add a property to a node
  */
@@ -1463,11 +1497,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 	*next = prop;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-#ifdef CONFIG_PROC_DEVICETREE
 	/* try to add to proc as well if it was initialized */
-	if (np->pde)
-		proc_device_tree_add_prop(np->pde, prop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_add_proc_dt_prop_entry(np, prop);
 
 	return 0;
 }
@@ -1509,11 +1540,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	if (!found)
 		return -ENODEV;
 
-#ifdef CONFIG_PROC_DEVICETREE
-	/* try to remove the proc node as well */
-	if (np->pde)
-		proc_device_tree_remove_prop(np->pde, prop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_remove_proc_dt_prop_entry(np, prop);
 
 	return 0;
 }
@@ -1563,11 +1590,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	if (!found)
 		return -ENODEV;
 
-#ifdef CONFIG_PROC_DEVICETREE
 	/* try to add to proc as well if it was initialized */
-	if (np->pde)
-		proc_device_tree_update_prop(np->pde, newprop, oldprop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_update_proc_dt_prop_entry(np, newprop, oldprop);
 
 	return 0;
 }
@@ -1603,22 +1627,6 @@ int of_reconfig_notify(unsigned long action, void *p)
 	return notifier_to_errno(rc);
 }
 
-#ifdef CONFIG_PROC_DEVICETREE
-static void of_add_proc_dt_entry(struct device_node *dn)
-{
-	struct proc_dir_entry *ent;
-
-	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
-	if (ent)
-		proc_device_tree_add_node(dn, ent);
-}
-#else
-static void of_add_proc_dt_entry(struct device_node *dn)
-{
-	return;
-}
-#endif
-
 /**
  * of_attach_node - Plug a device node into the tree and global list.
  */
@@ -1644,12 +1652,12 @@ int of_attach_node(struct device_node *np)
 }
 
 #ifdef CONFIG_PROC_DEVICETREE
-static void of_remove_proc_dt_entry(struct device_node *dn)
+void of_remove_proc_dt_entry(struct device_node *dn)
 {
 	proc_remove(dn->pde);
 }
 #else
-static void of_remove_proc_dt_entry(struct device_node *dn)
+void of_remove_proc_dt_entry(struct device_node *dn)
 {
 	return;
 }
diff --git a/include/linux/of.h b/include/linux/of.h
index 6fec255..a56c450 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -318,6 +318,35 @@ static inline int of_property_notify(int action, struct device_node *np,
 }
 #endif
 
+#ifdef CONFIG_PROC_DEVICETREE
+
+extern void of_add_proc_dt_entry(struct device_node *dn);
+extern void of_remove_proc_dt_entry(struct device_node *dn);
+
+extern void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop);
+
+extern void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop);
+
+extern void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop);
+#else
+
+static inline void of_add_proc_dt_entry(struct device_node *dn) { }
+static inline void of_remove_proc_dt_entry(struct device_node *dn) { }
+
+static inline void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop) { }
+
+static inline void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop) { }
+
+static inline void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop) { }
+
+#endif
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
-- 
1.7.12

[PATCH v3 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Introduce helper functions for working with the live DT tree.

__of_free_property() frees a dynamically created property
__of_free_tree() recursively frees a device node tree
__of_copy_property() copies a property dynamically
__of_create_empty_node() creates an empty node
__of_find_node_by_full_name() finds the node with the full name
and
of_multi_prop_cmp() performs a multi property compare but without
having to take locks.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/Makefile |   2 +-
 drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h  |  59 ++++++++++++
 3 files changed, 313 insertions(+), 1 deletion(-)
 create mode 100644 drivers/of/util.c

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index efd0510..9bc6d8c 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,4 +1,4 @@
-obj-y = base.o device.o platform.o
+obj-y = base.o device.o platform.o util.o
 obj-$(CONFIG_OF_FLATTREE) += fdt.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
 obj-$(CONFIG_OF_ADDRESS)  += address.o
diff --git a/drivers/of/util.c b/drivers/of/util.c
new file mode 100644
index 0000000..5117e2b
--- /dev/null
+++ b/drivers/of/util.c
@@ -0,0 +1,253 @@
+/*
+ * Utility functions for working with device tree(s)
+ *
+ * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+/**
+ * __of_free_property - release the memory of an allocated property
+ * @prop:	Property to release
+ *
+ * Release the memory of an allocated property only after checking
+ * that the property has been marked as OF_DYNAMIC.
+ * Only call on known allocated properties.
+ */
+void __of_free_property(struct property *prop)
+{
+	if (prop == NULL)
+		return;
+
+	if (of_property_check_flag(prop, OF_DYNAMIC)) {
+		kfree(prop->value);
+		kfree(prop->name);
+		kfree(prop);
+	} else {
+		pr_warn("%s: property %p cannot be freed; memory is gone\n",
+				__func__, prop);
+	}
+}
+
+/**
+ * __of_free_tree - release the memory of a device tree node and
+ *		    of all it's children + properties.
+ * @node:	Device Tree node to release
+ *
+ * Release the memory of a device tree node and of all it's children.
+ * Also release the properties and the dead properties.
+ * Only call on detached node trees, and you better be sure that
+ * no pointer exist for any properties. Only safe to do if you 
+ * absolutely control the life cycle of the node.
+ * Also note that the node is not removed from the all_nodes list,
+ * neither from the parent's child list; this should be handled before
+ * calling this function.
+ */
+void __of_free_tree(struct device_node *node)
+{
+	struct property *prop;
+	struct device_node *noden;
+
+	/* sanity check */
+	if (!node)
+		return;
+
+	/* free recursively any children */
+	while ((noden = node->child) != NULL) {
+		node->child = noden->sibling;
+		__of_free_tree(noden);
+	}
+
+	/* free every property already allocated */
+	while ((prop = node->properties) != NULL) {
+		node->properties = prop->next;
+		__of_free_property(prop);
+	}
+
+	/* free dead properties already allocated */
+	while ((prop = node->deadprops) != NULL) {
+		node->deadprops = prop->next;
+		__of_free_property(prop);
+	}
+
+	if (of_node_check_flag(node, OF_DYNAMIC)) {
+		kfree(node->type);
+		kfree(node->name);
+		kfree(node);
+	} else {
+		pr_warn("%s: node %p cannot be freed; memory is gone\n",
+				__func__, node);
+	}
+}
+
+/**
+ * __of_copy_property - Copy a property dynamically.
+ * @prop:	Property to copy
+ * @flags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Copy a property by dynamically allocating the memory of both the
+ * property stucture and the property name & contents. The property's
+ * flags have the OF_DYNAMIC bit set so that we can differentiate between
+ * dynamically allocated properties and not.
+ * Returns the newly allocated property or NULL on out of memory error.
+ */
+struct property *__of_copy_property(const struct property *prop, gfp_t flags)
+{
+	struct property *propn;
+
+	propn = kzalloc(sizeof(*prop), flags);
+	if (propn == NULL)
+		return NULL;
+
+	propn->name = kstrdup(prop->name, flags);
+	if (propn->name == NULL)
+		goto err_fail_name;
+
+	if (prop->length > 0) {
+		propn->value = kmalloc(prop->length, flags);
+		if (propn->value == NULL)
+			goto err_fail_value;
+		memcpy(propn->value, prop->value, prop->length);
+		propn->length = prop->length;
+	}
+
+	/* mark the property as dynamic */
+	of_property_set_flag(propn, OF_DYNAMIC);
+
+	return propn;
+
+err_fail_value:
+	kfree(propn->name);
+err_fail_name:
+	kfree(propn);
+	return NULL;
+}
+
+/**
+ * __of_create_empty_node - Create an empty device node dynamically.
+ * @name:	Name of the new device node
+ * @type:	Type of the new device node
+ * @full_name:	Full name of the new device node
+ * @phandle:	Phandle of the new device node
+ * @flags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Create an empty device tree node, suitable for further modification.
+ * The node data are dynamically allocated and all the node flags
+ * have the OF_DYNAMIC & OF_DETACHED bits set.
+ * Returns the newly allocated node or NULL on out of memory error.
+ */
+struct device_node *__of_create_empty_node(
+		const char *name, const char *type, const char *full_name,
+		phandle phandle, gfp_t flags)
+{
+	struct device_node *node;
+
+	node = kzalloc(sizeof(*node), flags);
+	if (node == NULL)
+		return NULL;
+
+	node->name = kstrdup(name, flags);
+	if (node->name == NULL)
+		goto err_return;
+
+	node->type = kstrdup(type, flags);
+	if (node->type == NULL)
+		goto err_return;
+
+	node->full_name = kstrdup(full_name, flags);
+	if (node->type == NULL)
+		goto err_return;
+
+	node->phandle = phandle;
+	kref_init(&node->kref);
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+
+	return node;
+
+err_return:
+	__of_free_tree(node);
+	return NULL;
+}
+
+/**
+ * __of_find_node_by_full_name - Find a node with the full name recursively
+ * @node:	Root of the tree to perform the search
+ * @full_name:	Full name of the node to find.
+ *
+ * Find a node with the give full name by recursively following any of 
+ * the child node links.
+ * Returns the matching node, or NULL if not found.
+ * Note that the devtree lock is not taken, so this function is only
+ * safe to call on either detached trees, or when devtree lock is already
+ * taken.
+ */
+struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name)
+{
+	struct device_node *child, *found;
+
+	if (node == NULL)
+		return NULL;
+
+	/* check */
+	if (of_node_cmp(node->full_name, full_name) == 0)
+		return node;
+
+	__for_each_child_of_node(node, child) {
+		found = __of_find_node_by_full_name(child, full_name);
+		if (found != NULL)
+			return found;
+	}
+
+	return NULL;
+}
+
+/**
+ * of_multi_prop_cmp - Check if a property matches a value
+ * @prop:	Property to check
+ * @value:	Value to check against
+ *
+ * Check whether a property matches a value, using the standard
+ * of_compat_cmp() test on each string. It is similar to the test
+ * of_device_is_compatible() makes, but it can be performed without
+ * taking the devtree_lock, which is required in some cases.
+ * Returns 0 on a match, -1 on no match.
+ */
+int of_multi_prop_cmp(const struct property *prop, const char *value)
+{
+	const char *cp;
+	int cplen, vlen, l;
+
+	if (prop == NULL || value == NULL)
+		return -1;
+
+	cp = prop->value;
+	cplen = prop->length;
+	vlen = strlen(value);
+
+	while (cplen > 0) {
+		if (of_compat_cmp(cp, value, vlen) == 0)
+			return 0;
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
+
+	return -1;
+}
+
diff --git a/include/linux/of.h b/include/linux/of.h
index a56c450..9d69bd2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -662,4 +662,63 @@ extern void proc_device_tree_update_prop(struct proc_dir_entry *pde,
 					 struct property *oldprop);
 #endif
 
+/**
+ * General utilities for working with live trees.
+ *
+ * All functions with two leading underscores operate
+ * without taking node references, so you either have to
+ * own the devtree lock or work on detached trees only.
+ */
+
+#ifdef CONFIG_OF
+
+/* iterator for internal use; not references, neither affects devtree lock */
+#define __for_each_child_of_node(dn, chld) \
+	for (chld = (dn)->child; chld != NULL; chld = chld->sibling)
+
+void __of_free_property(struct property *prop);
+void __of_free_tree(struct device_node *node);
+struct property *__of_copy_property(const struct property *prop, gfp_t flags);
+struct device_node *__of_create_empty_node( const char *name,
+		const char *type, const char *full_name,
+		phandle phandle, gfp_t flags);
+struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name);
+int of_multi_prop_cmp(const struct property *prop, const char *value);
+
+#else /* !CONFIG_OF */
+
+#define __for_each_child_of_node(dn, chld) \
+	while (0)
+
+static inline void __of_free_property(struct property *prop) { }
+
+static inline void __of_free_tree(struct device_node *node) { }
+
+static inline struct property *__of_copy_property(const struct property *prop,
+		gfp_t flags)
+{
+	return NULL;
+}
+
+static inline struct device_node *__of_create_empty_node( const char *name,
+		const char *type, const char *full_name,
+		phandle phandle, gfp_t flags)
+{
+	return NULL;
+}
+
+static inline struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name)
+{
+	return NULL;
+}
+
+static inline int of_multi_prop_cmp(const struct property *prop, const char *value)
+{
+	return -1;
+}
+
+#endif	/* !CONFIG_OF */
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

Re: [PATCH v3 5/5] OF: Introduce utility helper functions

[Alexander Sverdlin]

Hello Pantelis,

On 07/11/13 21:17, ext Pantelis Antoniou wrote:
> Introduce helper functions for working with the live DT tree.
> 
> __of_free_property() frees a dynamically created property
> __of_free_tree() recursively frees a device node tree
> __of_copy_property() copies a property dynamically
> __of_create_empty_node() creates an empty node
> __of_find_node_by_full_name() finds the node with the full name
> and
> of_multi_prop_cmp() performs a multi property compare but without
> having to take locks.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/of/Makefile |   2 +-
>  drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h  |  59 ++++++++++++
>  3 files changed, 313 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/of/util.c
> 

...

> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
> +{
> +	struct property *propn;
> +
> +	propn = kzalloc(sizeof(*prop), flags);
> +	if (propn == NULL)
> +		return NULL;
> +
> +	propn->name = kstrdup(prop->name, flags);
> +	if (propn->name == NULL)
> +		goto err_fail_name;
> +
> +	if (prop->length > 0) {
        ^^^^^^^^^^^^^^^^^^^^^^^
As Ioan already mentioned, this is really a problem.
There is a bunch of places, where properties without values are used.
Like gpio-controller; ranges; interrupt-controller;
Refer, for example, to of_irq_map_raw() which checks
of_get_property(ipar, "interrupt-controller", NULL) != NULL
and some other occurrences of exactly same construct.
This will simply be broken for merged device-tree parts.

> +		propn->value = kmalloc(prop->length, flags);
> +		if (propn->value == NULL)
> +			goto err_fail_value;
> +		memcpy(propn->value, prop->value, prop->length);
> +		propn->length = prop->length;
> +	}
> +
> +	/* mark the property as dynamic */
> +	of_property_set_flag(propn, OF_DYNAMIC);
> +
> +	return propn;
> +
> +err_fail_value:
> +	kfree(propn->name);
> +err_fail_name:
> +	kfree(propn);
> +	return NULL;
> +}
> +

...

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v3 5/5] OF: Introduce utility helper functions

[Guenter Roeck]

On 11/08/2013 01:08 AM, Alexander Sverdlin wrote:
> Hello Pantelis,
>
> On 07/11/13 21:17, ext Pantelis Antoniou wrote:
>> Introduce helper functions for working with the live DT tree.
>>
>> __of_free_property() frees a dynamically created property
>> __of_free_tree() recursively frees a device node tree
>> __of_copy_property() copies a property dynamically
>> __of_create_empty_node() creates an empty node
>> __of_find_node_by_full_name() finds the node with the full name
>> and
>> of_multi_prop_cmp() performs a multi property compare but without
>> having to take locks.
>>
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> ---
>>   drivers/of/Makefile |   2 +-
>>   drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of.h  |  59 ++++++++++++
>>   3 files changed, 313 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/of/util.c
>>
>
> ...
>
>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
>> +{
>> +	struct property *propn;
>> +
>> +	propn = kzalloc(sizeof(*prop), flags);
>> +	if (propn == NULL)
>> +		return NULL;
>> +
>> +	propn->name = kstrdup(prop->name, flags);
>> +	if (propn->name == NULL)
>> +		goto err_fail_name;
>> +
>> +	if (prop->length > 0) {
>          ^^^^^^^^^^^^^^^^^^^^^^^
> As Ioan already mentioned, this is really a problem.
> There is a bunch of places, where properties without values are used.
> Like gpio-controller; ranges; interrupt-controller;
> Refer, for example, to of_irq_map_raw() which checks
> of_get_property(ipar, "interrupt-controller", NULL) != NULL
> and some other occurrences of exactly same construct.
> This will simply be broken for merged device-tree parts.
>

Folks,

it might help if you explain what exactly is broken, and how to fix it.
It is not as if the property is not copied, only its value
is not copied. And the value does not exist.

What do you think the code needs to do differently ? Obviously it can
not copy a non-existing value. So what would have to be in the else case ?

Thanks,
Guenter

>> +		propn->value = kmalloc(prop->length, flags);
>> +		if (propn->value == NULL)
>> +			goto err_fail_value;
>> +		memcpy(propn->value, prop->value, prop->length);
>> +		propn->length = prop->length;
>> +	}
>> +
>> +	/* mark the property as dynamic */
>> +	of_property_set_flag(propn, OF_DYNAMIC);
>> +
>> +	return propn;
>> +
>> +err_fail_value:
>> +	kfree(propn->name);
>> +err_fail_name:
>> +	kfree(propn);
>> +	return NULL;
>> +}
>> +
>
> ...
>

Re: [PATCH v3 5/5] OF: Introduce utility helper functions

[Alexander Sverdlin]

Hi!

On 08/11/13 15:54, ext Guenter Roeck wrote:
>>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
>>> +{
>>> +    struct property *propn;
>>> +
>>> +    propn = kzalloc(sizeof(*prop), flags);
>>> +    if (propn == NULL)
>>> +        return NULL;
>>> +
>>> +    propn->name = kstrdup(prop->name, flags);
>>> +    if (propn->name == NULL)
>>> +        goto err_fail_name;
>>> +
>>> +    if (prop->length > 0) {
>>          ^^^^^^^^^^^^^^^^^^^^^^^
>> As Ioan already mentioned, this is really a problem.
>> There is a bunch of places, where properties without values are used.
>> Like gpio-controller; ranges; interrupt-controller;
>> Refer, for example, to of_irq_map_raw() which checks
>> of_get_property(ipar, "interrupt-controller", NULL) != NULL
>> and some other occurrences of exactly same construct.
>> This will simply be broken for merged device-tree parts.
>>
> 
> Folks,
> 
> it might help if you explain what exactly is broken, and how to fix it.
> It is not as if the property is not copied, only its value
> is not copied. And the value does not exist.

Existing kernel code relies on the fact, that when the value doesn't exist, the pointer is still not NULL.
>From the db-unflattening code there will be a pointer from kmalloc(0, ...).
 
> What do you think the code needs to do differently ? Obviously it can
> not copy a non-existing value. So what would have to be in the else case ?

Actually, it can copy non-existing value. memcpy(..., ..., 0) works perfectly fine and
kmalloc(0, flags) does exactly what is required here.

So we fixed this just by removing the if() statement, executing the block unconditionally.
There can be other solutions, but all of them are larger from the code foot-print.

> Thanks,
> Guenter
> 
>>> +        propn->value = kmalloc(prop->length, flags);
>>> +        if (propn->value == NULL)
>>> +            goto err_fail_value;
>>> +        memcpy(propn->value, prop->value, prop->length);
>>> +        propn->length = prop->length;
>>> +    }
>>> +
>>> +    /* mark the property as dynamic */
>>> +    of_property_set_flag(propn, OF_DYNAMIC);
>>> +
>>> +    return propn;
>>> +
>>> +err_fail_value:
>>> +    kfree(propn->name);
>>> +err_fail_name:
>>> +    kfree(propn);
>>> +    return NULL;
>>> +}
>>> +
>>
>> ...
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v3 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Guenter,

On Nov 8, 2013, at 4:54 PM, Guenter Roeck wrote:

> On 11/08/2013 01:08 AM, Alexander Sverdlin wrote:
>> Hello Pantelis,
>> 
>> On 07/11/13 21:17, ext Pantelis Antoniou wrote:
>>> Introduce helper functions for working with the live DT tree.
>>> 
>>> __of_free_property() frees a dynamically created property
>>> __of_free_tree() recursively frees a device node tree
>>> __of_copy_property() copies a property dynamically
>>> __of_create_empty_node() creates an empty node
>>> __of_find_node_by_full_name() finds the node with the full name
>>> and
>>> of_multi_prop_cmp() performs a multi property compare but without
>>> having to take locks.
>>> 
>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>> ---
>>>  drivers/of/Makefile |   2 +-
>>>  drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/of.h  |  59 ++++++++++++
>>>  3 files changed, 313 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/of/util.c
>>> 
>> 
>> ...
>> 
>>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
>>> +{
>>> +	struct property *propn;
>>> +
>>> +	propn = kzalloc(sizeof(*prop), flags);
>>> +	if (propn == NULL)
>>> +		return NULL;
>>> +
>>> +	propn->name = kstrdup(prop->name, flags);
>>> +	if (propn->name == NULL)
>>> +		goto err_fail_name;
>>> +
>>> +	if (prop->length > 0) {
>>         ^^^^^^^^^^^^^^^^^^^^^^^
>> As Ioan already mentioned, this is really a problem.
>> There is a bunch of places, where properties without values are used.
>> Like gpio-controller; ranges; interrupt-controller;
>> Refer, for example, to of_irq_map_raw() which checks
>> of_get_property(ipar, "interrupt-controller", NULL) != NULL
>> and some other occurrences of exactly same construct.
>> This will simply be broken for merged device-tree parts.
>> 
> 
> Folks,
> 
> it might help if you explain what exactly is broken, and how to fix it.
> It is not as if the property is not copied, only its value
> is not copied. And the value does not exist.
> 
> What do you think the code needs to do differently ? Obviously it can
> not copy a non-existing value. So what would have to be in the else case ?
> 

This was fixed by Ionut, so let me copy his email verbatim here.

> We're using the device tree overlay patches applied on top of a
> vanilla kernel. While working with it I discovered a problem
> with the __of_copy_property() function. When copying properties
> that have no value, such as "interrupt-controller;" their value
> will be set to NULL.
> 
> By contrast, if such a property exists in the initial device tree 
> at boot time it will have length 0 and value set to the empty
> string.
> 
> Some parts of the linux kernel code actually rely on this.
> For example in drivers/of/irq.c, of_irq_map_raw() checks that
> a node is an interrupt controller by calling:
> 
> of_get_property(ipar, "interrupt-controller", NULL)
> 
> and checking that it returns a non-null value.
> 
> For fixing this, we can safely use kmalloc with size 0 which
> will return ZERO_SIZE_PTR like in the patch below. This will 
> cause all the tests for non-null values to pass.

It's a bug fix. Zero length properties exist, but their
value pointer is not set to NULL.

So when copying them we have to follow the same principle,
i.e. property length set to 0, but not a NULL value pointer.

Anyway, this is addressed in the next version of patch series
which I will post shortly.


> Thanks,
> Guenter
> 
>>> +		propn->value = kmalloc(prop->length, flags);
>>> +		if (propn->value == NULL)
>>> +			goto err_fail_value;
>>> +		memcpy(propn->value, prop->value, prop->length);
>>> +		propn->length = prop->length;
>>> +	}
>>> +
>>> +	/* mark the property as dynamic */
>>> +	of_property_set_flag(propn, OF_DYNAMIC);
>>> +
>>> +	return propn;
>>> +
>>> +err_fail_value:
>>> +	kfree(propn->name);
>>> +err_fail_name:
>>> +	kfree(propn);
>>> +	return NULL;
>>> +}
>>> +
>> 
>> ...
>> 
> 

Regards

-- Pantelis