At Thu, 23 May 2013 01:04:16 +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.
The bug is introduced by your patch, so better to fold into it.
Moreover...
>
> Signed-off-by: Wang Xingchao <xingchao.wang at linux.intel.com>
> ---
> sound/pci/hda/hda_intel.c | 105 +++++++++++++++++++++++++++-------------------
> 1 file changed, 62 insertions(+), 43 deletions(-)
>
> 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;
Don't do this.
> #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);
The initialization must be done earlier, in azx_create().
> + schedule_delayed_work(&chip->probe_work, 0);
You shouldn't do async probe unless needed.
That is, this is required only for Haswell.
Also, if you delay the call of hda_i915_init(), you need to have a
flag to indicate whether this initialization has been done, and call
the counterpart in azx_free() only if the flag is set. Otherwise, you
might call hda_i915_exit() & co even before calling hda_i915_init().
Another point to fix is to make sure to cancel the leftover work in
destructor.
Last not but least, I guess the call of pm_runtime_put_noidle() in
azx_probe() might be problematic. In theory it allows the runtime
suspend before hda_i915_init() is done.
Maybe we should move pm_runtime_put_noidle() into
azx_probe_continue(). And put the counterpart to azx_free()
conditionally called with chip->running, or so.
But, this doesn't exclude the explicit suspend/resume -- you are
calling hda_display_power() and this might be also before the actual
initialization. Again, this must be conditional, too.
Takashi
> + return 0;
> +out_free:
> + snd_card_free(card);
> + pci_set_drvdata(pci, NULL);
> return err;
> }
