On Tue, 07 Nov 2023 09:17:59 +0100, 강신형 wrote:
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 s47.kang@samsung.com
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
--- a/sound/core/info.c +++ b/sound/core/info.c @@ -56,7 +56,7 @@ struct snd_info_private_data { };
static int snd_info_version_init(void); -static void snd_info_disconnect(struct snd_info_entry *entry); +static void snd_info_clear_entries(struct snd_info_entry *entry);
/*
@@ -569,11 +569,16 @@ void snd_info_card_disconnect(struct snd_card *card) { if (!card) return; - mutex_lock(&info_mutex); + proc_remove(card->proc_root_link); - card->proc_root_link = NULL; if (card->proc_root) - snd_info_disconnect(card->proc_root); + proc_remove(card->proc_root->p); + + mutex_lock(&info_mutex); + if (card->proc_root) + snd_info_clear_entries(card->proc_root); + card->proc_root_link = NULL; + card->proc_root = NULL; mutex_unlock(&info_mutex); }
@@ -745,15 +750,14 @@ struct snd_info_entry *snd_info_create_card_entry(struct snd_card *card, } EXPORT_SYMBOL(snd_info_create_card_entry);
-static void snd_info_disconnect(struct snd_info_entry *entry) +static void snd_info_clear_entries(struct snd_info_entry *entry) { struct snd_info_entry *p;
if (!entry->p) return; list_for_each_entry(p, &entry->children, list) - snd_info_disconnect(p); - proc_remove(entry->p); + snd_info_clear_entries(p); entry->p = NULL; }
@@ -770,8 +774,9 @@ void snd_info_free_entry(struct snd_info_entry * entry) 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); }