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

From: Takashi Iwai
Date: Tue Oct 09 2012 - 06:04:03 EST


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/