[PATCH] Fix ALSA resume

[PATCH] Fix ALSA resume

[Martin Josefsson]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

Some time ago, a patch was merged that removed pci_save_state() and
pci_restore_state() from various ALSA drivers. That patch also added
pci_restore_state() to sound/core/init.c but didn't add pci_save_state()
anywhere. This is needed since the core pci handling doesn't do this for
us anymore.

My laptop doesn't resume (gets what I assume is an ACPI timeout and
hangs solid) without this small obvious patch.

Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
Fixed-by: Takashi Iwai <tiwai@suse.de>

--- linux/sound/core/init.c	8 Nov 2004 11:37:08 -0000	1.48
+++ linux/sound/core/init.c	12 Nov 2004 13:56:32 -0000
@@ -782,12 +782,15 @@<br>
 int snd_card_pci_suspend(struct pci_dev *dev, u32 state)
 {
 	snd_card_t *card = pci_get_drvdata(dev);
+	int err;
 	if (! card || ! card->pm_suspend)
 		return 0;
 	if (card->power_state == SNDRV_CTL_POWER_D3hot)
 		return 0;
 	/* FIXME: correct state value? */
-	return card->pm_suspend(card, 0);
+	err = card->pm_suspend(card, 0);
+	pci_save_state(dev);
+	return err;
 }

 int snd_card_pci_resume(struct pci_dev *dev)


-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix ALSA resume

[Lee Revell]

Please cc: alsa-devel@lists.sourceforge.net on all ALSA issues.

On Sat, 2004-12-04 at 22:23 +0100, Martin Josefsson wrote:
> Some time ago, a patch was merged that removed pci_save_state() and
> pci_restore_state() from various ALSA drivers. That patch also added
> pci_restore_state() to sound/core/init.c but didn't add pci_save_state()
> anywhere. This is needed since the core pci handling doesn't do this for
> us anymore.
> 
> My laptop doesn't resume (gets what I assume is an ACPI timeout and
> hangs solid) without this small obvious patch.
> 
> Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
> Fixed-by: Takashi Iwai <tiwai@suse.de>
> 
> --- linux/sound/core/init.c	8 Nov 2004 11:37:08 -0000	1.48
> +++ linux/sound/core/init.c	12 Nov 2004 13:56:32 -0000
> @@ -782,12 +782,15 @@<br>
>  int snd_card_pci_suspend(struct pci_dev *dev, u32 state)
>  {
>  	snd_card_t *card = pci_get_drvdata(dev);
> +	int err;
>  	if (! card || ! card->pm_suspend)
>  		return 0;
>  	if (card->power_state == SNDRV_CTL_POWER_D3hot)
>  		return 0;
>  	/* FIXME: correct state value? */
> -	return card->pm_suspend(card, 0);
> +	err = card->pm_suspend(card, 0);
> +	pci_save_state(dev);
> +	return err;
>  }
> 
>  int snd_card_pci_resume(struct pci_dev *dev)
> 
> 
-- 
Lee Revell <rlrevell@joe-job.com>

Re: [PATCH] Fix ALSA resume

[Andrew Morton]

Martin Josefsson <gandalf@wlug.westbo.se> wrote:
>
> Some time ago, a patch was merged that removed pci_save_state() and
> pci_restore_state() from various ALSA drivers. That patch also added
> pci_restore_state() to sound/core/init.c but didn't add pci_save_state()
> anywhere. This is needed since the core pci handling doesn't do this for
> us anymore.
> 
> My laptop doesn't resume (gets what I assume is an ACPI timeout and
> hangs solid) without this small obvious patch.
> 
> Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
> Fixed-by: Takashi Iwai <tiwai@suse.de>
> 
> --- linux/sound/core/init.c	8 Nov 2004 11:37:08 -0000	1.48
> +++ linux/sound/core/init.c	12 Nov 2004 13:56:32 -0000
> @@ -782,12 +782,15 @@<br>
>  int snd_card_pci_suspend(struct pci_dev *dev, u32 state)
>  {
>  	snd_card_t *card = pci_get_drvdata(dev);
> +	int err;
>  	if (! card || ! card->pm_suspend)
>  		return 0;
>  	if (card->power_state == SNDRV_CTL_POWER_D3hot)
>  		return 0;
>  	/* FIXME: correct state value? */
> -	return card->pm_suspend(card, 0);
> +	err = card->pm_suspend(card, 0);
> +	pci_save_state(dev);
> +	return err;
>  }
> 
>  int snd_card_pci_resume(struct pci_dev *dev)

OK.  That's a better version of Joshua Kwan's patch:


From: Joshua Kwan <joshk@triplehelix.org>

Fix an intel8x0 problem which is breaking swsusp resumes.

Signed-off-by: Joshua Kwan <joshk@triplehelix.org>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/sound/pci/intel8x0.c  |    2 ++
 25-akpm/sound/pci/intel8x0m.c |    2 ++
 2 files changed, 4 insertions(+)

diff -puN sound/pci/intel8x0.c~intel8x0-pm-fix sound/pci/intel8x0.c
--- 25/sound/pci/intel8x0.c~intel8x0-pm-fix	2004-12-04 00:13:21.801532720 -0800
+++ 25-akpm/sound/pci/intel8x0.c	2004-12-04 00:13:21.808531656 -0800
@@ -2279,6 +2279,8 @@ static int intel8x0_suspend(snd_card_t *
 	for (i = 0; i < 3; i++)
 		if (chip->ac97[i])
 			snd_ac97_suspend(chip->ac97[i]);
+	pci_save_state(chip->pci);
+	pci_disable_device(chip->pci);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	return 0;
 }
diff -puN sound/pci/intel8x0m.c~intel8x0-pm-fix sound/pci/intel8x0m.c
--- 25/sound/pci/intel8x0m.c~intel8x0-pm-fix	2004-12-04 00:13:21.802532568 -0800
+++ 25-akpm/sound/pci/intel8x0m.c	2004-12-04 00:13:21.809531504 -0800
@@ -1091,6 +1091,8 @@ static int intel8x0m_suspend(snd_card_t 
 		snd_pcm_suspend_all(chip->pcm[i]);
 	if (chip->ac97)
 		snd_ac97_suspend(chip->ac97);
+	pci_save_state(chip->pci);
+	pci_disable_device(chip->pci);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	return 0;
 }
_


But Joshua crosses his heart and swears that the pci_disable_device() is
also needed for a successful swsusp resume.

Should snd_card_pci_suspend() be doing the pci_disable_device() as well, or
it that a responsibility of the driver which called snd_card_pci_suspend()?

Re: [PATCH] Fix ALSA resume

[Joshua Kwan]

Andrew Morton wrote:
> But Joshua crosses his heart and swears that the pci_disable_device() is
> also needed for a successful swsusp resume.

I never said I had any problems with resuming.

That said, I tried removing the pci_disable_device call and things seem
to still work. So I guess it can be left out?

-- 
Joshua Kwan

Re: [PATCH] Fix ALSA resume

[Andrew Morton]

Joshua Kwan <joshk@triplehelix.org> wrote:
>
> Andrew Morton wrote:
> > But Joshua crosses his heart and swears that the pci_disable_device() is
> > also needed for a successful swsusp resume.
> 
> I never said I had any problems with resuming.

OK, suspend was failing, yes?

> That said, I tried removing the pci_disable_device call and things seem
> to still work. So I guess it can be left out?

Can you please test Martin's patch?

Re: [PATCH] Fix ALSA resume

[Joshua Kwan]

Andrew Morton wrote:
> OK, suspend was failing, yes?

Yes.

> Can you please test Martin's patch?

Works for me.

-- 
Joshua Kwan

Re: [PATCH] Fix ALSA resume

[Martin Josefsson]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

On Sun, 2004-12-05 at 11:06, Joshua Kwan wrote:
> Andrew Morton wrote:
> > OK, suspend was failing, yes?
> 
> Yes.
> 
> > Can you please test Martin's patch?
> 
> Works for me.

Works here as well.
This is the only problem I've had with ALSA and swsusp.
It even handles suspending in the middle of playing music and then
resuming and continuing where it was.

-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix ALSA resume

[Takashi Iwai]

At Sat, 4 Dec 2004 17:28:55 -0800,
Andrew Morton wrote:
> 
> But Joshua crosses his heart and swears that the pci_disable_device() is
> also needed for a successful swsusp resume.

Yes.  This would make suspend safer.

The linux-sound bk tree already includes the fix above and the patches
to add pci_disable_device() in appropriate places. 
Andrew, could you update bk-alsa patch set?


> Should snd_card_pci_suspend() be doing the pci_disable_device() as well, or
> it that a responsibility of the driver which called snd_card_pci_suspend()?

So far, we suppose that the lowlevel suspend() callback should call
pci_disable_device() although we can move it to the common place,
snd_card_pci_suspend().


Takashi