Re: switcheroo registration vs switching race...

[Takashi Iwai <tiwai@suse.de>]

At Wed, 28 Nov 2012 09:45:39 +0100,
Takashi Iwai wrote:
> 
> At Wed, 28 Nov 2012 11:45:07 +0800,
> Daniel J Blueman wrote:
> > 
> > Hi Seth, Dave, Takashi,
> > 
> > If I power down the unused discrete GPU before lightdm starts by
> > fiddling with the sysfs file [1] in the upstart script, I see a race
> > manifesting as the discrete GPU's HDA controller timing out to
> > commands [2].
> > 
> > Adding some debug, I see that the registered audio devices are put
> > into D3 before the GPU is, but it turns out that the discrete (and
> > internal) GPU's HDA controller gets registered a bit later, so the
> > list is empty. The symptom is since the HDA driver it's talking to
> > hardware which is now in D3.
> > 
> > We could add a mutex to nouveau to allow us to wait for the DGPU HDA
> > controller, but perhaps this should be solved at a higher level in the
> > vgaswitcheroo code; what do you think?
> 
> Maybe it's a side effect for the recent effort to fix another race in
> the probe.  A part of them problem is that the registration is done at
> the very last of probing.
> 
> Instead of delaying the registration, how about the patch below?

Ping.  If this really works, I'd like to queue it for 3.8 merge, at
least...


thanks,

Takashi

> 
> 
> Takashi
> 
> ---
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 4bb52da..17fbd68 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -49,6 +49,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/clocksource.h>
>  #include <linux/time.h>
> +#include <linux/completion.h>
>  
>  #ifdef CONFIG_X86
>  /* for snoop control */
> @@ -469,6 +470,7 @@ struct azx {
>  	/* locks */
>  	spinlock_t reg_lock;
>  	struct mutex open_mutex;
> +	struct completion probe_wait;
>  
>  	/* streams (x num_streams) */
>  	struct azx_dev *azx_dev;
> @@ -2790,6 +2792,7 @@ static bool azx_vs_can_switch(struct pci_dev *pci)
>  	struct snd_card *card = pci_get_drvdata(pci);
>  	struct azx *chip = card->private_data;
>  
> +	wait_for_completion(&chip->probe_wait);
>  	if (chip->init_failed)
>  		return false;
>  	if (chip->disabled || !chip->bus)
> @@ -2851,6 +2854,9 @@ static int azx_free(struct azx *chip)
>  
>  	azx_notifier_unregister(chip);
>  
> +	chip->init_failed = 1; /* to be sure */
> +	complete(&chip->probe_wait);
> +
>  	if (use_vga_switcheroo(chip)) {
>  		if (chip->disabled && chip->bus)
>  			snd_hda_unlock_devices(chip->bus);
> @@ -3156,6 +3162,7 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci,
>  	INIT_LIST_HEAD(&chip->pcm_list);
>  	INIT_LIST_HEAD(&chip->list);
>  	init_vga_switcheroo(chip);
> +	init_completion(&chip->probe_wait);
>  
>  	chip->position_fix[0] = chip->position_fix[1] =
>  		check_position_fix(chip, position_fix[dev]);
> @@ -3462,6 +3469,13 @@ static int __devinit azx_probe(struct pci_dev *pci,
>  	}
>  #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>  
> +	err = register_vga_switcheroo(chip);
> +	if (err < 0) {
> +		snd_printk(KERN_ERR SFX
> +			   "Error registering VGA-switcheroo client\n");
> +		goto out_free;
> +	}
> +
>  	if (probe_now) {
>  		err = azx_probe_continue(chip);
>  		if (err < 0)
> @@ -3473,14 +3487,8 @@ static int __devinit azx_probe(struct pci_dev *pci,
>  	if (pci_dev_run_wake(pci))
>  		pm_runtime_put_noidle(&pci->dev);
>  
> -	err = register_vga_switcheroo(chip);
> -	if (err < 0) {
> -		snd_printk(KERN_ERR SFX
> -			   "Error registering VGA-switcheroo client\n");
> -		goto out_free;
> -	}
> -
>  	dev++;
> +	complete(&chip->probe_wait);
>  	return 0;
>  
>  out_free: