[PATCH] powerpc: add for_each_node_by_foo helpers

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> 
> > > I recently showed a variety of scenarios that would blow up in funny
> > > ways.. It's not enough.
> > 
> > A variety of scenarios?  I'm aware of only one [1], which is kind of a
> > corner case, and has never come up in practice to my knowledge.
> > 
> > If you know of others, I'd like to know the details.
> 
> I actually have one in mind that I remember, but I'm pretty sure I saw
> another one...

More details, please.  If you know of another issue besides the one
pointed out already, I'd prefer to know the nature of it before
putting more time into fixing this one.


> > Are we talking about the same thing?  I'm talking about the
> > of_find_node_by_foo iterators and they all hold the lock while
> > traversing the tree.
> 
> Yes but not between calls...

That's by design.  It's better to avoid exposing the lock to users of
the iterators.


> > So the basic issue is that traversing back into the tree from a node
> > that has been unplugged from the tree is invalid -- the state of the
> > tree is indeterminate with respect to the removed node's links into
> > it.
> > 
> > Seems to me that one way to address this is to keep a node from being
> > unplugged until all other users have dropped their references to it.

Here's an attempt at that -- build-tested only.  It's a bit hacky
since it's checking the kref refcount directly, but maybe I can come
up with something nicer later.

(It looks like klist would support doing the same thing, but at the
cost of bloating up struct device_node.)


@@ -1672,10 +1672,11 @@
  *		simplify writing of callers
  *
  */
-void of_node_put(struct device_node *node)
+int of_node_put(struct device_node *node)
 {
 	if (node)
-		kref_put(&node->kref, of_node_release);
+		return kref_put(&node->kref, of_node_release);
+	return 0;
 }
 EXPORT_SYMBOL(of_node_put);
 
@@ -1697,11 +1698,38 @@
  * a reference to the node.  The memory associated with the node
  * is not freed until its refcount goes to zero.
  */
-void of_detach_node(const struct device_node *np)
+void of_detach_node(struct device_node *np, int refs)
 {
 	struct device_node *parent;
 
+	/* Account for the reference the device tree itself has to the
+	 * node.
+	 */
+	of_node_put(np);
+retry:
+	/* Wait until refcount == the number of references this caller
+	 * has to the node.
+	 */
+	while (atomic_read(&np->kref.refcount) != refs)
+		schedule();
+
+	/* Someone else can grab a reference in this window... */
+
 	write_lock(&devtree_lock);
+
+	/* ... but not now (assuming they're using the proper
+	 * iterators).  Test whether the refcount got bumped in the
+	 * window and retry if necessary.
+	 *
+	 * The reason we do this is to prevent someone from iterating
+	 * from a detached node into the tree, e.g. through ->allnext.
+	 * The detached node's former neighbor in the list might not
+	 * be there.
+	 */
+	if (atomic_read(&np->kref.refcount) > refs) {
+		write_unlock(&devtree_lock);
+		goto retry;
+	}
 
 	parent = np->parent;
 
diff -r 8e71be242cff arch/powerpc/platforms/pseries/reconfig.c
--- a/arch/powerpc/platforms/pseries/reconfig.c	Mon Mar 13 08:41:27 2006 +0800
+++ b/arch/powerpc/platforms/pseries/reconfig.c	Mon Mar 13 01:52:53 2006 -0600
@@ -156,7 +156,7 @@
 	return err;
 }
 
-static int pSeries_reconfig_remove_node(struct device_node *np)
+static int pSeries_reconfig_remove_node(struct device_node *np, int refs)
 {
 	struct device_node *parent, *child;
 
@@ -173,10 +173,9 @@
 
 	notifier_call_chain(&pSeries_reconfig_chain,
 			    PSERIES_RECONFIG_REMOVE, np);
-	of_detach_node(np);
+	of_detach_node(np, refs);
 
 	of_node_put(parent);
-	of_node_put(np); /* Must decrement the refcount */
 	return 0;
 }
 
@@ -341,12 +340,13 @@
 static int do_remove_node(char *buf)
 {
 	struct device_node *node;
-	int rv = -ENODEV;
+	int deleted, rv = -ENODEV;
 
 	if ((node = of_find_node_by_path(buf)))
-		rv = pSeries_reconfig_remove_node(node);
-
-	of_node_put(node);
+		rv = pSeries_reconfig_remove_node(node, 1);
+
+	deleted = of_node_put(node);
+	BUG_ON(!deleted);
 	return rv;
 }
 
diff -r 8e71be242cff include/asm-powerpc/prom.h
--- a/include/asm-powerpc/prom.h	Mon Mar 13 08:41:27 2006 +0800
+++ b/include/asm-powerpc/prom.h	Mon Mar 13 01:52:53 2006 -0600
@@ -140,7 +140,7 @@
 					 const char *name,
 					 int *lenp);
 extern struct device_node *of_node_get(struct device_node *node);
-extern void of_node_put(struct device_node *node);
+extern int of_node_put(struct device_node *node);
 
 /* For scanning the flat device-tree at boot time */
 int __init of_scan_flat_dt(int (*it)(unsigned long node,
@@ -152,7 +152,7 @@
 
 /* For updating the device tree at runtime */
 extern void of_attach_node(struct device_node *);
-extern void of_detach_node(const struct device_node *);
+extern void of_detach_node(struct device_node *, int);
 
 /* Other Prototypes */
 extern void finish_device_tree(void);