Menu
▾
▴
emu10k1-devel
|
From: Takashi I. <ti...@su...> - 2006-05-30 12:44:31
|
At Tue, 30 May 2006 13:06:28 +0200,
Arjan van de Ven wrote:
>
> On Tue, 2006-05-30 at 147 +0159, Jiri Slaby wrote:
>
> (I've turned your backtrace upside down to show it "chronological")
>
> [<c05911e0>] alsa_emu10k1_synth_init+0x22/0x24
> [<c0333d04>] snd_seq_device_register_driver+0x8f/0xeb
>
> this one does:
>
> mutex_lock(&ops->reg_mutex);
> ...
> list_for_each(head, &ops->dev_list) {
> struct snd_seq_device *dev = list_entry(head, struct snd_seq_device, list);
> init_device(dev, ops);
> }
> mutex_unlock(&ops->reg_mutex);
>
> [<c0333537>] init_device+0x2c/0x94
> which calls into the driver
> [<c0352c39>] snd_emu10k1_synth_new_device+0xe7/0x14e
> [<c0353f50>] snd_emux_register+0x10d/0x13f
> [<c0358260>] snd_emux_init_seq_oss+0x35/0x9c
> [<c0333aa0>] snd_seq_device_new+0x96/0x111
>
> and this one does
> mutex_lock(&ops->reg_mutex);
> list_add_tail(&dev->list, &ops->dev_list);
> ops->num_devices++;
> mutex_unlock(&ops->reg_mutex);
>
>
> so... on first sight this looks like a real deadlock;
> unless the ALSA folks can tell me why "ops" is always different,
> and what the lock ordering rules between those is...
This ops is a unique object assigned to a different "id" string.
The first snd_seq_device_register_driver() called from emu10k1_synth.c
is the registration for the id "snd-synth-emu10k1".
Then in init_device(), the corresponding devices are initialized, and
one callback registers again another device for OSS sequencer with a
different id "snd-seq-oss" via snd_seq_device_new() inside the lock.
Now it hits the lock-detector but the lock should belong to a
different ops object in practice.
This nested lock may happen only in two drivers, emu10k1-synth and
opl3, and only together with OSS emulation. Since the OSS emulation
layer don't do active registration from itself, no deadlock should
happen (in theory -- I may oversee something :)
Takashi
|
|
From: Arjan v. de V. <ar...@li...> - 2006-05-30 12:59:30
|
On Tue, 2006-05-30 at 14:44 +0200, Takashi Iwai wrote:
> This ops is a unique object assigned to a different "id" string.
>
> The first snd_seq_device_register_driver() called from emu10k1_synth.c
> is the registration for the id "snd-synth-emu10k1".
> Then in init_device(), the corresponding devices are initialized, and
> one callback registers again another device for OSS sequencer with a
> different id "snd-seq-oss" via snd_seq_device_new() inside the lock.
> Now it hits the lock-detector but the lock should belong to a
> different ops object in practice.
>
> This nested lock may happen only in two drivers, emu10k1-synth and
> opl3, and only together with OSS emulation. Since the OSS emulation
> layer don't do active registration from itself, no deadlock should
> happen (in theory -- I may oversee something :)
ok fair enough
Jiri, can you test the patch below? (I don't have this hardware)
The ops structure has complex locking rules, where not all ops are
equal, some are subordinate on others for some complex sound cards. This
requires for lockdep checking that each individual reg_mutex is
considered in separation for its locking rules.
Signed-off-by: Arjan van de Ven <ar...@li...>
---
sound/core/seq/seq_device.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Index: linux-2.6.17-rc4-mm3-lockdep/sound/core/seq/seq_device.c
===================================================================
--- linux-2.6.17-rc4-mm3-lockdep.orig/sound/core/seq/seq_device.c
+++ linux-2.6.17-rc4-mm3-lockdep/sound/core/seq/seq_device.c
@@ -46,6 +46,7 @@
#include <linux/kmod.h>
#include <linux/slab.h>
#include <linux/mutex.h>
+#include <linux/lockdep.h>
MODULE_AUTHOR("Takashi Iwai <ti...@su...>");
MODULE_DESCRIPTION("ALSA sequencer device management");
@@ -73,6 +74,8 @@ struct ops_list {
struct mutex reg_mutex;
struct list_head list; /* next driver */
+
+ struct lockdep_type_key reg_mutex_key;
};
@@ -379,7 +382,7 @@ static struct ops_list * create_driver(c
/* set up driver entry */
strlcpy(ops->id, id, sizeof(ops->id));
- mutex_init(&ops->reg_mutex);
+ mutex_init_key(&ops->reg_mutex, id, &ops->reg_mutex_key);
ops->driver = DRIVER_EMPTY;
INIT_LIST_HEAD(&ops->dev_list);
/* lock this instance */
|
|
From: Jiri S. <jir...@gm...> - 2006-05-30 16:07:29
|
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Arjan van de Ven napsal(a):
> On Tue, 2006-05-30 at 14:44 +0200, Takashi Iwai wrote:
>> This ops is a unique object assigned to a different "id" string.
>>
>> The first snd_seq_device_register_driver() called from emu10k1_synth.c
>> is the registration for the id "snd-synth-emu10k1".
>> Then in init_device(), the corresponding devices are initialized, and
>> one callback registers again another device for OSS sequencer with a
>> different id "snd-seq-oss" via snd_seq_device_new() inside the lock.
>> Now it hits the lock-detector but the lock should belong to a
>> different ops object in practice.
>>
>> This nested lock may happen only in two drivers, emu10k1-synth and
>> opl3, and only together with OSS emulation. Since the OSS emulation
>> layer don't do active registration from itself, no deadlock should
>> happen (in theory -- I may oversee something :)
>
> ok fair enough
>
> Jiri, can you test the patch below? (I don't have this hardware)
Sure, but the day after tomorrow, I am going away from that machine now.
>
> The ops structure has complex locking rules, where not all ops are
> equal, some are subordinate on others for some complex sound cards. This
> requires for lockdep checking that each individual reg_mutex is
> considered in separation for its locking rules.
>
> Signed-off-by: Arjan van de Ven <ar...@li...>
>
> ---
> sound/core/seq/seq_device.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.17-rc4-mm3-lockdep/sound/core/seq/seq_device.c
> ===================================================================
> --- linux-2.6.17-rc4-mm3-lockdep.orig/sound/core/seq/seq_device.c
> +++ linux-2.6.17-rc4-mm3-lockdep/sound/core/seq/seq_device.c
> @@ -46,6 +46,7 @@
> #include <linux/kmod.h>
> #include <linux/slab.h>
> #include <linux/mutex.h>
> +#include <linux/lockdep.h>
>
> MODULE_AUTHOR("Takashi Iwai <ti...@su...>");
> MODULE_DESCRIPTION("ALSA sequencer device management");
> @@ -73,6 +74,8 @@ struct ops_list {
> struct mutex reg_mutex;
>
> struct list_head list; /* next driver */
> +
> + struct lockdep_type_key reg_mutex_key;
> };
>
>
> @@ -379,7 +382,7 @@ static struct ops_list * create_driver(c
>
> /* set up driver entry */
> strlcpy(ops->id, id, sizeof(ops->id));
> - mutex_init(&ops->reg_mutex);
> + mutex_init_key(&ops->reg_mutex, id, &ops->reg_mutex_key);
> ops->driver = DRIVER_EMPTY;
> INIT_LIST_HEAD(&ops->dev_list);
> /* lock this instance */
>
>
- --
Jiri Slaby www.fi.muni.cz/~xslaby
\_.-^-._ jir...@gm... _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org
iD8DBQFEfEQAMsxVwznUen4RAkp4AJwOMUsBK4kvE54X/oxcdcIWad8oGACghO2a
mZLtPAHamNSDsiNa1gOfgoE=
=CaIp
-----END PGP SIGNATURE-----
|
|
From: Jiri S. <jir...@gm...> - 2006-06-01 21:57:05
|
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Arjan van de Ven napsal(a):
> On Tue, 2006-05-30 at 14:44 +0200, Takashi Iwai wrote:
>> This ops is a unique object assigned to a different "id" string.
>>
>> The first snd_seq_device_register_driver() called from emu10k1_synth.c
>> is the registration for the id "snd-synth-emu10k1".
>> Then in init_device(), the corresponding devices are initialized, and
>> one callback registers again another device for OSS sequencer with a
>> different id "snd-seq-oss" via snd_seq_device_new() inside the lock.
>> Now it hits the lock-detector but the lock should belong to a
>> different ops object in practice.
>>
>> This nested lock may happen only in two drivers, emu10k1-synth and
>> opl3, and only together with OSS emulation. Since the OSS emulation
>> layer don't do active registration from itself, no deadlock should
>> happen (in theory -- I may oversee something :)
>
> ok fair enough
>
> Jiri, can you test the patch below? (I don't have this hardware)
It's gone in 2.6.17-rc5-mm2.
>
> The ops structure has complex locking rules, where not all ops are
> equal, some are subordinate on others for some complex sound cards. This
> requires for lockdep checking that each individual reg_mutex is
> considered in separation for its locking rules.
>
> Signed-off-by: Arjan van de Ven <ar...@li...>
>
> ---
> sound/core/seq/seq_device.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.17-rc4-mm3-lockdep/sound/core/seq/seq_device.c
> ===================================================================
> --- linux-2.6.17-rc4-mm3-lockdep.orig/sound/core/seq/seq_device.c
> +++ linux-2.6.17-rc4-mm3-lockdep/sound/core/seq/seq_device.c
> @@ -46,6 +46,7 @@
> #include <linux/kmod.h>
> #include <linux/slab.h>
> #include <linux/mutex.h>
> +#include <linux/lockdep.h>
>
> MODULE_AUTHOR("Takashi Iwai <ti...@su...>");
> MODULE_DESCRIPTION("ALSA sequencer device management");
> @@ -73,6 +74,8 @@ struct ops_list {
> struct mutex reg_mutex;
>
> struct list_head list; /* next driver */
> +
> + struct lockdep_type_key reg_mutex_key;
> };
>
>
> @@ -379,7 +382,7 @@ static struct ops_list * create_driver(c
>
> /* set up driver entry */
> strlcpy(ops->id, id, sizeof(ops->id));
> - mutex_init(&ops->reg_mutex);
> + mutex_init_key(&ops->reg_mutex, id, &ops->reg_mutex_key);
> ops->driver = DRIVER_EMPTY;
> INIT_LIST_HEAD(&ops->dev_list);
> /* lock this instance */
>
>
regards,
- --
Jiri Slaby www.fi.muni.cz/~xslaby
\_.-^-._ jir...@gm... _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org
iD8DBQFEfwdvMsxVwznUen4RAohGAKCChQYCo4EPEG0CWDPcld15mZnWTwCeKU+2
l26ws9ExkuBo1wlT5nJ+uWg=
=vgxE
-----END PGP SIGNATURE-----
|
|
From: Jiri S. <jir...@gm...> - 2006-05-30 13:41:17
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Andrew Morton napsal(a): > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.17-rc5/2.6.17-rc5-mm1/ ==================================== [ BUG: possible deadlock detected! ] - ------------------------------------ idle/1 is trying to acquire lock: (&ops->reg_mutex){--..}, at: [<c03ca763>] mutex_lock+0x8/0xa but task is already holding lock: (&ops->reg_mutex){--..}, at: [<c03ca763>] mutex_lock+0x8/0xa which could potentially lead to deadlocks! other info that might help us debug this: 1 locks held by idle/1: #0: (&ops->reg_mutex){--..}, at: [<c03ca763>] mutex_lock+0x8/0xa stack backtrace: [<c01042ac>] show_trace+0x1b/0x1d [<c01049f2>] dump_stack+0x26/0x28 [<c01422fa>] __lockdep_acquire+0xa58/0xd8e [<c0142b97>] lockdep_acquire+0x73/0x88 [<c03ca378>] __mutex_lock_slowpath+0xb3/0x496 [<c03ca763>] mutex_lock+0x8/0xa [<c0333aa0>] snd_seq_device_new+0x96/0x111 [<c0358260>] snd_emux_init_seq_oss+0x35/0x9c [<c0353f50>] snd_emux_register+0x10d/0x13f [<c0352c39>] snd_emu10k1_synth_new_device+0xe7/0x14e [<c0333537>] init_device+0x2c/0x94 [<c0333d04>] snd_seq_device_register_driver+0x8f/0xeb [<c05911e0>] alsa_emu10k1_synth_init+0x22/0x24 [<c01003cb>] init+0x12b/0x2f5 [<c0101005>] kernel_thread_helper+0x5/0xb If more info needed, feel free to ask. regards, - -- Jiri Slaby www.fi.muni.cz/~xslaby \_.-^-._ jir...@gm... _.-^-._/ B67499670407CE62ACC8 22A032CC55C339D47A7E -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.3 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iD8DBQFEfCLOMsxVwznUen4RArelAJ0WtN36nSYJ3VWB515Wik2ji8YXAACfe5jD jiPvjBzv4udC7XJPxTUtmOM= =vLLJ -----END PGP SIGNATURE----- |
|
From: Arjan v. de V. <ar...@in...> - 2006-05-30 11:06:57
|
On Tue, 2006-05-30 at 147 +0159, Jiri Slaby wrote:
(I've turned your backtrace upside down to show it "chronological")
[<c05911e0>] alsa_emu10k1_synth_init+0x22/0x24
[<c0333d04>] snd_seq_device_register_driver+0x8f/0xeb
this one does:
mutex_lock(&ops->reg_mutex);
...
list_for_each(head, &ops->dev_list) {
struct snd_seq_device *dev = list_entry(head, struct snd_seq_device, list);
init_device(dev, ops);
}
mutex_unlock(&ops->reg_mutex);
[<c0333537>] init_device+0x2c/0x94
which calls into the driver
[<c0352c39>] snd_emu10k1_synth_new_device+0xe7/0x14e
[<c0353f50>] snd_emux_register+0x10d/0x13f
[<c0358260>] snd_emux_init_seq_oss+0x35/0x9c
[<c0333aa0>] snd_seq_device_new+0x96/0x111
and this one does
mutex_lock(&ops->reg_mutex);
list_add_tail(&dev->list, &ops->dev_list);
ops->num_devices++;
mutex_unlock(&ops->reg_mutex);
so... on first sight this looks like a real deadlock;
unless the ALSA folks can tell me why "ops" is always different,
and what the lock ordering rules between those is...
|
Thanks for helping keep SourceForge clean.
X