All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Transaction DT support (v2)
@ 2014-07-04 16:58 Pantelis Antoniou
  2014-07-04 16:58 ` [PATCH 1/5] of: Do not free memory at of_node_release option Pantelis Antoniou
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-04 16:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou, Pantelis Antoniou

The following patchset introduces DT transactions as a core
dynamic DT feature.

A number of supporting patches preceed it, which introduce
the required infrastructure.

Changes since V1:
* Added an option for the non-freeing on of_node_release().
* Reworked the non-used methods for __of_* methods
* Reworked transactions as gcl asked - no more states,
and the live tree is not affected until the apply() call.
* Renamed commit() -> apply.
* Got rid of device_state_change.

Pantelis Antoniou (5):
  of: Do not free memory at of_node_release option
  OF: Export a number of __of_* methods
  OF: Utility helper functions for dynamic nodes
  of: Introduce tree change __foo_post methods.
  of: Transactional DT support.

 Documentation/devicetree/transactions.txt |  41 ++
 drivers/of/Makefile                       |   2 +-
 drivers/of/base.c                         | 171 +++++---
 drivers/of/dynamic.c                      | 281 +++++++++---
 drivers/of/of_private.h                   |  49 +++
 drivers/of/transaction.c                  | 699 ++++++++++++++++++++++++++++++
 include/linux/of.h                        |  91 ++++
 7 files changed, 1224 insertions(+), 110 deletions(-)
 create mode 100644 Documentation/devicetree/transactions.txt
 create mode 100644 drivers/of/transaction.c

-- 
1.7.12

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH 1/5] of: Do not free memory at of_node_release option
  2014-07-04 16:58 [PATCH 0/5] Transaction DT support (v2) Pantelis Antoniou
@ 2014-07-04 16:58 ` Pantelis Antoniou
  2014-07-07 13:10   ` Grant Likely
  2014-07-04 16:58 ` [PATCH 2/5] OF: Export a number of __of_* methods Pantelis Antoniou
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-04 16:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou, Pantelis Antoniou

Introduce an option whether to free or not the memory of a node
and it's properties when of_node_release is called.

The option is located in the chosen node, and its name is
of-node-keep. When this boolean property is missing the default
behaviour of freeing is preserved.

The rational behind this option is as follows:

The life-cycle of nodes and properties does not allow us to release
the memory taken by a device_node. Pointer to properties and nodes
might still be in use in drivers, so any memory free'ing is dangerous.

Simply move all the properties to the deadprops list, and the node
itself to of_alldeadnodes until the life-cycles issues are resolved.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c | 74 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 15 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index a5d6f4f..0e8457c 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -180,17 +180,29 @@ int of_detach_node(struct device_node *np)
 	return rc;
 }
 
+static struct device_node *of_alldeadnodes;
+static DEFINE_RAW_SPINLOCK(deadtree_lock);
+
 /**
  *	of_node_release - release a dynamically allocated node
  *	@kref:  kref element of the node to be released
  *
  *	In of_node_put() this function is passed to kref_put()
  *	as the destructor.
+ *
+ *	There is an option to not free the node, since due to the
+ *	way that node and property life-cycles are not completely
+ *	managed, we can't free the memory of a node at will.
+ *	Instead we move the node to the dead nodes list where it will
+ *	remain until the life-cycle issues are resolved.
  */
-static void of_node_release(struct kobject *kobj)
+void of_node_release(struct kobject *kobj)
 {
+	/*  we probe the of-node-keep chosen prop once */
+	static int node_keep = -1;
 	struct device_node *node = kobj_to_device_node(kobj);
-	struct property *prop = node->properties;
+	struct property *prop;
+	unsigned long flags;
 
 	/* We should never be releasing nodes that haven't been detached. */
 	if (!of_node_check_flag(node, OF_DETACHED)) {
@@ -199,23 +211,55 @@ static void of_node_release(struct kobject *kobj)
 		return;
 	}
 
-	if (!of_node_check_flag(node, OF_DYNAMIC))
+	/* we should not be trying to release the root */
+	if (WARN_ON(node == of_allnodes))
 		return;
 
-	while (prop) {
-		struct property *next = prop->next;
+	/* read the chosen boolean property "of-node-keep" once */
+	if (node_keep == -1)
+		node_keep = of_property_read_bool(of_chosen, "of-node-keep");
 
-		kfree(prop->name);
-		kfree(prop->value);
-		kfree(prop);
-		prop = next;
+	/* default is to free everything */
+	if (!node_keep) {
 
-		if (!prop) {
-			prop = node->deadprops;
-			node->deadprops = NULL;
+		/* free normal properties */
+		while ((prop = node->properties) != NULL) {
+			node->properties = prop->next;
+			kfree(prop->name);
+			kfree(prop->value);
+			kfree(prop);
 		}
+
+		/* free deap properties */
+		while ((prop = node->deadprops) != NULL) {
+			node->deadprops = prop->next;
+			kfree(prop->name);
+			kfree(prop->value);
+			kfree(prop);
+		}
+
+		/* free the node */
+		kfree(node->full_name);
+		kfree(node->data);
+		kfree(node);
+		return;
 	}
-	kfree(node->full_name);
-	kfree(node->data);
-	kfree(node);
+
+	pr_info("%s: dead node \"%s\"\n", __func__, node->full_name);
+
+	/* can't use devtree lock; at of_node_put caller might be holding it */
+	raw_spin_lock_irqsave(&deadtree_lock, flags);
+
+	/* move all properties to dead properties */
+	while ((prop = node->properties) != NULL) {
+		node->properties = prop->next;
+		prop->next = node->deadprops;
+		node->deadprops = prop;
+	}
+
+	/* move node to alldeadnodes */
+	node->allnext = of_alldeadnodes;
+	of_alldeadnodes = node;
+
+	raw_spin_unlock_irqrestore(&deadtree_lock, flags);
 }
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH 2/5] OF: Export a number of __of_* methods
  2014-07-04 16:58 [PATCH 0/5] Transaction DT support (v2) Pantelis Antoniou
  2014-07-04 16:58 ` [PATCH 1/5] of: Do not free memory at of_node_release option Pantelis Antoniou
@ 2014-07-04 16:58 ` Pantelis Antoniou
  2014-07-07 13:11   ` Grant Likely
  2014-07-04 16:58 ` [PATCH 3/5] OF: Utility helper functions for dynamic nodes Pantelis Antoniou
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-04 16:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou, Pantelis Antoniou

As part of moving to transactional DT, a number of methods are
reworked and exported.

The methods exported are:
__of_add_property, __of_remove_property, __of_update_property, __of_attach_node,
__of_detach_node

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/base.c       | 97 +++++++++++++++++++++++++++++--------------------
 drivers/of/dynamic.c    | 71 +++++++++++++++++++-----------------
 drivers/of/of_private.h |  8 ++++
 3 files changed, 102 insertions(+), 74 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index be0ec37..0d50318 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1659,7 +1659,7 @@ EXPORT_SYMBOL(of_count_phandle_with_args);
 /**
  * __of_add_property - Add a property to a node without lock operations
  */
-static int __of_add_property(struct device_node *np, struct property *prop)
+int __of_add_property(struct device_node *np, struct property *prop)
 {
 	struct property **next;
 
@@ -1701,6 +1701,25 @@ int of_add_property(struct device_node *np, struct property *prop)
 	return rc;
 }
 
+int __of_remove_property(struct device_node *np, struct property *prop)
+{
+	struct property **next;
+
+	for (next = &np->properties; *next; next = &(*next)->next) {
+		if (*next == prop)
+			break;
+	}
+	if (*next == NULL)
+		return -ENODEV;
+
+	/* found the node */
+	*next = prop->next;
+	prop->next = np->deadprops;
+	np->deadprops = prop;
+
+	return 0;
+}
+
 /**
  * of_remove_property - Remove a property from a node.
  *
@@ -1711,9 +1730,7 @@ int of_add_property(struct device_node *np, struct property *prop)
  */
 int of_remove_property(struct device_node *np, struct property *prop)
 {
-	struct property **next;
 	unsigned long flags;
-	int found = 0;
 	int rc;
 
 	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
@@ -1721,22 +1738,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
 		return rc;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	next = &np->properties;
-	while (*next) {
-		if (*next == prop) {
-			/* found the node */
-			*next = prop->next;
-			prop->next = np->deadprops;
-			np->deadprops = prop;
-			found = 1;
-			break;
-		}
-		next = &(*next)->next;
-	}
+	rc = __of_remove_property(np, prop);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	if (!found)
-		return -ENODEV;
+	if (rc)
+		return rc;
 
 	/* at early boot, bail hear and defer setup to of_init() */
 	if (!of_kset)
@@ -1747,6 +1753,32 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	return 0;
 }
 
+int __of_update_property(struct device_node *np, struct property *newprop,
+		struct property **oldpropp)
+{
+	struct property **next, *oldprop;
+
+	for (next = &np->properties; *next; next = &(*next)->next) {
+		if (of_prop_cmp((*next)->name, newprop->name) == 0)
+			break;
+	}
+	*oldpropp = oldprop = *next;
+
+	if (oldprop) {
+		/* replace the node */
+		newprop->next = oldprop->next;
+		*next = newprop;
+		oldprop->next = np->deadprops;
+		np->deadprops = oldprop;
+	} else {
+		/* new node */
+		newprop->next = NULL;
+		*next = newprop;
+	}
+
+	return 0;
+}
+
 /*
  * of_update_property - Update a property in a node, if the property does
  * not exist, add it.
@@ -1756,36 +1788,21 @@ int of_remove_property(struct device_node *np, struct property *prop)
  * Instead we just move the property to the "dead properties" list,
  * and add the new property to the property list
  */
-int of_update_property(struct device_node *np, struct property *newprop)
+int of_update_property(struct device_node *np, struct property *prop)
 {
-	struct property **next, *oldprop;
+	struct property *oldprop;
 	unsigned long flags;
 	int rc;
 
-	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
+	if (!prop->name)
+		return -EINVAL;
+
+	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 	if (rc)
 		return rc;
 
-	if (!newprop->name)
-		return -EINVAL;
-
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	next = &np->properties;
-	oldprop = __of_find_property(np, newprop->name, NULL);
-	if (!oldprop) {
-		/* add the new node */
-		rc = __of_add_property(np, newprop);
-	} else while (*next) {
-		/* replace the node */
-		if (*next == oldprop) {
-			newprop->next = oldprop->next;
-			*next = newprop;
-			oldprop->next = np->deadprops;
-			np->deadprops = oldprop;
-			break;
-		}
-		next = &(*next)->next;
-	}
+	rc = __of_update_property(np, prop, &oldprop);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	if (rc)
 		return rc;
@@ -1797,7 +1814,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	/* Update the sysfs attribute */
 	if (oldprop)
 		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
-	__of_add_property_sysfs(np, newprop);
+	__of_add_property_sysfs(np, prop);
 
 	return 0;
 }
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 0e8457c..eb1126f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -95,8 +95,17 @@ int of_property_notify(int action, struct device_node *np,
 	return of_reconfig_notify(action, &pr);
 }
 
+void __of_attach_node(struct device_node *np)
+{
+	np->sibling = np->parent->child;
+	np->allnext = np->parent->allnext;
+	np->parent->allnext = np;
+	np->parent->child = np;
+	of_node_clear_flag(np, OF_DETACHED);
+}
+
 /**
- * of_attach_node() - Plug a device node into the tree and global list.
+ * of_attach_node - Plug a device node into the tree and global list.
  */
 int of_attach_node(struct device_node *np)
 {
@@ -108,52 +117,29 @@ int of_attach_node(struct device_node *np)
 		return rc;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	np->sibling = np->parent->child;
-	np->allnext = of_allnodes;
-	np->parent->child = np;
-	of_allnodes = np;
-	of_node_clear_flag(np, OF_DETACHED);
+	__of_attach_node(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	of_node_add(np);
 	return 0;
 }
 
-/**
- * of_detach_node() - "Unplug" a node from the device tree.
- *
- * The caller must hold a reference to the node.  The memory associated with
- * the node is not freed until its refcount goes to zero.
- */
-int of_detach_node(struct device_node *np)
+void __of_detach_node(struct device_node *np)
 {
 	struct device_node *parent;
-	unsigned long flags;
-	int rc = 0;
-
-	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
-	if (rc)
-		return rc;
+	struct device_node *prev;
+	struct device_node *prevsib;
 
-	raw_spin_lock_irqsave(&devtree_lock, flags);
-
-	if (of_node_check_flag(np, OF_DETACHED)) {
-		/* someone already detached it */
-		raw_spin_unlock_irqrestore(&devtree_lock, flags);
-		return rc;
-	}
+	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
+		return;
 
 	parent = np->parent;
-	if (!parent) {
-		raw_spin_unlock_irqrestore(&devtree_lock, flags);
-		return rc;
-	}
+	if (WARN_ON(!parent))
+		return;
 
 	if (of_allnodes == np)
 		of_allnodes = np->allnext;
 	else {
-		struct device_node *prev;
-
 		for (prev = of_allnodes;
 		     prev->allnext != np;
 		     prev = prev->allnext)
@@ -164,8 +150,6 @@ int of_detach_node(struct device_node *np)
 	if (parent->child == np)
 		parent->child = np->sibling;
 	else {
-		struct device_node *prevsib;
-
 		for (prevsib = np->parent->child;
 		     prevsib->sibling != np;
 		     prevsib = prevsib->sibling)
@@ -174,6 +158,25 @@ int of_detach_node(struct device_node *np)
 	}
 
 	of_node_set_flag(np, OF_DETACHED);
+}
+
+/**
+ * of_detach_node - "Unplug" a node from the device tree.
+ *
+ * The caller must hold a reference to the node.  The memory associated with
+ * the node is not freed until its refcount goes to zero.
+ */
+int of_detach_node(struct device_node *np)
+{
+	unsigned long flags;
+	int rc = 0;
+
+	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
+	if (rc)
+		return rc;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	__of_detach_node(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	of_node_remove(np);
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index a541b81..b2c25b5 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -59,4 +59,12 @@ static inline int of_reconfig_notify(unsigned long action, void *p)
 }
 #endif /* CONFIG_OF_DYNAMIC */
 
+extern int __of_add_property(struct device_node *np, struct property *prop);
+extern int __of_remove_property(struct device_node *np, struct property *prop);
+extern int __of_update_property(struct device_node *np,
+		struct property *newprop, struct property **oldprop);
+
+extern void __of_attach_node(struct device_node *np);
+extern void __of_detach_node(struct device_node *np);
+
 #endif /* _LINUX_OF_PRIVATE_H */
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH 3/5] OF: Utility helper functions for dynamic nodes
  2014-07-04 16:58 [PATCH 0/5] Transaction DT support (v2) Pantelis Antoniou
  2014-07-04 16:58 ` [PATCH 1/5] of: Do not free memory at of_node_release option Pantelis Antoniou
  2014-07-04 16:58 ` [PATCH 2/5] OF: Export a number of __of_* methods Pantelis Antoniou
@ 2014-07-04 16:58 ` Pantelis Antoniou
  2014-07-07  7:04   ` Alexander Sverdlin
  2014-07-07 13:13   ` Grant Likely
  2014-07-04 16:58 ` [PATCH 4/5] of: Introduce tree change __foo_post methods Pantelis Antoniou
  2014-07-04 16:58 ` [PATCH 5/5] of: Transactional DT support Pantelis Antoniou
  4 siblings, 2 replies; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-04 16:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou, Pantelis Antoniou

Introduce helper functions for working with the live DT tree,
all of them related to dynamically adding/removing nodes and
properties.

__of_copy_property() copies a property dynamically
__of_create_empty_node() creates an empty node

Bug fix about prop->len == 0 by Ionut Nicu <ioan.nicu.ext@nsn.com>

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c    | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/of_private.h |  14 ++++++
 include/linux/of.h      |   9 ++++
 3 files changed, 147 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index eb1126f..90c09b6 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -266,3 +266,127 @@ void of_node_release(struct kobject *kobj)
 
 	raw_spin_unlock_irqrestore(&deadtree_lock, flags);
 }
+
+/**
+ * __of_copy_property - Copy a property dynamically.
+ * @prop:	Property to copy
+ * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
+ * @propflags:	Property flags
+ *
+ * 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 allocflags, unsigned long propflags)
+{
+	struct property *propn;
+
+	propn = kzalloc(sizeof(*prop), allocflags);
+	if (propn == NULL)
+		return NULL;
+
+	propn->_flags = propflags;
+
+	if (of_property_check_flag(propn, OF_ALLOCNAME)) {
+		propn->name = kstrdup(prop->name, allocflags);
+		if (propn->name == NULL)
+			goto err_fail_name;
+	} else
+		propn->name = prop->name;
+
+	/*
+	 * NOTE: There is no check for zero length value.
+	 * In case of a boolean property This will allocate a value
+	 * of zero bytes. We do this to work around the use
+	 * of of_get_property() calls on boolean values.
+	 */
+	if (of_property_check_flag(propn, OF_ALLOCVALUE)) {
+		propn->value = kmalloc(prop->length, allocflags);
+		if (propn->value == NULL)
+			goto err_fail_value;
+		memcpy(propn->value, prop->value, prop->length);
+	} else
+		propn->value = prop->value;
+
+	propn->length = prop->length;
+
+	/* mark the property as dynamic */
+	of_property_set_flag(propn, OF_DYNAMIC);
+
+	return propn;
+
+err_fail_value:
+	if (of_property_check_flag(propn, OF_ALLOCNAME))
+		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
+ * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
+ * @nodeflags:	Node flags
+ *
+ * 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 allocflags, unsigned long nodeflags)
+{
+	struct device_node *node;
+
+	node = kzalloc(sizeof(*node), allocflags);
+	if (node == NULL)
+		return NULL;
+
+	node->_flags = nodeflags;
+
+	if (of_node_check_flag(node, OF_ALLOCNAME)) {
+		node->name = kstrdup(name, allocflags);
+		if (node->name == NULL)
+			goto err_free_node;
+	} else
+		node->name = name;
+
+	if (of_node_check_flag(node, OF_ALLOCTYPE)) {
+		node->type = kstrdup(type, allocflags);
+		if (node->type == NULL)
+			goto err_free_name;
+	} else
+		node->type = type;
+
+	if (of_node_check_flag(node, OF_ALLOCFULL)) {
+		node->full_name = kstrdup(full_name, allocflags);
+		if (node->full_name == NULL)
+			goto err_free_type;
+	} else
+		node->full_name = full_name;
+
+	node->phandle = phandle;
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+
+	of_node_init(node);
+
+	return node;
+err_free_type:
+	if (of_node_check_flag(node, OF_ALLOCTYPE))
+		kfree(node->type);
+err_free_name:
+	if (of_node_check_flag(node, OF_ALLOCNAME))
+		kfree(node->name);
+err_free_node:
+	kfree(node);
+	return NULL;
+}
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index b2c25b5..e544247 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -67,4 +67,18 @@ extern int __of_update_property(struct device_node *np,
 extern void __of_attach_node(struct device_node *np);
 extern void __of_detach_node(struct device_node *np);
 
+/**
+ * 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.
+ */
+
+struct property *__of_copy_property(const struct property *prop,
+		gfp_t allocflags, unsigned long propflags);
+struct device_node *__of_create_empty_node(const char *name,
+		const char *type, const char *full_name,
+		phandle phandle, gfp_t allocflags, unsigned long nodeflags);
+
 #endif /* _LINUX_OF_PRIVATE_H */
diff --git a/include/linux/of.h b/include/linux/of.h
index 8e4fb82..b7c322c 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -206,6 +206,15 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 #define OF_POPULATED	3 /* device already created for the node */
 #define OF_POPULATED_BUS	4 /* of_platform_populate recursed
 				   * to children of this node */
+#define OF_ALLOCNAME	5 /* name was kmalloc-ed */
+#define OF_ALLOCTYPE	6 /* type was kmalloc-ed */
+#define OF_ALLOCFULL	7 /* full_name was kmalloc-ed */
+#define OF_ALLOCVALUE	8 /* value was kmalloc-ed */
+
+#define OF_NODE_ALLOCALL \
+	((1 << OF_ALLOCNAME) | (1 << OF_ALLOCTYPE) | (1 << OF_ALLOCFULL))
+#define OF_PROP_ALLOCALL \
+	((1 << OF_ALLOCNAME) | (1 << OF_ALLOCVALUE))
 
 #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
 #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH 4/5] of: Introduce tree change __foo_post methods.
  2014-07-04 16:58 [PATCH 0/5] Transaction DT support (v2) Pantelis Antoniou
                   ` (2 preceding siblings ...)
  2014-07-04 16:58 ` [PATCH 3/5] OF: Utility helper functions for dynamic nodes Pantelis Antoniou
@ 2014-07-04 16:58 ` Pantelis Antoniou
  2014-07-10  0:55   ` Grant Likely
  2014-07-04 16:58 ` [PATCH 5/5] of: Transactional DT support Pantelis Antoniou
  4 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-04 16:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou, Pantelis Antoniou

As part of transactional DT support there needs to be a clearer
definition of how each dynamic tree change method works.

Let's take for example the of_add_property method. There are
discrete steps taken in sequence.

1. The OF_RECONFIG_ADD_PROPERTY notifier will be fired.
2. The devtree lock will be taken.
3. Insertion of the property in the live tree.
4. The devtree lock will be released.
5. The property will be inserted in the sysfs tree.

For each of the of_attach_node, of_detach_node, of_add_property,
of_remove_property and of_update_property methods we export (and
introduce if not available), their counterparts of
__of_attach_node, __of_attach_node_post, __of_detach_node,
__of_detach_node_post, __of_add_property, __of_add_property_post,
__of_remove_property, __of_remove_property_post, __of_update_property
and __of_update_property post.

So each of the tree modification methods is following the same
pattern, i.e. for method foo we have:

Firing of the notifer, lock, __foo, unlock, __foo_post

This breakdown is required when we introduce transaction.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/base.c       | 60 +++++++++++++++++++++++++++++++++++--------------
 drivers/of/dynamic.c    | 14 ++++++++++--
 drivers/of/of_private.h |  8 +++++++
 3 files changed, 63 insertions(+), 19 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0d50318..b52fd6b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -127,13 +127,24 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
 	return name;
 }
 
-static int __of_add_property_sysfs(struct device_node *np, struct property *pp)
+static int __of_add_property_sysfs(struct device_node *np, struct property *pp,
+		int ignore_dup_name)
 {
 	int rc;
 
 	/* Important: Don't leak passwords */
 	bool secure = strncmp(pp->name, "security-", 9) == 0;
 
+	/* ignore duplicate name (transaction case) */
+	if (ignore_dup_name) {
+		struct kernfs_node *kn = sysfs_get_dirent(np->kobj.sd,
+				pp->name);
+		if (kn) {
+			sysfs_put(kn);
+			return 0;
+		}
+	}
+
 	sysfs_bin_attr_init(&pp->attr);
 	pp->attr.attr.name = safe_name(&np->kobj, pp->name);
 	pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
@@ -167,7 +178,7 @@ static int __of_node_add(struct device_node *np)
 		return rc;
 
 	for_each_property_of_node(np, pp)
-		__of_add_property_sysfs(np, pp);
+		__of_add_property_sysfs(np, pp, 1);
 
 	return 0;
 }
@@ -1677,6 +1688,13 @@ int __of_add_property(struct device_node *np, struct property *prop)
 	return 0;
 }
 
+void __of_add_property_post(struct device_node *np, struct property *prop,
+		int ignore_dup_name)
+{
+	if (of_node_is_attached(np))
+		__of_add_property_sysfs(np, prop, ignore_dup_name);
+}
+
 /**
  * of_add_property - Add a property to a node
  */
@@ -1695,8 +1713,7 @@ int of_add_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
-	if (of_node_is_attached(np))
-		__of_add_property_sysfs(np, prop);
+	__of_add_property_post(np, prop, 0);
 
 	return rc;
 }
@@ -1720,6 +1737,13 @@ int __of_remove_property(struct device_node *np, struct property *prop)
 	return 0;
 }
 
+void __of_remove_property_post(struct device_node *np, struct property *prop)
+{
+	/* at early boot, bail here and defer setup to of_init() */
+	if (of_kset)
+		sysfs_remove_bin_file(&np->kobj, &prop->attr);
+}
+
 /**
  * of_remove_property - Remove a property from a node.
  *
@@ -1744,11 +1768,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
-	/* at early boot, bail hear and defer setup to of_init() */
-	if (!of_kset)
-		return 0;
-
-	sysfs_remove_bin_file(&np->kobj, &prop->attr);
+	__of_remove_property_post(np, prop);
 
 	return 0;
 }
@@ -1779,6 +1799,19 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 	return 0;
 }
 
+void __of_update_property_post(struct device_node *np, struct property *newprop,
+		struct property *oldprop)
+{
+	/* At early boot, bail out and defer setup to of_init() */
+	if (!of_kset)
+		return;
+
+	/* Update the sysfs attribute */
+	if (oldprop)
+		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
+	__of_add_property_sysfs(np, newprop, 0);
+}
+
 /*
  * of_update_property - Update a property in a node, if the property does
  * not exist, add it.
@@ -1807,14 +1840,7 @@ int of_update_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
-	/* At early boot, bail out and defer setup to of_init() */
-	if (!of_kset)
-		return 0;
-
-	/* Update the sysfs attribute */
-	if (oldprop)
-		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
-	__of_add_property_sysfs(np, prop);
+	__of_update_property_post(np, prop, oldprop);
 
 	return 0;
 }
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 90c09b6..a995de3 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -104,6 +104,11 @@ void __of_attach_node(struct device_node *np)
 	of_node_clear_flag(np, OF_DETACHED);
 }
 
+void __of_attach_node_post(struct device_node *np)
+{
+	of_node_add(np);
+}
+
 /**
  * of_attach_node - Plug a device node into the tree and global list.
  */
@@ -120,7 +125,7 @@ int of_attach_node(struct device_node *np)
 	__of_attach_node(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	of_node_add(np);
+	__of_attach_node_post(np);
 	return 0;
 }
 
@@ -160,6 +165,11 @@ void __of_detach_node(struct device_node *np)
 	of_node_set_flag(np, OF_DETACHED);
 }
 
+void __of_detach_node_post(struct device_node *np)
+{
+	of_node_remove(np);
+}
+
 /**
  * of_detach_node - "Unplug" a node from the device tree.
  *
@@ -179,7 +189,7 @@ int of_detach_node(struct device_node *np)
 	__of_detach_node(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	of_node_remove(np);
+	__of_detach_node_post(np);
 	return rc;
 }
 
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index e544247..ca95100 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -60,12 +60,20 @@ static inline int of_reconfig_notify(unsigned long action, void *p)
 #endif /* CONFIG_OF_DYNAMIC */
 
 extern int __of_add_property(struct device_node *np, struct property *prop);
+extern void __of_add_property_post(struct device_node *np,
+		struct property *prop, int ignore_dup_name);
 extern int __of_remove_property(struct device_node *np, struct property *prop);
+extern void __of_remove_property_post(struct device_node *np,
+		struct property *prop);
 extern int __of_update_property(struct device_node *np,
 		struct property *newprop, struct property **oldprop);
+extern void __of_update_property_post(struct device_node *np,
+		struct property *newprop, struct property *oldprop);
 
 extern void __of_attach_node(struct device_node *np);
+extern void __of_attach_node_post(struct device_node *np);
 extern void __of_detach_node(struct device_node *np);
+extern void __of_detach_node_post(struct device_node *np);
 
 /**
  * General utilities for working with live trees.
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH 5/5] of: Transactional DT support.
  2014-07-04 16:58 [PATCH 0/5] Transaction DT support (v2) Pantelis Antoniou
                   ` (3 preceding siblings ...)
  2014-07-04 16:58 ` [PATCH 4/5] of: Introduce tree change __foo_post methods Pantelis Antoniou
@ 2014-07-04 16:58 ` Pantelis Antoniou
  2014-07-11  5:24   ` Grant Likely
  4 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-04 16:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou, Pantelis Antoniou

Introducing DT transactional support.

A DT transaction is a method which allows one to apply changes
in the live tree, in such a way that either the full set of changes
take effect, or the state of the tree can be rolled-back to the
state it was before it was attempted. An applied transaction
can be rolled-back at any time.

If any transaction results in the change of a state of a device
node, a notifier for that node will be fired which allows a bus
to create/destroy devices as directed.

Documentation is in
	Documentation/devicetree/transactions.txt

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 Documentation/devicetree/transactions.txt |  41 ++
 drivers/of/Makefile                       |   2 +-
 drivers/of/base.c                         |  34 +-
 drivers/of/of_private.h                   |  19 +
 drivers/of/transaction.c                  | 699 ++++++++++++++++++++++++++++++
 include/linux/of.h                        |  82 ++++
 6 files changed, 864 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/transactions.txt
 create mode 100644 drivers/of/transaction.c

diff --git a/Documentation/devicetree/transactions.txt b/Documentation/devicetree/transactions.txt
new file mode 100644
index 0000000..59ff5b7
--- /dev/null
+++ b/Documentation/devicetree/transactions.txt
@@ -0,0 +1,41 @@
+A DT transaction is a method which allows one to apply changes
+in the live tree, in such a way that either the full set of changes
+take effect, or the state of the tree can be rolled-back to the
+state it was before it was attempted. An applied transaction
+can be rolled-back at any time.
+
+If any transaction results in the change of a state of a device
+node, a notifier for that node will be fired which allows a bus
+to create/destroy devices as directed.
+
+The sequence of a transaction is as follows.
+
+1. of_transaction_init() - initializes a transaction
+
+2. of_transaction_start() - starts a transaction; a global
+transaction lock is taken at this point and disallowes any other
+dynamic tree changes.
+
+3. A number of DT tree change calls, of_transaction_attach_node(),
+of_transaction_detach_node(), of_transaction_add_property(),
+of_transaction_remove_property, of_transaction_update_property()
+modify the live tree (but without any user-space visible changes).
+All changes are recorded in the list of of_transaction_entry of
+the transaction.
+
+4.a. No errors occured during application, so a call to
+of_transaction_commit() is issued. All the changes are made visible
+to user-space and all device creation/destruction notifiers are
+fired. The global transaction is released.
+
+4.b. An error occured, so a of_transaction_abort() call is issued.
+All changes to the live tree will be reverted. The global transaction
+lock will be released.
+
+5.a If the transaction needs not be reverted, a call to
+of_transaction_destroy will release the resources of the transaction.
+
+5.b If the transaction is to be reverted, a call to
+of_transaction_revert will do so. All device notifiers will be
+fired, and the state of the tree will be the same as before
+the call to of_start_transaction().
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 08e6c0f..9a68cc9 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,5 +1,5 @@
 obj-y = base.o device.o platform.o
-obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
+obj-$(CONFIG_OF_DYNAMIC) += dynamic.o transaction.o
 obj-$(CONFIG_OF_FLATTREE) += fdt.o
 obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
diff --git a/drivers/of/base.c b/drivers/of/base.c
index b52fd6b..206af65 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -40,7 +40,9 @@ static struct device_node *of_stdout;
 static struct kset *of_kset;
 
 /*
- * Used to protect the of_aliases, to hold off addition of nodes to sysfs
+ * Used to protect the of_aliases, to hold off addition of
+ * nodes to sysfs and in the case of a transaction is in
+ * progress.
  */
 DEFINE_MUTEX(of_mutex);
 
@@ -1707,13 +1709,16 @@ int of_add_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
+	mutex_lock(&of_mutex);
+
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_add_property(np, prop);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-	if (rc)
-		return rc;
 
-	__of_add_property_post(np, prop, 0);
+	if (rc == 0)
+		__of_add_property_post(np, prop, 0);
+
+	mutex_unlock(&of_mutex);
 
 	return rc;
 }
@@ -1761,16 +1766,18 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
+	mutex_lock(&of_mutex);
+
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_remove_property(np, prop);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	if (rc)
-		return rc;
+	if (rc == 0)
+		__of_remove_property_post(np, prop);
 
-	__of_remove_property_post(np, prop);
+	mutex_unlock(&of_mutex);
 
-	return 0;
+	return rc;
 }
 
 int __of_update_property(struct device_node *np, struct property *newprop,
@@ -1834,15 +1841,18 @@ int of_update_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
+	mutex_lock(&of_mutex);
+
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_update_property(np, prop, &oldprop);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-	if (rc)
-		return rc;
 
-	__of_update_property_post(np, prop, oldprop);
+	if (rc == 0)
+		__of_update_property_post(np, prop, oldprop);
 
-	return 0;
+	mutex_unlock(&of_mutex);
+
+	return rc;
 }
 
 static void of_alias_add(struct alias_prop *ap, struct device_node *np,
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index ca95100..00c119a 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -89,4 +89,23 @@ struct device_node *__of_create_empty_node(const char *name,
 		const char *type, const char *full_name,
 		phandle phandle, gfp_t allocflags, unsigned long nodeflags);
 
+/* iterators for transactions, used for overlays */
+/* forward iterator */
+#define for_each_transaction_entry(_oft, _te) \
+	list_for_each_entry(_te, &(_oft)->te_list, node)
+
+/* reverse iterator */
+#define for_each_transaction_entry_reverse(_oft, _te) \
+	list_for_each_entry_reverse(_te, &(_oft)->te_list, node)
+
+/* special find property method for use by transaction users */
+extern struct property *of_transaction_find_property(
+		struct of_transaction *oft,
+		const struct device_node *np, const char *name, int *lenp);
+extern int of_transaction_device_is_available(struct of_transaction *oft,
+		const struct device_node *np);
+extern struct device_node *of_transaction_get_child_by_name(
+		struct of_transaction *oft, struct device_node *node,
+		const char *name);
+
 #endif /* _LINUX_OF_PRIVATE_H */
diff --git a/drivers/of/transaction.c b/drivers/of/transaction.c
new file mode 100644
index 0000000..1094395
--- /dev/null
+++ b/drivers/of/transaction.c
@@ -0,0 +1,699 @@
+/*
+ * Functions for DT transactions
+ *
+ * Copyright (C) 2014 Pantelis Antoniou <pantelis.antoniou@konsulko.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+#undef DEBUG
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+#include "of_private.h"
+
+static struct of_transaction_entry *__of_transaction_entry_create(
+		struct of_transaction *oft, unsigned long action,
+		struct device_node *dn, struct property *prop)
+{
+	struct of_transaction_entry *te;
+
+	te = kzalloc(sizeof(*te), GFP_KERNEL);
+	if (te == NULL) {
+		pr_err("%s: Failed to allocate\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+	/* get a reference to the node */
+	te->action = action;
+	te->np = of_node_get(dn);
+	te->prop = prop;
+
+	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
+		te->old_prop = of_transaction_find_property(oft, dn,
+				prop->name, NULL);
+
+	return te;
+}
+
+static void __of_transaction_entry_destroy(struct of_transaction_entry *te)
+{
+	of_node_put(te->np);
+	list_del(&te->node);
+	kfree(te);
+}
+
+#ifdef DEBUG
+static void __of_transaction_entry_dump(struct of_transaction_entry *te)
+{
+	switch (te->action) {
+	case OF_RECONFIG_ADD_PROPERTY:
+		pr_debug("%p: %s %s/%s\n",
+			te, "ADD_PROPERTY   ", te->np->full_name,
+			te->prop->name);
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		pr_debug("%p: %s %s/%s\n",
+			te, "REMOVE_PROPERTY", te->np->full_name,
+			te->prop->name);
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		pr_debug("%p: %s %s/%s\n",
+			te, "UPDATE_PROPERTY", te->np->full_name,
+			te->prop->name);
+		break;
+	case OF_RECONFIG_ATTACH_NODE:
+		pr_debug("%p: %s %s\n",
+			te, "ATTACH_NODE    ", te->np->full_name);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		pr_debug("%p: %s %s\n",
+			te, "DETACH_NODE    ", te->np->full_name);
+		break;
+	}
+}
+#else
+static inline void __of_transaction_entry_dump(struct of_transaction_entry *te)
+{
+	/* empty */
+}
+#endif
+
+static int __of_transaction_entry_device_state(struct of_transaction *oft,
+		struct of_transaction_entry *te, int revert)
+{
+	int curr, target;
+	unsigned long action;
+	struct property *prop;
+
+	/* filter */
+	if (te->action != OF_RECONFIG_ADD_PROPERTY &&
+	    te->action != OF_RECONFIG_REMOVE_PROPERTY &&
+	    te->action != OF_RECONFIG_UPDATE_PROPERTY &&
+	    te->action != OF_RECONFIG_ATTACH_NODE &&
+	    te->action != OF_RECONFIG_ATTACH_NODE)
+		return -1;
+
+	action = te->action;
+	prop = te->prop;
+
+	/* on revert convert to the opposite */
+	if (revert) {
+		if (action == OF_RECONFIG_ADD_PROPERTY)
+			action = OF_RECONFIG_REMOVE_PROPERTY;
+		else if (action == OF_RECONFIG_REMOVE_PROPERTY)
+			action = OF_RECONFIG_ADD_PROPERTY;
+		else if (action == OF_RECONFIG_UPDATE_PROPERTY)
+			prop = te->old_prop;
+		else if (action == OF_RECONFIG_ATTACH_NODE)
+			action = OF_RECONFIG_DETACH_NODE;
+		else
+			action = OF_RECONFIG_ATTACH_NODE;
+	}
+
+	/* we only support state changes on "status" property change */
+	if ((te->action == OF_RECONFIG_ADD_PROPERTY ||
+	    te->action == OF_RECONFIG_REMOVE_PROPERTY ||
+	    te->action == OF_RECONFIG_UPDATE_PROPERTY) &&
+		of_prop_cmp(prop->name, "status"))
+		return -1;
+
+	/* note that we don't use of_transaction_find_property() */
+
+	/* current device state */
+	curr = of_device_is_available(te->np) &&
+		of_find_property(te->np, "compatible", NULL) &&
+		of_find_property(te->np, "status", NULL);
+
+	switch (action) {
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+	case OF_RECONFIG_UPDATE_PROPERTY:
+
+		/* target device state */
+		if (action == OF_RECONFIG_ADD_PROPERTY ||
+		    action == OF_RECONFIG_UPDATE_PROPERTY)
+			target = !strcmp(prop->value, "okay") ||
+				!strcmp(prop->value, "ok");
+		else
+			target = 0;	/* NOTE: status removal -> disabled */
+
+		break;
+	case OF_RECONFIG_ATTACH_NODE:
+		target = curr;
+		curr = 0;
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		target = 0;
+		break;
+	}
+
+	return curr != target ? target : -1;
+}
+
+static int __of_transaction_entry_apply(struct of_transaction *oft,
+		struct of_transaction_entry *te)
+{
+	struct property *old_prop;
+	unsigned long flags;
+	int ret, state;
+
+	ret = 0;
+
+	state = __of_transaction_entry_device_state(oft, te, 0);
+
+	switch (te->action) {
+	case OF_RECONFIG_ATTACH_NODE:
+	case OF_RECONFIG_DETACH_NODE:
+		ret = of_reconfig_notify(te->action, te->np);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		ret = of_property_notify(te->action, te->np, te->prop);
+		break;
+	}
+
+	if (ret) {
+		pr_err("%s: notifier error @%s\n", __func__,
+				te->np->full_name);
+		return ret;
+	}
+
+	mutex_lock(&of_mutex);
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	switch (te->action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		__of_attach_node(te->np);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		__of_detach_node(te->np);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+		ret = __of_add_property(te->np, te->prop);
+		if (ret) {
+			pr_err("%s: add_property failed @%s/%s\n",
+				__func__, te->np->full_name,
+				te->prop->name);
+			break;
+		}
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		ret = __of_remove_property(te->np, te->prop);
+		if (ret) {
+			pr_err("%s: remove_property failed @%s/%s\n",
+				__func__, te->np->full_name,
+				te->prop->name);
+			break;
+		}
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		ret = __of_update_property(te->np, te->prop, &old_prop);
+		if (ret) {
+			pr_err("%s: update_property failed @%s/%s\n",
+				__func__, te->np->full_name,
+				te->prop->name);
+			break;
+		}
+		break;
+	}
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	mutex_unlock(&of_mutex);
+
+	if (ret) {
+		/* we can't rollback the notifier */
+		return ret;
+	}
+
+	switch (te->action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		__of_attach_node_post(te->np);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		__of_detach_node_post(te->np);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+		/* ignore duplicate names */
+		__of_add_property_post(te->np, te->prop, 1);
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		__of_remove_property_post(te->np, te->prop);
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		__of_update_property_post(te->np, te->prop, te->old_prop);
+		break;
+	}
+
+	if (state != -1) {
+		pr_debug("of_transaction: %s device for node '%s'\n",
+				state ? "create" : "remove",
+				te->np->full_name);
+
+		ret = of_reconfig_notify(state ?
+				OF_RECONFIG_DYNAMIC_CREATE_DEV :
+				OF_RECONFIG_DYNAMIC_DESTROY_DEV,
+				te->np);
+		if (ret != 0) {
+			pr_err("of_transaction: failed %s device @%s (%d)\n",
+					state ? "create" : "remove",
+					te->np->full_name, ret);
+			/* drop the error; devices probe fails; that's OK */
+			ret = 0;
+		}
+
+	}
+
+	return 0;
+}
+
+static int __of_transaction_entry_revert(struct of_transaction *oft,
+		struct of_transaction_entry *te)
+{
+	struct property *prop, *old_prop, **propp;
+	unsigned long action, flags;
+	struct device_node *np;
+	int ret, state;
+
+	state = __of_transaction_entry_device_state(oft, te, 1);
+
+	if (state != -1) {
+		pr_debug("of_transaction: %s device for node '%s'\n",
+				state  ? "create" : "remove",
+				te->np->full_name);
+
+		ret = of_reconfig_notify(state ?
+				OF_RECONFIG_DYNAMIC_CREATE_DEV :
+				OF_RECONFIG_DYNAMIC_DESTROY_DEV,
+				te->np);
+		if (ret != 0) {
+			pr_err("of_transaction: failed %s device @%s (%d)\n",
+					state ? "create" : "remove",
+					te->np->full_name, ret);
+			/* drop the error; devices probe fails; that's OK */
+			ret = 0;
+		}
+	}
+
+	/* get node and immediately put */
+	action = te->action;
+	np = te->np;
+	prop = te->prop;
+	old_prop = te->old_prop;
+
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		ret = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		ret = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+		ret = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY,
+				np, prop);
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		ret = of_property_notify(OF_RECONFIG_ADD_PROPERTY,
+				np, prop);
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		ret = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY,
+				np, old_prop);
+		break;
+	}
+
+	if (ret) {
+		pr_err("%s: notifier error @%s\n", __func__,
+				te->np->full_name);
+		goto out_revert;
+	}
+
+	mutex_lock(&of_mutex);
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		__of_detach_node(np);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		__of_attach_node(np);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+		ret = __of_remove_property(np, prop);
+		if (ret) {
+			pr_err("%s: remove_property failed @%s/%s\n",
+				__func__, np->full_name, prop->name);
+			break;
+		}
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		/* property is *always* on deadprops */
+		if (action == OF_RECONFIG_UPDATE_PROPERTY)
+			prop = old_prop;
+		propp = &np->deadprops;
+		while (*propp != NULL) {
+			if (*propp == prop)
+				break;
+			propp = &(*propp)->next;
+		}
+
+		/* we should find it in deadprops */
+		WARN_ON(*propp == NULL);
+
+		/* remove it from deadprops */
+		if (*propp != NULL)
+			*propp = prop->next;
+
+		if (action == OF_RECONFIG_REMOVE_PROPERTY) {
+			ret = __of_add_property(np, prop);
+			if (ret) {
+				pr_err("%s: add_property failed @%s/%s\n",
+					__func__, np->full_name,
+					prop->name);
+				break;
+			}
+		} else {
+			ret = __of_update_property(np, prop, &old_prop);
+			if (ret) {
+				pr_err("%s: update_property failed @%s/%s\n",
+					__func__, np->full_name,
+					prop->name);
+				break;
+			}
+		}
+		break;
+	}
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	if (ret)
+		goto out_unlock;
+
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		__of_detach_node_post(np);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		__of_attach_node_post(np);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+		__of_remove_property_post(np, prop);
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		__of_add_property_post(np, prop, 0);
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		__of_update_property_post(np, prop, old_prop);
+		break;
+	}
+
+out_unlock:
+	mutex_unlock(&of_mutex);
+
+out_revert:
+	/* revert creation of device */
+	if (ret && state != -1)
+		of_reconfig_notify(!state ?
+			OF_RECONFIG_DYNAMIC_CREATE_DEV :
+			OF_RECONFIG_DYNAMIC_DESTROY_DEV,
+			te->np);
+
+	return ret;
+}
+
+/**
+ * of_transaction_init - Initialize a transaction for use
+ *
+ * @oft:	transaction pointer
+ *
+ * Initialize a transaction structure
+ */
+void of_transaction_init(struct of_transaction *oft)
+{
+	memset(oft, 0, sizeof(*oft));
+	INIT_LIST_HEAD(&oft->te_list);
+}
+
+/**
+ * of_transaction_destroy - Destroy a transaction
+ *
+ * @oft:	transaction pointer
+ *
+ * Destroys a transaction. Note that if a transaction is applied,
+ * its changes to the tree cannot be reverted.
+ */
+void of_transaction_destroy(struct of_transaction *oft)
+{
+	struct of_transaction_entry *te, *ten;
+
+	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
+		__of_transaction_entry_destroy(te);
+}
+
+/**
+ * of_transaction_start - Start a transaction
+ *
+ * @oft:	transaction pointer
+ *
+ * Starts a transaction, by simply grabbing hold of the of_mutex.
+ * This prevents any modification of the live tree while the
+ * transaction is in process.
+ */
+void of_transaction_start(struct of_transaction *oft)
+{
+	/* take the global of transaction mutex */
+	mutex_lock(&of_mutex);
+}
+
+/**
+ * of_transaction_abort - Aborts a transaction in progress
+ *
+ * @oft:	transaction pointer
+ *
+ * Aborts a transaction, this simply releases the of_mutex
+ * and destroys all the pending transaction entries.
+ */
+void of_transaction_abort(struct of_transaction *oft)
+{
+	struct of_transaction_entry *te, *ten;
+
+	mutex_unlock(&of_mutex);
+
+	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
+		__of_transaction_entry_destroy(te);
+}
+
+/**
+ * of_transaction_apply - Applies a transaction
+ *
+ * @oft:	transaction pointer
+ * @force:	continue even in case of an error
+ *
+ * Applies a transaction to the live tree.
+ * Any side-effects of live tree state changes are applied here on
+ * sucess, like creation/destruction of devices and side-effects
+ * like creation of sysfs properties and directories.
+ * Returns 0 on success, a negative error value in case of an error.
+ * On error the partially applied effects are reverted if the @force
+ * parameter is not set.
+ */
+int of_transaction_apply(struct of_transaction *oft, int force)
+{
+	struct of_transaction_entry *te;
+	int ret;
+
+	/* drop the global lock here (notifiers and devices need it) */
+	mutex_unlock(&of_mutex);
+
+	ret = 0;
+
+	/* perform the rest of the work */
+	pr_debug("of_transaction: applying...\n");
+	list_for_each_entry(te, &oft->te_list, node) {
+		__of_transaction_entry_dump(te);
+		ret = __of_transaction_entry_apply(oft, te);
+		if (ret) {
+			pr_err("%s: Error applying transaction (%d)\n",
+					__func__, ret);
+			if (!force) {
+				list_for_each_entry_continue_reverse(te,
+						&oft->te_list, node) {
+					__of_transaction_entry_dump(te);
+					__of_transaction_entry_revert(oft, te);
+				}
+				return ret;
+			}
+		}
+	}
+
+	pr_debug("of_transaction: applied.\n");
+
+	return 0;
+}
+
+/**
+ * of_transaction_revert - Reverts an applied transaction
+ *
+ * @oft:	transaction pointer
+ * @force:	continue even in case of an error
+ *
+ * Reverts a transaction returning the state of the tree to what it
+ * was before the application.
+ * Any side-effects like creation/destruction of devices and
+ * removal of sysfs properties and directories are applied.
+ * Returns 0 on success, a negative error value in case of an error.
+ */
+int of_transaction_revert(struct of_transaction *oft, int force)
+{
+	struct of_transaction_entry *te, *ten;
+	int ret;
+
+	pr_debug("of_transaction: reverting...\n");
+	list_for_each_entry_reverse(te, &oft->te_list, node) {
+		__of_transaction_entry_dump(te);
+		ret = __of_transaction_entry_revert(oft, te);
+		if (ret) {
+			pr_err("%s: Error reverting transaction (%d)\n",
+					__func__, ret);
+			if (!force) {
+				list_for_each_entry_continue(te,
+						&oft->te_list, node) {
+					__of_transaction_entry_dump(te);
+					__of_transaction_entry_apply(oft, te);
+				}
+				return ret;
+			}
+		}
+	}
+
+	/* destroy everything */
+	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
+		__of_transaction_entry_destroy(te);
+	pr_debug("of_transaction: reverted.\n");
+
+	return 0;
+}
+
+/**
+ * of_transaction_action - Perform a transaction action
+ *
+ * @oft:	transaction pointer
+ * @action:	action to perform
+ * @np:		Pointer to device node
+ * @prop:	Pointer to property
+ *
+ * On action being one of:
+ * + OF_RECONFIG_ATTACH_NODE
+ * + OF_RECONFIG_DETACH_NODE,
+ * + OF_RECONFIG_ADD_PROPERTY
+ * + OF_RECONFIG_REMOVE_PROPERTY,
+ * + OF_RECONFIG_UPDATE_PROPERTY
+ * Returns 0 on success, a negative error value in case of an error.
+ */
+int of_transaction_action(struct of_transaction *oft, unsigned long action,
+		struct device_node *np, struct property *prop)
+{
+	struct of_transaction_entry *te;
+
+	/* create the transaction entry */
+	te = __of_transaction_entry_create(oft, action, np, prop);
+	if (IS_ERR(te)) {
+		pr_err("%s: failed to create entry for @%s\n",
+				__func__, np->full_name);
+		return PTR_ERR(te);
+	}
+
+	/* add it to the list */
+	list_add_tail(&te->node, &oft->te_list);
+	return 0;
+}
+
+/* utility functions for advanced transaction users */
+
+/* find a property in the transaction list while not applied */
+struct property *of_transaction_find_property(struct of_transaction *oft,
+		const struct device_node *np, const char *name, int *lenp)
+{
+	struct of_transaction_entry *te;
+
+	/* possibly exists in the transaction list as
+	 * part of an attachment action
+	 */
+	list_for_each_entry_reverse(te, &oft->te_list, node) {
+
+		if (te->action != OF_RECONFIG_ADD_PROPERTY &&
+		    te->action != OF_RECONFIG_REMOVE_PROPERTY &&
+		    te->action != OF_RECONFIG_UPDATE_PROPERTY)
+			continue;
+
+		/* match of node and name? */
+		if (te->np != np || strcmp(te->prop->name, name))
+			continue;
+
+		pr_debug("%s: found property \"%s\" in transaction list @%s\n",
+			__func__, name, te->np->full_name);
+
+		if (te->action == OF_RECONFIG_ADD_PROPERTY ||
+		    te->action == OF_RECONFIG_UPDATE_PROPERTY)
+			return te->prop;
+		else
+			return NULL;
+	}
+
+	/* now fallback to live tree */
+	return of_find_property(np, name, lenp);
+}
+
+/* special find property method for use by transaction users */
+int of_transaction_device_is_available(struct of_transaction *oft,
+		const struct device_node *np)
+{
+	struct property *prop;
+
+	if (!np)
+		return 0;
+
+	prop = of_transaction_find_property(oft, np, "status", NULL);
+	if (prop == NULL)
+		return 1;
+
+	return prop->length > 0 &&
+		(!strcmp(prop->value, "okay") || !strcmp(prop->value, "ok"));
+}
+
+struct device_node *of_transaction_get_child_by_name(
+		struct of_transaction *oft, struct device_node *node,
+		const char *name)
+{
+	struct of_transaction_entry *te;
+
+	/* possibly exists in the transaction list as
+	 * part of an attachment action
+	 */
+	list_for_each_entry_reverse(te, &oft->te_list, node) {
+
+		if (te->action != OF_RECONFIG_ATTACH_NODE &&
+		    te->action != OF_RECONFIG_DETACH_NODE)
+			continue;
+
+		/* look at the parent and if node matches return */
+		if (te->np->parent != node || strcmp(te->np->name, "name"))
+			continue;
+
+		pr_debug("%s: found child \"%s\" in transaction list @%s\n",
+			__func__, name, te->np->full_name);
+		return te->action == OF_RECONFIG_ATTACH_NODE ? te->np : NULL;
+	}
+
+	/* not found in the transaction list? try normal */
+	return of_get_child_by_name(node, name);
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index b7c322c..fa81b42 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -821,4 +821,86 @@ typedef void (*of_init_fn_1)(struct device_node *);
 #define OF_DECLARE_2(table, name, compat, fn) \
 		_OF_DECLARE(table, name, compat, fn, of_init_fn_2)
 
+/**
+ * struct of_transaction_entry	- Holds a transaction entry
+ *
+ * @node:	list_head for the log list
+ * @action:	notifier action
+ * @np:		pointer to the device node affected
+ * @prop:	pointer to the property affected
+ * @old_prop:	hold a pointer to the original property
+ *
+ * Every modification of the device tree during a transaction
+ * is held in a list of of_transaction_entry structures.
+ * That way we can recover from a partial application, or we can
+ * revert the transaction
+ */
+struct of_transaction_entry {
+	struct list_head node;
+	unsigned long action;
+	struct device_node *np;
+	struct property *prop;
+	struct property *old_prop;
+};
+
+/**
+ * struct of_transaction - transaction tracker structure
+ *
+ * @te_list:	list_head for the transaction entries
+ *
+ * Transactions are a convenient way to apply bulk changes to the
+ * live tree. In case of an error, changes are rolled-back.
+ * Transactions live on after initial application, and if not
+ * destroyed after use, they can be reverted in one single call.
+ */
+struct of_transaction {
+	struct list_head te_list;
+};
+
+#ifdef CONFIG_OF_DYNAMIC
+extern void of_transaction_init(struct of_transaction *oft);
+extern void of_transaction_destroy(struct of_transaction *oft);
+extern void of_transaction_start(struct of_transaction *oft);
+extern void of_transaction_abort(struct of_transaction *oft);
+extern int of_transaction_apply(struct of_transaction *oft, int force);
+extern int of_transaction_revert(struct of_transaction *oft, int force);
+extern int of_transaction_action(struct of_transaction *oft,
+		unsigned long action, struct device_node *np,
+		struct property *prop);
+
+static inline int of_transaction_attach_node(struct of_transaction *oft,
+		struct device_node *np)
+{
+	return of_transaction_action(oft, OF_RECONFIG_ATTACH_NODE, np, NULL);
+}
+
+static inline int of_transaction_detach_node(struct of_transaction *oft,
+		struct device_node *np)
+{
+	return of_transaction_action(oft,
+			OF_RECONFIG_DETACH_NODE, np, NULL);
+}
+
+static inline int of_transaction_add_property(struct of_transaction *oft,
+		struct device_node *np, struct property *prop)
+{
+	return of_transaction_action(oft,
+			OF_RECONFIG_ADD_PROPERTY, np, prop);
+}
+
+static inline int of_transaction_remove_property(struct of_transaction *oft,
+		struct device_node *np, struct property *prop)
+{
+	return of_transaction_action(oft,
+			OF_RECONFIG_REMOVE_PROPERTY, np, prop);
+}
+
+static inline int of_transaction_update_property(struct of_transaction *oft,
+		struct device_node *np, struct property *prop)
+{
+	return of_transaction_action(oft,
+			OF_RECONFIG_UPDATE_PROPERTY, np, prop);
+}
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* Re: [PATCH 3/5] OF: Utility helper functions for dynamic nodes
  2014-07-04 16:58 ` [PATCH 3/5] OF: Utility helper functions for dynamic nodes Pantelis Antoniou
@ 2014-07-07  7:04   ` Alexander Sverdlin
  2014-07-07 13:13   ` Grant Likely
  1 sibling, 0 replies; 24+ messages in thread
From: Alexander Sverdlin @ 2014-07-07  7:04 UTC (permalink / raw)
  To: ext Pantelis Antoniou, Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Michael Stickel, Guenter Roeck, Dirk Behme, Alan Tull,
	Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou

Hi!

On 04/07/14 18:58, ext Pantelis Antoniou wrote:
> Introduce helper functions for working with the live DT tree,
> all of them related to dynamically adding/removing nodes and
> properties.
> 
> __of_copy_property() copies a property dynamically
> __of_create_empty_node() creates an empty node
> 
> Bug fix about prop->len == 0 by Ionut Nicu <ioan.nicu.ext@nsn.com>
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nsn.com>

> ---
>  drivers/of/dynamic.c    | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/of_private.h |  14 ++++++
>  include/linux/of.h      |   9 ++++
>  3 files changed, 147 insertions(+)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index eb1126f..90c09b6 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -266,3 +266,127 @@ void of_node_release(struct kobject *kobj)
>  
>  	raw_spin_unlock_irqrestore(&deadtree_lock, flags);
>  }
> +
> +/**
> + * __of_copy_property - Copy a property dynamically.
> + * @prop:	Property to copy
> + * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
> + * @propflags:	Property flags
> + *
> + * 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 allocflags, unsigned long propflags)
> +{
> +	struct property *propn;
> +
> +	propn = kzalloc(sizeof(*prop), allocflags);
> +	if (propn == NULL)
> +		return NULL;
> +
> +	propn->_flags = propflags;
> +
> +	if (of_property_check_flag(propn, OF_ALLOCNAME)) {
> +		propn->name = kstrdup(prop->name, allocflags);
> +		if (propn->name == NULL)
> +			goto err_fail_name;
> +	} else
> +		propn->name = prop->name;
> +
> +	/*
> +	 * NOTE: There is no check for zero length value.
> +	 * In case of a boolean property This will allocate a value
> +	 * of zero bytes. We do this to work around the use
> +	 * of of_get_property() calls on boolean values.
> +	 */
> +	if (of_property_check_flag(propn, OF_ALLOCVALUE)) {
> +		propn->value = kmalloc(prop->length, allocflags);
> +		if (propn->value == NULL)
> +			goto err_fail_value;
> +		memcpy(propn->value, prop->value, prop->length);
> +	} else
> +		propn->value = prop->value;
> +
> +	propn->length = prop->length;
> +
> +	/* mark the property as dynamic */
> +	of_property_set_flag(propn, OF_DYNAMIC);
> +
> +	return propn;
> +
> +err_fail_value:
> +	if (of_property_check_flag(propn, OF_ALLOCNAME))
> +		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
> + * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
> + * @nodeflags:	Node flags
> + *
> + * 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 allocflags, unsigned long nodeflags)
> +{
> +	struct device_node *node;
> +
> +	node = kzalloc(sizeof(*node), allocflags);
> +	if (node == NULL)
> +		return NULL;
> +
> +	node->_flags = nodeflags;
> +
> +	if (of_node_check_flag(node, OF_ALLOCNAME)) {
> +		node->name = kstrdup(name, allocflags);
> +		if (node->name == NULL)
> +			goto err_free_node;
> +	} else
> +		node->name = name;
> +
> +	if (of_node_check_flag(node, OF_ALLOCTYPE)) {
> +		node->type = kstrdup(type, allocflags);
> +		if (node->type == NULL)
> +			goto err_free_name;
> +	} else
> +		node->type = type;
> +
> +	if (of_node_check_flag(node, OF_ALLOCFULL)) {
> +		node->full_name = kstrdup(full_name, allocflags);
> +		if (node->full_name == NULL)
> +			goto err_free_type;
> +	} else
> +		node->full_name = full_name;
> +
> +	node->phandle = phandle;
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +
> +	of_node_init(node);
> +
> +	return node;
> +err_free_type:
> +	if (of_node_check_flag(node, OF_ALLOCTYPE))
> +		kfree(node->type);
> +err_free_name:
> +	if (of_node_check_flag(node, OF_ALLOCNAME))
> +		kfree(node->name);
> +err_free_node:
> +	kfree(node);
> +	return NULL;
> +}
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index b2c25b5..e544247 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -67,4 +67,18 @@ extern int __of_update_property(struct device_node *np,
>  extern void __of_attach_node(struct device_node *np);
>  extern void __of_detach_node(struct device_node *np);
>  
> +/**
> + * 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.
> + */
> +
> +struct property *__of_copy_property(const struct property *prop,
> +		gfp_t allocflags, unsigned long propflags);
> +struct device_node *__of_create_empty_node(const char *name,
> +		const char *type, const char *full_name,
> +		phandle phandle, gfp_t allocflags, unsigned long nodeflags);
> +
>  #endif /* _LINUX_OF_PRIVATE_H */
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 8e4fb82..b7c322c 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -206,6 +206,15 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>  #define OF_POPULATED	3 /* device already created for the node */
>  #define OF_POPULATED_BUS	4 /* of_platform_populate recursed
>  				   * to children of this node */
> +#define OF_ALLOCNAME	5 /* name was kmalloc-ed */
> +#define OF_ALLOCTYPE	6 /* type was kmalloc-ed */
> +#define OF_ALLOCFULL	7 /* full_name was kmalloc-ed */
> +#define OF_ALLOCVALUE	8 /* value was kmalloc-ed */
> +
> +#define OF_NODE_ALLOCALL \
> +	((1 << OF_ALLOCNAME) | (1 << OF_ALLOCTYPE) | (1 << OF_ALLOCFULL))
> +#define OF_PROP_ALLOCALL \
> +	((1 << OF_ALLOCNAME) | (1 << OF_ALLOCVALUE))
>  
>  #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
>  #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
> 

-- 
Best regards,
Alexander Sverdlin.

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] of: Do not free memory at of_node_release option
  2014-07-04 16:58 ` [PATCH 1/5] of: Do not free memory at of_node_release option Pantelis Antoniou
@ 2014-07-07 13:10   ` Grant Likely
  2014-07-07 13:13     ` Pantelis Antoniou
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2014-07-07 13:10 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou

On Fri,  4 Jul 2014 19:58:45 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Introduce an option whether to free or not the memory of a node
> and it's properties when of_node_release is called.
> 
> The option is located in the chosen node, and its name is
> of-node-keep. When this boolean property is missing the default
> behaviour of freeing is preserved.

No way. The problem is buggy kernel code. We've got a plan to fix that.
In the mean time I'm okay with a CONFIG_ option that inhibits freeing,
or *maybe* a runtime global option that causes free() to not be called.

What this patch does is not helpful.

g.

> 
> The rational behind this option is as follows:
> 
> The life-cycle of nodes and properties does not allow us to release
> the memory taken by a device_node. Pointer to properties and nodes
> might still be in use in drivers, so any memory free'ing is dangerous.
> 
> Simply move all the properties to the deadprops list, and the node
> itself to of_alldeadnodes until the life-cycles issues are resolved.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/dynamic.c | 74 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index a5d6f4f..0e8457c 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -180,17 +180,29 @@ int of_detach_node(struct device_node *np)
>  	return rc;
>  }
>  
> +static struct device_node *of_alldeadnodes;
> +static DEFINE_RAW_SPINLOCK(deadtree_lock);
> +
>  /**
>   *	of_node_release - release a dynamically allocated node
>   *	@kref:  kref element of the node to be released
>   *
>   *	In of_node_put() this function is passed to kref_put()
>   *	as the destructor.
> + *
> + *	There is an option to not free the node, since due to the
> + *	way that node and property life-cycles are not completely
> + *	managed, we can't free the memory of a node at will.
> + *	Instead we move the node to the dead nodes list where it will
> + *	remain until the life-cycle issues are resolved.
>   */
> -static void of_node_release(struct kobject *kobj)
> +void of_node_release(struct kobject *kobj)
>  {
> +	/*  we probe the of-node-keep chosen prop once */
> +	static int node_keep = -1;
>  	struct device_node *node = kobj_to_device_node(kobj);
> -	struct property *prop = node->properties;
> +	struct property *prop;
> +	unsigned long flags;
>  
>  	/* We should never be releasing nodes that haven't been detached. */
>  	if (!of_node_check_flag(node, OF_DETACHED)) {
> @@ -199,23 +211,55 @@ static void of_node_release(struct kobject *kobj)
>  		return;
>  	}
>  
> -	if (!of_node_check_flag(node, OF_DYNAMIC))
> +	/* we should not be trying to release the root */
> +	if (WARN_ON(node == of_allnodes))
>  		return;
>  
> -	while (prop) {
> -		struct property *next = prop->next;
> +	/* read the chosen boolean property "of-node-keep" once */
> +	if (node_keep == -1)
> +		node_keep = of_property_read_bool(of_chosen, "of-node-keep");
>  
> -		kfree(prop->name);
> -		kfree(prop->value);
> -		kfree(prop);
> -		prop = next;
> +	/* default is to free everything */
> +	if (!node_keep) {
>  
> -		if (!prop) {
> -			prop = node->deadprops;
> -			node->deadprops = NULL;
> +		/* free normal properties */
> +		while ((prop = node->properties) != NULL) {
> +			node->properties = prop->next;
> +			kfree(prop->name);
> +			kfree(prop->value);
> +			kfree(prop);
>  		}
> +
> +		/* free deap properties */
> +		while ((prop = node->deadprops) != NULL) {
> +			node->deadprops = prop->next;
> +			kfree(prop->name);
> +			kfree(prop->value);
> +			kfree(prop);
> +		}
> +
> +		/* free the node */
> +		kfree(node->full_name);
> +		kfree(node->data);
> +		kfree(node);
> +		return;
>  	}
> -	kfree(node->full_name);
> -	kfree(node->data);
> -	kfree(node);
> +
> +	pr_info("%s: dead node \"%s\"\n", __func__, node->full_name);
> +
> +	/* can't use devtree lock; at of_node_put caller might be holding it */
> +	raw_spin_lock_irqsave(&deadtree_lock, flags);
> +
> +	/* move all properties to dead properties */
> +	while ((prop = node->properties) != NULL) {
> +		node->properties = prop->next;
> +		prop->next = node->deadprops;
> +		node->deadprops = prop;
> +	}
> +
> +	/* move node to alldeadnodes */
> +	node->allnext = of_alldeadnodes;
> +	of_alldeadnodes = node;
> +
> +	raw_spin_unlock_irqrestore(&deadtree_lock, flags);
>  }
> -- 
> 1.7.12
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] OF: Export a number of __of_* methods
  2014-07-04 16:58 ` [PATCH 2/5] OF: Export a number of __of_* methods Pantelis Antoniou
@ 2014-07-07 13:11   ` Grant Likely
  2014-07-07 13:17     ` Pantelis Antoniou
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2014-07-07 13:11 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou

On Fri,  4 Jul 2014 19:58:46 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> As part of moving to transactional DT, a number of methods are
> reworked and exported.
> 
> The methods exported are:
> __of_add_property, __of_remove_property, __of_update_property, __of_attach_node,
> __of_detach_node


??? The description doesn't match the patch. Nothing is getting
exported, and the commit message should really say why and how the
methods are being reworked.

g.

> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/base.c       | 97 +++++++++++++++++++++++++++++--------------------
>  drivers/of/dynamic.c    | 71 +++++++++++++++++++-----------------
>  drivers/of/of_private.h |  8 ++++
>  3 files changed, 102 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index be0ec37..0d50318 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1659,7 +1659,7 @@ EXPORT_SYMBOL(of_count_phandle_with_args);
>  /**
>   * __of_add_property - Add a property to a node without lock operations
>   */
> -static int __of_add_property(struct device_node *np, struct property *prop)
> +int __of_add_property(struct device_node *np, struct property *prop)
>  {
>  	struct property **next;
>  
> @@ -1701,6 +1701,25 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	return rc;
>  }
>  
> +int __of_remove_property(struct device_node *np, struct property *prop)
> +{
> +	struct property **next;
> +
> +	for (next = &np->properties; *next; next = &(*next)->next) {
> +		if (*next == prop)
> +			break;
> +	}
> +	if (*next == NULL)
> +		return -ENODEV;
> +
> +	/* found the node */
> +	*next = prop->next;
> +	prop->next = np->deadprops;
> +	np->deadprops = prop;
> +
> +	return 0;
> +}
> +
>  /**
>   * of_remove_property - Remove a property from a node.
>   *
> @@ -1711,9 +1730,7 @@ int of_add_property(struct device_node *np, struct property *prop)
>   */
>  int of_remove_property(struct device_node *np, struct property *prop)
>  {
> -	struct property **next;
>  	unsigned long flags;
> -	int found = 0;
>  	int rc;
>  
>  	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> @@ -1721,22 +1738,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  		return rc;
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> -	next = &np->properties;
> -	while (*next) {
> -		if (*next == prop) {
> -			/* found the node */
> -			*next = prop->next;
> -			prop->next = np->deadprops;
> -			np->deadprops = prop;
> -			found = 1;
> -			break;
> -		}
> -		next = &(*next)->next;
> -	}
> +	rc = __of_remove_property(np, prop);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	if (!found)
> -		return -ENODEV;
> +	if (rc)
> +		return rc;
>  
>  	/* at early boot, bail hear and defer setup to of_init() */
>  	if (!of_kset)
> @@ -1747,6 +1753,32 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	return 0;
>  }
>  
> +int __of_update_property(struct device_node *np, struct property *newprop,
> +		struct property **oldpropp)
> +{
> +	struct property **next, *oldprop;
> +
> +	for (next = &np->properties; *next; next = &(*next)->next) {
> +		if (of_prop_cmp((*next)->name, newprop->name) == 0)
> +			break;
> +	}
> +	*oldpropp = oldprop = *next;
> +
> +	if (oldprop) {
> +		/* replace the node */
> +		newprop->next = oldprop->next;
> +		*next = newprop;
> +		oldprop->next = np->deadprops;
> +		np->deadprops = oldprop;
> +	} else {
> +		/* new node */
> +		newprop->next = NULL;
> +		*next = newprop;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * of_update_property - Update a property in a node, if the property does
>   * not exist, add it.
> @@ -1756,36 +1788,21 @@ int of_remove_property(struct device_node *np, struct property *prop)
>   * Instead we just move the property to the "dead properties" list,
>   * and add the new property to the property list
>   */
> -int of_update_property(struct device_node *np, struct property *newprop)
> +int of_update_property(struct device_node *np, struct property *prop)
>  {
> -	struct property **next, *oldprop;
> +	struct property *oldprop;
>  	unsigned long flags;
>  	int rc;
>  
> -	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> +	if (!prop->name)
> +		return -EINVAL;
> +
> +	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>  	if (rc)
>  		return rc;
>  
> -	if (!newprop->name)
> -		return -EINVAL;
> -
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> -	next = &np->properties;
> -	oldprop = __of_find_property(np, newprop->name, NULL);
> -	if (!oldprop) {
> -		/* add the new node */
> -		rc = __of_add_property(np, newprop);
> -	} else while (*next) {
> -		/* replace the node */
> -		if (*next == oldprop) {
> -			newprop->next = oldprop->next;
> -			*next = newprop;
> -			oldprop->next = np->deadprops;
> -			np->deadprops = oldprop;
> -			break;
> -		}
> -		next = &(*next)->next;
> -	}
> +	rc = __of_update_property(np, prop, &oldprop);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  	if (rc)
>  		return rc;
> @@ -1797,7 +1814,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  	/* Update the sysfs attribute */
>  	if (oldprop)
>  		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> -	__of_add_property_sysfs(np, newprop);
> +	__of_add_property_sysfs(np, prop);
>  
>  	return 0;
>  }
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 0e8457c..eb1126f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -95,8 +95,17 @@ int of_property_notify(int action, struct device_node *np,
>  	return of_reconfig_notify(action, &pr);
>  }
>  
> +void __of_attach_node(struct device_node *np)
> +{
> +	np->sibling = np->parent->child;
> +	np->allnext = np->parent->allnext;
> +	np->parent->allnext = np;
> +	np->parent->child = np;
> +	of_node_clear_flag(np, OF_DETACHED);
> +}
> +
>  /**
> - * of_attach_node() - Plug a device node into the tree and global list.
> + * of_attach_node - Plug a device node into the tree and global list.
>   */
>  int of_attach_node(struct device_node *np)
>  {
> @@ -108,52 +117,29 @@ int of_attach_node(struct device_node *np)
>  		return rc;
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> -	np->sibling = np->parent->child;
> -	np->allnext = of_allnodes;
> -	np->parent->child = np;
> -	of_allnodes = np;
> -	of_node_clear_flag(np, OF_DETACHED);
> +	__of_attach_node(np);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
>  	of_node_add(np);
>  	return 0;
>  }
>  
> -/**
> - * of_detach_node() - "Unplug" a node from the device tree.
> - *
> - * The caller must hold a reference to the node.  The memory associated with
> - * the node is not freed until its refcount goes to zero.
> - */
> -int of_detach_node(struct device_node *np)
> +void __of_detach_node(struct device_node *np)
>  {
>  	struct device_node *parent;
> -	unsigned long flags;
> -	int rc = 0;
> -
> -	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> -	if (rc)
> -		return rc;
> +	struct device_node *prev;
> +	struct device_node *prevsib;
>  
> -	raw_spin_lock_irqsave(&devtree_lock, flags);
> -
> -	if (of_node_check_flag(np, OF_DETACHED)) {
> -		/* someone already detached it */
> -		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> -		return rc;
> -	}
> +	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
> +		return;
>  
>  	parent = np->parent;
> -	if (!parent) {
> -		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> -		return rc;
> -	}
> +	if (WARN_ON(!parent))
> +		return;
>  
>  	if (of_allnodes == np)
>  		of_allnodes = np->allnext;
>  	else {
> -		struct device_node *prev;
> -
>  		for (prev = of_allnodes;
>  		     prev->allnext != np;
>  		     prev = prev->allnext)
> @@ -164,8 +150,6 @@ int of_detach_node(struct device_node *np)
>  	if (parent->child == np)
>  		parent->child = np->sibling;
>  	else {
> -		struct device_node *prevsib;
> -
>  		for (prevsib = np->parent->child;
>  		     prevsib->sibling != np;
>  		     prevsib = prevsib->sibling)
> @@ -174,6 +158,25 @@ int of_detach_node(struct device_node *np)
>  	}
>  
>  	of_node_set_flag(np, OF_DETACHED);
> +}
> +
> +/**
> + * of_detach_node - "Unplug" a node from the device tree.
> + *
> + * The caller must hold a reference to the node.  The memory associated with
> + * the node is not freed until its refcount goes to zero.
> + */
> +int of_detach_node(struct device_node *np)
> +{
> +	unsigned long flags;
> +	int rc = 0;
> +
> +	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> +	if (rc)
> +		return rc;
> +
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	__of_detach_node(np);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
>  	of_node_remove(np);
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index a541b81..b2c25b5 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -59,4 +59,12 @@ static inline int of_reconfig_notify(unsigned long action, void *p)
>  }
>  #endif /* CONFIG_OF_DYNAMIC */
>  
> +extern int __of_add_property(struct device_node *np, struct property *prop);
> +extern int __of_remove_property(struct device_node *np, struct property *prop);
> +extern int __of_update_property(struct device_node *np,
> +		struct property *newprop, struct property **oldprop);
> +
> +extern void __of_attach_node(struct device_node *np);
> +extern void __of_detach_node(struct device_node *np);
> +
>  #endif /* _LINUX_OF_PRIVATE_H */
> -- 
> 1.7.12
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 3/5] OF: Utility helper functions for dynamic nodes
  2014-07-04 16:58 ` [PATCH 3/5] OF: Utility helper functions for dynamic nodes Pantelis Antoniou
  2014-07-07  7:04   ` Alexander Sverdlin
@ 2014-07-07 13:13   ` Grant Likely
  1 sibling, 0 replies; 24+ messages in thread
From: Grant Likely @ 2014-07-07 13:13 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou

On Fri,  4 Jul 2014 19:58:47 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Introduce helper functions for working with the live DT tree,
> all of them related to dynamically adding/removing nodes and
> properties.
> 
> __of_copy_property() copies a property dynamically
> __of_create_empty_node() creates an empty node
> 
> Bug fix about prop->len == 0 by Ionut Nicu <ioan.nicu.ext@nsn.com>
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/dynamic.c    | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/of_private.h |  14 ++++++
>  include/linux/of.h      |   9 ++++
>  3 files changed, 147 insertions(+)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index eb1126f..90c09b6 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -266,3 +266,127 @@ void of_node_release(struct kobject *kobj)
>  
>  	raw_spin_unlock_irqrestore(&deadtree_lock, flags);
>  }
> +
> +/**
> + * __of_copy_property - Copy a property dynamically.
> + * @prop:	Property to copy
> + * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
> + * @propflags:	Property flags
> + *
> + * 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 allocflags, unsigned long propflags)
> +{
> +	struct property *propn;
> +
> +	propn = kzalloc(sizeof(*prop), allocflags);
> +	if (propn == NULL)
> +		return NULL;
> +
> +	propn->_flags = propflags;
> +
> +	if (of_property_check_flag(propn, OF_ALLOCNAME)) {
> +		propn->name = kstrdup(prop->name, allocflags);
> +		if (propn->name == NULL)
> +			goto err_fail_name;
> +	} else
> +		propn->name = prop->name;
> +
> +	/*
> +	 * NOTE: There is no check for zero length value.
> +	 * In case of a boolean property This will allocate a value
> +	 * of zero bytes. We do this to work around the use
> +	 * of of_get_property() calls on boolean values.
> +	 */
> +	if (of_property_check_flag(propn, OF_ALLOCVALUE)) {
> +		propn->value = kmalloc(prop->length, allocflags);
> +		if (propn->value == NULL)
> +			goto err_fail_value;
> +		memcpy(propn->value, prop->value, prop->length);
> +	} else
> +		propn->value = prop->value;
> +
> +	propn->length = prop->length;
> +
> +	/* mark the property as dynamic */
> +	of_property_set_flag(propn, OF_DYNAMIC);
> +
> +	return propn;
> +
> +err_fail_value:
> +	if (of_property_check_flag(propn, OF_ALLOCNAME))
> +		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
> + * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
> + * @nodeflags:	Node flags
> + *
> + * 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 allocflags, unsigned long nodeflags)
> +{
> +	struct device_node *node;
> +
> +	node = kzalloc(sizeof(*node), allocflags);
> +	if (node == NULL)
> +		return NULL;
> +
> +	node->_flags = nodeflags;
> +
> +	if (of_node_check_flag(node, OF_ALLOCNAME)) {
> +		node->name = kstrdup(name, allocflags);
> +		if (node->name == NULL)
> +			goto err_free_node;
> +	} else
> +		node->name = name;
> +
> +	if (of_node_check_flag(node, OF_ALLOCTYPE)) {
> +		node->type = kstrdup(type, allocflags);
> +		if (node->type == NULL)
> +			goto err_free_name;
> +	} else
> +		node->type = type;
> +
> +	if (of_node_check_flag(node, OF_ALLOCFULL)) {
> +		node->full_name = kstrdup(full_name, allocflags);
> +		if (node->full_name == NULL)
> +			goto err_free_type;
> +	} else
> +		node->full_name = full_name;
> +
> +	node->phandle = phandle;
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +
> +	of_node_init(node);
> +
> +	return node;
> +err_free_type:
> +	if (of_node_check_flag(node, OF_ALLOCTYPE))
> +		kfree(node->type);
> +err_free_name:
> +	if (of_node_check_flag(node, OF_ALLOCNAME))
> +		kfree(node->name);
> +err_free_node:
> +	kfree(node);
> +	return NULL;
> +}
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index b2c25b5..e544247 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -67,4 +67,18 @@ extern int __of_update_property(struct device_node *np,
>  extern void __of_attach_node(struct device_node *np);
>  extern void __of_detach_node(struct device_node *np);
>  
> +/**
> + * 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.
> + */
> +
> +struct property *__of_copy_property(const struct property *prop,
> +		gfp_t allocflags, unsigned long propflags);
> +struct device_node *__of_create_empty_node(const char *name,
> +		const char *type, const char *full_name,
> +		phandle phandle, gfp_t allocflags, unsigned long nodeflags);
> +
>  #endif /* _LINUX_OF_PRIVATE_H */
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 8e4fb82..b7c322c 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -206,6 +206,15 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>  #define OF_POPULATED	3 /* device already created for the node */
>  #define OF_POPULATED_BUS	4 /* of_platform_populate recursed
>  				   * to children of this node */
> +#define OF_ALLOCNAME	5 /* name was kmalloc-ed */
> +#define OF_ALLOCTYPE	6 /* type was kmalloc-ed */
> +#define OF_ALLOCFULL	7 /* full_name was kmalloc-ed */
> +#define OF_ALLOCVALUE	8 /* value was kmalloc-ed */
> +

We talked about this. Drop all the fine grained alloc tracking.

g.

> +#define OF_NODE_ALLOCALL \
> +	((1 << OF_ALLOCNAME) | (1 << OF_ALLOCTYPE) | (1 << OF_ALLOCFULL))
> +#define OF_PROP_ALLOCALL \
> +	((1 << OF_ALLOCNAME) | (1 << OF_ALLOCVALUE))
>  
>  #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
>  #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
> -- 
> 1.7.12
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] of: Do not free memory at of_node_release option
  2014-07-07 13:10   ` Grant Likely
@ 2014-07-07 13:13     ` Pantelis Antoniou
  2014-07-09 18:18       ` Grant Likely
  0 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-07 13:13 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

Hi Grant,

On Jul 7, 2014, at 4:10 PM, Grant Likely wrote:

> On Fri,  4 Jul 2014 19:58:45 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>> Introduce an option whether to free or not the memory of a node
>> and it's properties when of_node_release is called.
>> 
>> The option is located in the chosen node, and its name is
>> of-node-keep. When this boolean property is missing the default
>> behaviour of freeing is preserved.
> 
> No way. The problem is buggy kernel code. We've got a plan to fix that.
> In the mean time I'm okay with a CONFIG_ option that inhibits freeing,
> or *maybe* a runtime global option that causes free() to not be called.
> 
> What this patch does is not helpful.
> 

Well, it was a long shot. FWIW I don't need it for overlays, this is just to
make sure we don't crash in case we have a buggy driver calling of_node_put()
twice or something.

I'm open to suggestions on how to track and fix those.

> g.
> 

Regards

-- Pantelis

>> 
>> The rational behind this option is as follows:
>> 
>> The life-cycle of nodes and properties does not allow us to release
>> the memory taken by a device_node. Pointer to properties and nodes
>> might still be in use in drivers, so any memory free'ing is dangerous.
>> 
>> Simply move all the properties to the deadprops list, and the node
>> itself to of_alldeadnodes until the life-cycles issues are resolved.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/dynamic.c | 74 +++++++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 59 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index a5d6f4f..0e8457c 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -180,17 +180,29 @@ int of_detach_node(struct device_node *np)
>> 	return rc;
>> }
>> 
>> +static struct device_node *of_alldeadnodes;
>> +static DEFINE_RAW_SPINLOCK(deadtree_lock);
>> +
>> /**
>>  *	of_node_release - release a dynamically allocated node
>>  *	@kref:  kref element of the node to be released
>>  *
>>  *	In of_node_put() this function is passed to kref_put()
>>  *	as the destructor.
>> + *
>> + *	There is an option to not free the node, since due to the
>> + *	way that node and property life-cycles are not completely
>> + *	managed, we can't free the memory of a node at will.
>> + *	Instead we move the node to the dead nodes list where it will
>> + *	remain until the life-cycle issues are resolved.
>>  */
>> -static void of_node_release(struct kobject *kobj)
>> +void of_node_release(struct kobject *kobj)
>> {
>> +	/*  we probe the of-node-keep chosen prop once */
>> +	static int node_keep = -1;
>> 	struct device_node *node = kobj_to_device_node(kobj);
>> -	struct property *prop = node->properties;
>> +	struct property *prop;
>> +	unsigned long flags;
>> 
>> 	/* We should never be releasing nodes that haven't been detached. */
>> 	if (!of_node_check_flag(node, OF_DETACHED)) {
>> @@ -199,23 +211,55 @@ static void of_node_release(struct kobject *kobj)
>> 		return;
>> 	}
>> 
>> -	if (!of_node_check_flag(node, OF_DYNAMIC))
>> +	/* we should not be trying to release the root */
>> +	if (WARN_ON(node == of_allnodes))
>> 		return;
>> 
>> -	while (prop) {
>> -		struct property *next = prop->next;
>> +	/* read the chosen boolean property "of-node-keep" once */
>> +	if (node_keep == -1)
>> +		node_keep = of_property_read_bool(of_chosen, "of-node-keep");
>> 
>> -		kfree(prop->name);
>> -		kfree(prop->value);
>> -		kfree(prop);
>> -		prop = next;
>> +	/* default is to free everything */
>> +	if (!node_keep) {
>> 
>> -		if (!prop) {
>> -			prop = node->deadprops;
>> -			node->deadprops = NULL;
>> +		/* free normal properties */
>> +		while ((prop = node->properties) != NULL) {
>> +			node->properties = prop->next;
>> +			kfree(prop->name);
>> +			kfree(prop->value);
>> +			kfree(prop);
>> 		}
>> +
>> +		/* free deap properties */
>> +		while ((prop = node->deadprops) != NULL) {
>> +			node->deadprops = prop->next;
>> +			kfree(prop->name);
>> +			kfree(prop->value);
>> +			kfree(prop);
>> +		}
>> +
>> +		/* free the node */
>> +		kfree(node->full_name);
>> +		kfree(node->data);
>> +		kfree(node);
>> +		return;
>> 	}
>> -	kfree(node->full_name);
>> -	kfree(node->data);
>> -	kfree(node);
>> +
>> +	pr_info("%s: dead node \"%s\"\n", __func__, node->full_name);
>> +
>> +	/* can't use devtree lock; at of_node_put caller might be holding it */
>> +	raw_spin_lock_irqsave(&deadtree_lock, flags);
>> +
>> +	/* move all properties to dead properties */
>> +	while ((prop = node->properties) != NULL) {
>> +		node->properties = prop->next;
>> +		prop->next = node->deadprops;
>> +		node->deadprops = prop;
>> +	}
>> +
>> +	/* move node to alldeadnodes */
>> +	node->allnext = of_alldeadnodes;
>> +	of_alldeadnodes = node;
>> +
>> +	raw_spin_unlock_irqrestore(&deadtree_lock, flags);
>> }
>> -- 
>> 1.7.12
>> 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] OF: Export a number of __of_* methods
  2014-07-07 13:11   ` Grant Likely
@ 2014-07-07 13:17     ` Pantelis Antoniou
  2014-07-09 18:58       ` Grant Likely
  0 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-07 13:17 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

Hi Grant,

On Jul 7, 2014, at 4:11 PM, Grant Likely wrote:

> On Fri,  4 Jul 2014 19:58:46 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>> As part of moving to transactional DT, a number of methods are
>> reworked and exported.
>> 
>> The methods exported are:
>> __of_add_property, __of_remove_property, __of_update_property, __of_attach_node,
>> __of_detach_node
> 
> 
> ??? The description doesn't match the patch. Nothing is getting
> exported, and the commit message should really say why and how the
> methods are being reworked.
> 

Err, are they not exported out of their file scope?

static int foo -> int foo?

The _post methods are 'exported' in the same manner.

The explanation is included in the next patch in the series, copying verbatim:

> As part of transactional DT support there needs to be a clearer
> definition of how each dynamic tree change method works.
> 
> Let's take for example the of_add_property method. There are
> discrete steps taken in sequence.
> 
> 1. The OF_RECONFIG_ADD_PROPERTY notifier will be fired.
> 2. The devtree lock will be taken.
> 3. Insertion of the property in the live tree.
> 4. The devtree lock will be released.
> 5. The property will be inserted in the sysfs tree.
> 
> For each of the of_attach_node, of_detach_node, of_add_property,
> of_remove_property and of_update_property methods we export (and
> introduce if not available), their counterparts of
> __of_attach_node, __of_attach_node_post, __of_detach_node,
> __of_detach_node_post, __of_add_property, __of_add_property_post,
> __of_remove_property, __of_remove_property_post, __of_update_property
> and __of_update_property post.
> 
> So each of the tree modification methods is following the same
> pattern, i.e. for method foo we have:
> 
> Firing of the notifer, lock, __foo, unlock, __foo_post
> 
> This breakdown is required when we introduce transaction.

Regards

-- Pantelis

> g.
> 
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/base.c       | 97 +++++++++++++++++++++++++++++--------------------
>> drivers/of/dynamic.c    | 71 +++++++++++++++++++-----------------
>> drivers/of/of_private.h |  8 ++++
>> 3 files changed, 102 insertions(+), 74 deletions(-)
>> 
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index be0ec37..0d50318 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1659,7 +1659,7 @@ EXPORT_SYMBOL(of_count_phandle_with_args);
>> /**
>>  * __of_add_property - Add a property to a node without lock operations
>>  */
>> -static int __of_add_property(struct device_node *np, struct property *prop)
>> +int __of_add_property(struct device_node *np, struct property *prop)
>> {
>> 	struct property **next;
>> 
>> @@ -1701,6 +1701,25 @@ int of_add_property(struct device_node *np, struct property *prop)
>> 	return rc;
>> }
>> 
>> +int __of_remove_property(struct device_node *np, struct property *prop)
>> +{
>> +	struct property **next;
>> +
>> +	for (next = &np->properties; *next; next = &(*next)->next) {
>> +		if (*next == prop)
>> +			break;
>> +	}
>> +	if (*next == NULL)
>> +		return -ENODEV;
>> +
>> +	/* found the node */
>> +	*next = prop->next;
>> +	prop->next = np->deadprops;
>> +	np->deadprops = prop;
>> +
>> +	return 0;
>> +}
>> +
>> /**
>>  * of_remove_property - Remove a property from a node.
>>  *
>> @@ -1711,9 +1730,7 @@ int of_add_property(struct device_node *np, struct property *prop)
>>  */
>> int of_remove_property(struct device_node *np, struct property *prop)
>> {
>> -	struct property **next;
>> 	unsigned long flags;
>> -	int found = 0;
>> 	int rc;
>> 
>> 	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
>> @@ -1721,22 +1738,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
>> 		return rc;
>> 
>> 	raw_spin_lock_irqsave(&devtree_lock, flags);
>> -	next = &np->properties;
>> -	while (*next) {
>> -		if (*next == prop) {
>> -			/* found the node */
>> -			*next = prop->next;
>> -			prop->next = np->deadprops;
>> -			np->deadprops = prop;
>> -			found = 1;
>> -			break;
>> -		}
>> -		next = &(*next)->next;
>> -	}
>> +	rc = __of_remove_property(np, prop);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>> -	if (!found)
>> -		return -ENODEV;
>> +	if (rc)
>> +		return rc;
>> 
>> 	/* at early boot, bail hear and defer setup to of_init() */
>> 	if (!of_kset)
>> @@ -1747,6 +1753,32 @@ int of_remove_property(struct device_node *np, struct property *prop)
>> 	return 0;
>> }
>> 
>> +int __of_update_property(struct device_node *np, struct property *newprop,
>> +		struct property **oldpropp)
>> +{
>> +	struct property **next, *oldprop;
>> +
>> +	for (next = &np->properties; *next; next = &(*next)->next) {
>> +		if (of_prop_cmp((*next)->name, newprop->name) == 0)
>> +			break;
>> +	}
>> +	*oldpropp = oldprop = *next;
>> +
>> +	if (oldprop) {
>> +		/* replace the node */
>> +		newprop->next = oldprop->next;
>> +		*next = newprop;
>> +		oldprop->next = np->deadprops;
>> +		np->deadprops = oldprop;
>> +	} else {
>> +		/* new node */
>> +		newprop->next = NULL;
>> +		*next = newprop;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> /*
>>  * of_update_property - Update a property in a node, if the property does
>>  * not exist, add it.
>> @@ -1756,36 +1788,21 @@ int of_remove_property(struct device_node *np, struct property *prop)
>>  * Instead we just move the property to the "dead properties" list,
>>  * and add the new property to the property list
>>  */
>> -int of_update_property(struct device_node *np, struct property *newprop)
>> +int of_update_property(struct device_node *np, struct property *prop)
>> {
>> -	struct property **next, *oldprop;
>> +	struct property *oldprop;
>> 	unsigned long flags;
>> 	int rc;
>> 
>> -	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
>> +	if (!prop->name)
>> +		return -EINVAL;
>> +
>> +	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>> 	if (rc)
>> 		return rc;
>> 
>> -	if (!newprop->name)
>> -		return -EINVAL;
>> -
>> 	raw_spin_lock_irqsave(&devtree_lock, flags);
>> -	next = &np->properties;
>> -	oldprop = __of_find_property(np, newprop->name, NULL);
>> -	if (!oldprop) {
>> -		/* add the new node */
>> -		rc = __of_add_property(np, newprop);
>> -	} else while (*next) {
>> -		/* replace the node */
>> -		if (*next == oldprop) {
>> -			newprop->next = oldprop->next;
>> -			*next = newprop;
>> -			oldprop->next = np->deadprops;
>> -			np->deadprops = oldprop;
>> -			break;
>> -		}
>> -		next = &(*next)->next;
>> -	}
>> +	rc = __of_update_property(np, prop, &oldprop);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 	if (rc)
>> 		return rc;
>> @@ -1797,7 +1814,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
>> 	/* Update the sysfs attribute */
>> 	if (oldprop)
>> 		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
>> -	__of_add_property_sysfs(np, newprop);
>> +	__of_add_property_sysfs(np, prop);
>> 
>> 	return 0;
>> }
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index 0e8457c..eb1126f 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -95,8 +95,17 @@ int of_property_notify(int action, struct device_node *np,
>> 	return of_reconfig_notify(action, &pr);
>> }
>> 
>> +void __of_attach_node(struct device_node *np)
>> +{
>> +	np->sibling = np->parent->child;
>> +	np->allnext = np->parent->allnext;
>> +	np->parent->allnext = np;
>> +	np->parent->child = np;
>> +	of_node_clear_flag(np, OF_DETACHED);
>> +}
>> +
>> /**
>> - * of_attach_node() - Plug a device node into the tree and global list.
>> + * of_attach_node - Plug a device node into the tree and global list.
>>  */
>> int of_attach_node(struct device_node *np)
>> {
>> @@ -108,52 +117,29 @@ int of_attach_node(struct device_node *np)
>> 		return rc;
>> 
>> 	raw_spin_lock_irqsave(&devtree_lock, flags);
>> -	np->sibling = np->parent->child;
>> -	np->allnext = of_allnodes;
>> -	np->parent->child = np;
>> -	of_allnodes = np;
>> -	of_node_clear_flag(np, OF_DETACHED);
>> +	__of_attach_node(np);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>> 	of_node_add(np);
>> 	return 0;
>> }
>> 
>> -/**
>> - * of_detach_node() - "Unplug" a node from the device tree.
>> - *
>> - * The caller must hold a reference to the node.  The memory associated with
>> - * the node is not freed until its refcount goes to zero.
>> - */
>> -int of_detach_node(struct device_node *np)
>> +void __of_detach_node(struct device_node *np)
>> {
>> 	struct device_node *parent;
>> -	unsigned long flags;
>> -	int rc = 0;
>> -
>> -	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
>> -	if (rc)
>> -		return rc;
>> +	struct device_node *prev;
>> +	struct device_node *prevsib;
>> 
>> -	raw_spin_lock_irqsave(&devtree_lock, flags);
>> -
>> -	if (of_node_check_flag(np, OF_DETACHED)) {
>> -		/* someone already detached it */
>> -		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> -		return rc;
>> -	}
>> +	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
>> +		return;
>> 
>> 	parent = np->parent;
>> -	if (!parent) {
>> -		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> -		return rc;
>> -	}
>> +	if (WARN_ON(!parent))
>> +		return;
>> 
>> 	if (of_allnodes == np)
>> 		of_allnodes = np->allnext;
>> 	else {
>> -		struct device_node *prev;
>> -
>> 		for (prev = of_allnodes;
>> 		     prev->allnext != np;
>> 		     prev = prev->allnext)
>> @@ -164,8 +150,6 @@ int of_detach_node(struct device_node *np)
>> 	if (parent->child == np)
>> 		parent->child = np->sibling;
>> 	else {
>> -		struct device_node *prevsib;
>> -
>> 		for (prevsib = np->parent->child;
>> 		     prevsib->sibling != np;
>> 		     prevsib = prevsib->sibling)
>> @@ -174,6 +158,25 @@ int of_detach_node(struct device_node *np)
>> 	}
>> 
>> 	of_node_set_flag(np, OF_DETACHED);
>> +}
>> +
>> +/**
>> + * of_detach_node - "Unplug" a node from the device tree.
>> + *
>> + * The caller must hold a reference to the node.  The memory associated with
>> + * the node is not freed until its refcount goes to zero.
>> + */
>> +int of_detach_node(struct device_node *np)
>> +{
>> +	unsigned long flags;
>> +	int rc = 0;
>> +
>> +	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
>> +	if (rc)
>> +		return rc;
>> +
>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	__of_detach_node(np);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>> 	of_node_remove(np);
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index a541b81..b2c25b5 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -59,4 +59,12 @@ static inline int of_reconfig_notify(unsigned long action, void *p)
>> }
>> #endif /* CONFIG_OF_DYNAMIC */
>> 
>> +extern int __of_add_property(struct device_node *np, struct property *prop);
>> +extern int __of_remove_property(struct device_node *np, struct property *prop);
>> +extern int __of_update_property(struct device_node *np,
>> +		struct property *newprop, struct property **oldprop);
>> +
>> +extern void __of_attach_node(struct device_node *np);
>> +extern void __of_detach_node(struct device_node *np);
>> +
>> #endif /* _LINUX_OF_PRIVATE_H */
>> -- 
>> 1.7.12
>> 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] of: Do not free memory at of_node_release option
  2014-07-07 13:13     ` Pantelis Antoniou
@ 2014-07-09 18:18       ` Grant Likely
  2014-07-11 20:29         ` Pantelis Antoniou
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2014-07-09 18:18 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

On Mon, 7 Jul 2014 16:13:42 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
> 
> On Jul 7, 2014, at 4:10 PM, Grant Likely wrote:
> 
> > On Fri,  4 Jul 2014 19:58:45 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> >> Introduce an option whether to free or not the memory of a node
> >> and it's properties when of_node_release is called.
> >> 
> >> The option is located in the chosen node, and its name is
> >> of-node-keep. When this boolean property is missing the default
> >> behaviour of freeing is preserved.
> > 
> > No way. The problem is buggy kernel code. We've got a plan to fix that.
> > In the mean time I'm okay with a CONFIG_ option that inhibits freeing,
> > or *maybe* a runtime global option that causes free() to not be called.
> > 
> > What this patch does is not helpful.
> > 
> 
> Well, it was a long shot. FWIW I don't need it for overlays, this is just to
> make sure we don't crash in case we have a buggy driver calling of_node_put()
> twice or something.
> 
> I'm open to suggestions on how to track and fix those.

Switching to RCU is the first step. That lets us convert users into
using rcu barriers instead of manually tracking of_node_get/put
ordering. I would also like to talk to Julie about getting coccinelle
report on node and property pointer references being held outside the
rcu barriers without an of_node_get()

> 
> > g.
> > 
> 
> Regards
> 
> -- Pantelis
> 
> >> 
> >> The rational behind this option is as follows:
> >> 
> >> The life-cycle of nodes and properties does not allow us to release
> >> the memory taken by a device_node. Pointer to properties and nodes
> >> might still be in use in drivers, so any memory free'ing is dangerous.
> >> 
> >> Simply move all the properties to the deadprops list, and the node
> >> itself to of_alldeadnodes until the life-cycles issues are resolved.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >> ---
> >> drivers/of/dynamic.c | 74 +++++++++++++++++++++++++++++++++++++++++-----------
> >> 1 file changed, 59 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> >> index a5d6f4f..0e8457c 100644
> >> --- a/drivers/of/dynamic.c
> >> +++ b/drivers/of/dynamic.c
> >> @@ -180,17 +180,29 @@ int of_detach_node(struct device_node *np)
> >> 	return rc;
> >> }
> >> 
> >> +static struct device_node *of_alldeadnodes;
> >> +static DEFINE_RAW_SPINLOCK(deadtree_lock);
> >> +
> >> /**
> >>  *	of_node_release - release a dynamically allocated node
> >>  *	@kref:  kref element of the node to be released
> >>  *
> >>  *	In of_node_put() this function is passed to kref_put()
> >>  *	as the destructor.
> >> + *
> >> + *	There is an option to not free the node, since due to the
> >> + *	way that node and property life-cycles are not completely
> >> + *	managed, we can't free the memory of a node at will.
> >> + *	Instead we move the node to the dead nodes list where it will
> >> + *	remain until the life-cycle issues are resolved.
> >>  */
> >> -static void of_node_release(struct kobject *kobj)
> >> +void of_node_release(struct kobject *kobj)
> >> {
> >> +	/*  we probe the of-node-keep chosen prop once */
> >> +	static int node_keep = -1;
> >> 	struct device_node *node = kobj_to_device_node(kobj);
> >> -	struct property *prop = node->properties;
> >> +	struct property *prop;
> >> +	unsigned long flags;
> >> 
> >> 	/* We should never be releasing nodes that haven't been detached. */
> >> 	if (!of_node_check_flag(node, OF_DETACHED)) {
> >> @@ -199,23 +211,55 @@ static void of_node_release(struct kobject *kobj)
> >> 		return;
> >> 	}
> >> 
> >> -	if (!of_node_check_flag(node, OF_DYNAMIC))
> >> +	/* we should not be trying to release the root */
> >> +	if (WARN_ON(node == of_allnodes))
> >> 		return;
> >> 
> >> -	while (prop) {
> >> -		struct property *next = prop->next;
> >> +	/* read the chosen boolean property "of-node-keep" once */
> >> +	if (node_keep == -1)
> >> +		node_keep = of_property_read_bool(of_chosen, "of-node-keep");
> >> 
> >> -		kfree(prop->name);
> >> -		kfree(prop->value);
> >> -		kfree(prop);
> >> -		prop = next;
> >> +	/* default is to free everything */
> >> +	if (!node_keep) {
> >> 
> >> -		if (!prop) {
> >> -			prop = node->deadprops;
> >> -			node->deadprops = NULL;
> >> +		/* free normal properties */
> >> +		while ((prop = node->properties) != NULL) {
> >> +			node->properties = prop->next;
> >> +			kfree(prop->name);
> >> +			kfree(prop->value);
> >> +			kfree(prop);
> >> 		}
> >> +
> >> +		/* free deap properties */
> >> +		while ((prop = node->deadprops) != NULL) {
> >> +			node->deadprops = prop->next;
> >> +			kfree(prop->name);
> >> +			kfree(prop->value);
> >> +			kfree(prop);
> >> +		}
> >> +
> >> +		/* free the node */
> >> +		kfree(node->full_name);
> >> +		kfree(node->data);
> >> +		kfree(node);
> >> +		return;
> >> 	}
> >> -	kfree(node->full_name);
> >> -	kfree(node->data);
> >> -	kfree(node);
> >> +
> >> +	pr_info("%s: dead node \"%s\"\n", __func__, node->full_name);
> >> +
> >> +	/* can't use devtree lock; at of_node_put caller might be holding it */
> >> +	raw_spin_lock_irqsave(&deadtree_lock, flags);
> >> +
> >> +	/* move all properties to dead properties */
> >> +	while ((prop = node->properties) != NULL) {
> >> +		node->properties = prop->next;
> >> +		prop->next = node->deadprops;
> >> +		node->deadprops = prop;
> >> +	}
> >> +
> >> +	/* move node to alldeadnodes */
> >> +	node->allnext = of_alldeadnodes;
> >> +	of_alldeadnodes = node;
> >> +
> >> +	raw_spin_unlock_irqrestore(&deadtree_lock, flags);
> >> }
> >> -- 
> >> 1.7.12
> >> 
> > 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] OF: Export a number of __of_* methods
  2014-07-07 13:17     ` Pantelis Antoniou
@ 2014-07-09 18:58       ` Grant Likely
  0 siblings, 0 replies; 24+ messages in thread
From: Grant Likely @ 2014-07-09 18:58 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

On Mon, 7 Jul 2014 16:17:58 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
> 
> On Jul 7, 2014, at 4:11 PM, Grant Likely wrote:
> 
> > On Fri,  4 Jul 2014 19:58:46 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> >> As part of moving to transactional DT, a number of methods are
> >> reworked and exported.
> >> 
> >> The methods exported are:
> >> __of_add_property, __of_remove_property, __of_update_property, __of_attach_node,
> >> __of_detach_node
> > 
> > 
> > ??? The description doesn't match the patch. Nothing is getting
> > exported, and the commit message should really say why and how the
> > methods are being reworked.
> > 
> 
> Err, are they not exported out of their file scope?

Talking about exporting typically means EXPORT_SYMBOL(). Hence my
question.

> static int foo -> int foo?
> 
> The _post methods are 'exported' in the same manner.
> 
> The explanation is included in the next patch in the series, copying verbatim:
> 
> > As part of transactional DT support there needs to be a clearer
> > definition of how each dynamic tree change method works.
> > 
> > Let's take for example the of_add_property method. There are
> > discrete steps taken in sequence.
> > 
> > 1. The OF_RECONFIG_ADD_PROPERTY notifier will be fired.
> > 2. The devtree lock will be taken.
> > 3. Insertion of the property in the live tree.
> > 4. The devtree lock will be released.
> > 5. The property will be inserted in the sysfs tree.
> > 
> > For each of the of_attach_node, of_detach_node, of_add_property,
> > of_remove_property and of_update_property methods we export (and
> > introduce if not available), their counterparts of
> > __of_attach_node, __of_attach_node_post, __of_detach_node,
> > __of_detach_node_post, __of_add_property, __of_add_property_post,
> > __of_remove_property, __of_remove_property_post, __of_update_property
> > and __of_update_property post.
> > 
> > So each of the tree modification methods is following the same
> > pattern, i.e. for method foo we have:
> > 
> > Firing of the notifer, lock, __foo, unlock, __foo_post
> > 
> > This breakdown is required when we introduce transaction.
> 
> Regards
> 
> -- Pantelis
> 
> > g.
> > 
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >> ---
> >> drivers/of/base.c       | 97 +++++++++++++++++++++++++++++--------------------
> >> drivers/of/dynamic.c    | 71 +++++++++++++++++++-----------------
> >> drivers/of/of_private.h |  8 ++++
> >> 3 files changed, 102 insertions(+), 74 deletions(-)
> >> 
> >> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >> index be0ec37..0d50318 100644
> >> --- a/drivers/of/base.c
> >> +++ b/drivers/of/base.c
> >> @@ -1659,7 +1659,7 @@ EXPORT_SYMBOL(of_count_phandle_with_args);
> >> /**
> >>  * __of_add_property - Add a property to a node without lock operations
> >>  */
> >> -static int __of_add_property(struct device_node *np, struct property *prop)
> >> +int __of_add_property(struct device_node *np, struct property *prop)
> >> {
> >> 	struct property **next;
> >> 
> >> @@ -1701,6 +1701,25 @@ int of_add_property(struct device_node *np, struct property *prop)
> >> 	return rc;
> >> }
> >> 
> >> +int __of_remove_property(struct device_node *np, struct property *prop)
> >> +{
> >> +	struct property **next;
> >> +
> >> +	for (next = &np->properties; *next; next = &(*next)->next) {
> >> +		if (*next == prop)
> >> +			break;
> >> +	}
> >> +	if (*next == NULL)
> >> +		return -ENODEV;
> >> +
> >> +	/* found the node */
> >> +	*next = prop->next;
> >> +	prop->next = np->deadprops;
> >> +	np->deadprops = prop;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> /**
> >>  * of_remove_property - Remove a property from a node.
> >>  *
> >> @@ -1711,9 +1730,7 @@ int of_add_property(struct device_node *np, struct property *prop)
> >>  */
> >> int of_remove_property(struct device_node *np, struct property *prop)
> >> {
> >> -	struct property **next;
> >> 	unsigned long flags;
> >> -	int found = 0;
> >> 	int rc;
> >> 
> >> 	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> >> @@ -1721,22 +1738,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
> >> 		return rc;
> >> 
> >> 	raw_spin_lock_irqsave(&devtree_lock, flags);
> >> -	next = &np->properties;
> >> -	while (*next) {
> >> -		if (*next == prop) {
> >> -			/* found the node */
> >> -			*next = prop->next;
> >> -			prop->next = np->deadprops;
> >> -			np->deadprops = prop;
> >> -			found = 1;
> >> -			break;
> >> -		}
> >> -		next = &(*next)->next;
> >> -	}
> >> +	rc = __of_remove_property(np, prop);
> >> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >> 
> >> -	if (!found)
> >> -		return -ENODEV;
> >> +	if (rc)
> >> +		return rc;
> >> 
> >> 	/* at early boot, bail hear and defer setup to of_init() */
> >> 	if (!of_kset)
> >> @@ -1747,6 +1753,32 @@ int of_remove_property(struct device_node *np, struct property *prop)
> >> 	return 0;
> >> }
> >> 
> >> +int __of_update_property(struct device_node *np, struct property *newprop,
> >> +		struct property **oldpropp)
> >> +{
> >> +	struct property **next, *oldprop;
> >> +
> >> +	for (next = &np->properties; *next; next = &(*next)->next) {
> >> +		if (of_prop_cmp((*next)->name, newprop->name) == 0)
> >> +			break;
> >> +	}
> >> +	*oldpropp = oldprop = *next;
> >> +
> >> +	if (oldprop) {
> >> +		/* replace the node */
> >> +		newprop->next = oldprop->next;
> >> +		*next = newprop;
> >> +		oldprop->next = np->deadprops;
> >> +		np->deadprops = oldprop;
> >> +	} else {
> >> +		/* new node */
> >> +		newprop->next = NULL;
> >> +		*next = newprop;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> /*
> >>  * of_update_property - Update a property in a node, if the property does
> >>  * not exist, add it.
> >> @@ -1756,36 +1788,21 @@ int of_remove_property(struct device_node *np, struct property *prop)
> >>  * Instead we just move the property to the "dead properties" list,
> >>  * and add the new property to the property list
> >>  */
> >> -int of_update_property(struct device_node *np, struct property *newprop)
> >> +int of_update_property(struct device_node *np, struct property *prop)
> >> {
> >> -	struct property **next, *oldprop;
> >> +	struct property *oldprop;
> >> 	unsigned long flags;
> >> 	int rc;
> >> 
> >> -	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> >> +	if (!prop->name)
> >> +		return -EINVAL;
> >> +
> >> +	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, prop);
> >> 	if (rc)
> >> 		return rc;
> >> 
> >> -	if (!newprop->name)
> >> -		return -EINVAL;
> >> -
> >> 	raw_spin_lock_irqsave(&devtree_lock, flags);
> >> -	next = &np->properties;
> >> -	oldprop = __of_find_property(np, newprop->name, NULL);
> >> -	if (!oldprop) {
> >> -		/* add the new node */
> >> -		rc = __of_add_property(np, newprop);
> >> -	} else while (*next) {
> >> -		/* replace the node */
> >> -		if (*next == oldprop) {
> >> -			newprop->next = oldprop->next;
> >> -			*next = newprop;
> >> -			oldprop->next = np->deadprops;
> >> -			np->deadprops = oldprop;
> >> -			break;
> >> -		}
> >> -		next = &(*next)->next;
> >> -	}
> >> +	rc = __of_update_property(np, prop, &oldprop);
> >> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >> 	if (rc)
> >> 		return rc;
> >> @@ -1797,7 +1814,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
> >> 	/* Update the sysfs attribute */
> >> 	if (oldprop)
> >> 		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> >> -	__of_add_property_sysfs(np, newprop);
> >> +	__of_add_property_sysfs(np, prop);
> >> 
> >> 	return 0;
> >> }
> >> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> >> index 0e8457c..eb1126f 100644
> >> --- a/drivers/of/dynamic.c
> >> +++ b/drivers/of/dynamic.c
> >> @@ -95,8 +95,17 @@ int of_property_notify(int action, struct device_node *np,
> >> 	return of_reconfig_notify(action, &pr);
> >> }
> >> 
> >> +void __of_attach_node(struct device_node *np)
> >> +{
> >> +	np->sibling = np->parent->child;
> >> +	np->allnext = np->parent->allnext;
> >> +	np->parent->allnext = np;
> >> +	np->parent->child = np;
> >> +	of_node_clear_flag(np, OF_DETACHED);
> >> +}
> >> +
> >> /**
> >> - * of_attach_node() - Plug a device node into the tree and global list.
> >> + * of_attach_node - Plug a device node into the tree and global list.
> >>  */
> >> int of_attach_node(struct device_node *np)
> >> {
> >> @@ -108,52 +117,29 @@ int of_attach_node(struct device_node *np)
> >> 		return rc;
> >> 
> >> 	raw_spin_lock_irqsave(&devtree_lock, flags);
> >> -	np->sibling = np->parent->child;
> >> -	np->allnext = of_allnodes;
> >> -	np->parent->child = np;
> >> -	of_allnodes = np;
> >> -	of_node_clear_flag(np, OF_DETACHED);
> >> +	__of_attach_node(np);
> >> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >> 
> >> 	of_node_add(np);
> >> 	return 0;
> >> }
> >> 
> >> -/**
> >> - * of_detach_node() - "Unplug" a node from the device tree.
> >> - *
> >> - * The caller must hold a reference to the node.  The memory associated with
> >> - * the node is not freed until its refcount goes to zero.
> >> - */
> >> -int of_detach_node(struct device_node *np)
> >> +void __of_detach_node(struct device_node *np)
> >> {
> >> 	struct device_node *parent;
> >> -	unsigned long flags;
> >> -	int rc = 0;
> >> -
> >> -	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> >> -	if (rc)
> >> -		return rc;
> >> +	struct device_node *prev;
> >> +	struct device_node *prevsib;
> >> 
> >> -	raw_spin_lock_irqsave(&devtree_lock, flags);
> >> -
> >> -	if (of_node_check_flag(np, OF_DETACHED)) {
> >> -		/* someone already detached it */
> >> -		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >> -		return rc;
> >> -	}
> >> +	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
> >> +		return;
> >> 
> >> 	parent = np->parent;
> >> -	if (!parent) {
> >> -		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >> -		return rc;
> >> -	}
> >> +	if (WARN_ON(!parent))
> >> +		return;
> >> 
> >> 	if (of_allnodes == np)
> >> 		of_allnodes = np->allnext;
> >> 	else {
> >> -		struct device_node *prev;
> >> -
> >> 		for (prev = of_allnodes;
> >> 		     prev->allnext != np;
> >> 		     prev = prev->allnext)
> >> @@ -164,8 +150,6 @@ int of_detach_node(struct device_node *np)
> >> 	if (parent->child == np)
> >> 		parent->child = np->sibling;
> >> 	else {
> >> -		struct device_node *prevsib;
> >> -
> >> 		for (prevsib = np->parent->child;
> >> 		     prevsib->sibling != np;
> >> 		     prevsib = prevsib->sibling)
> >> @@ -174,6 +158,25 @@ int of_detach_node(struct device_node *np)
> >> 	}
> >> 
> >> 	of_node_set_flag(np, OF_DETACHED);
> >> +}
> >> +
> >> +/**
> >> + * of_detach_node - "Unplug" a node from the device tree.
> >> + *
> >> + * The caller must hold a reference to the node.  The memory associated with
> >> + * the node is not freed until its refcount goes to zero.
> >> + */
> >> +int of_detach_node(struct device_node *np)
> >> +{
> >> +	unsigned long flags;
> >> +	int rc = 0;
> >> +
> >> +	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> >> +	__of_detach_node(np);
> >> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >> 
> >> 	of_node_remove(np);
> >> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> >> index a541b81..b2c25b5 100644
> >> --- a/drivers/of/of_private.h
> >> +++ b/drivers/of/of_private.h
> >> @@ -59,4 +59,12 @@ static inline int of_reconfig_notify(unsigned long action, void *p)
> >> }
> >> #endif /* CONFIG_OF_DYNAMIC */
> >> 
> >> +extern int __of_add_property(struct device_node *np, struct property *prop);
> >> +extern int __of_remove_property(struct device_node *np, struct property *prop);
> >> +extern int __of_update_property(struct device_node *np,
> >> +		struct property *newprop, struct property **oldprop);
> >> +
> >> +extern void __of_attach_node(struct device_node *np);
> >> +extern void __of_detach_node(struct device_node *np);
> >> +
> >> #endif /* _LINUX_OF_PRIVATE_H */
> >> -- 
> >> 1.7.12
> >> 
> > 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] of: Introduce tree change __foo_post methods.
  2014-07-04 16:58 ` [PATCH 4/5] of: Introduce tree change __foo_post methods Pantelis Antoniou
@ 2014-07-10  0:55   ` Grant Likely
  2014-07-11 20:04     ` Pantelis Antoniou
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2014-07-10  0:55 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou

On Fri,  4 Jul 2014 19:58:48 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> As part of transactional DT support there needs to be a clearer
> definition of how each dynamic tree change method works.
> 
> Let's take for example the of_add_property method. There are
> discrete steps taken in sequence.
> 
> 1. The OF_RECONFIG_ADD_PROPERTY notifier will be fired.
> 2. The devtree lock will be taken.
> 3. Insertion of the property in the live tree.
> 4. The devtree lock will be released.
> 5. The property will be inserted in the sysfs tree.
> 
> For each of the of_attach_node, of_detach_node, of_add_property,
> of_remove_property and of_update_property methods we export (and
> introduce if not available), their counterparts of
> __of_attach_node, __of_attach_node_post, __of_detach_node,
> __of_detach_node_post, __of_add_property, __of_add_property_post,
> __of_remove_property, __of_remove_property_post, __of_update_property
> and __of_update_property post.

We actually already have some code in this direction, but it uses _sysfs
as the suffix instead of _post. I've got a modified version of this
patch in my tree now that eliminates some of the duplication in this
patch and uses the _sysfs name. I'm going to play around with it a bit
before sending it to you.

The locking is also a mess. of_mutex needs to be held I think whenever
changes are being made. In the version I have, I've pulled in the
of_mutex cleanups from patch 5 and also put in a few of my own.

Comments below...

> 
> So each of the tree modification methods is following the same
> pattern, i.e. for method foo we have:
> 
> Firing of the notifer, lock, __foo, unlock, __foo_post
> 
> This breakdown is required when we introduce transaction.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/base.c       | 60 +++++++++++++++++++++++++++++++++++--------------
>  drivers/of/dynamic.c    | 14 ++++++++++--
>  drivers/of/of_private.h |  8 +++++++
>  3 files changed, 63 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 0d50318..b52fd6b 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -127,13 +127,24 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
>  	return name;
>  }
>  
> -static int __of_add_property_sysfs(struct device_node *np, struct property *pp)
> +static int __of_add_property_sysfs(struct device_node *np, struct property *pp,
> +		int ignore_dup_name)
>  {
>  	int rc;
>  
>  	/* Important: Don't leak passwords */
>  	bool secure = strncmp(pp->name, "security-", 9) == 0;
>  
> +	/* ignore duplicate name (transaction case) */
> +	if (ignore_dup_name) {
> +		struct kernfs_node *kn = sysfs_get_dirent(np->kobj.sd,
> +				pp->name);
> +		if (kn) {
> +			sysfs_put(kn);
> +			return 0;
> +		}
> +	}
> +

What is this? There is no mention of it in the commit text, and the comment
is far from sufficient. Under what conditions would this be valid? I'm
particularly concerned because this changes behaviour and the new
behaviour is used in the next hunk, so clearly it's not
transaction-only?

I've dropped this hunk from the version I have in my tree.

>  	sysfs_bin_attr_init(&pp->attr);
>  	pp->attr.attr.name = safe_name(&np->kobj, pp->name);
>  	pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
> @@ -167,7 +178,7 @@ static int __of_node_add(struct device_node *np)
>  		return rc;
>  
>  	for_each_property_of_node(np, pp)
> -		__of_add_property_sysfs(np, pp);
> +		__of_add_property_sysfs(np, pp, 1);
>  
>  	return 0;
>  }
> @@ -1677,6 +1688,13 @@ int __of_add_property(struct device_node *np, struct property *prop)
>  	return 0;
>  }
>  
> +void __of_add_property_post(struct device_node *np, struct property *prop,
> +		int ignore_dup_name)
> +{
> +	if (of_node_is_attached(np))
> +		__of_add_property_sysfs(np, prop, ignore_dup_name);
> +}
> +

of_node_is_attached() call could just be rolled into
__of_add_property_sysfs(). There aren't any other users, and the test is
alwasy valid for these functions.

>  /**
>   * of_add_property - Add a property to a node
>   */
> @@ -1695,8 +1713,7 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	if (rc)
>  		return rc;
>  
> -	if (of_node_is_attached(np))
> -		__of_add_property_sysfs(np, prop);
> +	__of_add_property_post(np, prop, 0);
>  
>  	return rc;
>  }
> @@ -1720,6 +1737,13 @@ int __of_remove_property(struct device_node *np, struct property *prop)
>  	return 0;
>  }
>  
> +void __of_remove_property_post(struct device_node *np, struct property *prop)
> +{
> +	/* at early boot, bail here and defer setup to of_init() */
> +	if (of_kset)
> +		sysfs_remove_bin_file(&np->kobj, &prop->attr);
> +}
> +
>  /**
>   * of_remove_property - Remove a property from a node.
>   *
> @@ -1744,11 +1768,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	if (rc)
>  		return rc;
>  
> -	/* at early boot, bail hear and defer setup to of_init() */
> -	if (!of_kset)
> -		return 0;
> -
> -	sysfs_remove_bin_file(&np->kobj, &prop->attr);
> +	__of_remove_property_post(np, prop);
>  
>  	return 0;
>  }
> @@ -1779,6 +1799,19 @@ int __of_update_property(struct device_node *np, struct property *newprop,
>  	return 0;
>  }
>  
> +void __of_update_property_post(struct device_node *np, struct property *newprop,
> +		struct property *oldprop)
> +{
> +	/* At early boot, bail out and defer setup to of_init() */
> +	if (!of_kset)
> +		return;
> +
> +	/* Update the sysfs attribute */
> +	if (oldprop)
> +		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> +	__of_add_property_sysfs(np, newprop, 0);
> +}
> +
>  /*
>   * of_update_property - Update a property in a node, if the property does
>   * not exist, add it.
> @@ -1807,14 +1840,7 @@ int of_update_property(struct device_node *np, struct property *prop)
>  	if (rc)
>  		return rc;
>  
> -	/* At early boot, bail out and defer setup to of_init() */
> -	if (!of_kset)
> -		return 0;
> -
> -	/* Update the sysfs attribute */
> -	if (oldprop)
> -		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> -	__of_add_property_sysfs(np, prop);
> +	__of_update_property_post(np, prop, oldprop);
>  
>  	return 0;
>  }
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 90c09b6..a995de3 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -104,6 +104,11 @@ void __of_attach_node(struct device_node *np)
>  	of_node_clear_flag(np, OF_DETACHED);
>  }
>  
> +void __of_attach_node_post(struct device_node *np)
> +{
> +	of_node_add(np);
> +}

There is only one user of of_node_add(), so this is kind of silly. I've
actually removed of_node_add() entirely now and have renamed
__of_node_add to __of_attach_node_sysfs()

> +
>  /**
>   * of_attach_node - Plug a device node into the tree and global list.
>   */
> @@ -120,7 +125,7 @@ int of_attach_node(struct device_node *np)
>  	__of_attach_node(np);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	of_node_add(np);
> +	__of_attach_node_post(np);
>  	return 0;
>  }
>  
> @@ -160,6 +165,11 @@ void __of_detach_node(struct device_node *np)
>  	of_node_set_flag(np, OF_DETACHED);
>  }
>  
> +void __of_detach_node_post(struct device_node *np)
> +{
> +	of_node_remove(np);
> +}

Also silly.

> +
>  /**
>   * of_detach_node - "Unplug" a node from the device tree.
>   *
> @@ -179,7 +189,7 @@ int of_detach_node(struct device_node *np)
>  	__of_detach_node(np);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	of_node_remove(np);
> +	__of_detach_node_post(np);
>  	return rc;
>  }
>  
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index e544247..ca95100 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -60,12 +60,20 @@ static inline int of_reconfig_notify(unsigned long action, void *p)
>  #endif /* CONFIG_OF_DYNAMIC */
>  
>  extern int __of_add_property(struct device_node *np, struct property *prop);
> +extern void __of_add_property_post(struct device_node *np,
> +		struct property *prop, int ignore_dup_name);
>  extern int __of_remove_property(struct device_node *np, struct property *prop);
> +extern void __of_remove_property_post(struct device_node *np,
> +		struct property *prop);
>  extern int __of_update_property(struct device_node *np,
>  		struct property *newprop, struct property **oldprop);
> +extern void __of_update_property_post(struct device_node *np,
> +		struct property *newprop, struct property *oldprop);
>  
>  extern void __of_attach_node(struct device_node *np);
> +extern void __of_attach_node_post(struct device_node *np);
>  extern void __of_detach_node(struct device_node *np);
> +extern void __of_detach_node_post(struct device_node *np);
>  
>  /**
>   * General utilities for working with live trees.
> -- 
> 1.7.12
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] of: Transactional DT support.
  2014-07-04 16:58 ` [PATCH 5/5] of: Transactional DT support Pantelis Antoniou
@ 2014-07-11  5:24   ` Grant Likely
  2014-07-11 20:28     ` Pantelis Antoniou
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2014-07-11  5:24 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev, Pantelis Antoniou

On Fri,  4 Jul 2014 19:58:49 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Introducing DT transactional support.
> 
> A DT transaction is a method which allows one to apply changes
> in the live tree, in such a way that either the full set of changes
> take effect, or the state of the tree can be rolled-back to the
> state it was before it was attempted. An applied transaction
> can be rolled-back at any time.
> 
> If any transaction results in the change of a state of a device
> node, a notifier for that node will be fired which allows a bus
> to create/destroy devices as directed.
> 
> Documentation is in
> 	Documentation/devicetree/transactions.txt

Hi Pantelis,

I've got a lot of comments below, but you don't need to respin the
series immediately. I've got a tree that I'm working with to sort out
what I can queue up for the next kernel. Give me a few days and I'll
send you what I've gotten to.

> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  Documentation/devicetree/transactions.txt |  41 ++

I've been thinking about this for a while, 'transaction' doesn't strike me as the correct term. We aren't allowing the callers to see any intermediate state, and so there isn't really a concept of committing or reverting. As far as the caller is concerned, it provides a set of modifications, and they either get completely applied, or not at all.

Changeset is a better term.

>  drivers/of/Makefile                       |   2 +-
>  drivers/of/base.c                         |  34 +-
>  drivers/of/of_private.h                   |  19 +
>  drivers/of/transaction.c                  | 699 ++++++++++++++++++++++++++++++

drivers/of/dynamic.c. This is really important code. This code shouldn't
merely be one way to modify the device tree. Ultimately it should be the
*only* way to modify the tree. Once it is merged I want to convert the
powerpc users over to use the new interface.

>  include/linux/of.h                        |  82 ++++
>  6 files changed, 864 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/devicetree/transactions.txt
>  create mode 100644 drivers/of/transaction.c
> 
> diff --git a/Documentation/devicetree/transactions.txt b/Documentation/devicetree/transactions.txt
> new file mode 100644
> index 0000000..59ff5b7
> --- /dev/null
> +++ b/Documentation/devicetree/transactions.txt
> @@ -0,0 +1,41 @@
> +A DT transaction is a method which allows one to apply changes
> +in the live tree, in such a way that either the full set of changes
> +take effect, or the state of the tree can be rolled-back to the
> +state it was before it was attempted. An applied transaction
> +can be rolled-back at any time.
> +
> +If any transaction results in the change of a state of a device
> +node, a notifier for that node will be fired which allows a bus
> +to create/destroy devices as directed.
> +
> +The sequence of a transaction is as follows.
> +
> +1. of_transaction_init() - initializes a transaction
> +
> +2. of_transaction_start() - starts a transaction; a global
> +transaction lock is taken at this point and disallowes any other
> +dynamic tree changes.

Instead of of_transaction_start() the caller should be told to
explicitly take the of_mutex() so that it becomes the only editor.

> +3. A number of DT tree change calls, of_transaction_attach_node(),
> +of_transaction_detach_node(), of_transaction_add_property(),
> +of_transaction_remove_property, of_transaction_update_property()
> +modify the live tree (but without any user-space visible changes).
> +All changes are recorded in the list of of_transaction_entry of
> +the transaction.
> +
> +4.a. No errors occured during application, so a call to
> +of_transaction_commit() is issued. All the changes are made visible
> +to user-space and all device creation/destruction notifiers are
> +fired. The global transaction is released.
> +
> +4.b. An error occured, so a of_transaction_abort() call is issued.
> +All changes to the live tree will be reverted. The global transaction
> +lock will be released.

The above isn't accurate anymore. of_changeset_apply() should either
completely work or completely fail. There should not be an _abort()
call at all.

I would make it the responsibility of the caller to release the of_mutex
lock for consistency. of_changeset_apply() should require the of_mutex
to be held and it should drop/reaquire the lock for just the period when
it sends notifiers.

> +
> +5.a If the transaction needs not be reverted, a call to
> +of_transaction_destroy will release the resources of the transaction.

of_changeset_free() would probably be a better name.

> +
> +5.b If the transaction is to be reverted, a call to
> +of_transaction_revert will do so. All device notifiers will be
> +fired, and the state of the tree will be the same as before
> +the call to of_start_transaction().
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 08e6c0f..9a68cc9 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,5 +1,5 @@
>  obj-y = base.o device.o platform.o
> -obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> +obj-$(CONFIG_OF_DYNAMIC) += dynamic.o transaction.o
>  obj-$(CONFIG_OF_FLATTREE) += fdt.o
>  obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
>  obj-$(CONFIG_OF_PROMTREE) += pdt.o
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index b52fd6b..206af65 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -40,7 +40,9 @@ static struct device_node *of_stdout;
>  static struct kset *of_kset;
>  
>  /*
> - * Used to protect the of_aliases, to hold off addition of nodes to sysfs
> + * Used to protect the of_aliases, to hold off addition of
> + * nodes to sysfs and in the case of a transaction is in
> + * progress.
>   */
>  DEFINE_MUTEX(of_mutex);
>  
> @@ -1707,13 +1709,16 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	if (rc)
>  		return rc;
>  
> +	mutex_lock(&of_mutex);
> +

These mutex hunks I've moved to the earlier patch that makes the _sysfs
variants consistent in the version I've got in my tree.

>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	rc = __of_add_property(np, prop);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> -	if (rc)
> -		return rc;
>  
> -	__of_add_property_post(np, prop, 0);
> +	if (rc == 0)
> +		__of_add_property_post(np, prop, 0);
> +
> +	mutex_unlock(&of_mutex);
>  
>  	return rc;
>  }
> @@ -1761,16 +1766,18 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	if (rc)
>  		return rc;
>  
> +	mutex_lock(&of_mutex);
> +
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	rc = __of_remove_property(np, prop);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	if (rc)
> -		return rc;
> +	if (rc == 0)
> +		__of_remove_property_post(np, prop);
>  
> -	__of_remove_property_post(np, prop);
> +	mutex_unlock(&of_mutex);
>  
> -	return 0;
> +	return rc;
>  }
>  
>  int __of_update_property(struct device_node *np, struct property *newprop,
> @@ -1834,15 +1841,18 @@ int of_update_property(struct device_node *np, struct property *prop)
>  	if (rc)
>  		return rc;
>  
> +	mutex_lock(&of_mutex);
> +
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	rc = __of_update_property(np, prop, &oldprop);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> -	if (rc)
> -		return rc;
>  
> -	__of_update_property_post(np, prop, oldprop);
> +	if (rc == 0)
> +		__of_update_property_post(np, prop, oldprop);
>  
> -	return 0;
> +	mutex_unlock(&of_mutex);
> +
> +	return rc;
>  }
>  
>  static void of_alias_add(struct alias_prop *ap, struct device_node *np,
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index ca95100..00c119a 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -89,4 +89,23 @@ struct device_node *__of_create_empty_node(const char *name,
>  		const char *type, const char *full_name,
>  		phandle phandle, gfp_t allocflags, unsigned long nodeflags);
>  
> +/* iterators for transactions, used for overlays */
> +/* forward iterator */
> +#define for_each_transaction_entry(_oft, _te) \
> +	list_for_each_entry(_te, &(_oft)->te_list, node)
> +
> +/* reverse iterator */
> +#define for_each_transaction_entry_reverse(_oft, _te) \
> +	list_for_each_entry_reverse(_te, &(_oft)->te_list, node)
> +
> +/* special find property method for use by transaction users */
> +extern struct property *of_transaction_find_property(
> +		struct of_transaction *oft,
> +		const struct device_node *np, const char *name, int *lenp);
> +extern int of_transaction_device_is_available(struct of_transaction *oft,
> +		const struct device_node *np);
> +extern struct device_node *of_transaction_get_child_by_name(
> +		struct of_transaction *oft, struct device_node *node,
> +		const char *name);
> +
>  #endif /* _LINUX_OF_PRIVATE_H */
> diff --git a/drivers/of/transaction.c b/drivers/of/transaction.c
> new file mode 100644
> index 0000000..1094395
> --- /dev/null
> +++ b/drivers/of/transaction.c
> @@ -0,0 +1,699 @@
> +/*
> + * Functions for DT transactions
> + *
> + * Copyright (C) 2014 Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + */
> +#undef DEBUG
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/string.h>
> +#include <linux/ctype.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +
> +#include "of_private.h"
> +
> +static struct of_transaction_entry *__of_transaction_entry_create(
> +		struct of_transaction *oft, unsigned long action,
> +		struct device_node *dn, struct property *prop)
> +{
> +	struct of_transaction_entry *te;
> +
> +	te = kzalloc(sizeof(*te), GFP_KERNEL);
> +	if (te == NULL) {
> +		pr_err("%s: Failed to allocate\n", __func__);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	/* get a reference to the node */
> +	te->action = action;
> +	te->np = of_node_get(dn);
> +	te->prop = prop;
> +
> +	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
> +		te->old_prop = of_transaction_find_property(oft, dn,
> +				prop->name, NULL);
> +
> +	return te;
> +}

There is only one user of this function: of_transaction_action(). Roll
it all into one function.

> +
> +static void __of_transaction_entry_destroy(struct of_transaction_entry *te)
> +{
> +	of_node_put(te->np);
> +	list_del(&te->node);
> +	kfree(te);
> +}
> +
> +#ifdef DEBUG
> +static void __of_transaction_entry_dump(struct of_transaction_entry *te)
> +{
> +	switch (te->action) {
> +	case OF_RECONFIG_ADD_PROPERTY:
> +		pr_debug("%p: %s %s/%s\n",
> +			te, "ADD_PROPERTY   ", te->np->full_name,
> +			te->prop->name);
> +		break;
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +		pr_debug("%p: %s %s/%s\n",
> +			te, "REMOVE_PROPERTY", te->np->full_name,
> +			te->prop->name);
> +		break;
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		pr_debug("%p: %s %s/%s\n",
> +			te, "UPDATE_PROPERTY", te->np->full_name,
> +			te->prop->name);
> +		break;
> +	case OF_RECONFIG_ATTACH_NODE:
> +		pr_debug("%p: %s %s\n",
> +			te, "ATTACH_NODE    ", te->np->full_name);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		pr_debug("%p: %s %s\n",
> +			te, "DETACH_NODE    ", te->np->full_name);
> +		break;
> +	}
> +}
> +#else
> +static inline void __of_transaction_entry_dump(struct of_transaction_entry *te)
> +{
> +	/* empty */
> +}
> +#endif
> +
> +static int __of_transaction_entry_device_state(struct of_transaction *oft,
> +		struct of_transaction_entry *te, int revert)
> +{
> +	int curr, target;
> +	unsigned long action;
> +	struct property *prop;
> +
> +	/* filter */
> +	if (te->action != OF_RECONFIG_ADD_PROPERTY &&
> +	    te->action != OF_RECONFIG_REMOVE_PROPERTY &&
> +	    te->action != OF_RECONFIG_UPDATE_PROPERTY &&
> +	    te->action != OF_RECONFIG_ATTACH_NODE &&
> +	    te->action != OF_RECONFIG_ATTACH_NODE)
> +		return -1;
> +
> +	action = te->action;
> +	prop = te->prop;
> +
> +	/* on revert convert to the opposite */
> +	if (revert) {
> +		if (action == OF_RECONFIG_ADD_PROPERTY)
> +			action = OF_RECONFIG_REMOVE_PROPERTY;
> +		else if (action == OF_RECONFIG_REMOVE_PROPERTY)
> +			action = OF_RECONFIG_ADD_PROPERTY;
> +		else if (action == OF_RECONFIG_UPDATE_PROPERTY)
> +			prop = te->old_prop;
> +		else if (action == OF_RECONFIG_ATTACH_NODE)
> +			action = OF_RECONFIG_DETACH_NODE;
> +		else
> +			action = OF_RECONFIG_ATTACH_NODE;
> +	}
> +
> +	/* we only support state changes on "status" property change */
> +	if ((te->action == OF_RECONFIG_ADD_PROPERTY ||
> +	    te->action == OF_RECONFIG_REMOVE_PROPERTY ||
> +	    te->action == OF_RECONFIG_UPDATE_PROPERTY) &&
> +		of_prop_cmp(prop->name, "status"))
> +		return -1;
> +
> +	/* note that we don't use of_transaction_find_property() */
> +
> +	/* current device state */
> +	curr = of_device_is_available(te->np) &&
> +		of_find_property(te->np, "compatible", NULL) &&
> +		of_find_property(te->np, "status", NULL);
> +
> +	switch (action) {
> +	case OF_RECONFIG_ADD_PROPERTY:
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +
> +		/* target device state */
> +		if (action == OF_RECONFIG_ADD_PROPERTY ||
> +		    action == OF_RECONFIG_UPDATE_PROPERTY)
> +			target = !strcmp(prop->value, "okay") ||
> +				!strcmp(prop->value, "ok");
> +		else
> +			target = 0;	/* NOTE: status removal -> disabled */
> +
> +		break;
> +	case OF_RECONFIG_ATTACH_NODE:
> +		target = curr;
> +		curr = 0;
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		target = 0;
> +		break;
> +	}
> +
> +	return curr != target ? target : -1;
> +}

I've completely dropped the above function from the version I've got in
my tree. Adding changesets is not dependent on device state notifiers.
It really must be in a separate patch.

This patch should only add the functionality of changesets within the
context of the way tree changes currently work. Adding a new notifier is
a separate feature. Besides, if you're adding a new notifier, then you
need to make sure the old functions also emit the new notifier under
similar conditions. It's just best to keep that as a separate change.

(And I'm yet to be convinced that device_state notifiers are needed)

> +
> +static int __of_transaction_entry_apply(struct of_transaction *oft,
> +		struct of_transaction_entry *te)
> +{
> +	struct property *old_prop;
> +	unsigned long flags;
> +	int ret, state;
> +
> +	ret = 0;
> +
> +	state = __of_transaction_entry_device_state(oft, te, 0);
> +
> +	switch (te->action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +	case OF_RECONFIG_DETACH_NODE:
> +		ret = of_reconfig_notify(te->action, te->np);
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		ret = of_property_notify(te->action, te->np, te->prop);
> +		break;
> +	}

The way I read this, the notifier is sent as each entry is applied. So
the sequence would be:
	entry 1 notify
	entry 1 apply
	entry 2 notify
	entry 2 apply
	etc.
Correct?

For this patch, that is probably exactly what should happen because it
matches the existing behaviour. However, it isn't what we want. We need
a seperate patch to reorganize the notifiers to be emitted after the
change takes place as we discussed on IRC, and powerpc needs to be moved
to the new model. At the same time, all of the notifiers need to be
emitted together instead of interleaved with the individual entries so
that the order looks like this:
	entry 1 apply
	entry 2 apply
	etc.
	entry 1 notify
	entry 2 notify
	etc.

> +
> +	if (ret) {
> +		pr_err("%s: notifier error @%s\n", __func__,
> +				te->np->full_name);
> +		return ret;
> +	}
> +
> +	mutex_lock(&of_mutex);

This doesn't look right. The mutex need to be held over the entire time
of applying changes. It cannot be dropped between entries of the
changeset. ie:

	mutex_lock()
	entry 1 apply
	entry 2 apply
	etc.
	mutex_unlock()
	entry 1 notify
	entry 2 notify
	etc.

If the mutex gets obtained and droped for each entry, then there is no
protection against another thread making a modification in the middle.

> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	switch (te->action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		__of_attach_node(te->np);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		__of_detach_node(te->np);
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +		ret = __of_add_property(te->np, te->prop);
> +		if (ret) {
> +			pr_err("%s: add_property failed @%s/%s\n",
> +				__func__, te->np->full_name,
> +				te->prop->name);
> +			break;
> +		}
> +		break;
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +		ret = __of_remove_property(te->np, te->prop);
> +		if (ret) {
> +			pr_err("%s: remove_property failed @%s/%s\n",
> +				__func__, te->np->full_name,
> +				te->prop->name);
> +			break;
> +		}
> +		break;
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		ret = __of_update_property(te->np, te->prop, &old_prop);
> +		if (ret) {
> +			pr_err("%s: update_property failed @%s/%s\n",
> +				__func__, te->np->full_name,
> +				te->prop->name);
> +			break;
> +		}
> +		break;
> +	}
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +	mutex_unlock(&of_mutex);
> +
> +	if (ret) {
> +		/* we can't rollback the notifier */
> +		return ret;
> +	}
> +
> +	switch (te->action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		__of_attach_node_post(te->np);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		__of_detach_node_post(te->np);
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +		/* ignore duplicate names */
> +		__of_add_property_post(te->np, te->prop, 1);
> +		break;
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +		__of_remove_property_post(te->np, te->prop);
> +		break;
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		__of_update_property_post(te->np, te->prop, te->old_prop);
> +		break;
> +	}
> +
> +	if (state != -1) {
> +		pr_debug("of_transaction: %s device for node '%s'\n",
> +				state ? "create" : "remove",
> +				te->np->full_name);
> +
> +		ret = of_reconfig_notify(state ?
> +				OF_RECONFIG_DYNAMIC_CREATE_DEV :
> +				OF_RECONFIG_DYNAMIC_DESTROY_DEV,
> +				te->np);
> +		if (ret != 0) {
> +			pr_err("of_transaction: failed %s device @%s (%d)\n",
> +					state ? "create" : "remove",
> +					te->np->full_name, ret);
> +			/* drop the error; devices probe fails; that's OK */
> +			ret = 0;
> +		}
> +
> +	}
> +
> +	return 0;
> +}
> +
> +static int __of_transaction_entry_revert(struct of_transaction *oft,
> +		struct of_transaction_entry *te)
> +{
> +	struct property *prop, *old_prop, **propp;
> +	unsigned long action, flags;
> +	struct device_node *np;
> +	int ret, state;
> +
> +	state = __of_transaction_entry_device_state(oft, te, 1);
> +
> +	if (state != -1) {
> +		pr_debug("of_transaction: %s device for node '%s'\n",
> +				state  ? "create" : "remove",
> +				te->np->full_name);
> +
> +		ret = of_reconfig_notify(state ?
> +				OF_RECONFIG_DYNAMIC_CREATE_DEV :
> +				OF_RECONFIG_DYNAMIC_DESTROY_DEV,
> +				te->np);
> +		if (ret != 0) {
> +			pr_err("of_transaction: failed %s device @%s (%d)\n",
> +					state ? "create" : "remove",
> +					te->np->full_name, ret);
> +			/* drop the error; devices probe fails; that's OK */
> +			ret = 0;
> +		}
> +	}
> +
> +	/* get node and immediately put */
> +	action = te->action;
> +	np = te->np;
> +	prop = te->prop;
> +	old_prop = te->old_prop;
> +
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		ret = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		ret = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +		ret = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY,
> +				np, prop);
> +		break;
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +		ret = of_property_notify(OF_RECONFIG_ADD_PROPERTY,
> +				np, prop);
> +		break;
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		ret = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY,
> +				np, old_prop);
> +		break;
> +	}
> +
> +	if (ret) {
> +		pr_err("%s: notifier error @%s\n", __func__,
> +				te->np->full_name);
> +		goto out_revert;
> +	}
> +
> +	mutex_lock(&of_mutex);
> +
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		__of_detach_node(np);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		__of_attach_node(np);
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +		ret = __of_remove_property(np, prop);
> +		if (ret) {
> +			pr_err("%s: remove_property failed @%s/%s\n",
> +				__func__, np->full_name, prop->name);
> +			break;
> +		}
> +		break;
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		/* property is *always* on deadprops */
> +		if (action == OF_RECONFIG_UPDATE_PROPERTY)
> +			prop = old_prop;
> +		propp = &np->deadprops;
> +		while (*propp != NULL) {
> +			if (*propp == prop)
> +				break;
> +			propp = &(*propp)->next;
> +		}
> +
> +		/* we should find it in deadprops */
> +		WARN_ON(*propp == NULL);
> +
> +		/* remove it from deadprops */
> +		if (*propp != NULL)
> +			*propp = prop->next;
> +
> +		if (action == OF_RECONFIG_REMOVE_PROPERTY) {
> +			ret = __of_add_property(np, prop);
> +			if (ret) {
> +				pr_err("%s: add_property failed @%s/%s\n",
> +					__func__, np->full_name,
> +					prop->name);
> +				break;
> +			}
> +		} else {
> +			ret = __of_update_property(np, prop, &old_prop);
> +			if (ret) {
> +				pr_err("%s: update_property failed @%s/%s\n",
> +					__func__, np->full_name,
> +					prop->name);
> +				break;
> +			}
> +		}
> +		break;
> +	}
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +	if (ret)
> +		goto out_unlock;
> +
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		__of_detach_node_post(np);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		__of_attach_node_post(np);
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +		__of_remove_property_post(np, prop);
> +		break;
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +		__of_add_property_post(np, prop, 0);
> +		break;
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		__of_update_property_post(np, prop, old_prop);
> +		break;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&of_mutex);
> +
> +out_revert:
> +	/* revert creation of device */
> +	if (ret && state != -1)
> +		of_reconfig_notify(!state ?
> +			OF_RECONFIG_DYNAMIC_CREATE_DEV :
> +			OF_RECONFIG_DYNAMIC_DESTROY_DEV,
> +			te->np);
> +
> +	return ret;
> +}
> +
> +/**
> + * of_transaction_init - Initialize a transaction for use
> + *
> + * @oft:	transaction pointer
> + *
> + * Initialize a transaction structure
> + */
> +void of_transaction_init(struct of_transaction *oft)
> +{
> +	memset(oft, 0, sizeof(*oft));
> +	INIT_LIST_HEAD(&oft->te_list);
> +}
> +
> +/**
> + * of_transaction_destroy - Destroy a transaction
> + *
> + * @oft:	transaction pointer
> + *
> + * Destroys a transaction. Note that if a transaction is applied,
> + * its changes to the tree cannot be reverted.
> + */
> +void of_transaction_destroy(struct of_transaction *oft)
> +{
> +	struct of_transaction_entry *te, *ten;
> +
> +	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
> +		__of_transaction_entry_destroy(te);
> +}
> +
> +/**
> + * of_transaction_start - Start a transaction
> + *
> + * @oft:	transaction pointer
> + *
> + * Starts a transaction, by simply grabbing hold of the of_mutex.
> + * This prevents any modification of the live tree while the
> + * transaction is in process.
> + */
> +void of_transaction_start(struct of_transaction *oft)
> +{
> +	/* take the global of transaction mutex */
> +	mutex_lock(&of_mutex);
> +}
> +
> +/**
> + * of_transaction_abort - Aborts a transaction in progress
> + *
> + * @oft:	transaction pointer
> + *
> + * Aborts a transaction, this simply releases the of_mutex
> + * and destroys all the pending transaction entries.
> + */
> +void of_transaction_abort(struct of_transaction *oft)
> +{
> +	struct of_transaction_entry *te, *ten;
> +
> +	mutex_unlock(&of_mutex);
> +
> +	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
> +		__of_transaction_entry_destroy(te);
> +}

This function shouldn't be needed anymore.

> +
> +/**
> + * of_transaction_apply - Applies a transaction
> + *
> + * @oft:	transaction pointer
> + * @force:	continue even in case of an error
> + *
> + * Applies a transaction to the live tree.
> + * Any side-effects of live tree state changes are applied here on
> + * sucess, like creation/destruction of devices and side-effects
> + * like creation of sysfs properties and directories.
> + * Returns 0 on success, a negative error value in case of an error.
> + * On error the partially applied effects are reverted if the @force
> + * parameter is not set.
> + */
> +int of_transaction_apply(struct of_transaction *oft, int force)

Is there a user of the force flag? I'd rather it not be there at all.

> +{
> +	struct of_transaction_entry *te;
> +	int ret;
> +
> +	/* drop the global lock here (notifiers and devices need it) */
> +	mutex_unlock(&of_mutex);
> +
> +	ret = 0;
> +
> +	/* perform the rest of the work */
> +	pr_debug("of_transaction: applying...\n");
> +	list_for_each_entry(te, &oft->te_list, node) {
> +		__of_transaction_entry_dump(te);
> +		ret = __of_transaction_entry_apply(oft, te);
> +		if (ret) {
> +			pr_err("%s: Error applying transaction (%d)\n",
> +					__func__, ret);
> +			if (!force) {
> +				list_for_each_entry_continue_reverse(te,
> +						&oft->te_list, node) {
> +					__of_transaction_entry_dump(te);
> +					__of_transaction_entry_revert(oft, te);
> +				}
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	pr_debug("of_transaction: applied.\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * of_transaction_revert - Reverts an applied transaction
> + *
> + * @oft:	transaction pointer
> + * @force:	continue even in case of an error
> + *
> + * Reverts a transaction returning the state of the tree to what it
> + * was before the application.
> + * Any side-effects like creation/destruction of devices and
> + * removal of sysfs properties and directories are applied.
> + * Returns 0 on success, a negative error value in case of an error.
> + */
> +int of_transaction_revert(struct of_transaction *oft, int force)
> +{
> +	struct of_transaction_entry *te, *ten;
> +	int ret;
> +
> +	pr_debug("of_transaction: reverting...\n");
> +	list_for_each_entry_reverse(te, &oft->te_list, node) {
> +		__of_transaction_entry_dump(te);
> +		ret = __of_transaction_entry_revert(oft, te);
> +		if (ret) {
> +			pr_err("%s: Error reverting transaction (%d)\n",
> +					__func__, ret);
> +			if (!force) {
> +				list_for_each_entry_continue(te,
> +						&oft->te_list, node) {
> +					__of_transaction_entry_dump(te);
> +					__of_transaction_entry_apply(oft, te);
> +				}
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	/* destroy everything */
> +	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
> +		__of_transaction_entry_destroy(te);
> +	pr_debug("of_transaction: reverted.\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * of_transaction_action - Perform a transaction action
> + *
> + * @oft:	transaction pointer
> + * @action:	action to perform
> + * @np:		Pointer to device node
> + * @prop:	Pointer to property
> + *
> + * On action being one of:
> + * + OF_RECONFIG_ATTACH_NODE
> + * + OF_RECONFIG_DETACH_NODE,
> + * + OF_RECONFIG_ADD_PROPERTY
> + * + OF_RECONFIG_REMOVE_PROPERTY,
> + * + OF_RECONFIG_UPDATE_PROPERTY
> + * Returns 0 on success, a negative error value in case of an error.
> + */
> +int of_transaction_action(struct of_transaction *oft, unsigned long action,
> +		struct device_node *np, struct property *prop)
> +{
> +	struct of_transaction_entry *te;
> +
> +	/* create the transaction entry */
> +	te = __of_transaction_entry_create(oft, action, np, prop);
> +	if (IS_ERR(te)) {
> +		pr_err("%s: failed to create entry for @%s\n",
> +				__func__, np->full_name);
> +		return PTR_ERR(te);
> +	}
> +
> +	/* add it to the list */
> +	list_add_tail(&te->node, &oft->te_list);
> +	return 0;
> +}
> +
> +/* utility functions for advanced transaction users */
> +
> +/* find a property in the transaction list while not applied */
> +struct property *of_transaction_find_property(struct of_transaction *oft,
> +		const struct device_node *np, const char *name, int *lenp)
> +{
> +	struct of_transaction_entry *te;
> +
> +	/* possibly exists in the transaction list as
> +	 * part of an attachment action
> +	 */
> +	list_for_each_entry_reverse(te, &oft->te_list, node) {
> +
> +		if (te->action != OF_RECONFIG_ADD_PROPERTY &&
> +		    te->action != OF_RECONFIG_REMOVE_PROPERTY &&
> +		    te->action != OF_RECONFIG_UPDATE_PROPERTY)
> +			continue;
> +
> +		/* match of node and name? */
> +		if (te->np != np || strcmp(te->prop->name, name))
> +			continue;
> +
> +		pr_debug("%s: found property \"%s\" in transaction list @%s\n",
> +			__func__, name, te->np->full_name);
> +
> +		if (te->action == OF_RECONFIG_ADD_PROPERTY ||
> +		    te->action == OF_RECONFIG_UPDATE_PROPERTY)
> +			return te->prop;
> +		else
> +			return NULL;
> +	}
> +
> +	/* now fallback to live tree */
> +	return of_find_property(np, name, lenp);
> +}

What's the use case for modifying a property in a node that was added in
the same changeset? If the node isn't attached yet, then shouldn't the
property just be added to the node immediately instead of adding it as a
changeset entry?

> +
> +/* special find property method for use by transaction users */
> +int of_transaction_device_is_available(struct of_transaction *oft,
> +		const struct device_node *np)
> +{
> +	struct property *prop;
> +
> +	if (!np)
> +		return 0;
> +
> +	prop = of_transaction_find_property(oft, np, "status", NULL);
> +	if (prop == NULL)
> +		return 1;
> +
> +	return prop->length > 0 &&
> +		(!strcmp(prop->value, "okay") || !strcmp(prop->value, "ok"));
> +}
> +struct device_node *of_transaction_get_child_by_name(
> +		struct of_transaction *oft, struct device_node *node,
> +		const char *name)
> +{
> +	struct of_transaction_entry *te;
> +
> +	/* possibly exists in the transaction list as
> +	 * part of an attachment action
> +	 */
> +	list_for_each_entry_reverse(te, &oft->te_list, node) {
> +
> +		if (te->action != OF_RECONFIG_ATTACH_NODE &&
> +		    te->action != OF_RECONFIG_DETACH_NODE)
> +			continue;
> +
> +		/* look at the parent and if node matches return */
> +		if (te->np->parent != node || strcmp(te->np->name, "name"))
> +			continue;
> +
> +		pr_debug("%s: found child \"%s\" in transaction list @%s\n",
> +			__func__, name, te->np->full_name);
> +		return te->action == OF_RECONFIG_ATTACH_NODE ? te->np : NULL;
> +	}
> +
> +	/* not found in the transaction list? try normal */
> +	return of_get_child_by_name(node, name);
> +}
> diff --git a/include/linux/of.h b/include/linux/of.h
> index b7c322c..fa81b42 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -821,4 +821,86 @@ typedef void (*of_init_fn_1)(struct device_node *);
>  #define OF_DECLARE_2(table, name, compat, fn) \
>  		_OF_DECLARE(table, name, compat, fn, of_init_fn_2)
>  
> +/**
> + * struct of_transaction_entry	- Holds a transaction entry
> + *
> + * @node:	list_head for the log list
> + * @action:	notifier action
> + * @np:		pointer to the device node affected
> + * @prop:	pointer to the property affected
> + * @old_prop:	hold a pointer to the original property
> + *
> + * Every modification of the device tree during a transaction
> + * is held in a list of of_transaction_entry structures.
> + * That way we can recover from a partial application, or we can
> + * revert the transaction
> + */
> +struct of_transaction_entry {
> +	struct list_head node;
> +	unsigned long action;
> +	struct device_node *np;
> +	struct property *prop;
> +	struct property *old_prop;
> +};
> +
> +/**
> + * struct of_transaction - transaction tracker structure
> + *
> + * @te_list:	list_head for the transaction entries
> + *
> + * Transactions are a convenient way to apply bulk changes to the
> + * live tree. In case of an error, changes are rolled-back.
> + * Transactions live on after initial application, and if not
> + * destroyed after use, they can be reverted in one single call.
> + */
> +struct of_transaction {
> +	struct list_head te_list;
> +};
> +
> +#ifdef CONFIG_OF_DYNAMIC
> +extern void of_transaction_init(struct of_transaction *oft);
> +extern void of_transaction_destroy(struct of_transaction *oft);
> +extern void of_transaction_start(struct of_transaction *oft);
> +extern void of_transaction_abort(struct of_transaction *oft);
> +extern int of_transaction_apply(struct of_transaction *oft, int force);
> +extern int of_transaction_revert(struct of_transaction *oft, int force);
> +extern int of_transaction_action(struct of_transaction *oft,
> +		unsigned long action, struct device_node *np,
> +		struct property *prop);
> +
> +static inline int of_transaction_attach_node(struct of_transaction *oft,
> +		struct device_node *np)
> +{
> +	return of_transaction_action(oft, OF_RECONFIG_ATTACH_NODE, np, NULL);
> +}
> +
> +static inline int of_transaction_detach_node(struct of_transaction *oft,
> +		struct device_node *np)
> +{
> +	return of_transaction_action(oft,
> +			OF_RECONFIG_DETACH_NODE, np, NULL);
> +}
> +
> +static inline int of_transaction_add_property(struct of_transaction *oft,
> +		struct device_node *np, struct property *prop)
> +{
> +	return of_transaction_action(oft,
> +			OF_RECONFIG_ADD_PROPERTY, np, prop);
> +}
> +
> +static inline int of_transaction_remove_property(struct of_transaction *oft,
> +		struct device_node *np, struct property *prop)
> +{
> +	return of_transaction_action(oft,
> +			OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> +}
> +
> +static inline int of_transaction_update_property(struct of_transaction *oft,
> +		struct device_node *np, struct property *prop)
> +{
> +	return of_transaction_action(oft,
> +			OF_RECONFIG_UPDATE_PROPERTY, np, prop);
> +}
> +#endif
> +
>  #endif /* _LINUX_OF_H */
> -- 
> 1.7.12
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] of: Introduce tree change __foo_post methods.
  2014-07-10  0:55   ` Grant Likely
@ 2014-07-11 20:04     ` Pantelis Antoniou
  2014-07-16 13:20       ` Grant Likely
  0 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-11 20:04 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

Hi Grant,

On Jul 10, 2014, at 3:55 AM, Grant Likely wrote:

> On Fri,  4 Jul 2014 19:58:48 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>> As part of transactional DT support there needs to be a clearer
>> definition of how each dynamic tree change method works.
>> 
>> Let's take for example the of_add_property method. There are
>> discrete steps taken in sequence.
>> 
>> 1. The OF_RECONFIG_ADD_PROPERTY notifier will be fired.
>> 2. The devtree lock will be taken.
>> 3. Insertion of the property in the live tree.
>> 4. The devtree lock will be released.
>> 5. The property will be inserted in the sysfs tree.
>> 
>> For each of the of_attach_node, of_detach_node, of_add_property,
>> of_remove_property and of_update_property methods we export (and
>> introduce if not available), their counterparts of
>> __of_attach_node, __of_attach_node_post, __of_detach_node,
>> __of_detach_node_post, __of_add_property, __of_add_property_post,
>> __of_remove_property, __of_remove_property_post, __of_update_property
>> and __of_update_property post.
> 
> We actually already have some code in this direction, but it uses _sysfs
> as the suffix instead of _post. I've got a modified version of this
> patch in my tree now that eliminates some of the duplication in this
> patch and uses the _sysfs name. I'm going to play around with it a bit
> before sending it to you.
> 

OK

> The locking is also a mess. of_mutex needs to be held I think whenever
> changes are being made. In the version I have, I've pulled in the
> of_mutex cleanups from patch 5 and also put in a few of my own.
> 

Yeah, locking is ever a problem. I will explain some of the locking
decisions later in the post.

> Comments below...
> 
>> 
>> So each of the tree modification methods is following the same
>> pattern, i.e. for method foo we have:
>> 
>> Firing of the notifer, lock, __foo, unlock, __foo_post
>> 
>> This breakdown is required when we introduce transaction.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/base.c       | 60 +++++++++++++++++++++++++++++++++++--------------
>> drivers/of/dynamic.c    | 14 ++++++++++--
>> drivers/of/of_private.h |  8 +++++++
>> 3 files changed, 63 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 0d50318..b52fd6b 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -127,13 +127,24 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
>> 	return name;
>> }
>> 
>> -static int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>> +static int __of_add_property_sysfs(struct device_node *np, struct property *pp,
>> +		int ignore_dup_name)
>> {
>> 	int rc;
>> 
>> 	/* Important: Don't leak passwords */
>> 	bool secure = strncmp(pp->name, "security-", 9) == 0;
>> 
>> +	/* ignore duplicate name (transaction case) */
>> +	if (ignore_dup_name) {
>> +		struct kernfs_node *kn = sysfs_get_dirent(np->kobj.sd,
>> +				pp->name);
>> +		if (kn) {
>> +			sysfs_put(kn);
>> +			return 0;
>> +		}
>> +	}
>> +
> 
> What is this? There is no mention of it in the commit text, and the comment
> is far from sufficient. Under what conditions would this be valid? I'm
> particularly concerned because this changes behaviour and the new
> behaviour is used in the next hunk, so clearly it's not
> transaction-only?
> 
> I've dropped this hunk from the version I have in my tree.
> 

You need it. When you attach a new node to the tree, and then add a new
property to the same node, you end up calling __of_add_property_sysfs twice.

This is the easiest way to get around the problem.

>> 	sysfs_bin_attr_init(&pp->attr);
>> 	pp->attr.attr.name = safe_name(&np->kobj, pp->name);
>> 	pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
>> @@ -167,7 +178,7 @@ static int __of_node_add(struct device_node *np)
>> 		return rc;
>> 
>> 	for_each_property_of_node(np, pp)
>> -		__of_add_property_sysfs(np, pp);
>> +		__of_add_property_sysfs(np, pp, 1);
>> 
>> 	return 0;
>> }
>> @@ -1677,6 +1688,13 @@ int __of_add_property(struct device_node *np, struct property *prop)
>> 	return 0;
>> }
>> 
>> +void __of_add_property_post(struct device_node *np, struct property *prop,
>> +		int ignore_dup_name)
>> +{
>> +	if (of_node_is_attached(np))
>> +		__of_add_property_sysfs(np, prop, ignore_dup_name);
>> +}
>> +
> 
> of_node_is_attached() call could just be rolled into
> __of_add_property_sysfs(). There aren't any other users, and the test is
> alwasy valid for these functions.

OK.

> 
>> /**
>>  * of_add_property - Add a property to a node
>>  */
>> @@ -1695,8 +1713,7 @@ int of_add_property(struct device_node *np, struct property *prop)
>> 	if (rc)
>> 		return rc;
>> 
>> -	if (of_node_is_attached(np))
>> -		__of_add_property_sysfs(np, prop);
>> +	__of_add_property_post(np, prop, 0);
>> 
>> 	return rc;
>> }
>> @@ -1720,6 +1737,13 @@ int __of_remove_property(struct device_node *np, struct property *prop)
>> 	return 0;
>> }
>> 
>> +void __of_remove_property_post(struct device_node *np, struct property *prop)
>> +{
>> +	/* at early boot, bail here and defer setup to of_init() */
>> +	if (of_kset)
>> +		sysfs_remove_bin_file(&np->kobj, &prop->attr);
>> +}
>> +
>> /**
>>  * of_remove_property - Remove a property from a node.
>>  *
>> @@ -1744,11 +1768,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
>> 	if (rc)
>> 		return rc;
>> 
>> -	/* at early boot, bail hear and defer setup to of_init() */
>> -	if (!of_kset)
>> -		return 0;
>> -
>> -	sysfs_remove_bin_file(&np->kobj, &prop->attr);
>> +	__of_remove_property_post(np, prop);
>> 
>> 	return 0;
>> }
>> @@ -1779,6 +1799,19 @@ int __of_update_property(struct device_node *np, struct property *newprop,
>> 	return 0;
>> }
>> 
>> +void __of_update_property_post(struct device_node *np, struct property *newprop,
>> +		struct property *oldprop)
>> +{
>> +	/* At early boot, bail out and defer setup to of_init() */
>> +	if (!of_kset)
>> +		return;
>> +
>> +	/* Update the sysfs attribute */
>> +	if (oldprop)
>> +		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
>> +	__of_add_property_sysfs(np, newprop, 0);
>> +}
>> +
>> /*
>>  * of_update_property - Update a property in a node, if the property does
>>  * not exist, add it.
>> @@ -1807,14 +1840,7 @@ int of_update_property(struct device_node *np, struct property *prop)
>> 	if (rc)
>> 		return rc;
>> 
>> -	/* At early boot, bail out and defer setup to of_init() */
>> -	if (!of_kset)
>> -		return 0;
>> -
>> -	/* Update the sysfs attribute */
>> -	if (oldprop)
>> -		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
>> -	__of_add_property_sysfs(np, prop);
>> +	__of_update_property_post(np, prop, oldprop);
>> 
>> 	return 0;
>> }
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index 90c09b6..a995de3 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -104,6 +104,11 @@ void __of_attach_node(struct device_node *np)
>> 	of_node_clear_flag(np, OF_DETACHED);
>> }
>> 
>> +void __of_attach_node_post(struct device_node *np)
>> +{
>> +	of_node_add(np);
>> +}
> 
> There is only one user of of_node_add(), so this is kind of silly. I've
> actually removed of_node_add() entirely now and have renamed
> __of_node_add to __of_attach_node_sysfs()

Hmm, OK.

> 
>> +
>> /**
>>  * of_attach_node - Plug a device node into the tree and global list.
>>  */
>> @@ -120,7 +125,7 @@ int of_attach_node(struct device_node *np)
>> 	__of_attach_node(np);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>> -	of_node_add(np);
>> +	__of_attach_node_post(np);
>> 	return 0;
>> }
>> 
>> @@ -160,6 +165,11 @@ void __of_detach_node(struct device_node *np)
>> 	of_node_set_flag(np, OF_DETACHED);
>> }
>> 
>> +void __of_detach_node_post(struct device_node *np)
>> +{
>> +	of_node_remove(np);
>> +}
> 
> Also silly.
> 

It's regular...

>> +
>> /**
>>  * of_detach_node - "Unplug" a node from the device tree.
>>  *
>> @@ -179,7 +189,7 @@ int of_detach_node(struct device_node *np)
>> 	__of_detach_node(np);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>> -	of_node_remove(np);
>> +	__of_detach_node_post(np);
>> 	return rc;
>> }
>> 
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index e544247..ca95100 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -60,12 +60,20 @@ static inline int of_reconfig_notify(unsigned long action, void *p)
>> #endif /* CONFIG_OF_DYNAMIC */
>> 
>> extern int __of_add_property(struct device_node *np, struct property *prop);
>> +extern void __of_add_property_post(struct device_node *np,
>> +		struct property *prop, int ignore_dup_name);
>> extern int __of_remove_property(struct device_node *np, struct property *prop);
>> +extern void __of_remove_property_post(struct device_node *np,
>> +		struct property *prop);
>> extern int __of_update_property(struct device_node *np,
>> 		struct property *newprop, struct property **oldprop);
>> +extern void __of_update_property_post(struct device_node *np,
>> +		struct property *newprop, struct property *oldprop);
>> 
>> extern void __of_attach_node(struct device_node *np);
>> +extern void __of_attach_node_post(struct device_node *np);
>> extern void __of_detach_node(struct device_node *np);
>> +extern void __of_detach_node_post(struct device_node *np);
>> 
>> /**
>>  * General utilities for working with live trees.
>> -- 
>> 1.7.12
>> 
> 

Regards

-- Pantelis

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] of: Transactional DT support.
  2014-07-11  5:24   ` Grant Likely
@ 2014-07-11 20:28     ` Pantelis Antoniou
  2014-07-14 18:09       ` Grant Likely
  2014-07-16 13:39       ` Grant Likely
  0 siblings, 2 replies; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-11 20:28 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

Hi Grant,

On Jul 11, 2014, at 8:24 AM, Grant Likely wrote:

> On Fri,  4 Jul 2014 19:58:49 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>> Introducing DT transactional support.
>> 
>> A DT transaction is a method which allows one to apply changes
>> in the live tree, in such a way that either the full set of changes
>> take effect, or the state of the tree can be rolled-back to the
>> state it was before it was attempted. An applied transaction
>> can be rolled-back at any time.
>> 
>> If any transaction results in the change of a state of a device
>> node, a notifier for that node will be fired which allows a bus
>> to create/destroy devices as directed.
>> 
>> Documentation is in
>> 	Documentation/devicetree/transactions.txt
> 
> Hi Pantelis,
> 
> I've got a lot of comments below, but you don't need to respin the
> series immediately. I've got a tree that I'm working with to sort out
> what I can queue up for the next kernel. Give me a few days and I'll
> send you what I've gotten to.
> 

OK, that would be good.

>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> Documentation/devicetree/transactions.txt |  41 ++
> 
> I've been thinking about this for a while, 'transaction' doesn't strike me as the correct term. We aren't allowing the callers to see any intermediate state, and so there isn't really a concept of committing or reverting. As far as the caller is concerned, it provides a set of modifications, and they either get completely applied, or not at all.
> 
> Changeset is a better term.
> 

OK

>> drivers/of/Makefile                       |   2 +-
>> drivers/of/base.c                         |  34 +-
>> drivers/of/of_private.h                   |  19 +
>> drivers/of/transaction.c                  | 699 ++++++++++++++++++++++++++++++
> 
> drivers/of/dynamic.c. This is really important code. This code shouldn't
> merely be one way to modify the device tree. Ultimately it should be the
> *only* way to modify the tree. Once it is merged I want to convert the
> powerpc users over to use the new interface.
> 

OK :)

>> include/linux/of.h                        |  82 ++++
>> 6 files changed, 864 insertions(+), 13 deletions(-)
>> create mode 100644 Documentation/devicetree/transactions.txt
>> create mode 100644 drivers/of/transaction.c
>> 
>> diff --git a/Documentation/devicetree/transactions.txt b/Documentation/devicetree/transactions.txt
>> new file mode 100644
>> index 0000000..59ff5b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/transactions.txt
>> @@ -0,0 +1,41 @@
>> +A DT transaction is a method which allows one to apply changes
>> +in the live tree, in such a way that either the full set of changes
>> +take effect, or the state of the tree can be rolled-back to the
>> +state it was before it was attempted. An applied transaction
>> +can be rolled-back at any time.
>> +
>> +If any transaction results in the change of a state of a device
>> +node, a notifier for that node will be fired which allows a bus
>> +to create/destroy devices as directed.
>> +
>> +The sequence of a transaction is as follows.
>> +
>> +1. of_transaction_init() - initializes a transaction
>> +
>> +2. of_transaction_start() - starts a transaction; a global
>> +transaction lock is taken at this point and disallowes any other
>> +dynamic tree changes.
> 
> Instead of of_transaction_start() the caller should be told to
> explicitly take the of_mutex() so that it becomes the only editor.
> 

OK

>> +3. A number of DT tree change calls, of_transaction_attach_node(),
>> +of_transaction_detach_node(), of_transaction_add_property(),
>> +of_transaction_remove_property, of_transaction_update_property()
>> +modify the live tree (but without any user-space visible changes).
>> +All changes are recorded in the list of of_transaction_entry of
>> +the transaction.
>> +
>> +4.a. No errors occured during application, so a call to
>> +of_transaction_commit() is issued. All the changes are made visible
>> +to user-space and all device creation/destruction notifiers are
>> +fired. The global transaction is released.
>> +
>> +4.b. An error occured, so a of_transaction_abort() call is issued.
>> +All changes to the live tree will be reverted. The global transaction
>> +lock will be released.
> 
> The above isn't accurate anymore. of_changeset_apply() should either
> completely work or completely fail. There should not be an _abort()
> call at all.
> 
> I would make it the responsibility of the caller to release the of_mutex
> lock for consistency. of_changeset_apply() should require the of_mutex
> to be held and it should drop/reaquire the lock for just the period when
> it sends notifiers.

The notifiers bit is what makes it interesting. I agree that the of_mutex
usage is only a stop-gap, but I'm really hesitant on dropping the locks.

> 
>> +
>> +5.a If the transaction needs not be reverted, a call to
>> +of_transaction_destroy will release the resources of the transaction.
> 
> of_changeset_free() would probably be a better name.
> 

OK.

>> +
>> +5.b If the transaction is to be reverted, a call to
>> +of_transaction_revert will do so. All device notifiers will be
>> +fired, and the state of the tree will be the same as before
>> +the call to of_start_transaction().
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 08e6c0f..9a68cc9 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,5 +1,5 @@
>> obj-y = base.o device.o platform.o
>> -obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
>> +obj-$(CONFIG_OF_DYNAMIC) += dynamic.o transaction.o
>> obj-$(CONFIG_OF_FLATTREE) += fdt.o
>> obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
>> obj-$(CONFIG_OF_PROMTREE) += pdt.o
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index b52fd6b..206af65 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -40,7 +40,9 @@ static struct device_node *of_stdout;
>> static struct kset *of_kset;
>> 
>> /*
>> - * Used to protect the of_aliases, to hold off addition of nodes to sysfs
>> + * Used to protect the of_aliases, to hold off addition of
>> + * nodes to sysfs and in the case of a transaction is in
>> + * progress.
>>  */
>> DEFINE_MUTEX(of_mutex);
>> 
>> @@ -1707,13 +1709,16 @@ int of_add_property(struct device_node *np, struct property *prop)
>> 	if (rc)
>> 		return rc;
>> 
>> +	mutex_lock(&of_mutex);
>> +
> 
> These mutex hunks I've moved to the earlier patch that makes the _sysfs
> variants consistent in the version I've got in my tree.
> 

OK, please sent me a link to said tree when you can.

>> 	raw_spin_lock_irqsave(&devtree_lock, flags);
>> 	rc = __of_add_property(np, prop);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> -	if (rc)
>> -		return rc;
>> 
>> -	__of_add_property_post(np, prop, 0);
>> +	if (rc == 0)
>> +		__of_add_property_post(np, prop, 0);
>> +
>> +	mutex_unlock(&of_mutex);
>> 
>> 	return rc;
>> }
>> @@ -1761,16 +1766,18 @@ int of_remove_property(struct device_node *np, struct property *prop)
>> 	if (rc)
>> 		return rc;
>> 
>> +	mutex_lock(&of_mutex);
>> +
>> 	raw_spin_lock_irqsave(&devtree_lock, flags);
>> 	rc = __of_remove_property(np, prop);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>> -	if (rc)
>> -		return rc;
>> +	if (rc == 0)
>> +		__of_remove_property_post(np, prop);
>> 
>> -	__of_remove_property_post(np, prop);
>> +	mutex_unlock(&of_mutex);
>> 
>> -	return 0;
>> +	return rc;
>> }
>> 
>> int __of_update_property(struct device_node *np, struct property *newprop,
>> @@ -1834,15 +1841,18 @@ int of_update_property(struct device_node *np, struct property *prop)
>> 	if (rc)
>> 		return rc;
>> 
>> +	mutex_lock(&of_mutex);
>> +
>> 	raw_spin_lock_irqsave(&devtree_lock, flags);
>> 	rc = __of_update_property(np, prop, &oldprop);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> -	if (rc)
>> -		return rc;
>> 
>> -	__of_update_property_post(np, prop, oldprop);
>> +	if (rc == 0)
>> +		__of_update_property_post(np, prop, oldprop);
>> 
>> -	return 0;
>> +	mutex_unlock(&of_mutex);
>> +
>> +	return rc;
>> }
>> 
>> static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index ca95100..00c119a 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -89,4 +89,23 @@ struct device_node *__of_create_empty_node(const char *name,
>> 		const char *type, const char *full_name,
>> 		phandle phandle, gfp_t allocflags, unsigned long nodeflags);
>> 
>> +/* iterators for transactions, used for overlays */
>> +/* forward iterator */
>> +#define for_each_transaction_entry(_oft, _te) \
>> +	list_for_each_entry(_te, &(_oft)->te_list, node)
>> +
>> +/* reverse iterator */
>> +#define for_each_transaction_entry_reverse(_oft, _te) \
>> +	list_for_each_entry_reverse(_te, &(_oft)->te_list, node)
>> +
>> +/* special find property method for use by transaction users */
>> +extern struct property *of_transaction_find_property(
>> +		struct of_transaction *oft,
>> +		const struct device_node *np, const char *name, int *lenp);
>> +extern int of_transaction_device_is_available(struct of_transaction *oft,
>> +		const struct device_node *np);
>> +extern struct device_node *of_transaction_get_child_by_name(
>> +		struct of_transaction *oft, struct device_node *node,
>> +		const char *name);
>> +
>> #endif /* _LINUX_OF_PRIVATE_H */
>> diff --git a/drivers/of/transaction.c b/drivers/of/transaction.c
>> new file mode 100644
>> index 0000000..1094395
>> --- /dev/null
>> +++ b/drivers/of/transaction.c
>> @@ -0,0 +1,699 @@
>> +/*
>> + * Functions for DT transactions
>> + *
>> + * Copyright (C) 2014 Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + */
>> +#undef DEBUG
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +
>> +#include "of_private.h"
>> +
>> +static struct of_transaction_entry *__of_transaction_entry_create(
>> +		struct of_transaction *oft, unsigned long action,
>> +		struct device_node *dn, struct property *prop)
>> +{
>> +	struct of_transaction_entry *te;
>> +
>> +	te = kzalloc(sizeof(*te), GFP_KERNEL);
>> +	if (te == NULL) {
>> +		pr_err("%s: Failed to allocate\n", __func__);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +	/* get a reference to the node */
>> +	te->action = action;
>> +	te->np = of_node_get(dn);
>> +	te->prop = prop;
>> +
>> +	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
>> +		te->old_prop = of_transaction_find_property(oft, dn,
>> +				prop->name, NULL);
>> +
>> +	return te;
>> +}
> 
> There is only one user of this function: of_transaction_action(). Roll
> it all into one function.
> 

OK

>> +
>> +static void __of_transaction_entry_destroy(struct of_transaction_entry *te)
>> +{
>> +	of_node_put(te->np);
>> +	list_del(&te->node);
>> +	kfree(te);
>> +}
>> +
>> +#ifdef DEBUG
>> +static void __of_transaction_entry_dump(struct of_transaction_entry *te)
>> +{
>> +	switch (te->action) {
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +		pr_debug("%p: %s %s/%s\n",
>> +			te, "ADD_PROPERTY   ", te->np->full_name,
>> +			te->prop->name);
>> +		break;
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +		pr_debug("%p: %s %s/%s\n",
>> +			te, "REMOVE_PROPERTY", te->np->full_name,
>> +			te->prop->name);
>> +		break;
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		pr_debug("%p: %s %s/%s\n",
>> +			te, "UPDATE_PROPERTY", te->np->full_name,
>> +			te->prop->name);
>> +		break;
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		pr_debug("%p: %s %s\n",
>> +			te, "ATTACH_NODE    ", te->np->full_name);
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		pr_debug("%p: %s %s\n",
>> +			te, "DETACH_NODE    ", te->np->full_name);
>> +		break;
>> +	}
>> +}
>> +#else
>> +static inline void __of_transaction_entry_dump(struct of_transaction_entry *te)
>> +{
>> +	/* empty */
>> +}
>> +#endif
>> +
>> +static int __of_transaction_entry_device_state(struct of_transaction *oft,
>> +		struct of_transaction_entry *te, int revert)
>> +{
>> +	int curr, target;
>> +	unsigned long action;
>> +	struct property *prop;
>> +
>> +	/* filter */
>> +	if (te->action != OF_RECONFIG_ADD_PROPERTY &&
>> +	    te->action != OF_RECONFIG_REMOVE_PROPERTY &&
>> +	    te->action != OF_RECONFIG_UPDATE_PROPERTY &&
>> +	    te->action != OF_RECONFIG_ATTACH_NODE &&
>> +	    te->action != OF_RECONFIG_ATTACH_NODE)
>> +		return -1;
>> +
>> +	action = te->action;
>> +	prop = te->prop;
>> +
>> +	/* on revert convert to the opposite */
>> +	if (revert) {
>> +		if (action == OF_RECONFIG_ADD_PROPERTY)
>> +			action = OF_RECONFIG_REMOVE_PROPERTY;
>> +		else if (action == OF_RECONFIG_REMOVE_PROPERTY)
>> +			action = OF_RECONFIG_ADD_PROPERTY;
>> +		else if (action == OF_RECONFIG_UPDATE_PROPERTY)
>> +			prop = te->old_prop;
>> +		else if (action == OF_RECONFIG_ATTACH_NODE)
>> +			action = OF_RECONFIG_DETACH_NODE;
>> +		else
>> +			action = OF_RECONFIG_ATTACH_NODE;
>> +	}
>> +
>> +	/* we only support state changes on "status" property change */
>> +	if ((te->action == OF_RECONFIG_ADD_PROPERTY ||
>> +	    te->action == OF_RECONFIG_REMOVE_PROPERTY ||
>> +	    te->action == OF_RECONFIG_UPDATE_PROPERTY) &&
>> +		of_prop_cmp(prop->name, "status"))
>> +		return -1;
>> +
>> +	/* note that we don't use of_transaction_find_property() */
>> +
>> +	/* current device state */
>> +	curr = of_device_is_available(te->np) &&
>> +		of_find_property(te->np, "compatible", NULL) &&
>> +		of_find_property(te->np, "status", NULL);
>> +
>> +	switch (action) {
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +
>> +		/* target device state */
>> +		if (action == OF_RECONFIG_ADD_PROPERTY ||
>> +		    action == OF_RECONFIG_UPDATE_PROPERTY)
>> +			target = !strcmp(prop->value, "okay") ||
>> +				!strcmp(prop->value, "ok");
>> +		else
>> +			target = 0;	/* NOTE: status removal -> disabled */
>> +
>> +		break;
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		target = curr;
>> +		curr = 0;
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		target = 0;
>> +		break;
>> +	}
>> +
>> +	return curr != target ? target : -1;
>> +}
> 
> I've completely dropped the above function from the version I've got in
> my tree. Adding changesets is not dependent on device state notifiers.
> It really must be in a separate patch.
> 
> This patch should only add the functionality of changesets within the
> context of the way tree changes currently work. Adding a new notifier is
> a separate feature. Besides, if you're adding a new notifier, then you
> need to make sure the old functions also emit the new notifier under
> similar conditions. It's just best to keep that as a separate change.
> 
> (And I'm yet to be convinced that device_state notifiers are needed)
> 

I'm open to suggestions on how to tackle this. Without the device state
changes being applied, all the rest is meaningless.

The new notifiers are the cleanest way to do so.

>> +
>> +static int __of_transaction_entry_apply(struct of_transaction *oft,
>> +		struct of_transaction_entry *te)
>> +{
>> +	struct property *old_prop;
>> +	unsigned long flags;
>> +	int ret, state;
>> +
>> +	ret = 0;
>> +
>> +	state = __of_transaction_entry_device_state(oft, te, 0);
>> +
>> +	switch (te->action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		ret = of_reconfig_notify(te->action, te->np);
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		ret = of_property_notify(te->action, te->np, te->prop);
>> +		break;
>> +	}
> 
> The way I read this, the notifier is sent as each entry is applied. So
> the sequence would be:
> 	entry 1 notify
> 	entry 1 apply
> 	entry 2 notify
> 	entry 2 apply
> 	etc.
> Correct?
> 
Yes.

> For this patch, that is probably exactly what should happen because it
> matches the existing behaviour. However, it isn't what we want. We need
> a seperate patch to reorganize the notifiers to be emitted after the
> change takes place as we discussed on IRC, and powerpc needs to be moved
> to the new model. At the same time, all of the notifiers need to be
> emitted together instead of interleaved with the individual entries so
> that the order looks like this:
> 	entry 1 apply
> 	entry 2 apply
> 	etc.
> 	entry 1 notify
> 	entry 2 notify
> 	etc.

It is simple enough to do. We will break anyone that depended on the old
notifier behaviour, but I doubt there's anyone out there.

> 
>> +
>> +	if (ret) {
>> +		pr_err("%s: notifier error @%s\n", __func__,
>> +				te->np->full_name);
>> +		return ret;
>> +	}
>> +
>> +	mutex_lock(&of_mutex);
> 
> This doesn't look right. The mutex need to be held over the entire time
> of applying changes. It cannot be dropped between entries of the
> changeset. ie:
> 
> 	mutex_lock()
> 	entry 1 apply
> 	entry 2 apply
> 	etc.
> 	mutex_unlock()
> 	entry 1 notify
> 	entry 2 notify
> 	etc.
> 
> If the mutex gets obtained and droped for each entry, then there is no
> protection against another thread making a modification in the middle.
> 

Remember that of_mutex is not the only mutex at play. Overlays have
another mutex taken before applying anything.

The problem with the mutex is the firing of the notifier as you've pointed out.
Keeping the old semantics, you have to drop of_mutex. The new ones do not
have this problem.

>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	switch (te->action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		__of_attach_node(te->np);
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		__of_detach_node(te->np);
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +		ret = __of_add_property(te->np, te->prop);
>> +		if (ret) {
>> +			pr_err("%s: add_property failed @%s/%s\n",
>> +				__func__, te->np->full_name,
>> +				te->prop->name);
>> +			break;
>> +		}
>> +		break;
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +		ret = __of_remove_property(te->np, te->prop);
>> +		if (ret) {
>> +			pr_err("%s: remove_property failed @%s/%s\n",
>> +				__func__, te->np->full_name,
>> +				te->prop->name);
>> +			break;
>> +		}
>> +		break;
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		ret = __of_update_property(te->np, te->prop, &old_prop);
>> +		if (ret) {
>> +			pr_err("%s: update_property failed @%s/%s\n",
>> +				__func__, te->np->full_name,
>> +				te->prop->name);
>> +			break;
>> +		}
>> +		break;
>> +	}
>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> +
>> +	mutex_unlock(&of_mutex);
>> +
>> +	if (ret) {
>> +		/* we can't rollback the notifier */
>> +		return ret;
>> +	}
>> +
>> +	switch (te->action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		__of_attach_node_post(te->np);
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		__of_detach_node_post(te->np);
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +		/* ignore duplicate names */
>> +		__of_add_property_post(te->np, te->prop, 1);
>> +		break;
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +		__of_remove_property_post(te->np, te->prop);
>> +		break;
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		__of_update_property_post(te->np, te->prop, te->old_prop);
>> +		break;
>> +	}
>> +
>> +	if (state != -1) {
>> +		pr_debug("of_transaction: %s device for node '%s'\n",
>> +				state ? "create" : "remove",
>> +				te->np->full_name);
>> +
>> +		ret = of_reconfig_notify(state ?
>> +				OF_RECONFIG_DYNAMIC_CREATE_DEV :
>> +				OF_RECONFIG_DYNAMIC_DESTROY_DEV,
>> +				te->np);
>> +		if (ret != 0) {
>> +			pr_err("of_transaction: failed %s device @%s (%d)\n",
>> +					state ? "create" : "remove",
>> +					te->np->full_name, ret);
>> +			/* drop the error; devices probe fails; that's OK */
>> +			ret = 0;
>> +		}
>> +
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int __of_transaction_entry_revert(struct of_transaction *oft,
>> +		struct of_transaction_entry *te)
>> +{
>> +	struct property *prop, *old_prop, **propp;
>> +	unsigned long action, flags;
>> +	struct device_node *np;
>> +	int ret, state;
>> +
>> +	state = __of_transaction_entry_device_state(oft, te, 1);
>> +
>> +	if (state != -1) {
>> +		pr_debug("of_transaction: %s device for node '%s'\n",
>> +				state  ? "create" : "remove",
>> +				te->np->full_name);
>> +
>> +		ret = of_reconfig_notify(state ?
>> +				OF_RECONFIG_DYNAMIC_CREATE_DEV :
>> +				OF_RECONFIG_DYNAMIC_DESTROY_DEV,
>> +				te->np);
>> +		if (ret != 0) {
>> +			pr_err("of_transaction: failed %s device @%s (%d)\n",
>> +					state ? "create" : "remove",
>> +					te->np->full_name, ret);
>> +			/* drop the error; devices probe fails; that's OK */
>> +			ret = 0;
>> +		}
>> +	}
>> +
>> +	/* get node and immediately put */
>> +	action = te->action;
>> +	np = te->np;
>> +	prop = te->prop;
>> +	old_prop = te->old_prop;
>> +
>> +	switch (action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		ret = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		ret = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +		ret = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY,
>> +				np, prop);
>> +		break;
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +		ret = of_property_notify(OF_RECONFIG_ADD_PROPERTY,
>> +				np, prop);
>> +		break;
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		ret = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY,
>> +				np, old_prop);
>> +		break;
>> +	}
>> +
>> +	if (ret) {
>> +		pr_err("%s: notifier error @%s\n", __func__,
>> +				te->np->full_name);
>> +		goto out_revert;
>> +	}
>> +
>> +	mutex_lock(&of_mutex);
>> +
>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	switch (action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		__of_detach_node(np);
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		__of_attach_node(np);
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +		ret = __of_remove_property(np, prop);
>> +		if (ret) {
>> +			pr_err("%s: remove_property failed @%s/%s\n",
>> +				__func__, np->full_name, prop->name);
>> +			break;
>> +		}
>> +		break;
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		/* property is *always* on deadprops */
>> +		if (action == OF_RECONFIG_UPDATE_PROPERTY)
>> +			prop = old_prop;
>> +		propp = &np->deadprops;
>> +		while (*propp != NULL) {
>> +			if (*propp == prop)
>> +				break;
>> +			propp = &(*propp)->next;
>> +		}
>> +
>> +		/* we should find it in deadprops */
>> +		WARN_ON(*propp == NULL);
>> +
>> +		/* remove it from deadprops */
>> +		if (*propp != NULL)
>> +			*propp = prop->next;
>> +
>> +		if (action == OF_RECONFIG_REMOVE_PROPERTY) {
>> +			ret = __of_add_property(np, prop);
>> +			if (ret) {
>> +				pr_err("%s: add_property failed @%s/%s\n",
>> +					__func__, np->full_name,
>> +					prop->name);
>> +				break;
>> +			}
>> +		} else {
>> +			ret = __of_update_property(np, prop, &old_prop);
>> +			if (ret) {
>> +				pr_err("%s: update_property failed @%s/%s\n",
>> +					__func__, np->full_name,
>> +					prop->name);
>> +				break;
>> +			}
>> +		}
>> +		break;
>> +	}
>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> +
>> +	if (ret)
>> +		goto out_unlock;
>> +
>> +	switch (action) {
>> +	case OF_RECONFIG_ATTACH_NODE:
>> +		__of_detach_node_post(np);
>> +		break;
>> +	case OF_RECONFIG_DETACH_NODE:
>> +		__of_attach_node_post(np);
>> +		break;
>> +	case OF_RECONFIG_ADD_PROPERTY:
>> +		__of_remove_property_post(np, prop);
>> +		break;
>> +	case OF_RECONFIG_REMOVE_PROPERTY:
>> +		__of_add_property_post(np, prop, 0);
>> +		break;
>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>> +		__of_update_property_post(np, prop, old_prop);
>> +		break;
>> +	}
>> +
>> +out_unlock:
>> +	mutex_unlock(&of_mutex);
>> +
>> +out_revert:
>> +	/* revert creation of device */
>> +	if (ret && state != -1)
>> +		of_reconfig_notify(!state ?
>> +			OF_RECONFIG_DYNAMIC_CREATE_DEV :
>> +			OF_RECONFIG_DYNAMIC_DESTROY_DEV,
>> +			te->np);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * of_transaction_init - Initialize a transaction for use
>> + *
>> + * @oft:	transaction pointer
>> + *
>> + * Initialize a transaction structure
>> + */
>> +void of_transaction_init(struct of_transaction *oft)
>> +{
>> +	memset(oft, 0, sizeof(*oft));
>> +	INIT_LIST_HEAD(&oft->te_list);
>> +}
>> +
>> +/**
>> + * of_transaction_destroy - Destroy a transaction
>> + *
>> + * @oft:	transaction pointer
>> + *
>> + * Destroys a transaction. Note that if a transaction is applied,
>> + * its changes to the tree cannot be reverted.
>> + */
>> +void of_transaction_destroy(struct of_transaction *oft)
>> +{
>> +	struct of_transaction_entry *te, *ten;
>> +
>> +	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
>> +		__of_transaction_entry_destroy(te);
>> +}
>> +
>> +/**
>> + * of_transaction_start - Start a transaction
>> + *
>> + * @oft:	transaction pointer
>> + *
>> + * Starts a transaction, by simply grabbing hold of the of_mutex.
>> + * This prevents any modification of the live tree while the
>> + * transaction is in process.
>> + */
>> +void of_transaction_start(struct of_transaction *oft)
>> +{
>> +	/* take the global of transaction mutex */
>> +	mutex_lock(&of_mutex);
>> +}
>> +
>> +/**
>> + * of_transaction_abort - Aborts a transaction in progress
>> + *
>> + * @oft:	transaction pointer
>> + *
>> + * Aborts a transaction, this simply releases the of_mutex
>> + * and destroys all the pending transaction entries.
>> + */
>> +void of_transaction_abort(struct of_transaction *oft)
>> +{
>> +	struct of_transaction_entry *te, *ten;
>> +
>> +	mutex_unlock(&of_mutex);
>> +
>> +	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
>> +		__of_transaction_entry_destroy(te);
>> +}
> 
> This function shouldn't be needed anymore.
> 

Well, there are cases where a transaction action fails, this could be still useful
to abort before pulling the trigger on commit. I guess we can roll the check
being the first thing on commit.

>> +
>> +/**
>> + * of_transaction_apply - Applies a transaction
>> + *
>> + * @oft:	transaction pointer
>> + * @force:	continue even in case of an error
>> + *
>> + * Applies a transaction to the live tree.
>> + * Any side-effects of live tree state changes are applied here on
>> + * sucess, like creation/destruction of devices and side-effects
>> + * like creation of sysfs properties and directories.
>> + * Returns 0 on success, a negative error value in case of an error.
>> + * On error the partially applied effects are reverted if the @force
>> + * parameter is not set.
>> + */
>> +int of_transaction_apply(struct of_transaction *oft, int force)
> 
> Is there a user of the force flag? I'd rather it not be there at all.
> 

It's a semantics issue. Transactions are lower level operations that overlays,
so there's no tree overlap check. What happens when you try to apply a
transaction and the operation fails? What is the state of the tree? Perhaps
on your application you can tolerate partial application.

The force flag allows you to choose. Setting it to 0, which is the default,
it will fail.

There is a similar problem on revert.

>> +{
>> +	struct of_transaction_entry *te;
>> +	int ret;
>> +
>> +	/* drop the global lock here (notifiers and devices need it) */
>> +	mutex_unlock(&of_mutex);
>> +
>> +	ret = 0;
>> +
>> +	/* perform the rest of the work */
>> +	pr_debug("of_transaction: applying...\n");
>> +	list_for_each_entry(te, &oft->te_list, node) {
>> +		__of_transaction_entry_dump(te);
>> +		ret = __of_transaction_entry_apply(oft, te);
>> +		if (ret) {
>> +			pr_err("%s: Error applying transaction (%d)\n",
>> +					__func__, ret);
>> +			if (!force) {
>> +				list_for_each_entry_continue_reverse(te,
>> +						&oft->te_list, node) {
>> +					__of_transaction_entry_dump(te);
>> +					__of_transaction_entry_revert(oft, te);
>> +				}
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +
>> +	pr_debug("of_transaction: applied.\n");
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * of_transaction_revert - Reverts an applied transaction
>> + *
>> + * @oft:	transaction pointer
>> + * @force:	continue even in case of an error
>> + *
>> + * Reverts a transaction returning the state of the tree to what it
>> + * was before the application.
>> + * Any side-effects like creation/destruction of devices and
>> + * removal of sysfs properties and directories are applied.
>> + * Returns 0 on success, a negative error value in case of an error.
>> + */
>> +int of_transaction_revert(struct of_transaction *oft, int force)
>> +{
>> +	struct of_transaction_entry *te, *ten;
>> +	int ret;
>> +
>> +	pr_debug("of_transaction: reverting...\n");
>> +	list_for_each_entry_reverse(te, &oft->te_list, node) {
>> +		__of_transaction_entry_dump(te);
>> +		ret = __of_transaction_entry_revert(oft, te);
>> +		if (ret) {
>> +			pr_err("%s: Error reverting transaction (%d)\n",
>> +					__func__, ret);
>> +			if (!force) {
>> +				list_for_each_entry_continue(te,
>> +						&oft->te_list, node) {
>> +					__of_transaction_entry_dump(te);
>> +					__of_transaction_entry_apply(oft, te);
>> +				}
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* destroy everything */
>> +	list_for_each_entry_safe_reverse(te, ten, &oft->te_list, node)
>> +		__of_transaction_entry_destroy(te);
>> +	pr_debug("of_transaction: reverted.\n");
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * of_transaction_action - Perform a transaction action
>> + *
>> + * @oft:	transaction pointer
>> + * @action:	action to perform
>> + * @np:		Pointer to device node
>> + * @prop:	Pointer to property
>> + *
>> + * On action being one of:
>> + * + OF_RECONFIG_ATTACH_NODE
>> + * + OF_RECONFIG_DETACH_NODE,
>> + * + OF_RECONFIG_ADD_PROPERTY
>> + * + OF_RECONFIG_REMOVE_PROPERTY,
>> + * + OF_RECONFIG_UPDATE_PROPERTY
>> + * Returns 0 on success, a negative error value in case of an error.
>> + */
>> +int of_transaction_action(struct of_transaction *oft, unsigned long action,
>> +		struct device_node *np, struct property *prop)
>> +{
>> +	struct of_transaction_entry *te;
>> +
>> +	/* create the transaction entry */
>> +	te = __of_transaction_entry_create(oft, action, np, prop);
>> +	if (IS_ERR(te)) {
>> +		pr_err("%s: failed to create entry for @%s\n",
>> +				__func__, np->full_name);
>> +		return PTR_ERR(te);
>> +	}
>> +
>> +	/* add it to the list */
>> +	list_add_tail(&te->node, &oft->te_list);
>> +	return 0;
>> +}
>> +
>> +/* utility functions for advanced transaction users */
>> +
>> +/* find a property in the transaction list while not applied */
>> +struct property *of_transaction_find_property(struct of_transaction *oft,
>> +		const struct device_node *np, const char *name, int *lenp)
>> +{
>> +	struct of_transaction_entry *te;
>> +
>> +	/* possibly exists in the transaction list as
>> +	 * part of an attachment action
>> +	 */
>> +	list_for_each_entry_reverse(te, &oft->te_list, node) {
>> +
>> +		if (te->action != OF_RECONFIG_ADD_PROPERTY &&
>> +		    te->action != OF_RECONFIG_REMOVE_PROPERTY &&
>> +		    te->action != OF_RECONFIG_UPDATE_PROPERTY)
>> +			continue;
>> +
>> +		/* match of node and name? */
>> +		if (te->np != np || strcmp(te->prop->name, name))
>> +			continue;
>> +
>> +		pr_debug("%s: found property \"%s\" in transaction list @%s\n",
>> +			__func__, name, te->np->full_name);
>> +
>> +		if (te->action == OF_RECONFIG_ADD_PROPERTY ||
>> +		    te->action == OF_RECONFIG_UPDATE_PROPERTY)
>> +			return te->prop;
>> +		else
>> +			return NULL;
>> +	}
>> +
>> +	/* now fallback to live tree */
>> +	return of_find_property(np, name, lenp);
>> +}
> 
> What's the use case for modifying a property in a node that was added in
> the same changeset? If the node isn't attached yet, then shouldn't the
> property just be added to the node immediately instead of adding it as a
> changeset entry?

This has to do with detecting device state changes, in cases of successive
status property updates.

Using the current notifier mechanism, this is requires to keep the same
semantics.

> 
>> +
>> +/* special find property method for use by transaction users */
>> +int of_transaction_device_is_available(struct of_transaction *oft,
>> +		const struct device_node *np)
>> +{
>> +	struct property *prop;
>> +
>> +	if (!np)
>> +		return 0;
>> +
>> +	prop = of_transaction_find_property(oft, np, "status", NULL);
>> +	if (prop == NULL)
>> +		return 1;
>> +
>> +	return prop->length > 0 &&
>> +		(!strcmp(prop->value, "okay") || !strcmp(prop->value, "ok"));
>> +}
>> +struct device_node *of_transaction_get_child_by_name(
>> +		struct of_transaction *oft, struct device_node *node,
>> +		const char *name)
>> +{
>> +	struct of_transaction_entry *te;
>> +
>> +	/* possibly exists in the transaction list as
>> +	 * part of an attachment action
>> +	 */
>> +	list_for_each_entry_reverse(te, &oft->te_list, node) {
>> +
>> +		if (te->action != OF_RECONFIG_ATTACH_NODE &&
>> +		    te->action != OF_RECONFIG_DETACH_NODE)
>> +			continue;
>> +
>> +		/* look at the parent and if node matches return */
>> +		if (te->np->parent != node || strcmp(te->np->name, "name"))
>> +			continue;
>> +
>> +		pr_debug("%s: found child \"%s\" in transaction list @%s\n",
>> +			__func__, name, te->np->full_name);
>> +		return te->action == OF_RECONFIG_ATTACH_NODE ? te->np : NULL;
>> +	}
>> +
>> +	/* not found in the transaction list? try normal */
>> +	return of_get_child_by_name(node, name);
>> +}
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index b7c322c..fa81b42 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -821,4 +821,86 @@ typedef void (*of_init_fn_1)(struct device_node *);
>> #define OF_DECLARE_2(table, name, compat, fn) \
>> 		_OF_DECLARE(table, name, compat, fn, of_init_fn_2)
>> 
>> +/**
>> + * struct of_transaction_entry	- Holds a transaction entry
>> + *
>> + * @node:	list_head for the log list
>> + * @action:	notifier action
>> + * @np:		pointer to the device node affected
>> + * @prop:	pointer to the property affected
>> + * @old_prop:	hold a pointer to the original property
>> + *
>> + * Every modification of the device tree during a transaction
>> + * is held in a list of of_transaction_entry structures.
>> + * That way we can recover from a partial application, or we can
>> + * revert the transaction
>> + */
>> +struct of_transaction_entry {
>> +	struct list_head node;
>> +	unsigned long action;
>> +	struct device_node *np;
>> +	struct property *prop;
>> +	struct property *old_prop;
>> +};
>> +
>> +/**
>> + * struct of_transaction - transaction tracker structure
>> + *
>> + * @te_list:	list_head for the transaction entries
>> + *
>> + * Transactions are a convenient way to apply bulk changes to the
>> + * live tree. In case of an error, changes are rolled-back.
>> + * Transactions live on after initial application, and if not
>> + * destroyed after use, they can be reverted in one single call.
>> + */
>> +struct of_transaction {
>> +	struct list_head te_list;
>> +};
>> +
>> +#ifdef CONFIG_OF_DYNAMIC
>> +extern void of_transaction_init(struct of_transaction *oft);
>> +extern void of_transaction_destroy(struct of_transaction *oft);
>> +extern void of_transaction_start(struct of_transaction *oft);
>> +extern void of_transaction_abort(struct of_transaction *oft);
>> +extern int of_transaction_apply(struct of_transaction *oft, int force);
>> +extern int of_transaction_revert(struct of_transaction *oft, int force);
>> +extern int of_transaction_action(struct of_transaction *oft,
>> +		unsigned long action, struct device_node *np,
>> +		struct property *prop);
>> +
>> +static inline int of_transaction_attach_node(struct of_transaction *oft,
>> +		struct device_node *np)
>> +{
>> +	return of_transaction_action(oft, OF_RECONFIG_ATTACH_NODE, np, NULL);
>> +}
>> +
>> +static inline int of_transaction_detach_node(struct of_transaction *oft,
>> +		struct device_node *np)
>> +{
>> +	return of_transaction_action(oft,
>> +			OF_RECONFIG_DETACH_NODE, np, NULL);
>> +}
>> +
>> +static inline int of_transaction_add_property(struct of_transaction *oft,
>> +		struct device_node *np, struct property *prop)
>> +{
>> +	return of_transaction_action(oft,
>> +			OF_RECONFIG_ADD_PROPERTY, np, prop);
>> +}
>> +
>> +static inline int of_transaction_remove_property(struct of_transaction *oft,
>> +		struct device_node *np, struct property *prop)
>> +{
>> +	return of_transaction_action(oft,
>> +			OF_RECONFIG_REMOVE_PROPERTY, np, prop);
>> +}
>> +
>> +static inline int of_transaction_update_property(struct of_transaction *oft,
>> +		struct device_node *np, struct property *prop)
>> +{
>> +	return of_transaction_action(oft,
>> +			OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>> +}
>> +#endif
>> +
>> #endif /* _LINUX_OF_H */
>> -- 
>> 1.7.12
>> 
> 

Regards

-- Pantelis

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] of: Do not free memory at of_node_release option
  2014-07-09 18:18       ` Grant Likely
@ 2014-07-11 20:29         ` Pantelis Antoniou
  0 siblings, 0 replies; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-11 20:29 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

Hi Grant,

On Jul 9, 2014, at 9:18 PM, Grant Likely wrote:

> On Mon, 7 Jul 2014 16:13:42 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>> Hi Grant,
>> 
>> On Jul 7, 2014, at 4:10 PM, Grant Likely wrote:
>> 
>>> On Fri,  4 Jul 2014 19:58:45 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>>>> Introduce an option whether to free or not the memory of a node
>>>> and it's properties when of_node_release is called.
>>>> 
>>>> The option is located in the chosen node, and its name is
>>>> of-node-keep. When this boolean property is missing the default
>>>> behaviour of freeing is preserved.
>>> 
>>> No way. The problem is buggy kernel code. We've got a plan to fix that.
>>> In the mean time I'm okay with a CONFIG_ option that inhibits freeing,
>>> or *maybe* a runtime global option that causes free() to not be called.
>>> 
>>> What this patch does is not helpful.
>>> 
>> 
>> Well, it was a long shot. FWIW I don't need it for overlays, this is just to
>> make sure we don't crash in case we have a buggy driver calling of_node_put()
>> twice or something.
>> 
>> I'm open to suggestions on how to track and fix those.
> 
> Switching to RCU is the first step. That lets us convert users into
> using rcu barriers instead of manually tracking of_node_get/put
> ordering. I would also like to talk to Julie about getting coccinelle
> report on node and property pointer references being held outside the
> rcu barriers without an of_node_get()
> 

Sounds good to me. Let's see what monsters lie under the bed over there.

>> 
>>> g.
>>> 
>> 

Regards

-- Pantelis

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] of: Transactional DT support.
  2014-07-11 20:28     ` Pantelis Antoniou
@ 2014-07-14 18:09       ` Grant Likely
  2014-07-15  6:41         ` Pantelis Antoniou
  2014-07-16 13:39       ` Grant Likely
  1 sibling, 1 reply; 24+ messages in thread
From: Grant Likely @ 2014-07-14 18:09 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree@vger.kernel.org,
	Wolfram Sang, Linux I2C, Mark Brown, linux-spi,
	Linux Kernel Mailing List, Pete Popov, Dan Malek, Georgi Vlaev

On Fri, Jul 11, 2014 at 2:28 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
>
> On Jul 11, 2014, at 8:24 AM, Grant Likely wrote:
>
>> On Fri,  4 Jul 2014 19:58:49 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>>> Introducing DT transactional support.
>>>
>>> A DT transaction is a method which allows one to apply changes
>>> in the live tree, in such a way that either the full set of changes
>>> take effect, or the state of the tree can be rolled-back to the
>>> state it was before it was attempted. An applied transaction
>>> can be rolled-back at any time.
>>>
>>> If any transaction results in the change of a state of a device
>>> node, a notifier for that node will be fired which allows a bus
>>> to create/destroy devices as directed.
>>>
>>> Documentation is in
>>>      Documentation/devicetree/transactions.txt
>>
>> Hi Pantelis,
>>
>> I've got a lot of comments below, but you don't need to respin the
>> series immediately. I've got a tree that I'm working with to sort out
>> what I can queue up for the next kernel. Give me a few days and I'll
>> send you what I've gotten to.
>>
>
> OK, that would be good.

It is rather rough, and I haven't squashed the patches together yet,
but you can see what I have here. I'm busy writing some basic
testcases to prove out the functionality before I squash things down.

g.

The following changes since commit cd3de83f147601356395b57a8673e9c5ff1e59d1:

  Linux 3.16-rc4 (2014-07-06 12:37:51 -0700)

are available in the git repository at:

  git://git.secretlab.ca/git/linux devicetree/next-overlay

for you to fetch changes up to 35129420765f8312f2f08bcec68e5daf0f0f7bff:

  fixup: Fix error when applying node changes (2014-07-14 11:54:20 -0600)

----------------------------------------------------------------
Grant Likely (18):
      of/platform: Fix of_platform_device_destroy iteration of devices
      of: Move CONFIG_OF_DYNAMIC code into a separate file
      fixup: Make of_kset non-static
      fixup: Remove the device state notifiers
      fixup: roll of_transaction_entry_create into of_transaction_action()
      fixup: remove of_transaction_device_is_available()
      fixup: remove of_transaction_find_property() and
of_transaction_get_child_by_name()
      fixup: eliminate the 'force' arguement to of_transaction_{apply,revert}
      fixup: emit all notifiers at once instead of one at a time while
changes are being applied
      fixup: get rid of destroying transaction on revert
      fixup: remove of_transaction_{start,abort}
      fixup: Add helper to invert a transaction entry
      fixup: implement of_transaction_entry_revert() using
of_transaction_entry_apply()
      fixup: move the dump operation into of_transaction_entry_apply()
      fixup: s/transaction/changeset/g
      fixup: Move changeset code into dynamic.c
      fixup: Add changeset testcase
      fixup: Fix error when applying node changes

Pantelis Antoniou (5):
      of: rename of_aliases_mutex to just of_mutex
      OF: Utility helper functions for dynamic nodes
      of: Create unlocked versions of node and property add/remove functions
      of: Make devicetree sysfs update functions consistent.
      of: Transactional DT support.

 Documentation/devicetree/changesets.txt |  41 ++
 drivers/of/Kconfig                      |   2 +-
 drivers/of/Makefile                     |   1 +
 drivers/of/base.c                       | 410 +++++---------------
 drivers/of/device.c                     |   4 +-
 drivers/of/dynamic.c                    | 666 ++++++++++++++++++++++++++++++++
 drivers/of/of_private.h                 |  58 ++-
 drivers/of/platform.c                   |  32 +-
 drivers/of/selftest.c                   |  73 ++++
 drivers/of/testcase-data/testcases.dtsi |  10 +
 include/linux/of.h                      |  79 +++-
 include/linux/of_platform.h             |   7 +-
 12 files changed, 1040 insertions(+), 343 deletions(-)
 create mode 100644 Documentation/devicetree/changesets.txt
 create mode 100644 drivers/of/dynamic.c

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] of: Transactional DT support.
  2014-07-14 18:09       ` Grant Likely
@ 2014-07-15  6:41         ` Pantelis Antoniou
  2014-07-16  5:54           ` Grant Likely
  0 siblings, 1 reply; 24+ messages in thread
From: Pantelis Antoniou @ 2014-07-15  6:41 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree@vger.kernel.org,
	Wolfram Sang, Linux I2C, Mark Brown, linux-spi,
	Linux Kernel Mailing List, Pete Popov, Dan Malek, Georgi Vlaev

Hi Grant,

On Jul 14, 2014, at 9:09 PM, Grant Likely wrote:

> On Fri, Jul 11, 2014 at 2:28 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Hi Grant,
>> 
>> On Jul 11, 2014, at 8:24 AM, Grant Likely wrote:
>> 
>>> On Fri,  4 Jul 2014 19:58:49 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>>>> Introducing DT transactional support.
>>>> 
>>>> A DT transaction is a method which allows one to apply changes
>>>> in the live tree, in such a way that either the full set of changes
>>>> take effect, or the state of the tree can be rolled-back to the
>>>> state it was before it was attempted. An applied transaction
>>>> can be rolled-back at any time.
>>>> 
>>>> If any transaction results in the change of a state of a device
>>>> node, a notifier for that node will be fired which allows a bus
>>>> to create/destroy devices as directed.
>>>> 
>>>> Documentation is in
>>>>     Documentation/devicetree/transactions.txt
>>> 
>>> Hi Pantelis,
>>> 
>>> I've got a lot of comments below, but you don't need to respin the
>>> series immediately. I've got a tree that I'm working with to sort out
>>> what I can queue up for the next kernel. Give me a few days and I'll
>>> send you what I've gotten to.
>>> 
>> 
>> OK, that would be good.
> 
> It is rather rough, and I haven't squashed the patches together yet,
> but you can see what I have here. I'm busy writing some basic
> testcases to prove out the functionality before I squash things down.
> 

Thanks for that. I'll pick this branch up and perform some testing from my
own side.

> g.
> 

Regards

-- Pantelis

> The following changes since commit cd3de83f147601356395b57a8673e9c5ff1e59d1:
> 
>  Linux 3.16-rc4 (2014-07-06 12:37:51 -0700)
> 
> are available in the git repository at:
> 
>  git://git.secretlab.ca/git/linux devicetree/next-overlay
> 
> for you to fetch changes up to 35129420765f8312f2f08bcec68e5daf0f0f7bff:
> 
>  fixup: Fix error when applying node changes (2014-07-14 11:54:20 -0600)
> 
> ----------------------------------------------------------------
> Grant Likely (18):
>      of/platform: Fix of_platform_device_destroy iteration of devices
>      of: Move CONFIG_OF_DYNAMIC code into a separate file
>      fixup: Make of_kset non-static
>      fixup: Remove the device state notifiers
>      fixup: roll of_transaction_entry_create into of_transaction_action()
>      fixup: remove of_transaction_device_is_available()
>      fixup: remove of_transaction_find_property() and
> of_transaction_get_child_by_name()
>      fixup: eliminate the 'force' arguement to of_transaction_{apply,revert}
>      fixup: emit all notifiers at once instead of one at a time while
> changes are being applied
>      fixup: get rid of destroying transaction on revert
>      fixup: remove of_transaction_{start,abort}
>      fixup: Add helper to invert a transaction entry
>      fixup: implement of_transaction_entry_revert() using
> of_transaction_entry_apply()
>      fixup: move the dump operation into of_transaction_entry_apply()
>      fixup: s/transaction/changeset/g
>      fixup: Move changeset code into dynamic.c
>      fixup: Add changeset testcase
>      fixup: Fix error when applying node changes
> 
> Pantelis Antoniou (5):
>      of: rename of_aliases_mutex to just of_mutex
>      OF: Utility helper functions for dynamic nodes
>      of: Create unlocked versions of node and property add/remove functions
>      of: Make devicetree sysfs update functions consistent.
>      of: Transactional DT support.
> 
> Documentation/devicetree/changesets.txt |  41 ++
> drivers/of/Kconfig                      |   2 +-
> drivers/of/Makefile                     |   1 +
> drivers/of/base.c                       | 410 +++++---------------
> drivers/of/device.c                     |   4 +-
> drivers/of/dynamic.c                    | 666 ++++++++++++++++++++++++++++++++
> drivers/of/of_private.h                 |  58 ++-
> drivers/of/platform.c                   |  32 +-
> drivers/of/selftest.c                   |  73 ++++
> drivers/of/testcase-data/testcases.dtsi |  10 +
> include/linux/of.h                      |  79 +++-
> include/linux/of_platform.h             |   7 +-
> 12 files changed, 1040 insertions(+), 343 deletions(-)
> create mode 100644 Documentation/devicetree/changesets.txt
> create mode 100644 drivers/of/dynamic.c

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] of: Transactional DT support.
  2014-07-15  6:41         ` Pantelis Antoniou
@ 2014-07-16  5:54           ` Grant Likely
  0 siblings, 0 replies; 24+ messages in thread
From: Grant Likely @ 2014-07-16  5:54 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree@vger.kernel.org,
	Wolfram Sang, Linux I2C, Mark Brown, linux-spi,
	Linux Kernel Mailing List, Pete Popov, Dan Malek, Georgi Vlaev

On Tue, Jul 15, 2014 at 12:41 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
>
> On Jul 14, 2014, at 9:09 PM, Grant Likely wrote:
>
>> On Fri, Jul 11, 2014 at 2:28 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Hi Grant,
>>>
>>> On Jul 11, 2014, at 8:24 AM, Grant Likely wrote:
>>>
>>>> On Fri,  4 Jul 2014 19:58:49 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>>>>> Introducing DT transactional support.
>>>>>
>>>>> A DT transaction is a method which allows one to apply changes
>>>>> in the live tree, in such a way that either the full set of changes
>>>>> take effect, or the state of the tree can be rolled-back to the
>>>>> state it was before it was attempted. An applied transaction
>>>>> can be rolled-back at any time.
>>>>>
>>>>> If any transaction results in the change of a state of a device
>>>>> node, a notifier for that node will be fired which allows a bus
>>>>> to create/destroy devices as directed.
>>>>>
>>>>> Documentation is in
>>>>>     Documentation/devicetree/transactions.txt
>>>>
>>>> Hi Pantelis,
>>>>
>>>> I've got a lot of comments below, but you don't need to respin the
>>>> series immediately. I've got a tree that I'm working with to sort out
>>>> what I can queue up for the next kernel. Give me a few days and I'll
>>>> send you what I've gotten to.
>>>>
>>>
>>> OK, that would be good.
>>
>> It is rather rough, and I haven't squashed the patches together yet,
>> but you can see what I have here. I'm busy writing some basic
>> testcases to prove out the functionality before I squash things down.
>>
>
> Thanks for that. I'll pick this branch up and perform some testing from my
> own side.

I pushed out some extra bug fixes and new changes tonight, so make
sure you grab the latest tree.

g.

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] of: Introduce tree change __foo_post methods.
  2014-07-11 20:04     ` Pantelis Antoniou
@ 2014-07-16 13:20       ` Grant Likely
  0 siblings, 0 replies; 24+ messages in thread
From: Grant Likely @ 2014-07-16 13:20 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

On Fri, 11 Jul 2014 23:04:55 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
> 
> On Jul 10, 2014, at 3:55 AM, Grant Likely wrote:
> 
> > On Fri,  4 Jul 2014 19:58:48 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> >> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >> index 0d50318..b52fd6b 100644
> >> --- a/drivers/of/base.c
> >> +++ b/drivers/of/base.c
> >> @@ -127,13 +127,24 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
> >> 	return name;
> >> }
> >> 
> >> -static int __of_add_property_sysfs(struct device_node *np, struct property *pp)
> >> +static int __of_add_property_sysfs(struct device_node *np, struct property *pp,
> >> +		int ignore_dup_name)
> >> {
> >> 	int rc;
> >> 
> >> 	/* Important: Don't leak passwords */
> >> 	bool secure = strncmp(pp->name, "security-", 9) == 0;
> >> 
> >> +	/* ignore duplicate name (transaction case) */
> >> +	if (ignore_dup_name) {
> >> +		struct kernfs_node *kn = sysfs_get_dirent(np->kobj.sd,
> >> +				pp->name);
> >> +		if (kn) {
> >> +			sysfs_put(kn);
> >> +			return 0;
> >> +		}
> >> +	}
> >> +
> > 
> > What is this? There is no mention of it in the commit text, and the comment
> > is far from sufficient. Under what conditions would this be valid? I'm
> > particularly concerned because this changes behaviour and the new
> > behaviour is used in the next hunk, so clearly it's not
> > transaction-only?
> > 
> > I've dropped this hunk from the version I have in my tree.
> > 
> 
> You need it. When you attach a new node to the tree, and then add a new
> property to the same node, you end up calling __of_add_property_sysfs twice.
> 
> This is the easiest way to get around the problem.

Ugh. That's nasty. It only papers over the problem and when something
goes wrong it will silently fail. If this is necessary then it says to
me that there is a problem in the design. Either the property should be
modified direcly on the node before the node is attached, or modifying
properties on an added node should be illegal.

> >> +
> >> /**
> >>  * of_attach_node - Plug a device node into the tree and global list.
> >>  */
> >> @@ -120,7 +125,7 @@ int of_attach_node(struct device_node *np)
> >> 	__of_attach_node(np);
> >> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >> 
> >> -	of_node_add(np);
> >> +	__of_attach_node_post(np);
> >> 	return 0;
> >> }
> >> 
> >> @@ -160,6 +165,11 @@ void __of_detach_node(struct device_node *np)
> >> 	of_node_set_flag(np, OF_DETACHED);
> >> }
> >> 
> >> +void __of_detach_node_post(struct device_node *np)
> >> +{
> >> +	of_node_remove(np);
> >> +}
> > 
> > Also silly.
> > 
> 
> It's regular...

Heh, my point was that of_node_remove() can be renamed.

g.

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] of: Transactional DT support.
  2014-07-11 20:28     ` Pantelis Antoniou
  2014-07-14 18:09       ` Grant Likely
@ 2014-07-16 13:39       ` Grant Likely
  1 sibling, 0 replies; 24+ messages in thread
From: Grant Likely @ 2014-07-16 13:39 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Stephen Warren, Matt Porter, Koen Kooi,
	Greg Kroah-Hartman, Alison Chaiken, Dinh Nguyen, Jan Lubbe,
	Alexander Sverdlin, Michael Stickel, Guenter Roeck, Dirk Behme,
	Alan Tull, Sascha Hauer, Michael Bohan, Ionut Nicu, Michal Simek,
	Matt Ranostay, Joel Becker, devicetree, Wolfram Sang, linux-i2c,
	Mark Brown, linux-spi, linux-kernel, Pete Popov, Dan Malek,
	Georgi Vlaev

On Fri, 11 Jul 2014 23:28:45 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> On Jul 11, 2014, at 8:24 AM, Grant Likely wrote:
> > On Fri,  4 Jul 2014 19:58:49 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> >> +static int __of_transaction_entry_apply(struct of_transaction *oft,
> >> +		struct of_transaction_entry *te)
> >> +{
> >> +	struct property *old_prop;
> >> +	unsigned long flags;
> >> +	int ret, state;
> >> +
> >> +	ret = 0;
> >> +
> >> +	state = __of_transaction_entry_device_state(oft, te, 0);
> >> +
> >> +	switch (te->action) {
> >> +	case OF_RECONFIG_ATTACH_NODE:
> >> +	case OF_RECONFIG_DETACH_NODE:
> >> +		ret = of_reconfig_notify(te->action, te->np);
> >> +		break;
> >> +	case OF_RECONFIG_ADD_PROPERTY:
> >> +	case OF_RECONFIG_REMOVE_PROPERTY:
> >> +	case OF_RECONFIG_UPDATE_PROPERTY:
> >> +		ret = of_property_notify(te->action, te->np, te->prop);
> >> +		break;
> >> +	}
> > 
> > The way I read this, the notifier is sent as each entry is applied. So
> > the sequence would be:
> > 	entry 1 notify
> > 	entry 1 apply
> > 	entry 2 notify
> > 	entry 2 apply
> > 	etc.
> > Correct?
> > 
> Yes.
> 
> > For this patch, that is probably exactly what should happen because it
> > matches the existing behaviour. However, it isn't what we want. We need
> > a seperate patch to reorganize the notifiers to be emitted after the
> > change takes place as we discussed on IRC, and powerpc needs to be moved
> > to the new model. At the same time, all of the notifiers need to be
> > emitted together instead of interleaved with the individual entries so
> > that the order looks like this:
> > 	entry 1 apply
> > 	entry 2 apply
> > 	etc.
> > 	entry 1 notify
> > 	entry 2 notify
> > 	etc.
> 
> It is simple enough to do. We will break anyone that depended on the old
> notifier behaviour, but I doubt there's anyone out there.

PowerPC has some dependency, but I'm working on fixing that.

> > 
> >> +
> >> +	if (ret) {
> >> +		pr_err("%s: notifier error @%s\n", __func__,
> >> +				te->np->full_name);
> >> +		return ret;
> >> +	}
> >> +
> >> +	mutex_lock(&of_mutex);
> > 
> > This doesn't look right. The mutex need to be held over the entire time
> > of applying changes. It cannot be dropped between entries of the
> > changeset. ie:
> > 
> > 	mutex_lock()
> > 	entry 1 apply
> > 	entry 2 apply
> > 	etc.
> > 	mutex_unlock()
> > 	entry 1 notify
> > 	entry 2 notify
> > 	etc.
> > 
> > If the mutex gets obtained and droped for each entry, then there is no
> > protection against another thread making a modification in the middle.
> > 
> 
> Remember that of_mutex is not the only mutex at play. Overlays have
> another mutex taken before applying anything.

I want of_mutex to be available to cover the entire period where the
tree needs to be stable for the purpose of making modifications,
regardless of the user. Overlays is just one user and I don't want to
relay on overlay locking to get the core behaviour correct.

g.

^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2014-07-16 13:39 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-04 16:58 [PATCH 0/5] Transaction DT support (v2) Pantelis Antoniou
2014-07-04 16:58 ` [PATCH 1/5] of: Do not free memory at of_node_release option Pantelis Antoniou
2014-07-07 13:10   ` Grant Likely
2014-07-07 13:13     ` Pantelis Antoniou
2014-07-09 18:18       ` Grant Likely
2014-07-11 20:29         ` Pantelis Antoniou
2014-07-04 16:58 ` [PATCH 2/5] OF: Export a number of __of_* methods Pantelis Antoniou
2014-07-07 13:11   ` Grant Likely
2014-07-07 13:17     ` Pantelis Antoniou
2014-07-09 18:58       ` Grant Likely
2014-07-04 16:58 ` [PATCH 3/5] OF: Utility helper functions for dynamic nodes Pantelis Antoniou
2014-07-07  7:04   ` Alexander Sverdlin
2014-07-07 13:13   ` Grant Likely
2014-07-04 16:58 ` [PATCH 4/5] of: Introduce tree change __foo_post methods Pantelis Antoniou
2014-07-10  0:55   ` Grant Likely
2014-07-11 20:04     ` Pantelis Antoniou
2014-07-16 13:20       ` Grant Likely
2014-07-04 16:58 ` [PATCH 5/5] of: Transactional DT support Pantelis Antoniou
2014-07-11  5:24   ` Grant Likely
2014-07-11 20:28     ` Pantelis Antoniou
2014-07-14 18:09       ` Grant Likely
2014-07-15  6:41         ` Pantelis Antoniou
2014-07-16  5:54           ` Grant Likely
2014-07-16 13:39       ` Grant Likely

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.