Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
[PATCH 5/5] OF: Introduce utility helper functions
1 view
Skip to first unread message
Pantelis Antoniou
Nov 5, 2013, 1:00:02 PM11/5/13
to
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 <pa...@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 <pa...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
__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 <pa...@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 <pa...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Pantelis Antoniou
Nov 5, 2013, 1:00:03 PM11/5/13
to
of_property_notify can be utilized by other users too, export it.
drivers/of/base.c | 8 +-------
include/linux/of.h | 11 +++++++++++
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index ca10916..0ffc5a9 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,6 @@ 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;
-}
#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);
drivers/of/base.c | 8 +-------
include/linux/of.h | 11 +++++++++++
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index ca10916..0ffc5a9 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,6 @@ 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;
-}
#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);
Pantelis Antoniou
Nov 5, 2013, 1:00:03 PM11/5/13
to
Helper functions for working with device node flags.
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);
/*
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);
/*
Pantelis Antoniou
Nov 5, 2013, 1:00:03 PM11/5/13
to
When attaching a node always clear the detach flag. Without this change
the sequence detach, attach fails.
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);
the sequence detach, attach fails.
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);
Pantelis Antoniou
Nov 5, 2013, 1:00:03 PM11/5/13
to
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.
Pantelis Antoniou (5):
OF: Clear detach flag on attach
OF: Introduce device tree node flag helpers.
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 | 77 ++++++++--------
drivers/of/util.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of.h | 119 ++++++++++++++++++++++++
4 files changed, 413 insertions(+), 38 deletions(-)
create mode 100644 drivers/of/util.c
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.
Pantelis Antoniou (5):
OF: Clear detach flag on attach
OF: Introduce device tree node flag helpers.
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 | 77 ++++++++--------
drivers/of/util.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of.h | 119 ++++++++++++++++++++++++
4 files changed, 413 insertions(+), 38 deletions(-)
create mode 100644 drivers/of/util.c
Pantelis Antoniou
Nov 5, 2013, 1:00:03 PM11/5/13
to
There are other users for the proc DT functions.
Export them.
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 0ffc5a9..c3c5391 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1435,6 +1435,40 @@ int of_property_notify(int action, struct device_node *np,
}
#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,
+ proc_device_tree_add_prop(np->pde, prop);
+}
+
+void of_remove_proc_dt_prop_entry(struct device_node *np,
+ 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
*/
@@ -1462,11 +1496,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;
}
@@ -1508,11 +1539,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;
}
@@ -1562,11 +1589,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;
}
@@ -1602,22 +1626,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.
*/
@@ -1643,12 +1651,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 *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 *newprop, struct property *oldprop) { }
Export them.
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 0ffc5a9..c3c5391 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1435,6 +1435,40 @@ int of_property_notify(int action, struct device_node *np,
}
#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
*/
@@ -1462,11 +1496,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;
}
@@ -1508,11 +1539,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;
}
@@ -1562,11 +1589,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;
}
@@ -1602,22 +1626,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.
*/
@@ -1643,12 +1651,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);
+#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);
Pantelis Antoniou
Nov 5, 2013, 1:20:04 PM11/5/13
to
Hi Matt,
On Nov 5, 2013, at 8:12 PM, Matt Porter wrote:
> EXPORT_SYMBOL[_GPL]?
>
Duh, good catch.
Will fix on next revision.
> -Matt
Regards
-- Pantelis
On Nov 5, 2013, at 8:12 PM, Matt Porter wrote:
>
Duh, good catch.
Will fix on next revision.
> -Matt
Regards
-- Pantelis
Matt Porter
Nov 5, 2013, 1:20:03 PM11/5/13
to
On Tue, Nov 05, 2013 at 07:50:14PM +0200, Pantelis Antoniou wrote:
-Matt
Gerhard Sittig
Nov 5, 2013, 2:50:01 PM11/5/13
to
On Tue, Nov 05, 2013 at 19:50 +0200, Pantelis Antoniou wrote:
>
> --- 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);
Does this add a call to a routine which only gets introduced in a
>
> --- 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);
subsequent patch (2/5)? If so, it would break builds during the
series, and thus would hinder bisection.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
Pantelis Antoniou
Nov 5, 2013, 3:10:02 PM11/5/13
to
Hi Gerhard,
On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
> On Tue, Nov 05, 2013 at 19:50 +0200, Pantelis Antoniou wrote:
>>
>> --- 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);
>
> Does this add a call to a routine which only gets introduced in a
> subsequent patch (2/5)? If so, it would break builds during the
> series, and thus would hinder bisection.
>
You're right, I'll re-order on the next series.
>
> virtually yours
> Gerhard Sittig
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
Regards
-- Pantelis
On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
> On Tue, Nov 05, 2013 at 19:50 +0200, Pantelis Antoniou wrote:
>>
>> --- 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);
>
> Does this add a call to a routine which only gets introduced in a
> subsequent patch (2/5)? If so, it would break builds during the
> series, and thus would hinder bisection.
>
>
> virtually yours
> Gerhard Sittig
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
-- Pantelis
Alexander Sverdlin
Nov 6, 2013, 3:50:02 AM11/6/13
to
Hello Pantelis,
On 05/11/13 21:03, ext Pantelis Antoniou wrote:
> On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
>>> --- 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);
>>
>> Does this add a call to a routine which only gets introduced in a
>> subsequent patch (2/5)? If so, it would break builds during the
>> series, and thus would hinder bisection.
>>
>
> You're right, I'll re-order on the next series.
Is it necessary at all now, after these fixes:
9e401275 of: fdt: fix memory initialization for expanded DT
0640332e of: Fix missing memory initialization on FDT unflattening
92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening
?
--
Best regards,
Alexander Sverdlin.
On 05/11/13 21:03, ext Pantelis Antoniou wrote:
> On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
>>> --- 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);
>>
>> Does this add a call to a routine which only gets introduced in a
>> subsequent patch (2/5)? If so, it would break builds during the
>> series, and thus would hinder bisection.
>>
>
> You're right, I'll re-order on the next series.
9e401275 of: fdt: fix memory initialization for expanded DT
0640332e of: Fix missing memory initialization on FDT unflattening
92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening
?
--
Best regards,
Alexander Sverdlin.
Pantelis Antoniou
Nov 6, 2013, 4:00:02 AM11/6/13
to
Hi Alexander,
I'm not exactly sure, but I think it is still needed.
Since at that point the tree is attached.
Grant?
Regards
-- Pantelis
I'm not exactly sure, but I think it is still needed.
Since at that point the tree is attached.
Grant?
Regards
-- Pantelis
Ionut Nicu
Nov 6, 2013, 4:30:03 AM11/6/13
to
Hi,
First of all, good to see this patch set being submitted again!
We're using an older version of your patch set for some time and
they're working good for us.
I think the prop->length check, should be removed. Properties with length 0, such
as "interrupt-controller;" have length 0 and some parts of the linux kernel actually
rely on their value being not NULL.
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.
We can safely use kmalloc with size 0 which will return ZERO_SIZE_PTR. This will cause
all the tests for non-null values in case of zero length properties to pass.
I've sent you a patch a while ago for this. I'm not sure you had time to review it.
Thanks,
Ionut
First of all, good to see this patch set being submitted again!
We're using an older version of your patch set for some time and
they're working good for us.
as "interrupt-controller;" have length 0 and some parts of the linux kernel actually
rely on their value being not NULL.
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.
We can safely use kmalloc with size 0 which will return ZERO_SIZE_PTR. This will cause
all the tests for non-null values in case of zero length properties to pass.
I've sent you a patch a while ago for this. I'm not sure you had time to review it.
Thanks,
Ionut
Pantelis Antoniou
Nov 6, 2013, 4:40:03 AM11/6/13
to
Hi Ionut,
On Nov 6, 2013, at 11:21 AM, Ionut Nicu wrote:
> Hi,
>
> First of all, good to see this patch set being submitted again!
>
> We're using an older version of your patch set for some time and
> they're working good for us.
>
Thanks, that's good to know.
I am aware of that, and your patch looks sane.
As I mentioned earlier I'm trying to get this accepted in general term and then we'll
get around fixing any minor problems.
> Thanks,
> Ionut
>
Regards
-- Pantelis
On Nov 6, 2013, at 11:21 AM, Ionut Nicu wrote:
> Hi,
>
> First of all, good to see this patch set being submitted again!
>
> We're using an older version of your patch set for some time and
> they're working good for us.
>
As I mentioned earlier I'm trying to get this accepted in general term and then we'll
get around fixing any minor problems.
> Thanks,
> Ionut
>
Regards
-- Pantelis
Alexander Sverdlin
Nov 6, 2013, 5:10:01 AM11/6/13
to
Hi!
On 06/11/13 09:49, ext Pantelis Antoniou wrote:
> I'm not exactly sure, but I think it is still needed.
> Since at that point the tree is attached.
Yes, now I think it's necessary. If you consider multiple detach-attach sequences.
I only thought about first fdt unflattering, which is the case in overlay_proc_release(), I suppose.
So the call to of_node_clear_flag() is superfluous, but doesn't hurt.
On 06/11/13 09:49, ext Pantelis Antoniou wrote:
> I'm not exactly sure, but I think it is still needed.
> Since at that point the tree is attached.
I only thought about first fdt unflattering, which is the case in overlay_proc_release(), I suppose.
So the call to of_node_clear_flag() is superfluous, but doesn't hurt.
Guenter Roeck
Nov 6, 2013, 10:00:03 AM11/6/13
to
Hi Pantelis,
On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
>
> As I mentioned earlier I'm trying to get this accepted in general term and then we'll
> get around fixing any minor problems.
>
I think it is quite likely at this point that your series will be accepted.
However, from my experience with larger submissions like this one, it is quite common
that the maintainers ask for all feedback to be addressed before that happens -
especially for feedback like this, where there is common understanding that some
changes to the code are necessary.
Maybe the device tree maintainers could chime in at this point and let us all know
what will need to happen to get the series accepted.
Thanks,
Guenter
On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
>
> As I mentioned earlier I'm trying to get this accepted in general term and then we'll
> get around fixing any minor problems.
>
However, from my experience with larger submissions like this one, it is quite common
that the maintainers ask for all feedback to be addressed before that happens -
especially for feedback like this, where there is common understanding that some
changes to the code are necessary.
Maybe the device tree maintainers could chime in at this point and let us all know
what will need to happen to get the series accepted.
Thanks,
Guenter
Pantelis Antoniou
Nov 6, 2013, 10:10:01 AM11/6/13
to
Hi Guenter,
On Nov 6, 2013, at 4:53 PM, Guenter Roeck wrote:
> Hi Pantelis,
>
> On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
>
>>
>> As I mentioned earlier I'm trying to get this accepted in general term and then we'll
>> get around fixing any minor problems.
>>
>
> I think it is quite likely at this point that your series will be accepted.
>
Heh, maybe.
> However, from my experience with larger submissions like this one, it is quite common
> that the maintainers ask for all feedback to be addressed before that happens -
> especially for feedback like this, where there is common understanding that some
> changes to the code are necessary.
>
Yep, I will have to spin out new versions addressing everything.
> Maybe the device tree maintainers could chime in at this point and let us all know
> what will need to happen to get the series accepted.
>
That would be nice indeed.
> Thanks,
> Guenter
>
>
Regards
-- Pantelis--
On Nov 6, 2013, at 4:53 PM, Guenter Roeck wrote:
> Hi Pantelis,
>
> On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
>
>>
>> As I mentioned earlier I'm trying to get this accepted in general term and then we'll
>> get around fixing any minor problems.
>>
>
> I think it is quite likely at this point that your series will be accepted.
>
> However, from my experience with larger submissions like this one, it is quite common
> that the maintainers ask for all feedback to be addressed before that happens -
> especially for feedback like this, where there is common understanding that some
> changes to the code are necessary.
>
> Maybe the device tree maintainers could chime in at this point and let us all know
> what will need to happen to get the series accepted.
>
> Thanks,
> Guenter
>
>
Regards
-- Pantelis--
Alexander Sverdlin
Nov 6, 2013, 10:50:02 AM11/6/13
to
Hi!
On 05/11/13 18:50, ext Pantelis Antoniou wrote:
> 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.
>
> Pantelis Antoniou (5):
> OF: Clear detach flag on attach
> OF: Introduce device tree node flag helpers.
> OF: export of_property_notify
> OF: Export all DT proc update functions
> OF: Introduce utility helper functions
All patches there we use as they are since July on our MIPS64 platforms. So, for all 5 patches:
Tested-by: Alexander Sverdlin <alexander...@nsn.com>
Reviewed-by: Alexander Sverdlin <alexander...@nsn.com>
> drivers/of/Makefile | 2 +-
> drivers/of/base.c | 77 ++++++++--------
> drivers/of/util.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/of.h | 119 ++++++++++++++++++++++++
> 4 files changed, 413 insertions(+), 38 deletions(-)
> create mode 100644 drivers/of/util.c
>
--
Best regards,
Alexander Sverdlin.
On 05/11/13 18:50, ext Pantelis Antoniou wrote:
> 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.
>
> Pantelis Antoniou (5):
> OF: Clear detach flag on attach
> OF: Introduce device tree node flag helpers.
> OF: export of_property_notify
> OF: Export all DT proc update functions
> OF: Introduce utility helper functions
Tested-by: Alexander Sverdlin <alexander...@nsn.com>
Reviewed-by: Alexander Sverdlin <alexander...@nsn.com>
> drivers/of/Makefile | 2 +-
> drivers/of/base.c | 77 ++++++++--------
> drivers/of/util.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/of.h | 119 ++++++++++++++++++++++++
> 4 files changed, 413 insertions(+), 38 deletions(-)
> create mode 100644 drivers/of/util.c
>
--
Alexander Sverdlin.
Pantelis Antoniou
Nov 7, 2013, 2:40:01 PM11/7/13
to
There are other users for the proc DT functions.
Export them.
Export them.
drivers/of/base.c | 70 ++++++++++++++++++++++++++++++------------------------
include/linux/of.h | 29 ++++++++++++++++++++++
2 files changed, 68 insertions(+), 31 deletions(-)
include/linux/of.h | 29 ++++++++++++++++++++++
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0ffc5a9..c3c5391 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1435,6 +1435,40 @@ int of_property_notify(int action, struct device_node *np,
}
#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)
}
#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,
+{
+ 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)
+ proc_device_tree_add_prop(np->pde, prop);
+}
+
+void of_remove_proc_dt_prop_entry(struct device_node *np,
+{
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) { }
}
#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,
+
+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
+ 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
Pantelis Antoniou
Nov 7, 2013, 2:40:01 PM11/7/13
to
Helper functions for working with device node flags.
index f95aee3..786c4f6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
+
include/linux/of.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
1 file changed, 20 insertions(+)
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);
+}
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);
/*
+{
+ 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);
/*
Pantelis Antoniou
Nov 7, 2013, 2:40:01 PM11/7/13
to
When attaching a node always clear the detach flag. Without this change
the sequence detach, attach fails.
the sequence detach, attach fails.
drivers/of/base.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/base.c b/drivers/of/base.c
1 file changed, 1 insertion(+)
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);
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);
Pantelis Antoniou
Nov 7, 2013, 2:40:02 PM11/7/13
to
of_property_notify can be utilized by other users too, export it.
drivers/of/base.c | 8 +-------
include/linux/of.h | 11 +++++++++++
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
include/linux/of.h | 11 +++++++++++
2 files changed, 12 insertions(+), 7 deletions(-)
index ca10916..0ffc5a9 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,6 @@ 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;
-}
#endif
/**
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,6 @@ 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;
-}
#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,
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
+ 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);
+#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);
Pantelis Antoniou
Nov 7, 2013, 2:40:02 PM11/7/13
to
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 V2:
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.
* 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
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 | 77 ++++++++--------
drivers/of/util.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of.h | 119 ++++++++++++++++++++++++
4 files changed, 413 insertions(+), 38 deletions(-)
create mode 100644 drivers/of/util.c
drivers/of/base.c | 77 ++++++++--------
drivers/of/util.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of.h | 119 ++++++++++++++++++++++++
4 files changed, 413 insertions(+), 38 deletions(-)
create mode 100644 drivers/of/util.c
Pantelis Antoniou
Nov 7, 2013, 2:40:02 PM11/7/13
to
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.
__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.
+{
index a56c450..9d69bd2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
+
+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 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 */
+{
+ return -1;
+}
+
+#endif /* !CONFIG_OF */
+
#endif /* _LINUX_OF_H */
Pantelis Antoniou
Nov 7, 2013, 3:20:01 PM11/7/13
to
There are other users for the proc DT functions.
Export them.
Export them.
drivers/of/base.c | 70 ++++++++++++++++++++++++++++++------------------------
include/linux/of.h | 29 ++++++++++++++++++++++
2 files changed, 68 insertions(+), 31 deletions(-)
include/linux/of.h | 29 ++++++++++++++++++++++
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)
+#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,
+{
+ 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)
+ proc_device_tree_add_prop(np->pde, prop);
+}
+
+void of_remove_proc_dt_prop_entry(struct device_node *np,
+{
+ 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)
+ 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
*/
*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)
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;
}
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)
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;
}
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 -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;
}
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_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.
*/
}
#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
#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;
}
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,
}
#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);
+
+ 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) { }
+ 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,
+
+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);
+#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);
Pantelis Antoniou
Nov 7, 2013, 3:20:01 PM11/7/13
to
of_property_notify can be utilized by other users too, export it.
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,
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;
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);
return of_reconfig_notify(action, &pr);
}
-#else
-static int of_property_notify(int action, struct device_node *np,
- struct property *prop)
-{
- return 0;
-}
#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,
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
+ struct property *prop)
+{
+ return 0;
+{
Pantelis Antoniou
Nov 7, 2013, 3:20:01 PM11/7/13
to
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.
__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.
+{
index a56c450..9d69bd2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
+
Pantelis Antoniou
Nov 7, 2013, 3:20:02 PM11/7/13
to
Helper functions for working with device node flags.
index f95aee3..786c4f6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
+
include/linux/of.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
1 file changed, 20 insertions(+)
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);
+}
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);
/*
+{
+ 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);
/*
Pantelis Antoniou
Nov 7, 2013, 3:20:02 PM11/7/13
to
When attaching a node always clear the detach flag. Without this change
the sequence detach, attach fails.
the sequence detach, attach fails.
drivers/of/base.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/base.c b/drivers/of/base.c
1 file changed, 1 insertion(+)
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);
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);
Pantelis Antoniou
Nov 7, 2013, 3:20:02 PM11/7/13
to
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:
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.
* 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/util.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of.h | 119 ++++++++++++++++++++++++
4 files changed, 414 insertions(+), 38 deletions(-)
include/linux/of.h | 119 ++++++++++++++++++++++++
create mode 100644 drivers/of/util.c
Alexander Sverdlin
Nov 8, 2013, 4:10:02 AM11/8/13
to
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 <pa...@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.
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 <pa...@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.
Guenter Roeck
Nov 8, 2013, 10:00:01 AM11/8/13
to
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;
>> +}
>> +
>
> ...
>
--
Alexander Sverdlin
Nov 8, 2013, 10:10:01 AM11/8/13
to
Hi!
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
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 ?
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;
>>> +}
>>> +
>>
>> ...
>>
>
> --
> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Best regards,
Alexander Sverdlin.
Best regards,
Alexander Sverdlin.
Pantelis Antoniou
Nov 8, 2013, 10:10:01 AM11/8/13
to
of_property_notify can be utilized by other users too, export it.
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
include/linux/of.h | 11 +++++++++++
2 files changed, 13 insertions(+), 7 deletions(-)
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
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
/**
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
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
Pantelis Antoniou
Nov 8, 2013, 10:10:01 AM11/8/13
to
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 V4:
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.
* Bug fix about prop->len == 0 by Ionut Nicu <ioan.n...@nsn.com>
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 ++++++++--------
include/linux/of.h | 119 +++++++++++++++++++++++++
4 files changed, 412 insertions(+), 38 deletions(-)
create mode 100644 drivers/of/util.c
Pantelis Antoniou
Nov 8, 2013, 10:10:02 AM11/8/13
to
There are other users for the proc DT functions.
Export them.
Export them.
drivers/of/base.c | 70 ++++++++++++++++++++++++++++++------------------------
include/linux/of.h | 29 ++++++++++++++++++++++
2 files changed, 68 insertions(+), 31 deletions(-)
include/linux/of.h | 29 ++++++++++++++++++++++
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,
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);
+}
+
+ 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,
+ proc_device_tree_add_prop(np->pde, prop);
+}
+
+ struct property *prop)
+{
+{
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,
}
#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);
+
+ 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 *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) { }
+
+ 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);
+#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);
Pantelis Antoniou
Nov 8, 2013, 10:10:02 AM11/8/13
to
Hi Guenter,
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.
> 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
> 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
> a node is an interrupt controller by calling:
>
> of_get_property(ipar, "interrupt-controller", NULL)
>
> and checking that it returns a non-null value.
>
> 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;
>>> +}
>>> +
>>
>> ...
>>
>
-- Pantelis
Pantelis Antoniou
Nov 8, 2013, 10:10:02 AM11/8/13
to
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.
drivers/of/util.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++
__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.
include/linux/of.h | 59 ++++++++++++
3 files changed, 311 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
--- /dev/null
+++ b/drivers/of/util.c
@@ -0,0 +1,251 @@
+{
+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;
+
+{
+ 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;
+
+ 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;
+}
+
+ 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)
+ * __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,
+{
+ struct device_node *node;
+
+ node = kzalloc(sizeof(*node), flags);
+ if (node == NULL)
+
+ node = kzalloc(sizeof(*node), flags);
+ 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);
+ 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)
+ * __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;
+
+ 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;
+ }
+
+ 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;
+}
+
+}
+
index a56c450..9d69bd2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
+
+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)
+{
+ 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)
+{
+ 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 */
+{
+ return -1;
+}
+
+#endif /* !CONFIG_OF */
+
#endif /* _LINUX_OF_H */
Pantelis Antoniou
Nov 8, 2013, 10:10:02 AM11/8/13
to
Helper functions for working with device node flags.
index f95aee3..786c4f6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
+
include/linux/of.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
1 file changed, 20 insertions(+)
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);
+}
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);
/*
+{
+ 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);
/*
Pantelis Antoniou
Nov 8, 2013, 10:10:03 AM11/8/13
to
When attaching a node always clear the detach flag. Without this change
the sequence detach, attach fails.
the sequence detach, attach fails.
drivers/of/base.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/base.c b/drivers/of/base.c
1 file changed, 1 insertion(+)
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);
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);
Guenter Roeck
Nov 8, 2013, 2:20:02 PM11/8/13
to
On Fri, Nov 08, 2013 at 05:04:00PM +0200, 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.
>
> Bug fix about prop->len == 0 by Ionut Nicu <ioan.n...@nsn.com>
>
> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
Hi Pantelis,
> 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.
>
> Bug fix about prop->len == 0 by Ionut Nicu <ioan.n...@nsn.com>
>
> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
nitpick:
.git/rebase-apply/patch:77: trailing whitespace.
* no pointer exist for any properties. Only safe to do if you
.git/rebase-apply/patch:213: trailing whitespace.
* Find a node with the give full name by recursively following any of
.git/rebase-apply/patch:274: new blank line at EOF.
+
warning: 3 lines add whitespace errors.
Guenter
Grant Likely
Nov 11, 2013, 9:40:01 PM11/11/13
to
On Wed, 6 Nov 2013 10:49:44 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
> >>> subsequent patch (2/5)? If so, it would break builds during the
> >>> series, and thus would hinder bisection.
> >>>
> >>
> >> You're right, I'll re-order on the next series.
> >
> > Is it necessary at all now, after these fixes:
> > 9e401275 of: fdt: fix memory initialization for expanded DT
> > 0640332e of: Fix missing memory initialization on FDT unflattening
> > 92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening
>
> Hi Alexander,
>
> I'm not exactly sure, but I think it is still needed.
> Since at that point the tree is attached.
>
> Grant?
In one sense it is a little odd because it isn't something that any of
the existing users (of which there are 2) would be affected by. It isn't
a bad idea though. Merged patches 2 & 1.
g.
> On Nov 6, 2013, at 10:46 AM, Alexander Sverdlin wrote:
>
> > Hello Pantelis,
> >
> > On 05/11/13 21:03, ext Pantelis Antoniou wrote:
>
> > Hello Pantelis,
> >
> >> On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
> >>>> --- 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);
> >>>
> >>> Does this add a call to a routine which only gets introduced in a
> >>>> --- 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);
> >>>
> >>> subsequent patch (2/5)? If so, it would break builds during the
> >>> series, and thus would hinder bisection.
> >>>
> >>
> >> You're right, I'll re-order on the next series.
> >
> > Is it necessary at all now, after these fixes:
> > 9e401275 of: fdt: fix memory initialization for expanded DT
> > 0640332e of: Fix missing memory initialization on FDT unflattening
> > 92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening
>
> Hi Alexander,
>
> I'm not exactly sure, but I think it is still needed.
> Since at that point the tree is attached.
>
> Grant?
In one sense it is a little odd because it isn't something that any of
the existing users (of which there are 2) would be affected by. It isn't
a bad idea though. Merged patches 2 & 1.
g.
Grant Likely
Nov 11, 2013, 9:40:01 PM11/11/13
to
On Tue, 5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> 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 <pa...@antoniou-consulting.com>
> 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.
>
So, this all looks like private stuff, or stuff that belongs in
drivers/of/base.c. Can you move stuff around. I've made more comments
below.
g.
> ---
> 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
>
> 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
> --- /dev/null
> +++ b/drivers/of/util.c
> @@ -0,0 +1,253 @@
if anything still holds a reference to a node. The proper way to handle
removal of properties is to have a release method when the last
of_node_put is called.
> +
> +/**
> + * __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.
> + */
duplicated property be immediately modified? If so, what happens if the
property needs to be grown in size?
> +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;
> +
allocate nodes. It will make for less chance of breakage if the fdt and
pdt paths change something, but this function isn't updated.
g.
> +{
> + 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;
of unflattening an overlay tree, is there a reason that the passed in
names cannot be used directly instead of kmallocing them?
> +{
> + 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;
> + }
of_fund_node_by_name() work here?
I agree that of_find_node_by_name is not particularly elegant and it
would be good to have something more efficient, but it works and
following the same method would be consistent.
> +
> + 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
over the way different OFW implementations work. Don't use the same
function. Don't use it for any property other than compatible because
that will just encourage bad behaviour.
> + * 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.
function that doesn't take the lock.
> + */
> +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;
> +}
> +
> index a56c450..9d69bd2 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> +
> +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
>
> +{
> + return -1;
> +}
> +
> +#endif /* !CONFIG_OF */
> +
> #endif /* _LINUX_OF_H */
> --
> 1.7.12
>
Grant Likely
Nov 11, 2013, 9:40:02 PM11/11/13
to
On Tue, 5 Nov 2013 19:50:14 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
> of_property_notify can be utilized by other users too, export it.
Please keep this patch in the series with the patch that actually uses
> of_property_notify can be utilized by other users too, export it.
it.
g.
> drivers/of/base.c | 8 +-------
> include/linux/of.h | 11 +++++++++++
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ca10916..0ffc5a9 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,6 @@ static int of_property_notify(int action, struct device_node *np,
> 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;
> 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;
> -}
> #endif
> return of_reconfig_notify(action, &pr);
> }
> -#else
> -static int of_property_notify(int action, struct device_node *np,
> - struct property *prop)
> -{
> - return 0;
> -}
>
> /**
> 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,
> 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
> + 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);
>
> +#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);
>
Grant Likely
Nov 11, 2013, 9:40:02 PM11/11/13
to
On Tue, 5 Nov 2013 19:50:13 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
> Helper functions for working with device node flags.
>
> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
Merged, thanks
> Helper functions for working with device node flags.
>
> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
g.
> ---
> include/linux/of.h | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> 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);
> +}
> 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);
>
> /*
> +{
> + 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);
>
> /*
Grant Likely
Nov 11, 2013, 9:40:02 PM11/11/13
to
On Wed, 06 Nov 2013 06:53:13 -0800, Guenter Roeck <li...@roeck-us.net> wrote:
> Hi Pantelis,
>
> On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
>
> >
> > As I mentioned earlier I'm trying to get this accepted in general term and then we'll
> > get around fixing any minor problems.
> >
>
> I think it is quite likely at this point that your series will be accepted.
>
> However, from my experience with larger submissions like this one, it is quite common
> that the maintainers ask for all feedback to be addressed before that happens -
> especially for feedback like this, where there is common understanding that some
> changes to the code are necessary.
>
> Maybe the device tree maintainers could chime in at this point and let us all know
> what will need to happen to get the series accepted.
Since it is a big series, I would like to be able to "front-load" the
series of changes with straighforward changes that are easy to merge.
There are some exceptions though, such as when new APIs with users later
in the series. I don't really want to pull in a new API separately from
the user most of the time.
g.
> Hi Pantelis,
>
> On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
>
> >
> > As I mentioned earlier I'm trying to get this accepted in general term and then we'll
> > get around fixing any minor problems.
> >
>
> I think it is quite likely at this point that your series will be accepted.
>
> However, from my experience with larger submissions like this one, it is quite common
> that the maintainers ask for all feedback to be addressed before that happens -
> especially for feedback like this, where there is common understanding that some
> changes to the code are necessary.
>
> Maybe the device tree maintainers could chime in at this point and let us all know
> what will need to happen to get the series accepted.
Since it is a big series, I would like to be able to "front-load" the
series of changes with straighforward changes that are easy to merge.
There are some exceptions though, such as when new APIs with users later
in the series. I don't really want to pull in a new API separately from
the user most of the time.
g.
Grant Likely
Nov 11, 2013, 9:40:03 PM11/11/13
to
On Tue, 5 Nov 2013 19:50:15 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
> There are other users for the proc DT functions.
> Export them.
>
> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
These are all very much internal DT APIs. There is no way that anything
> There are other users for the proc DT functions.
> Export them.
>
> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
outside of drivers/of should ever be calling them. (In fact, I want to
be rid of the /proc/devicetree implementation entirely and put it into
sysfs, but that is a separate patch set). If they need to be broken out
into a header file, can they be put into something like
drivers/of/of_private.h?
g.
> ---
> drivers/of/base.c | 70 ++++++++++++++++++++++++++++++------------------------
> include/linux/of.h | 29 ++++++++++++++++++++++
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 0ffc5a9..c3c5391 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1435,6 +1435,40 @@ int of_property_notify(int action, struct device_node *np,
> }
> #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,
> #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);
> +}
> +
> + 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,
> + proc_device_tree_add_prop(np->pde, prop);
> +}
> +
> + 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
> */
> @@ -1462,11 +1496,8 @@ int of_add_property(struct device_node *np, struct property *prop)
> + 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
> */
> *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;
> }
> @@ -1508,11 +1539,7 @@ int of_remove_property(struct device_node *np, struct property *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;
> }
> 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;
> }
> @@ -1562,11 +1589,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
> 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;
> }
> 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;
> }
> @@ -1602,22 +1626,6 @@ int of_reconfig_notify(unsigned long action, void *p)
> 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;
> }
> 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.
> */
> @@ -1643,12 +1651,12 @@ int of_attach_node(struct device_node *np)
> }
>
> -#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.
> */
> }
>
> #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
>
> #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;
> }
> 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,
> }
> #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);
> +
> + 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 *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) { }
> +
> + 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
>
> +#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
>
Pantelis Antoniou
Nov 12, 2013, 4:40:02 AM11/12/13
to
Hi Grant,
On Nov 11, 2013, at 5:06 PM, Grant Likely wrote:
> On Tue, 5 Nov 2013 19:50:14 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
>> of_property_notify can be utilized by other users too, export it.
>
> Please keep this patch in the series with the patch that actually uses
> it.
>
OK.
> g.
>
Regards
-- Pantelis
On Nov 11, 2013, at 5:06 PM, Grant Likely wrote:
> On Tue, 5 Nov 2013 19:50:14 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
>> of_property_notify can be utilized by other users too, export it.
>
> Please keep this patch in the series with the patch that actually uses
> it.
>
> g.
>
Regards
-- Pantelis
Pantelis Antoniou
Nov 12, 2013, 5:30:02 AM11/12/13
to
Hi Grant,
On Nov 11, 2013, at 5:09 PM, Grant Likely wrote:
> On Tue, 5 Nov 2013 19:50:15 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
>> There are other users for the proc DT functions.
>> Export them.
>>
>> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
>
> These are all very much internal DT APIs. There is no way that anything
> outside of drivers/of should ever be calling them. (In fact, I want to
> be rid of the /proc/devicetree implementation entirely and put it into
> sysfs, but that is a separate patch set). If they need to be broken out
> into a header file, can they be put into something like
> drivers/of/of_private.h?
>
That's fine by me.
I can move them to of_private.h, no problem there.
> g.
>
Regards
-- Pantelis
On Nov 11, 2013, at 5:09 PM, Grant Likely wrote:
> On Tue, 5 Nov 2013 19:50:15 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> wrote:
>> There are other users for the proc DT functions.
>> Export them.
>>
>> Signed-off-by: Pantelis Antoniou <pa...@antoniou-consulting.com>
>
> These are all very much internal DT APIs. There is no way that anything
> outside of drivers/of should ever be calling them. (In fact, I want to
> be rid of the /proc/devicetree implementation entirely and put it into
> sysfs, but that is a separate patch set). If they need to be broken out
> into a header file, can they be put into something like
> drivers/of/of_private.h?
>
I can move them to of_private.h, no problem there.
> g.
>
Regards
-- Pantelis
Pantelis Antoniou
Nov 12, 2013, 5:40:01 AM11/12/13
to
Hi Grant,
On Nov 11, 2013, at 5:37 PM, Grant Likely wrote:
> On Tue, 5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> 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 <pa...@antoniou-consulting.com>
>
> So, this all looks like private stuff, or stuff that belongs in
> drivers/of/base.c. Can you move stuff around. I've made more comments
> below.
>
Placement is no big issue;
> g.
>
[snip]
>> + } else {
>> + pr_warn("%s: node %p cannot be freed; memory is gone\n",
>> + __func__, node);
>> + }
>> +}
>
> All of the above is potentially dangerous. There is no way to determine
> if anything still holds a reference to a node. The proper way to handle
> removal of properties is to have a release method when the last
> of_node_put is called.
>
This is safe, and expected to be called only on a dynamically created tree,
that's what all the checks against OF_DYNAMIC guard against.
It is not ever meant to be called on an arbitrary tree, created by unflattening
a blob.
Perhaps we could have a switch to control whether an unflattened tree is
created dynamically and then freeing/releasing will work.
kobject-ifcation will require it anyway, don't you agree?
>> +
>> +/**
>> + * __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.
>> + */
>
> What do you intend the use-case to be for this function? Will the
> duplicated property be immediately modified? If so, what happens if the
> property needs to be grown in size?
>
No, the property will no be modified. If it needs to grow it will be moved to
deadprops (since we don't track refs to props) and a new one will be allocated.
Hmm, where do you think it can be used? Perhaps the unflatten parts?
> g.
>
>> +{
>> + 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;
>
> Again, who do you expect the user of this function to be? If it is part
> of unflattening an overlay tree, is there a reason that the passed in
> names cannot be used directly instead of kmallocing them?
>
I want to be able to get rid of the blob eventually; I don't need to keep
dragging it around.
Yes.
>> +{
>> + 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;
>> + }
>
> I'm not a huge fan of recursive calls. Why doesn't a slightly modified
> of_fund_node_by_name() work here?
>
> I agree that of_find_node_by_name is not particularly elegant and it
> would be good to have something more efficient, but it works and
> following the same method would be consistent.
>
of_find_node_by_name is not iterating on the passed tree and it's subnodes,
it is a global search starting from:
np = from ? from->allnext : of_allnodes;
This is just broken, since it depends on unflattening creating nodes in the
allnodes list in sequence. I.e. that child nodes are after the parent node.
This assumption breaks down when doing dynamic insertions of nodes.
A detached tree in not linked on of_allnodes anyway.
>> +
>> + 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_compat_cmp() is only for compatible strings, and it purely to paper
> over the way different OFW implementations work. Don't use the same
> function. Don't use it for any property other than compatible because
> that will just encourage bad behaviour.
>
OK
>> + * 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.
>
> That's what __of_device_is_compatible() is for. It is a version of the
> function that doesn't take the lock.
I see. The original version of the patches started before this was
available. Will remove.
>
>> + */
>> +int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> + const char *cp;
>> +
[snip]
Regards
-- Pantelis
On Nov 11, 2013, at 5:37 PM, Grant Likely wrote:
> On Tue, 5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <pa...@antoniou-consulting.com> 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 <pa...@antoniou-consulting.com>
>
> So, this all looks like private stuff, or stuff that belongs in
> drivers/of/base.c. Can you move stuff around. I've made more comments
> below.
>
> g.
>
[snip]
>> + } else {
>> + pr_warn("%s: node %p cannot be freed; memory is gone\n",
>> + __func__, node);
>> + }
>> +}
>
> All of the above is potentially dangerous. There is no way to determine
> if anything still holds a reference to a node. The proper way to handle
> removal of properties is to have a release method when the last
> of_node_put is called.
>
that's what all the checks against OF_DYNAMIC guard against.
It is not ever meant to be called on an arbitrary tree, created by unflattening
a blob.
Perhaps we could have a switch to control whether an unflattened tree is
created dynamically and then freeing/releasing will work.
kobject-ifcation will require it anyway, don't you agree?
>> +
>> +/**
>> + * __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.
>> + */
>
> What do you intend the use-case to be for this function? Will the
> duplicated property be immediately modified? If so, what happens if the
> property needs to be grown in size?
>
deadprops (since we don't track refs to props) and a new one will be allocated.
> g.
>
>> +{
>> + 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;
>
> Again, who do you expect the user of this function to be? If it is part
> of unflattening an overlay tree, is there a reason that the passed in
> names cannot be used directly instead of kmallocing them?
>
dragging it around.
>> +{
>> + 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;
>> + }
>
> I'm not a huge fan of recursive calls. Why doesn't a slightly modified
> of_fund_node_by_name() work here?
>
> I agree that of_find_node_by_name is not particularly elegant and it
> would be good to have something more efficient, but it works and
> following the same method would be consistent.
>
it is a global search starting from:
np = from ? from->allnext : of_allnodes;
This is just broken, since it depends on unflattening creating nodes in the
allnodes list in sequence. I.e. that child nodes are after the parent node.
This assumption breaks down when doing dynamic insertions of nodes.
A detached tree in not linked on of_allnodes anyway.
>> +
>> + 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_compat_cmp() is only for compatible strings, and it purely to paper
> over the way different OFW implementations work. Don't use the same
> function. Don't use it for any property other than compatible because
> that will just encourage bad behaviour.
>
>> + * 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.
>
> That's what __of_device_is_compatible() is for. It is a version of the
> function that doesn't take the lock.
available. Will remove.
>
>> + */
>> +int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> + const char *cp;
>> +
Regards
-- Pantelis
Grant Likely
Nov 12, 2013, 11:00:02 PM11/12/13
to
when a driver or other code holds a reference to a dynamic nodes, but
doesn't release it correctly. The memory must not be freed until all of
the references are relased. OF_DYNAMIC doesn't actually help in that
case, and it is the reason for of_node_get()/of_node_put()
> Perhaps we could have a switch to control whether an unflattened tree is
> created dynamically and then freeing/releasing will work.
>
> kobject-ifcation will require it anyway, don't you agree?
real OFW (pdt.c).
>
> > g.
> >
> >> +{
> >> + 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;
> >
> > Again, who do you expect the user of this function to be? If it is part
> > of unflattening an overlay tree, is there a reason that the passed in
> > names cannot be used directly instead of kmallocing them?
> >
>
> I want to be able to get rid of the blob eventually; I don't need to keep
> dragging it around.
copied.
and the overlay trees so that a common iterator can be implemented.
g.
Pantelis Antoniou
Nov 13, 2013, 4:10:02 AM11/13/13
to
Hi Grant,
I know, but even that is not enough. of_node_get()/of_node_put() handles the
case of references to the nodes, but not what happens with references to
properties. deadprops is mitigating the problem somewhat, but if we're going
to go to all the trouble of kobjectification let's do the props as well.
of_get_property could be modified to return a devm_kmalloced copy of the real
property and that would deal with most of the callers. Of course for
the small sized scalar data we can avoid the copy.
By using the devm_* interface we also avoid having to mess too much with the callers.
I.e. what about something like devm_of_get_property()?
Copying data lead to less problems that having to drag that blob around.
That's just preference, so not a big issue.
I don't know. The overlay trees while being built are not affecting the
kernel in any way until they're applied. They're just a bunch of private
data.
Consider the case of an overlay tree that's not applied, but contains bad
data, for example a name of a node clashes with one of the live tree.
Linking it into the kernel's real live tree can lead to problems, like
shadowing the real node.
> g.
>
Regards
-- Pantelis
case of references to the nodes, but not what happens with references to
properties. deadprops is mitigating the problem somewhat, but if we're going
to go to all the trouble of kobjectification let's do the props as well.
of_get_property could be modified to return a devm_kmalloced copy of the real
property and that would deal with most of the callers. Of course for
the small sized scalar data we can avoid the copy.
By using the devm_* interface we also avoid having to mess too much with the callers.
I.e. what about something like devm_of_get_property()?
That's just preference, so not a big issue.
kernel in any way until they're applied. They're just a bunch of private
data.
Consider the case of an overlay tree that's not applied, but contains bad
data, for example a name of a node clashes with one of the live tree.
Linking it into the kernel's real live tree can lead to problems, like
shadowing the real node.
> g.
>
Regards
-- Pantelis
Grant Likely
Nov 13, 2013, 9:20:02 PM11/13/13
to
see a better way to handle it in the dynamic case, so we're stuck with
it, but I don't want to make it any harder. Adding ref counting to
properties will make it harder than it already is to get the code right.
I'm absolutely fine with a little bit of wasted memory in the form of
deadprops when the alternative is so horrible. References at the node
level is enough granularity.
I don't think kduping the property is the solution either. I strongly
suspect that will be far more expensive than the deadprop solution.
the blob?
g.
Pantelis Antoniou
Nov 14, 2013, 5:00:01 AM11/14/13
to
Hi Grant,
As long as we can live with deadprops all is fine. Perhaps a devm_of_get_property()
makes sense for new drivers though? What do you think? Perhaps copying to a
user supplied buffer as well?
It's a kind of drag. That means you get handed a device_node pointer you are not
able to free it without having the blob along with the container/accessor of it.
I.e. For the normal case where the blob comes from a request_firmware() call
You have to keep the firmware structure around.
Depending on what other method you're going to use tends to make the code a little
bit messier.
>
> g.
Regards
-- Pantelis
makes sense for new drivers though? What do you think? Perhaps copying to a
user supplied buffer as well?
able to free it without having the blob along with the container/accessor of it.
I.e. For the normal case where the blob comes from a request_firmware() call
You have to keep the firmware structure around.
Depending on what other method you're going to use tends to make the code a little
bit messier.
>
> g.
Regards
-- Pantelis
Grant Likely
Nov 15, 2013, 10:10:02 AM11/15/13
to
be shorter than the node lifetime.
> It's a kind of drag. That means you get handed a device_node pointer you are not
> able to free it without having the blob along with the container/accessor of it.
> I.e. For the normal case where the blob comes from a request_firmware() call
> You have to keep the firmware structure around.
>
> Depending on what other method you're going to use tends to make the code a little
> bit messier.
reworkd in the future if necessary since there are no associated
userspace ABI issues.
g.
Pantelis Antoniou
Nov 16, 2013, 7:30:02 AM11/16/13
to
Hi Grant,
OK, will do.
Regards
-- Pantelis--
Regards
-- Pantelis--
0 new messages