[PATCH] ALSA: hda: intel: Don't free interrupt when suspending

[PATCH] ALSA: hda: intel: Don't free interrupt when suspending

[Heiner Kallweit]

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(-)

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

Re: [PATCH] ALSA: hda: intel: Don't free interrupt when suspending

[Takashi Iwai]

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
> 

Re: [PATCH] ALSA: hda: intel: Don't free interrupt when suspending

[Heiner Kallweit]

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
>>

Re: [PATCH] ALSA: hda: intel: Don't free interrupt when suspending

[Takashi Iwai]

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
> >>
> 

Re: [PATCH] ALSA: hda: intel: Don't free interrupt when suspending

[Takashi Iwai]

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