Messages in this thread Patch in this message | | | Date | Mon, 03 Dec 2012 15:40:37 +0100 | From | Takashi Iwai <> | Subject | Re: switcheroo registration vs switching race... |
| |
At Mon, 3 Dec 2012 22:25:52 +0800, Daniel J Blueman wrote: > > On 3 December 2012 19:17, Takashi Iwai <tiwai@suse.de> wrote: > > 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... > > Ping ack; I was trying to find time to understand another race that > occurs with GPU probing after switching, but is separate from the > situation before switching, here. > > In the context of writing the switch, it looks like struct azx isn't > allocated by the time azx_vs_set_state accesses it [1,2]; racing with > azx_codec_create?
It was allocated, but it wasn't assigned properly in pci drvdata.
Below is the revised patch. Just moved pci_set_drvdata() before register_vga_switcheroo(). Could you retest with it?
thanks,
Takashi
--- diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 4bb52da..b7c482a 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,17 +3469,8 @@ static int __devinit azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */ - if (probe_now) { - err = azx_probe_continue(chip); - if (err < 0) - goto out_free; - } - pci_set_drvdata(pci, card); - 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 @@ -3480,11 +3478,22 @@ static int __devinit azx_probe(struct pci_dev *pci, goto out_free; } + if (probe_now) { + err = azx_probe_continue(chip); + if (err < 0) + goto out_free; + } + + if (pci_dev_run_wake(pci)) + pm_runtime_put_noidle(&pci->dev); + dev++; + complete(&chip->probe_wait); return 0; out_free: snd_card_free(card); + pci_set_drvdata(pci, NULL); return err; }
| |