| Message ID | 1534202114-29164-1-git-send-email-yong.zhi@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 12eeeb4f4733bbc4481d01df35933fc15beb8b19 |
| Headers | show |
| Series | ASoC: Intel: Skylake: Acquire irq after RIRB allocation | expand |
On 8/13/18 6:15 PM, Yong Zhi wrote: > Cold reboot stress test found that the hda irq could access rirb ring > buffer before its memory gets allocated which resulting in null > pointer dereference inside snd_hdac_bus_update_rirb(). > > Fix it by moving the skl_acquire_irq after ring buffer allocation. > While here, also change err return from -EBUSY to actual error code. I am not that familiar with PCI gory details but that patch was reviewed internally with no objections raised; there was also an agreement that the SOF driver would follow the same sequence, so Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > Signed-off-by: Yong Zhi <yong.zhi@intel.com> > --- > sound/soc/intel/skylake/skl.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > index dce649485649..cf09721ca13e 100644 > --- a/sound/soc/intel/skylake/skl.c > +++ b/sound/soc/intel/skylake/skl.c > @@ -838,11 +838,7 @@ static int skl_first_init(struct hdac_bus *bus) > > snd_hdac_bus_parse_capabilities(bus); > > - if (skl_acquire_irq(bus, 0) < 0) > - return -EBUSY; > - > pci_set_master(pci); > - synchronize_irq(bus->irq); > > gcap = snd_hdac_chip_readw(bus, GCAP); > dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap); > @@ -875,6 +871,12 @@ static int skl_first_init(struct hdac_bus *bus) > if (err < 0) > return err; > > + err = skl_acquire_irq(bus, 0); > + if (err < 0) > + return err; > + > + synchronize_irq(bus->irq); > + > /* initialize chip */ > skl_init_pci(skl); > >
On Tue, 14 Aug 2018 16:50:51 +0200, Pierre-Louis Bossart wrote: > > On 8/13/18 6:15 PM, Yong Zhi wrote: > > Cold reboot stress test found that the hda irq could access rirb ring > > buffer before its memory gets allocated which resulting in null > > pointer dereference inside snd_hdac_bus_update_rirb(). > > > > Fix it by moving the skl_acquire_irq after ring buffer allocation. > > While here, also change err return from -EBUSY to actual error code. > > I am not that familiar with PCI gory details but that patch was > reviewed internally with no objections raised; there was also an > agreement that the SOF driver would follow the same sequence, so > > Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> It's a standard idiom for a driver allocating a shared irq line. Because an irq may be issued by another device on the same line, the registered irq handler may be kicked off before the registers or whatever else is ready for use, eventually leading to some Oops. The destructor is other way round; first free the irq handler, then release the rest resources. Takashi > > > > > Signed-off-by: Yong Zhi <yong.zhi@intel.com> > > --- > > sound/soc/intel/skylake/skl.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > > index dce649485649..cf09721ca13e 100644 > > --- a/sound/soc/intel/skylake/skl.c > > +++ b/sound/soc/intel/skylake/skl.c > > @@ -838,11 +838,7 @@ static int skl_first_init(struct hdac_bus *bus) > > snd_hdac_bus_parse_capabilities(bus); > > - if (skl_acquire_irq(bus, 0) < 0) > > - return -EBUSY; > > - > > pci_set_master(pci); > > - synchronize_irq(bus->irq); > > gcap = snd_hdac_chip_readw(bus, GCAP); > > dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap); > > @@ -875,6 +871,12 @@ static int skl_first_init(struct hdac_bus *bus) > > if (err < 0) > > return err; > > + err = skl_acquire_irq(bus, 0); > > + if (err < 0) > > + return err; > > + > > + synchronize_irq(bus->irq); > > + > > /* initialize chip */ > > skl_init_pci(skl); > > > > > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On Tue, Aug 14, 2018 at 05:16:38PM +0200, Takashi Iwai wrote: > Pierre-Louis Bossart wrote: > > I am not that familiar with PCI gory details but that patch was > > reviewed internally with no objections raised; there was also an > > agreement that the SOF driver would follow the same sequence, so > It's a standard idiom for a driver allocating a shared irq line. > Because an irq may be issued by another device on the same line, the > registered irq handler may be kicked off before the registers or > whatever else is ready for use, eventually leading to some Oops. > The destructor is other way round; first free the irq handler, then > release the rest resources. Right, it's nothing to do with PCI specifically. We even have a debug option in the kernel which will simulate the interrupt firing on potentially shared lines immediately on registering to try to catch such bugs.
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index dce649485649..cf09721ca13e 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -838,11 +838,7 @@ static int skl_first_init(struct hdac_bus *bus) snd_hdac_bus_parse_capabilities(bus); - if (skl_acquire_irq(bus, 0) < 0) - return -EBUSY; - pci_set_master(pci); - synchronize_irq(bus->irq); gcap = snd_hdac_chip_readw(bus, GCAP); dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap); @@ -875,6 +871,12 @@ static int skl_first_init(struct hdac_bus *bus) if (err < 0) return err; + err = skl_acquire_irq(bus, 0); + if (err < 0) + return err; + + synchronize_irq(bus->irq); + /* initialize chip */ skl_init_pci(skl);
Cold reboot stress test found that the hda irq could access rirb ring buffer before its memory gets allocated which resulting in null pointer dereference inside snd_hdac_bus_update_rirb(). Fix it by moving the skl_acquire_irq after ring buffer allocation. While here, also change err return from -EBUSY to actual error code. Signed-off-by: Yong Zhi <yong.zhi@intel.com> --- sound/soc/intel/skylake/skl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)