[alsa-devel] [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
There's deadlock when request_module(i915) in azx_probe. It looks like: device_lock(audio pci device) -> azx_probe -> module_request (or symbol_request) -> modprobe (userspace) -> i915 init -> drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach -> which in turn tries all locks on pci bus, and when it tries the one on the audio device, it will deadlock.
This patch introduce a work to store remaining probe stuff, and let request_module run in safe work context.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- sound/pci/hda/hda_i915.c | 13 ++++-- sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7547b20 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -42,13 +42,18 @@ int hda_i915_init(void) { int err = 0;
- get_power = symbol_request(i915_request_power_well); + get_power = symbol_get(i915_request_power_well); if (!get_power) { - snd_printk(KERN_WARNING "hda-i915: get_power symbol get fail\n"); - return -ENODEV; + request_module("i915"); + get_power = symbol_get(i915_request_power_well); + if (!get_power) { + snd_printk(KERN_WARNING "hda-i915: get_power symbol get fail\n"); + return -ENODEV; + } + snd_printdd("hda-i915: get_power symbol get successful\n"); }
- put_power = symbol_request(i915_release_power_well); + put_power = symbol_get(i915_release_power_well); if (!put_power) { symbol_put(i915_request_power_well); get_power = NULL; diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index f20a88c..1bc7c3b 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -76,6 +76,7 @@ static int probe_only[SNDRV_CARDS]; static int jackpoll_ms[SNDRV_CARDS]; static bool single_cmd; static int enable_msi = -1; +static int dev; #ifdef CONFIG_SND_HDA_PATCH_LOADER static char *patch[SNDRV_CARDS]; #endif @@ -542,6 +543,8 @@ struct azx { /* for pending irqs */ struct work_struct irq_pending_work;
+ struct delayed_work probe_work; + /* reboot notifier (for mysterious hangup problem at power-down) */ struct notifier_block reboot_notifier;
@@ -3670,58 +3673,22 @@ static void azx_firmware_cb(const struct firmware *fw, void *context) } #endif
-static int azx_probe(struct pci_dev *pci, - const struct pci_device_id *pci_id) +static void azx_probe_work(struct work_struct *work) { - static int dev; - struct snd_card *card; - struct azx *chip; + struct azx *chip = + container_of(work, struct azx, probe_work.work); + struct snd_card *card = chip->card; + struct pci_dev *pci = chip->pci; bool probe_now; int err;
- if (dev >= SNDRV_CARDS) - return -ENODEV; - if (!enable[dev]) { - dev++; - return -ENOENT; - } - - err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); - if (err < 0) { - snd_printk(KERN_ERR "hda-intel: Error creating card!\n"); - return err; - } - - snd_card_set_dev(card, &pci->dev); - - err = azx_create(card, pci, dev, pci_id->driver_data, &chip); - if (err < 0) - goto out_free; - card->private_data = chip; - - pci_set_drvdata(pci, card); - - err = register_vga_switcheroo(chip); - if (err < 0) { - snd_printk(KERN_ERR SFX - "%s: Error registering VGA-switcheroo client\n", pci_name(pci)); - goto out_free; - } - - if (check_hdmi_disabled(pci)) { - snd_printk(KERN_INFO SFX "%s: VGA controller is disabled\n", - pci_name(pci)); - snd_printk(KERN_INFO SFX "%s: Delaying initialization\n", pci_name(pci)); - chip->disabled = true; - } - /* Request power well for Haswell HDA controller and codec */ if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { #ifdef CONFIG_SND_HDA_I915 err = hda_i915_init(); if (err < 0) { snd_printk(KERN_ERR SFX "Error request power-well from i915\n"); - return err; + goto out_free; } hda_display_power(true); #else @@ -3760,7 +3727,7 @@ static int azx_probe(struct pci_dev *pci,
dev++; complete_all(&chip->probe_wait); - return 0; + return; out_free_power: if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { hda_display_power(false); @@ -3769,6 +3736,58 @@ out_free_power: out_free: snd_card_free(card); pci_set_drvdata(pci, NULL); +} + +static int azx_probe(struct pci_dev *pci, + const struct pci_device_id *pci_id) +{ + struct snd_card *card; + struct azx *chip; + int err; + + if (dev >= SNDRV_CARDS) + return -ENODEV; + if (!enable[dev]) { + dev++; + return -ENOENT; + } + + err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); + if (err < 0) { + snd_printk(KERN_ERR "hda-intel: Error creating card!\n"); + return err; + } + + snd_card_set_dev(card, &pci->dev); + + err = azx_create(card, pci, dev, pci_id->driver_data, &chip); + if (err < 0) + goto out_free; + card->private_data = chip; + + pci_set_drvdata(pci, card); + + err = register_vga_switcheroo(chip); + if (err < 0) { + snd_printk(KERN_ERR SFX + "%s: Error registering VGA-switcheroo client\n", pci_name(pci)); + goto out_free; + } + + if (check_hdmi_disabled(pci)) { + snd_printk(KERN_INFO SFX "%s: VGA controller is disabled\n", + pci_name(pci)); + snd_printk(KERN_INFO SFX "%s: Delaying initialization\n", pci_name(pci)); + chip->disabled = true; + } + + /* continue probing in work context as may trigger request module */ + INIT_DELAYED_WORK(&chip->probe_work, azx_probe_work); + schedule_delayed_work(&chip->probe_work, 0); + return 0; +out_free: + snd_card_free(card); + pci_set_drvdata(pci, NULL); return err; }
At Thu, 23 May 2013 09:51:07 +0800, Wang Xingchao wrote:
There's deadlock when request_module(i915) in azx_probe. It looks like: device_lock(audio pci device) -> azx_probe -> module_request (or symbol_request) -> modprobe (userspace) -> i915 init -> drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach -> which in turn tries all locks on pci bus, and when it tries the one on the audio device, it will deadlock.
This patch introduce a work to store remaining probe stuff, and let request_module run in safe work context.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 13 ++++-- sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7547b20 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -42,13 +42,18 @@ int hda_i915_init(void) { int err = 0;
- get_power = symbol_request(i915_request_power_well);
- get_power = symbol_get(i915_request_power_well); if (!get_power) {
snd_printk(KERN_WARNING "hda-i915: get_power symbol get fail\n");return -ENODEV;
request_module("i915");get_power = symbol_get(i915_request_power_well);if (!get_power) {snd_printk(KERN_WARNING "hda-i915: get_power symbol get fail\n");return -ENODEV;}snd_printdd("hda-i915: get_power symbol get successful\n");
Why do you need this change?
Takashi
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 2:49 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Wang, Xingchao; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 09:51:07 +0800, Wang Xingchao wrote:
There's deadlock when request_module(i915) in azx_probe. It looks like: device_lock(audio pci device) -> azx_probe -> module_request (or symbol_request) -> modprobe (userspace) -> i915 init -> drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach -> which in turn tries all locks on pci bus, and when it tries the one on the audio device, it will deadlock.
This patch introduce a work to store remaining probe stuff, and let request_module run in safe work context.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 13 ++++-- sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7547b20 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -42,13 +42,18 @@ int hda_i915_init(void) { int err = 0;
- get_power = symbol_request(i915_request_power_well);
- get_power = symbol_get(i915_request_power_well); if (!get_power) {
snd_printk(KERN_WARNING "hda-i915: get_power symbol getfail\n");
return -ENODEV;
request_module("i915");get_power = symbol_get(i915_request_power_well);if (!get_power) {snd_printk(KERN_WARNING "hda-i915: get_power symbol getfail\n");
return -ENODEV;}snd_printdd("hda-i915: get_power symbol get successful\n");Why do you need this change?
symbol_request() should be the better API in such case but in my test it doesnot really load i915 module, that's why I call request_module(i915) directly here.
Please note there's parameter difference: request_module("i915") symbol_request(i915_reauest_power_well)-->request_module("symbol:i915_request_power_well")
I donot know why the second one did not really load the module.
Thanks --xingchao
At Thu, 23 May 2013 10:19:27 +0000, Wang, Xingchao wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 2:49 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Wang, Xingchao; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 09:51:07 +0800, Wang Xingchao wrote:
There's deadlock when request_module(i915) in azx_probe. It looks like: device_lock(audio pci device) -> azx_probe -> module_request (or symbol_request) -> modprobe (userspace) -> i915 init -> drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach -> which in turn tries all locks on pci bus, and when it tries the one on the audio device, it will deadlock.
This patch introduce a work to store remaining probe stuff, and let request_module run in safe work context.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 13 ++++-- sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7547b20 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -42,13 +42,18 @@ int hda_i915_init(void) { int err = 0;
- get_power = symbol_request(i915_request_power_well);
- get_power = symbol_get(i915_request_power_well); if (!get_power) {
snd_printk(KERN_WARNING "hda-i915: get_power symbol getfail\n");
return -ENODEV;
request_module("i915");get_power = symbol_get(i915_request_power_well);if (!get_power) {snd_printk(KERN_WARNING "hda-i915: get_power symbol getfail\n");
return -ENODEV;}snd_printdd("hda-i915: get_power symbol get successful\n");Why do you need this change?
symbol_request() should be the better API in such case but in my test it doesnot really load i915 module, that's why I call request_module(i915) directly here.
Please note there's parameter difference: request_module("i915") symbol_request(i915_reauest_power_well)-->request_module("symbol:i915_request_power_well")
I donot know why the second one did not really load the module.
Well, something is really fishy. The patch can't be accepted only because it just works by moon phase...
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 6:27 PM To: Wang, Xingchao Cc: Wang Xingchao; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 10:19:27 +0000, Wang, Xingchao wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 2:49 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Wang, Xingchao; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 09:51:07 +0800, Wang Xingchao wrote:
There's deadlock when request_module(i915) in azx_probe. It looks like: device_lock(audio pci device) -> azx_probe -> module_request (or symbol_request) -> modprobe (userspace) -> i915 init -> drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach -> which -> in turn tries all locks on pci bus, and when it tries the one on the audio device, it will deadlock.
This patch introduce a work to store remaining probe stuff, and let request_module run in safe work context.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 13 ++++-- sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7547b20 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -42,13 +42,18 @@ int hda_i915_init(void) { int err = 0;
- get_power = symbol_request(i915_request_power_well);
- get_power = symbol_get(i915_request_power_well); if (!get_power) {
snd_printk(KERN_WARNING "hda-i915: get_power symbol getfail\n");
return -ENODEV;
request_module("i915");get_power = symbol_get(i915_request_power_well);if (!get_power) {snd_printk(KERN_WARNING "hda-i915: get_power symbolget
fail\n");
return -ENODEV;}snd_printdd("hda-i915: get_power symbol get successful\n");Why do you need this change?
symbol_request() should be the better API in such case but in my test it doesnot really load i915 module, that's why I call request_module(i915)
directly here.
Please note there's parameter difference: request_module("i915")
symbol_request(i915_reauest_power_well)-->request_module("symbol:i915_
request_power_well")
I donot know why the second one did not really load the module.
Well, something is really fishy. The patch can't be accepted only because it just works by moon phase...
I will continue to figure out the reason it doesnot work. :(
--xingchao
On 05/23/2013 12:29 PM, Wang, Xingchao wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 6:27 PM To: Wang, Xingchao Cc: Wang Xingchao; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 10:19:27 +0000, Wang, Xingchao wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 2:49 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Wang, Xingchao; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 09:51:07 +0800, Wang Xingchao wrote:
There's deadlock when request_module(i915) in azx_probe. It looks like: device_lock(audio pci device) -> azx_probe -> module_request (or symbol_request) -> modprobe (userspace) -> i915 init -> drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach -> which -> in turn tries all locks on pci bus, and when it tries the one on the audio device, it will deadlock.
This patch introduce a work to store remaining probe stuff, and let request_module run in safe work context.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 13 ++++-- sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7547b20 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -42,13 +42,18 @@ int hda_i915_init(void) { int err = 0;
- get_power = symbol_request(i915_request_power_well);
- get_power = symbol_get(i915_request_power_well); if (!get_power) {
snd_printk(KERN_WARNING "hda-i915: get_power symbol getfail\n");
return -ENODEV;
request_module("i915");get_power = symbol_get(i915_request_power_well);if (!get_power) {snd_printk(KERN_WARNING "hda-i915: get_power symbolget
fail\n");
return -ENODEV;}snd_printdd("hda-i915: get_power symbol get successful\n");Why do you need this change?
symbol_request() should be the better API in such case but in my test it doesnot really load i915 module, that's why I call request_module(i915)
directly here.
Please note there's parameter difference: request_module("i915")
symbol_request(i915_reauest_power_well)-->request_module("symbol:i915_
request_power_well")
I donot know why the second one did not really load the module.
Well, something is really fishy. The patch can't be accepted only because it just works by moon phase...
I will continue to figure out the reason it doesnot work. :(
To the rescue!
I've tried to track this down, and I think this is a problem in modprobe with blacklisted modules. I've just emailed the kmod maintainer (with Wang Xingchao in cc) and asked for clarification.
At Thu, 23 May 2013 19:41:58 +0200, David Henningsson wrote:
On 05/23/2013 12:29 PM, Wang, Xingchao wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 6:27 PM To: Wang, Xingchao Cc: Wang Xingchao; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 10:19:27 +0000, Wang, Xingchao wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, May 23, 2013 2:49 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Wang, Xingchao; Lin, Mengdong Subject: Re: [PATCH 4/4 V2] ALSA: hda - Continue probe in work context to avoid request_module deadlock
At Thu, 23 May 2013 09:51:07 +0800, Wang Xingchao wrote:
There's deadlock when request_module(i915) in azx_probe. It looks like: device_lock(audio pci device) -> azx_probe -> module_request (or symbol_request) -> modprobe (userspace) -> i915 init -> drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach -> which -> in turn tries all locks on pci bus, and when it tries the one on the audio device, it will deadlock.
This patch introduce a work to store remaining probe stuff, and let request_module run in safe work context.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 13 ++++-- sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7547b20 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -42,13 +42,18 @@ int hda_i915_init(void) { int err = 0;
- get_power = symbol_request(i915_request_power_well);
- get_power = symbol_get(i915_request_power_well); if (!get_power) {
snd_printk(KERN_WARNING "hda-i915: get_power symbol getfail\n");
return -ENODEV;
request_module("i915");get_power = symbol_get(i915_request_power_well);if (!get_power) {snd_printk(KERN_WARNING "hda-i915: get_power symbolget
fail\n");
return -ENODEV;}snd_printdd("hda-i915: get_power symbol get successful\n");Why do you need this change?
symbol_request() should be the better API in such case but in my test it doesnot really load i915 module, that's why I call request_module(i915)
directly here.
Please note there's parameter difference: request_module("i915")
symbol_request(i915_reauest_power_well)-->request_module("symbol:i915_
request_power_well")
I donot know why the second one did not really load the module.
Well, something is really fishy. The patch can't be accepted only because it just works by moon phase...
I will continue to figure out the reason it doesnot work. :(
To the rescue!
I've tried to track this down, and I think this is a problem in modprobe with blacklisted modules. I've just emailed the kmod maintainer (with Wang Xingchao in cc) and asked for clarification.
Thanks for figuring out!
Takashi
participants (4)
-
David Henningsson -
Takashi Iwai -
Wang Xingchao -
Wang, Xingchao