Skip to contentSkip to conversation

of: Fix locking vs. interrupts

PATCH Applied Benjamin Herrenschmidt 4 participants 5 messages Jun 12, 2013

Subsystems (2)
SPARC + UltraSPARC (sparc/sparc64) Device tree
lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 1/1 of: Fix locking vs. interrupts

The OF code uses irqsafe locks everywhere except in a handful of functions for no obvious reasons. Since the conversion from the old rwlocks, this now triggers lockdep warnings when used at interrupt time. At least one driver (ibmvscsi) seems to be doing that from softirq context.

This converts the few non-irqsafe locks into irqsafe ones, making them consistent with the rest of the code.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
CC <[email protected]> [v3.9+]

---

Note: It's silly to access the device-tree at interrupt time in most cases, and we should probably fix ibmvscsi, but for the time being, let's fix the obvious bug. Thomas, this can probably still go into 3.10... If not, I've CCed stable.

arch/sparc/kernel/prom_common.c +3 -2
index 9f20566..79cc0d1 100644
@@ -54,6 +54,7 @@ EXPORT_SYMBOL(of_set_property_mutex);
 int of_set_property(struct device_node *dp, const char *name, void *val, int len)
 {
 	struct property **prevp;
+	unsigned long flags;
 	void *new_val;
 	int err;
 

@@ -64,7 +65,7 @@ int of_set_property(struct device_node *dp, const char *name, void *val, int len
 	err = -ENODEV;
 
 	mutex_lock(&of_set_property_mutex);
-	raw_spin_lock(&devtree_lock);
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 	prevp = &dp->properties;
 	while (*prevp) {
 		struct property *prop = *prevp;

@@ -91,7 +92,7 @@ int of_set_property(struct device_node *dp, const char *name, void *val, int len
 		}
 		prevp = &(*prevp)->next;
 	}
-	raw_spin_unlock(&devtree_lock);
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	mutex_unlock(&of_set_property_mutex);
 
 	/* XXX Upate procfs if necessary... */
drivers/of/base.c +9 -6
index f53b992..951452c 100644
@@ -192,14 +192,15 @@ EXPORT_SYMBOL(of_find_property);
 struct device_node *of_find_all_nodes(struct device_node *prev)
 {
 	struct device_node *np;
+	unsigned long flags;
 
-	raw_spin_lock(&devtree_lock);
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np = prev ? prev->allnext : of_allnodes;
 	for (; np != NULL; np = np->allnext)
 		if (of_node_get(np))
 			break;
 	of_node_put(prev);
-	raw_spin_unlock(&devtree_lock);
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
 EXPORT_SYMBOL(of_find_all_nodes);

@@ -421,8 +422,9 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
 	struct device_node *prev)
 {
 	struct device_node *next;
+	unsigned long flags;
 
-	raw_spin_lock(&devtree_lock);
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 	next = prev ? prev->sibling : node->child;
 	for (; next; next = next->sibling) {
 		if (!__of_device_is_available(next))

@@ -431,7 +433,7 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
 			break;
 	}
 	of_node_put(prev);
-	raw_spin_unlock(&devtree_lock);
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return next;
 }
 EXPORT_SYMBOL(of_get_next_available_child);

@@ -735,13 +737,14 @@ EXPORT_SYMBOL_GPL(of_modalias_node);
 struct device_node *of_find_node_by_phandle(phandle handle)
 {
 	struct device_node *np;
+	unsigned long flags;
 
-	raw_spin_lock(&devtree_lock);
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 	for (np = of_allnodes; np; np = np->allnext)
 		if (np->phandle == handle)
 			break;
 	of_node_get(np);
-	raw_spin_unlock(&devtree_lock);
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
 EXPORT_SYMBOL(of_find_node_by_phandle);
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · Benjamin Herrenschmidt

The OF code uses irqsafe locks everywhere except in a handful of functions for no obvious reasons. Since the conversion from the old rwlocks, this now triggers lockdep warnings when used at interrupt time. At least one driver (ibmvscsi) seems to be doing that from softirq context.

This converts the few non-irqsafe locks into irqsafe ones, making them consistent with the rest of the code.

Fun. https://lkml.org/lkml/2013/2/4/416 seems to have got lost

Quoted 2 lines
Signed-off-by Benjamin Herrenschmidt <[email protected]>
CC <[email protected]> [v3.9+]
Acked-by Thomas Gleixner <[email protected]>
Quoted 4 lines

---

Note: It's silly to access the device-tree at interrupt time in most cases, and we should probably fix ibmvscsi, but for the time being, let's fix the

Right.

Quoted 2 lines

obvious bug. Thomas, this can probably still go into 3.10... If not, I've CCed stable.

Should go through Grant I think.

Thanks,

tglx

author
#2 lore raw
Quoted 22 lines · Thomas Gleixner
Quoted 7 lines · Benjamin Herrenschmidt

The OF code uses irqsafe locks everywhere except in a handful of functions for no obvious reasons. Since the conversion from the old rwlocks, this now triggers lockdep warnings when used at interrupt time. At least one driver (ibmvscsi) seems to be doing that from softirq context.

This converts the few non-irqsafe locks into irqsafe ones, making them consistent with the rest of the code.

Fun. https://lkml.org/lkml/2013/2/4/416 seems to have got lost

Quoted 2 lines
Signed-off-by Benjamin Herrenschmidt <[email protected]>
CC <[email protected]> [v3.9+]
Acked-by Thomas Gleixner <[email protected]>
Quoted 4 lines

---

Note: It's silly to access the device-tree at interrupt time in most cases, and we should probably fix ibmvscsi, but for the time being, let's fix the

Quoted 6 lines

Right.

Quoted 2 lines

obvious bug. Thomas, this can probably still go into 3.10... If not, I've CCed stable.

Should go through Grant I think.

Right, thinko. Sent to you due to the bug being exposed by your conversion to spinlocks. Anyway, Grant got it now.

Cheers,
Ben.

Applied
#3 lore raw
In reply to Thomas Gleixner
Quoted 22 lines · Thomas Gleixner
Quoted 7 lines · Benjamin Herrenschmidt

The OF code uses irqsafe locks everywhere except in a handful of functions for no obvious reasons. Since the conversion from the old rwlocks, this now triggers lockdep warnings when used at interrupt time. At least one driver (ibmvscsi) seems to be doing that from softirq context.

This converts the few non-irqsafe locks into irqsafe ones, making them consistent with the rest of the code.

Fun. https://lkml.org/lkml/2013/2/4/416 seems to have got lost

Quoted 2 lines
Signed-off-by Benjamin Herrenschmidt <[email protected]>
CC <[email protected]> [v3.9+]
Acked-by Thomas Gleixner <[email protected]>
Quoted 4 lines

---

Note: It's silly to access the device-tree at interrupt time in most cases, and we should probably fix ibmvscsi, but for the time being, let's fix the

Quoted 6 lines

Right.

Quoted 2 lines

obvious bug. Thomas, this can probably still go into 3.10... If not, I've CCed stable.

Should go through Grant I think.

Applied, thanks.

g.

maintainer Acked-by
#4 lore raw

From: Grant Likely <[email protected]>
Date: Wed, 12 Jun 2013 10:06:12 +0100

Quoted 23 lines · Thomas Gleixner
Quoted 22 lines · Benjamin Herrenschmidt
Quoted 8 lines

The OF code uses irqsafe locks everywhere except in a handful of functions for no obvious reasons. Since the conversion from the old rwlocks, this now triggers lockdep warnings when used at interrupt time. At least one driver (ibmvscsi) seems to be doing that from softirq context.

This converts the few non-irqsafe locks into irqsafe ones, making them consistent with the rest of the code.

Fun. https://lkml.org/lkml/2013/2/4/416 seems to have got lost

Quoted 2 lines
Signed-off-by Benjamin Herrenschmidt <[email protected]>
CC <[email protected]> [v3.9+]
Acked-by Thomas Gleixner <[email protected]>
Quoted 4 lines

---

Note: It's silly to access the device-tree at interrupt time in most cases, and we should probably fix ibmvscsi, but for the time being, let's fix the

Quoted 8 lines
Quoted 6 lines

Right.

Quoted 2 lines

obvious bug. Thomas, this can probably still go into 3.10... If not, I've CCed stable.

Should go through Grant I think.

Applied, thanks.

FWIW:

Acked-by David S. Miller <[email protected]>