[patch] sparc: Remove bogus of_set_property_mutex

[patch] sparc: Remove bogus of_set_property_mutex

[Thomas Gleixner]

of_set_property_mutex is taken inside the devtree_lock write locked
region which triggers the might_sleep check. The mutex protects the
call to prom_setprop() which is not necessary as the code is the only
caller of prom_setprop() and already serialized by devtree_lock. The
mutex is nowhere else used despite being exported. So it can be
removed safely.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
---
 arch/sparc/include/asm/prom.h   |    2 --
 arch/sparc/kernel/prom_common.c |    5 -----
 2 files changed, 7 deletions(-)

Index: linux-2.6/arch/sparc/include/asm/prom.h
=================================--- linux-2.6.orig/arch/sparc/include/asm/prom.h
+++ linux-2.6/arch/sparc/include/asm/prom.h
@@ -18,7 +18,6 @@
  */
 #include <linux/types.h>
 #include <linux/proc_fs.h>
-#include <linux/mutex.h>
 #include <asm/atomic.h>
 
 #define OF_ROOT_NODE_ADDR_CELLS_DEFAULT	2
@@ -74,7 +73,6 @@ struct of_irq_controller {
 
 extern struct device_node *of_find_node_by_cpuid(int cpuid);
 extern int of_set_property(struct device_node *node, const char *name, void *val, int len);
-extern struct mutex of_set_property_mutex;
 extern int of_getintprop_default(struct device_node *np,
 				 const char *name,
 				 int def);
Index: linux-2.6/arch/sparc/kernel/prom_common.c
=================================--- linux-2.6.orig/arch/sparc/kernel/prom_common.c
+++ linux-2.6/arch/sparc/kernel/prom_common.c
@@ -62,9 +62,6 @@ int of_getintprop_default(struct device_
 }
 EXPORT_SYMBOL(of_getintprop_default);
 
-DEFINE_MUTEX(of_set_property_mutex);
-EXPORT_SYMBOL(of_set_property_mutex);
-
 int of_set_property(struct device_node *dp, const char *name, void *val, int len)
 {
 	struct property **prevp;
@@ -88,9 +85,7 @@ int of_set_property(struct device_node *
 			void *old_val = prop->value;
 			int ret;
 
-			mutex_lock(&of_set_property_mutex);
 			ret = prom_setprop(dp->node, name, val, len);
-			mutex_unlock(&of_set_property_mutex);
 
 			err = -EINVAL;
 			if (ret >= 0) {

Re: [patch] sparc: Remove bogus of_set_property_mutex

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 07 Nov 2009 18:58:13 -0000

> of_set_property_mutex is taken inside the devtree_lock write locked
> region which triggers the might_sleep check. The mutex protects the
> call to prom_setprop() which is not necessary as the code is the only
> caller of prom_setprop() and already serialized by devtree_lock. The
> mutex is nowhere else used despite being exported. So it can be
> removed safely.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Please see commit 2481d76615d5e15340ccfb0243fe8779766dfc6e to see why
this mutex really is necessary, regardless of the locking done by the
caller(s) of of_set_property().  And how it will thus need to be used
in the future.

The long and short of it is that the firmware uses I2C accesses to
write the property values on some systems, and therefore the sparc64
I2C bus drivers will need this mutex to coordinate with callers of
prom_set_property().  Those I2C drivers aren't merged yet, but I
definitely plan to get them in soon :-)

Re: [patch] sparc: Remove bogus of_set_property_mutex

[Thomas Gleixner]

On Sat, 7 Nov 2009, David Miller wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Sat, 07 Nov 2009 18:58:13 -0000
> 
> > of_set_property_mutex is taken inside the devtree_lock write locked
> > region which triggers the might_sleep check. The mutex protects the
> > call to prom_setprop() which is not necessary as the code is the only
> > caller of prom_setprop() and already serialized by devtree_lock. The
> > mutex is nowhere else used despite being exported. So it can be
> > removed safely.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Please see commit 2481d76615d5e15340ccfb0243fe8779766dfc6e to see why
> this mutex really is necessary, regardless of the locking done by the
> caller(s) of of_set_property().  And how it will thus need to be used
> in the future.
> 
> The long and short of it is that the firmware uses I2C accesses to
> write the property values on some systems, and therefore the sparc64
> I2C bus drivers will need this mutex to coordinate with callers of
> prom_set_property().  Those I2C drivers aren't merged yet, but I
> definitely plan to get them in soon :-)

Then you need to fix the problem that you lock the mutex inside the
preempt disabled region under devtree_lock. :)

Thanks,

	tglx

Re: [patch] sparc: Remove bogus of_set_property_mutex

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sun, 8 Nov 2009 21:16:47 +0100 (CET)

> Then you need to fix the problem that you lock the mutex inside the
> preempt disabled region under devtree_lock. :)

That's simple, just move it outside the rwlock:

sparc: Move of_set_property_mutex acquisition outside of devtree_lock grab.

Otherwise we try to sleep with preemption disabled, etc.

Noticed by Thomas Gleixner.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/kernel/prom_common.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/sparc/kernel/prom_common.c b/arch/sparc/kernel/prom_common.c
index 138910c..d80a65d 100644
--- a/arch/sparc/kernel/prom_common.c
+++ b/arch/sparc/kernel/prom_common.c
@@ -79,6 +79,7 @@ int of_set_property(struct device_node *dp, const char *name, void *val, int len
 
 	err = -ENODEV;
 
+	mutex_lock(&of_set_property_mutex);
 	write_lock(&devtree_lock);
 	prevp = &dp->properties;
 	while (*prevp) {
@@ -88,9 +89,7 @@ int of_set_property(struct device_node *dp, const char *name, void *val, int len
 			void *old_val = prop->value;
 			int ret;
 
-			mutex_lock(&of_set_property_mutex);
 			ret = prom_setprop(dp->node, name, val, len);
-			mutex_unlock(&of_set_property_mutex);
 
 			err = -EINVAL;
 			if (ret >= 0) {
@@ -109,6 +108,7 @@ int of_set_property(struct device_node *dp, const char *name, void *val, int len
 		prevp = &(*prevp)->next;
 	}
 	write_unlock(&devtree_lock);
+	mutex_unlock(&of_set_property_mutex);
 
 	/* XXX Upate procfs if necessary... */
 
-- 
1.6.5.2