Re: [PATCH 4/4] ALSA: hda - Continue probe in work context to avoid request_module deadlock

[Takashi Iwai <tiwai@suse.de>]

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@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;
>  }