ALSA: core: Replace mutex_lock with mutex_trylock

Task1 waits for mutex_lock, and task2 waits for pde to be unused.(deadlock) /*call trace*/

    task1 Call trace:
            __switch_to+0x174/0x338
            schedule+0x7c/0xe8
            schedule_preempt_disabled+0x24/0x40
            mutex_lock+0x40/0xec
            snd_info_text_entry_open+0x28/0x120
            proc_reg_open+0xe4/0x248
            do_dentry_open+0x2a4/0x4e0

    task2 Call trace:
            schedule_timeout+0x44/0x1c8
            wait_for_completion+0x18/0x24
            proc_entry_rundown+0x60/0xf0
            remove_proc_subtree+0x180/0x218
            proc_remove+0x20/0x30
            snd_info_disconnect+0x4c/0x68
            snd_info_card_disconnect+0x3c/0x58
            snd_card_disconnect+0x130/0x264
            usb_audio_disconnect+0xc0/0x24c

/*the sequence*/
task1:
- proc_reg_open: set the use_pde
task2:
- usb_audio_disconnect: usb device disconnection occurs
- snd_info_card_disconnect: acquire the mutex_lock(&info_mutex)
- proc_entry_rundown: wait_for_completion(unuse_pde)
task1:
- wait for mutex_lock in snd_info_text_entry_open

To avoid it, a mutex without wating(mutex_trylock) shoud be used in snd_info_text_entry_open(task1).
Then, when mutex_lock acquisition fails, an error is returned, and the pde becomes unused, and the mutex_lock held by task2 is released.

Signed-off-by Shinhyung Kang <[email protected]>

Thanks for the patch. But this change may break the current working behavior; e.g. when two proc reads are running concurrently, one would be aborted unexpectedly.

IIUC, the problem is the call of proc_remove(), and this call itself can be outside the global mutex.

Could you check whether the patch below works instead? (Note that it's only compile-tested.) It makes the proc_remove() called at first, then clearing the internal entries. The function was renamed accordingly for avoiding confusion, too.

Takashi

sound/core/info.c +2 -1

 	if (!entry)
 		return;
 	if (entry->p) {
+		proc_remove(entry->p);
 		mutex_lock(&info_mutex);
-		snd_info_disconnect(entry);
+		snd_info_clear_entries(entry);
 		mutex_unlock(&info_mutex);
 	}

Quoted 8 lines

Thanks for the patch. But this change may break the current working behavior; e.g. when two proc reads are running concurrently, one would be aborted unexpectedly.

IIUC, the problem is the call of proc_remove(), and this call itself can be outside the global mutex.

Quoted 7 lines

Could you check whether the patch below works instead? (Note that it's only compile-tested.) It makes the proc_remove() called at first, then clearing the internal entries. The function was renamed accordingly for avoiding confusion, too.

Takashi

You are right. My patch is just for avoiding the deadlock. It may lead to other problem instead the deadlock(e.g. USB sound card registration failure)
Your patch works well without any problems.
But I can't confirm that the problem is solved or not. because the issue has occurred only once until now.
(Test method: USB insertion / removal during a call)

Quoted 16 lines · 강신형

Quoted 8 lines

Thanks for the patch. But this change may break the current working behavior; e.g. when two proc reads are running concurrently, one would be aborted unexpectedly.

IIUC, the problem is the call of proc_remove(), and this call itself can be outside the global mutex.

Quoted 7 lines

Could you check whether the patch below works instead? (Note that it's only compile-tested.) It makes the proc_remove() called at first, then clearing the internal entries. The function was renamed accordingly for avoiding confusion, too.

Takashi

Quoted 7 lines

You are right. My patch is just for avoiding the deadlock. It may lead to other problem instead the deadlock(e.g. USB sound card registration failure)
Your patch works well without any problems.
But I can't confirm that the problem is solved or not. because the issue has occurred only once until now.
(Test method: USB insertion / removal during a call)

Maybe you can reproduce it more easily by adding some delay (e.g. ssleep(2)) before mutex_lock() in snd_info_entry_open().

Then it's easier to cause a race.