[PATCH] Add pci_save_state() to ALSA

[PATCH] Add pci_save_state() to ALSA

[Martin Josefsson]

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

Hi

Some time ago, a patch was merged that removed pci_save_state() and
pci_restore_state() from various drivers. That patch also added
pci_restore_state() to sound/core/init.c but didn't add pci_save_state()
anywhere.

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>

--- linux-2.6.10-rc1-bk21.orig/sound/core/init.c	2004-11-11 18:51:17.000000000 +0100
+++ linux-2.6.10-rc1-bk21/sound/core/init.c	2004-11-11 20:57:52.000000000 +0100
@@ -789,6 +789,8 @@ int snd_card_pci_suspend(struct pci_dev 
 		return 0;
 	if (card->power_state == SNDRV_CTL_POWER_D3hot)
 		return 0;
+	/* save the PCI config space */
+	pci_save_state(dev);
 	/* FIXME: correct state value? */
 	return card->pm_suspend(card, 0);
 }

-- 
/Martin

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

Re: [PATCH] Add pci_save_state() to ALSA

[Takashi Iwai]

At Thu, 11 Nov 2004 21:39:46 +0100,
Martin Josefsson wrote:
> 
> Hi
> 
> Some time ago, a patch was merged that removed pci_save_state() and
> pci_restore_state() from various drivers. That patch also added
> pci_restore_state() to sound/core/init.c but didn't add pci_save_state()
> anywhere.

pci_save_state() is called internally in
drivers/pci/pci-driver.c:pci_device_suspend(), so it's redundant.

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

I'm wondering how this can fix your problem...


Takashi


> Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
> 
> --- linux-2.6.10-rc1-bk21.orig/sound/core/init.c	2004-11-11 18:51:17.000000000 +0100
> +++ linux-2.6.10-rc1-bk21/sound/core/init.c	2004-11-11 20:57:52.000000000 +0100
> @@ -789,6 +789,8 @@ int snd_card_pci_suspend(struct pci_dev 
>  		return 0;
>  	if (card->power_state == SNDRV_CTL_POWER_D3hot)
>  		return 0;
> +	/* save the PCI config space */
> +	pci_save_state(dev);
>  	/* FIXME: correct state value? */
>  	return card->pm_suspend(card, 0);
>  }
> 
> -- 
> /Martin

RE: [PATCH] Add pci_save_state() to ALSA

[Zhu, Yi]

Takashi Iwai wrote:
> pci_save_state() is called internally in
> drivers/pci/pci-driver.c:pci_device_suspend(), so it's redundant.
> 
>> My laptop doesn't resume (gets what I assume is an ACPI timeout and
>> hangs solid) without this small obvious patch.
> 
> I'm wondering how this can fix your problem...

For example, some devices call pci_save_state before pci_disable_device
in
->suspend, but don't pci_enable_device in ->resume. This works before,
but 
is broken after the pci_save_state() change. We need to find those
drivers out
and change the individual drivers instead of this simple fix.

Martin, which sound driver do you use?

Thanks,
-yi

>> Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
>> 
>> --- linux-2.6.10-rc1-bk21.orig/sound/core/init.c
> 2004-11-11 18:51:17.000000000 +0100
>> +++ linux-2.6.10-rc1-bk21/sound/core/init.c	2004-11-11
>> 20:57:52.000000000 +0100 @@ -789,6 +789,8 @@ int
>>  	snd_card_pci_suspend(struct pci_dev  		return 0; if
>>  		(card->power_state == SNDRV_CTL_POWER_D3hot) return 0;
>> +	/* save the PCI config space */
>> +	pci_save_state(dev);
>>  	/* FIXME: correct state value? */
>>  	return card->pm_suspend(card, 0);
>>  }
>> 
>> --
>> /Martin
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in the body of a message to
> majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add pci_save_state() to ALSA

[Takashi Iwai]

At Fri, 12 Nov 2004 21:00:30 +0800,
Zhu, Yi wrote:
> 
> Takashi Iwai wrote:
> > pci_save_state() is called internally in
> > drivers/pci/pci-driver.c:pci_device_suspend(), so it's redundant.
> > 
> >> My laptop doesn't resume (gets what I assume is an ACPI timeout and
> >> hangs solid) without this small obvious patch.
> > 
> > I'm wondering how this can fix your problem...
> 
> For example, some devices call pci_save_state before pci_disable_device
> in
> ->suspend, but don't pci_enable_device in ->resume. This works before,
> but 
> is broken after the pci_save_state() change. We need to find those
> drivers out
> and change the individual drivers instead of this simple fix.

But pci_save_state() is called again after the driver's suspend
callback is called.  So, the final saved state must be anyway same.

> Martin, which sound driver do you use?

Yep that's important to know.


Takashi

Re: [PATCH] Add pci_save_state() to ALSA

[Arjan van de Ven]

On Fri, 2004-11-12 at 14:26 +0100, Takashi Iwai wrote:
> But pci_save_state() is called again after the driver's suspend
> callback is called.  So, the final saved state must be anyway same.

no that changed recently in the upstream kernel.
pci_save_state() is now only called if there is no suspend callback in the driver!

Re: [PATCH] Add pci_save_state() to ALSA

[Takashi Iwai]

At Fri, 12 Nov 2004 14:45:40 +0100,
Arjan van de Ven wrote:
> 
> On Fri, 2004-11-12 at 14:26 +0100, Takashi Iwai wrote:
> > But pci_save_state() is called again after the driver's suspend
> > callback is called.  So, the final saved state must be anyway same.
> 
> no that changed recently in the upstream kernel.
> pci_save_state() is now only called if there is no suspend callback in the driver!

Ah, thanks, that explains why (I referred 2.6.10-rc1).

If so, the patch is almost correct, but pci_save_state() should be put
after the call of callback.


Takashi

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

Re: [PATCH] Add pci_save_state() to ALSA

[Martin Josefsson]

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

On Fri, 2004-11-12 at 14:58, Takashi Iwai wrote:
> At Fri, 12 Nov 2004 14:45:40 +0100,
> Arjan van de Ven wrote:
> > 
> > On Fri, 2004-11-12 at 14:26 +0100, Takashi Iwai wrote:
> > > But pci_save_state() is called again after the driver's suspend
> > > callback is called.  So, the final saved state must be anyway same.
> > 
> > no that changed recently in the upstream kernel.
> > pci_save_state() is now only called if there is no suspend callback in the driver!
> 
> Ah, thanks, that explains why (I referred 2.6.10-rc1).

I realized that I forgot to include som vital information, sorry about
that.

Kernel 2.6.10-rc1-bk21 which has the pci_save_state() change in
pci-driver.c

My driver(s) are intel8x0.c and intel8x0m.c

First I just thought the missing pci_save_state() was a small mistake
and wondered why nobody else had reported the problem, later I found out
that a "generic" pci_save_state() for all pci-devices was removed
recently but I didn't have time to send a mail.

> If so, the patch is almost correct, but pci_save_state() should be put
> after the call of callback.

Great, thanks for fixing the fix.

I havn't had any problems with my broken patch, I'll see if I have the
time to test this one as well this weekend.

-- 
/Martin

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