| Message ID | 1b7e109b-eb69-4542-8022-4ac8f9116474@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: intel: Don't free interrupt when suspending | expand |
On Thu, 31 Oct 2024 20:32:52 +0100, Heiner Kallweit wrote: > > There's no need to free/re-request the interrupt on system suspend. > PCI core takes care, using functions like pci_restore_msi_state(). Well, it wasn't always true for hibernation in the past, e.g. snd-intel8x0 driver had to deal with it. I guess it's no longer needed for modern platforms for HD-audio, so I'm going to take this for 6.13. thanks, Takashi > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > sound/pci/hda/hda_intel.c | 13 ------------- > 1 file changed, 13 deletions(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index b4540c5cd..9fc5e6c5d 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -1040,14 +1040,6 @@ static int azx_suspend(struct device *dev) > chip = card->private_data; > bus = azx_bus(chip); > azx_shutdown_chip(chip); > - if (bus->irq >= 0) { > - free_irq(bus->irq, chip); > - bus->irq = -1; > - chip->card->sync_irq = -1; > - } > - > - if (chip->msi) > - pci_disable_msi(chip->pci); > > trace_azx_suspend(chip); > return 0; > @@ -1062,11 +1054,6 @@ static int __maybe_unused azx_resume(struct device *dev) > return 0; > > chip = card->private_data; > - if (chip->msi) > - if (pci_enable_msi(chip->pci) < 0) > - chip->msi = 0; > - if (azx_acquire_irq(chip, 1) < 0) > - return -EIO; > > __azx_runtime_resume(chip); > > -- > 2.47.0 >
On 01.11.2024 09:21, Takashi Iwai wrote: > On Thu, 31 Oct 2024 20:32:52 +0100, > Heiner Kallweit wrote: >> >> There's no need to free/re-request the interrupt on system suspend. >> PCI core takes care, using functions like pci_restore_msi_state(). > > Well, it wasn't always true for hibernation in the past, > e.g. snd-intel8x0 driver had to deal with it. I guess it's no longer > needed for modern platforms for HD-audio, so I'm going to take this > for 6.13. > Interesting, do you have any link to what the issue with intel8x0 was? In the commit history I didn't find something related at a first glance. Maybe also PCI core simply is more mature meanwhile. Heiner > > thanks, > > Takashi > >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> sound/pci/hda/hda_intel.c | 13 ------------- >> 1 file changed, 13 deletions(-) >> >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >> index b4540c5cd..9fc5e6c5d 100644 >> --- a/sound/pci/hda/hda_intel.c >> +++ b/sound/pci/hda/hda_intel.c >> @@ -1040,14 +1040,6 @@ static int azx_suspend(struct device *dev) >> chip = card->private_data; >> bus = azx_bus(chip); >> azx_shutdown_chip(chip); >> - if (bus->irq >= 0) { >> - free_irq(bus->irq, chip); >> - bus->irq = -1; >> - chip->card->sync_irq = -1; >> - } >> - >> - if (chip->msi) >> - pci_disable_msi(chip->pci); >> >> trace_azx_suspend(chip); >> return 0; >> @@ -1062,11 +1054,6 @@ static int __maybe_unused azx_resume(struct device *dev) >> return 0; >> >> chip = card->private_data; >> - if (chip->msi) >> - if (pci_enable_msi(chip->pci) < 0) >> - chip->msi = 0; >> - if (azx_acquire_irq(chip, 1) < 0) >> - return -EIO; >> >> __azx_runtime_resume(chip); >> >> -- >> 2.47.0 >>
On Fri, 01 Nov 2024 10:49:23 +0100, Heiner Kallweit wrote: > > On 01.11.2024 09:21, Takashi Iwai wrote: > > On Thu, 31 Oct 2024 20:32:52 +0100, > > Heiner Kallweit wrote: > >> > >> There's no need to free/re-request the interrupt on system suspend. > >> PCI core takes care, using functions like pci_restore_msi_state(). > > > > Well, it wasn't always true for hibernation in the past, > > e.g. snd-intel8x0 driver had to deal with it. I guess it's no longer > > needed for modern platforms for HD-audio, so I'm going to take this > > for 6.13. > > > Interesting, do you have any link to what the issue with intel8x0 was? > In the commit history I didn't find something related at a first glance. I also don't remember exactly, sorry. It was decades ago. The old bug entry https://bugzilla.kernel.org/show_bug.cgi?id=219416 showed why the irq handler was freed at suspend on intel8x0. My vague memory says that this was the original fix, and there was another bug report of hibernation issue. I'm no longer sure whether it was intel8x0, though -- it might be another PCI sound driver, too. But the only certain thing is that it was tad old issue. > Maybe also PCI core simply is more mature meanwhile. Hopefully, and I bet for it by taking a risk to apply your cleanup :) thanks, Takashi > Heiner > > > > thanks, > > > > Takashi > > > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> sound/pci/hda/hda_intel.c | 13 ------------- > >> 1 file changed, 13 deletions(-) > >> > >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > >> index b4540c5cd..9fc5e6c5d 100644 > >> --- a/sound/pci/hda/hda_intel.c > >> +++ b/sound/pci/hda/hda_intel.c > >> @@ -1040,14 +1040,6 @@ static int azx_suspend(struct device *dev) > >> chip = card->private_data; > >> bus = azx_bus(chip); > >> azx_shutdown_chip(chip); > >> - if (bus->irq >= 0) { > >> - free_irq(bus->irq, chip); > >> - bus->irq = -1; > >> - chip->card->sync_irq = -1; > >> - } > >> - > >> - if (chip->msi) > >> - pci_disable_msi(chip->pci); > >> > >> trace_azx_suspend(chip); > >> return 0; > >> @@ -1062,11 +1054,6 @@ static int __maybe_unused azx_resume(struct device *dev) > >> return 0; > >> > >> chip = card->private_data; > >> - if (chip->msi) > >> - if (pci_enable_msi(chip->pci) < 0) > >> - chip->msi = 0; > >> - if (azx_acquire_irq(chip, 1) < 0) > >> - return -EIO; > >> > >> __azx_runtime_resume(chip); > >> > >> -- > >> 2.47.0 > >> >
On Thu, 31 Oct 2024 20:32:52 +0100, Heiner Kallweit wrote: > > There's no need to free/re-request the interrupt on system suspend. > PCI core takes care, using functions like pci_restore_msi_state(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied to for-next branch now. Thanks. Takashi
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index b4540c5cd..9fc5e6c5d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1040,14 +1040,6 @@ static int azx_suspend(struct device *dev) chip = card->private_data; bus = azx_bus(chip); azx_shutdown_chip(chip); - if (bus->irq >= 0) { - free_irq(bus->irq, chip); - bus->irq = -1; - chip->card->sync_irq = -1; - } - - if (chip->msi) - pci_disable_msi(chip->pci); trace_azx_suspend(chip); return 0; @@ -1062,11 +1054,6 @@ static int __maybe_unused azx_resume(struct device *dev) return 0; chip = card->private_data; - if (chip->msi) - if (pci_enable_msi(chip->pci) < 0) - chip->msi = 0; - if (azx_acquire_irq(chip, 1) < 0) - return -EIO; __azx_runtime_resume(chip);
There's no need to free/re-request the interrupt on system suspend. PCI core takes care, using functions like pci_restore_msi_state(). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- sound/pci/hda/hda_intel.c | 13 ------------- 1 file changed, 13 deletions(-)