Re: [3.6-rc7] switcheroo race with Intel HDA...

[Takashi Iwai]

At Tue, 9 Oct 2012 00:34:09 +0800,
Daniel J Blueman wrote:
> 
> On 8 October 2012 20:58, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > At Tue, 25 Sep 2012 13:20:05 +0800,
> > Daniel J Blueman wrote:
> >> On my Macbook with a discrete Nvidia GPU, there is a race between
> >> selecting the integrated GPU and putting the discrete GPU into D3 [1],
> >> reliably causing a kernel oops [2].
> >>
> >> Introducing a delay of ~1s between the calls prevents this. When the
> >> second 'OFF' write path executes, it looks like struct azx at
> >> card->private_data hasn't yet been allocated yet [3], so there is
> >> likely some locking missing.
> >
> > It's rather pci_get_drvdata() returning NULL (i.e. card is NULL, thus
> > card->private_data causes Oops).  Could you check the patch like below
> > and see whether you get a kernel warning (but no Oops) or the problem
> > gets fixed by shifting the assignment of pci drvdata?
> [...]
> 
> Good patching. Calling pci_set_drvdata later prevents the oops in HDA,
> though we see unexpected 0x0 responses in the response ring buffer
> [1], which we don't see when there's a >~1.5s delay between IGD and
> OFF.

If the previous patch fixed, it means that the switching occurred
during the device was being probed.  Maybe a better approach to
register the VGA switcheroo after the proper initialization.

The patch below is a revised one.  Please give it a try.


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index f09ff6c..dcbfd29 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -501,6 +501,7 @@ struct azx {
 
 	/* VGA-switcheroo setup */
 	unsigned int use_vga_switcheroo:1;
+	unsigned int vga_switcheroo_registered:1;
 	unsigned int init_failed:1; /* delayed init failed */
 	unsigned int disabled:1; /* disabled by VGA-switcher */
 
@@ -2684,14 +2685,20 @@ static const struct vga_switcheroo_client_ops azx_vs_ops = {
 
 static int __devinit register_vga_switcheroo(struct azx *chip)
 {
+	int err;
+
 	if (!chip->use_vga_switcheroo)
 		return 0;
 	/* FIXME: currently only handling DIS controller
 	 * is there any machine with two switchable HDMI audio controllers?
 	 */
-	return vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops,
+	err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops,
 						    VGA_SWITCHEROO_DIS,
 						    chip->bus != NULL);
+	if (err < 0)
+		return err;
+	chip->vga_switcheroo_registered = 1;
+	return 0;
 }
 #else
 #define init_vga_switcheroo(chip)		/* NOP */
@@ -2713,7 +2720,8 @@ static int azx_free(struct azx *chip)
 	if (use_vga_switcheroo(chip)) {
 		if (chip->disabled && chip->bus)
 			snd_hda_unlock_devices(chip->bus);
-		vga_switcheroo_unregister_client(chip->pci);
+		if (chip->vga_switcheroo_registered)
+			vga_switcheroo_unregister_client(chip->pci);
 	}
 
 	if (chip->initialized) {
@@ -3067,14 +3075,6 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci,
 	}
 
  ok:
-	err = register_vga_switcheroo(chip);
-	if (err < 0) {
-		snd_printk(KERN_ERR SFX
-			   "Error registering VGA-switcheroo client\n");
-		azx_free(chip);
-		return err;
-	}
-
 	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
 	if (err < 0) {
 		snd_printk(KERN_ERR SFX "Error creating device [card]!\n");
@@ -3345,6 +3345,13 @@ 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++;
 	return 0;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/