| Message ID | 20201027054001.1800-3-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HDA controller and PM cleanups | expand |
On Tue, 27 Oct 2020 06:39:59 +0100, Kai-Heng Feng wrote: > > The code predates 2005, it should be unnecessary now as PCI core handles > IRQ much better nowadays. > > So stop PCI IRQ mangling in suspend/resume callbacks. > > Takashi Iwai mentioned that IRQ number can change after S3 on some > really old hardwares. We should use quirks to handle those platforms, as > most modern systems won't have that issue. I believe it was S4. And this pretty much depends on BIOS, hence it's hard to apply the quirk, honestly speaking. And, if we know that we need a quirk, dropping the code completely now is a bad move. If any, this should be applied conditionally to the "known to be modern" platforms, but this will make the code rather messier, OTOH. Do we need this change inevitably? Otherwise I'd skip this one. thanks, Takashi > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > v2: > - Wording. > - Add info on IRQ # can change on old hardwares. > > sound/pci/hda/hda_intel.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 749b88090970..b4aa1dcf1aae 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev) > { > struct snd_card *card = dev_get_drvdata(dev); > struct azx *chip; > - struct hdac_bus *bus; > > if (!azx_is_pm_ready(card)) > return 0; > > chip = card->private_data; > - bus = azx_bus(chip); > snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); > /* An ugly workaround: direct call of __azx_runtime_suspend() and > * __azx_runtime_resume() for old Intel platforms that suffer from > @@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev) > __azx_runtime_suspend(chip); > else > pm_runtime_force_suspend(dev); > - 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; > @@ -1060,11 +1050,6 @@ static int 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; > > if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP) > __azx_runtime_resume(chip, false); > -- > 2.17.1 >
> On Oct 27, 2020, at 15:36, Takashi Iwai <tiwai@suse.de> wrote: > > On Tue, 27 Oct 2020 06:39:59 +0100, > Kai-Heng Feng wrote: >> >> The code predates 2005, it should be unnecessary now as PCI core handles >> IRQ much better nowadays. >> >> So stop PCI IRQ mangling in suspend/resume callbacks. >> >> Takashi Iwai mentioned that IRQ number can change after S3 on some >> really old hardwares. We should use quirks to handle those platforms, as >> most modern systems won't have that issue. > > I believe it was S4. And this pretty much depends on BIOS, hence it's > hard to apply the quirk, honestly speaking. Ok, S4 is indeed hard to handle. > > And, if we know that we need a quirk, dropping the code completely now > is a bad move. If any, this should be applied conditionally to the > "known to be modern" platforms, but this will make the code rather > messier, OTOH. > > Do we need this change inevitably? Otherwise I'd skip this one. Ok, please drop this one. Kai-Heng > > > thanks, > > Takashi > > >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> v2: >> - Wording. >> - Add info on IRQ # can change on old hardwares. >> >> sound/pci/hda/hda_intel.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >> index 749b88090970..b4aa1dcf1aae 100644 >> --- a/sound/pci/hda/hda_intel.c >> +++ b/sound/pci/hda/hda_intel.c >> @@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev) >> { >> struct snd_card *card = dev_get_drvdata(dev); >> struct azx *chip; >> - struct hdac_bus *bus; >> >> if (!azx_is_pm_ready(card)) >> return 0; >> >> chip = card->private_data; >> - bus = azx_bus(chip); >> snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); >> /* An ugly workaround: direct call of __azx_runtime_suspend() and >> * __azx_runtime_resume() for old Intel platforms that suffer from >> @@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev) >> __azx_runtime_suspend(chip); >> else >> pm_runtime_force_suspend(dev); >> - 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; >> @@ -1060,11 +1050,6 @@ static int 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; >> >> if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP) >> __azx_runtime_resume(chip, false); >> -- >> 2.17.1 >>
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 749b88090970..b4aa1dcf1aae 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1022,13 +1022,11 @@ static int azx_suspend(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev); struct azx *chip; - struct hdac_bus *bus; if (!azx_is_pm_ready(card)) return 0; chip = card->private_data; - bus = azx_bus(chip); snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); /* An ugly workaround: direct call of __azx_runtime_suspend() and * __azx_runtime_resume() for old Intel platforms that suffer from @@ -1038,14 +1036,6 @@ static int azx_suspend(struct device *dev) __azx_runtime_suspend(chip); else pm_runtime_force_suspend(dev); - 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; @@ -1060,11 +1050,6 @@ static int 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; if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP) __azx_runtime_resume(chip, false);
The code predates 2005, it should be unnecessary now as PCI core handles IRQ much better nowadays. So stop PCI IRQ mangling in suspend/resume callbacks. Takashi Iwai mentioned that IRQ number can change after S3 on some really old hardwares. We should use quirks to handle those platforms, as most modern systems won't have that issue. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v2: - Wording. - Add info on IRQ # can change on old hardwares. sound/pci/hda/hda_intel.c | 15 --------------- 1 file changed, 15 deletions(-)