Re: [PATCH 0/3] Synaptics - fix lockdep warnings
From: Jiri Kosina
Date: Thu Sep 14 2006 - 10:40:04 EST
On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
> Can we add lock_class_key to the struct psmouse and use it to define
> per-device mutex class regardless of whether it is a child, grandchild
> or a parent?
Hi Dmitry,
what do you think about the patches below? I have used a slightly
different approach, as we also need to get rid of the spurious lockdep
warning in case of recursive call of serio_interrupt(), which can't be
handled well with lock subclass stored in struct psmouse. What do you
think about this? It shuts up the lockdep, and seems much cleaner to me.
The first patch implements spin_lock_irqsave_nested(), which is needed by
the second one:
Signed-off-by: Jiri Kosina <jikos@xxxxxxxx>
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_smp.h linux-2.6.18-rc6-mm2/include/linux/spinlock_api_smp.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_smp.h 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock_api_smp.h 2006-09-14 01:24:08.000000000 +0200
@@ -32,6 +32,8 @@ void __lockfunc _read_lock_irq(rwlock_t
void __lockfunc _write_lock_irq(rwlock_t *lock) __acquires(lock);
unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
__acquires(lock);
+unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
+ __acquires(spinlock_t);
unsigned long __lockfunc _read_lock_irqsave(rwlock_t *lock)
__acquires(lock);
unsigned long __lockfunc _write_lock_irqsave(rwlock_t *lock)
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_up.h linux-2.6.18-rc6-mm2/include/linux/spinlock_api_up.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_up.h 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock_api_up.h 2006-09-14 01:24:05.000000000 +0200
@@ -59,6 +59,7 @@
#define _read_lock_irq(lock) __LOCK_IRQ(lock)
#define _write_lock_irq(lock) __LOCK_IRQ(lock)
#define _spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
+#define _spin_lock_irqsave_nested(lock, flags, subclass) __LOCK_IRQSAVE(lock, flags, subclass)
#define _read_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
#define _write_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
#define _spin_trylock(lock) ({ __LOCK(lock); 1; })
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock.h linux-2.6.18-rc6-mm2/include/linux/spinlock.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock.h 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock.h 2006-09-14 01:24:12.000000000 +0200
@@ -186,6 +186,11 @@ do { \
#define spin_lock_irqsave(lock, flags) flags = _spin_lock_irqsave(lock)
#define read_lock_irqsave(lock, flags) flags = _read_lock_irqsave(lock)
#define write_lock_irqsave(lock, flags) flags = _write_lock_irqsave(lock)
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define spin_lock_irqsave_nested(lock, flags, subclass) flags = _spin_lock_irqsave_nested(lock, subclass)
+#else
+#define spin_lock_irqsave_nested(lock, flags, subclass) flags = _spin_lock_irqsave(lock)
+#endif
#else
#define spin_lock_irqsave(lock, flags) _spin_lock_irqsave(lock, flags)
#define read_lock_irqsave(lock, flags) _read_lock_irqsave(lock, flags)
diff -rup linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c linux-2.6.18-rc6-mm2/kernel/spinlock.c
--- linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/kernel/spinlock.c 2006-09-14 01:27:17.000000000 +0200
@@ -304,6 +304,27 @@ void __lockfunc _spin_lock_nested(spinlo
}
EXPORT_SYMBOL(_spin_lock_nested);
+unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ preempt_disable();
+ spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+ /*
+ * On lockdep we dont want the hand-coded irq-enable of
+ * _raw_spin_lock_flags() code, because lockdep assumes
+ * that interrupts are not re-enabled during lock-acquire:
+ */
+#ifdef CONFIG_PROVE_SPIN_LOCKING
+ _raw_spin_lock(lock);
+#else
+ _raw_spin_lock_flags(lock, &flags);
+#endif
+ return flags;
+}
+
+EXPORT_SYMBOL(_spin_lock_irqsave_nested);
#endif
And this second one actually fixes the locking with respect to lockdep
validator:
Signed-off-by: Jiri Kosina <jikos@xxxxxxxx>
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/serio/libps2.c linux-2.6.18-rc6-mm2/drivers/input/serio/libps2.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/serio/libps2.c 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/serio/libps2.c 2006-09-14 16:16:06.000000000 +0200
@@ -182,7 +182,7 @@ int ps2_command(struct ps2dev *ps2dev, u
return -1;
}
- mutex_lock_nested(&ps2dev->cmd_mutex, SINGLE_DEPTH_NESTING);
+ mutex_lock_nested(&ps2dev->cmd_mutex, ps2dev->serio->depth);
serio_pause_rx(ps2dev->serio);
ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/serio/serio.c linux-2.6.18-rc6-mm2/drivers/input/serio/serio.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/serio/serio.c 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/serio/serio.c 2006-09-14 16:16:00.000000000 +0200
@@ -553,9 +553,11 @@ static void serio_add_port(struct serio
if (serio->parent) {
serio_pause_rx(serio->parent);
serio->parent->child = serio;
+ serio->depth = serio->parent->depth + 1;
serio_continue_rx(serio->parent);
- }
-
+ } else
+ serio->depth = 0;
+
list_add_tail(&serio->node, &serio_list);
if (serio->start)
serio->start(serio);
@@ -916,7 +918,7 @@ irqreturn_t serio_interrupt(struct serio
unsigned long flags;
irqreturn_t ret = IRQ_NONE;
- spin_lock_irqsave(&serio->lock, flags);
+ spin_lock_irqsave_nested(&serio->lock, flags, serio->depth);
if (likely(serio->drv)) {
ret = serio->drv->interrupt(serio, data, dfl, regs);
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/serio.h linux-2.6.18-rc6-mm2/include/linux/serio.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/serio.h 2006-09-14 16:20:56.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/serio.h 2006-09-14 15:15:41.000000000 +0200
@@ -41,6 +41,7 @@ struct serio {
void (*stop)(struct serio *);
struct serio *parent, *child;
+ unsigned int depth; /* level of nesting in parent-child hierarichy of serios */
struct serio_driver *drv; /* accessed from interrupt, must be protected by serio->lock and serio->sem */
struct mutex drv_mutex; /* protects serio->drv so attributes can pin driver */
--
JiKos.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/