BUG: possible deadlock detected! (sound) [Was: 2.6.17-rc5-mm1]

BUG: possible deadlock detected! (sound) [Was: 2.6.17-rc5-mm1]

[Jiri Slaby]

-----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
\_.-^-._   jirislaby@gmail.com   _.-^-._/
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-----

Re: BUG: possible deadlock detected! (sound) [Was: 2.6.17-rc5-mm1]

[Arjan van de Ven]

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...

Re: BUG: possible deadlock detected! (sound) [Was: 2.6.17-rc5-mm1]

[Takashi Iwai]

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

Re: BUG: possible deadlock detected! (sound) [Was: 2.6.17-rc5-mm1]

[Arjan van de Ven]

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 <arjan@linux.intel.com>

---
 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 <tiwai@suse.de>");
 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 */

Re: BUG: possible deadlock detected! (sound) [Was: 2.6.17-rc5-mm1]

[Jiri Slaby]

-----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 <arjan@linux.intel.com>
> 
> ---
>  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 <tiwai@suse.de>");
>  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
\_.-^-._   jirislaby@gmail.com   _.-^-._/
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-----

Re: BUG: possible deadlock detected! (sound) [Was: 2.6.17-rc5-mm1]

[Jiri Slaby]

-----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 <arjan@linux.intel.com>
> 
> ---
>  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 <tiwai@suse.de>");
>  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
\_.-^-._   jirislaby@gmail.com   _.-^-._/
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-----