PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Ondrej Zary]

Hello,
as hibernation (swsusp) started to work with my CPU, I found that my Turtle 
Beach Malibu stops working after resume from hibernation. It's caused by fact 
that the card is not enabled on the pnp layer during resume - and thus card 
registers are inaccessible (reads return FFs, writes go nowhere).

During resume, pnp_bus_resume() in drivers/pnp/driver.c is called for each pnp 
device. This function calls pnp_start_dev() only when the 
PNP_DRIVER_RES_DO_NOT_CHANGE bit is NOT seting pnp_drv->flags. But the cs4236 
driver in sound/isa/cs423x/cs4236.c explicitly sets the .flags to 
PNP_DRIVER_RES_DISABLE - it's value is 3 and that includes 
PNP_DRIVER_RES_DO_NOT_CHANGE bit.

The same .flags value is present in many of the ALSA ISA sound drivers.

Removing that .flags line caused this to appear inlog when loading snd_cs4236 
module:
CS4236+ WSS PnP manual resources are invalid, using auto config
CS4236+ CTRL PnP manual resources are invalid, using auto config
CS4236+ MPU401 PnP manual resources are invalid, using auto config

and the sound now works after resume!

So the question is: why is this line present?

Is this a bug? What's the correct fix?

-- 
Ondrej Zary

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rene Herman]

On 09-01-08 23:43, Ondrej Zary wrote:

Jaroslav -- in your role as ISA-PnP maintainer and Bjorn, in yours as having 
been foollish enough to touch PnP recently:

> as hibernation (swsusp) started to work with my CPU, I found that my Turtle 
> Beach Malibu stops working after resume from hibernation. It's caused by fact 
> that the card is not enabled on the pnp layer during resume - and thus card 
> registers are inaccessible (reads return FFs, writes go nowhere).
> 
> During resume, pnp_bus_resume() in drivers/pnp/driver.c is called for each pnp 
> device. This function calls pnp_start_dev() only when the 
> PNP_DRIVER_RES_DO_NOT_CHANGE bit is NOT seting pnp_drv->flags. But the cs4236 
> driver in sound/isa/cs423x/cs4236.c explicitly sets the .flags to 
> PNP_DRIVER_RES_DISABLE - it's value is 3 and that includes 
> PNP_DRIVER_RES_DO_NOT_CHANGE bit.

Ehm. Isn't that a bit unexpected:

#define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
of the device */
#define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
disabled */

I'd say that disabling is changing, so isn't this just a braino where 
someone meant to write 2 instead of 3?

> The same .flags value is present in many of the ALSA ISA sound drivers.
> 
> Removing that .flags line caused this to appear inlog when loading snd_cs4236 
> module:
> CS4236+ WSS PnP manual resources are invalid, using auto config
> CS4236+ CTRL PnP manual resources are invalid, using auto config
> CS4236+ MPU401 PnP manual resources are invalid, using auto config
> 
> and the sound now works after resume!
> 
> So the question is: why is this line present?
> 
> Is this a bug? What's the correct fix?

Rene.

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Jaroslav Kysela]

On Thu, 10 Jan 2008, Rene Herman wrote:

> On 09-01-08 23:43, Ondrej Zary wrote:
> 
> Jaroslav -- in your role as ISA-PnP maintainer and Bjorn, in yours as 
> having been foollish enough to touch PnP recently:
> 
> > as hibernation (swsusp) started to work with my CPU, I found that my Turtle 
> > Beach Malibu stops working after resume from hibernation. It's caused by fact 
> > that the card is not enabled on the pnp layer during resume - and thus card 
> > registers are inaccessible (reads return FFs, writes go nowhere).
> > 
> > During resume, pnp_bus_resume() in drivers/pnp/driver.c is called for each pnp 
> > device. This function calls pnp_start_dev() only when the 
> > PNP_DRIVER_RES_DO_NOT_CHANGE bit is NOT seting pnp_drv->flags. But the cs4236 
> > driver in sound/isa/cs423x/cs4236.c explicitly sets the .flags to 
> > PNP_DRIVER_RES_DISABLE - it's value is 3 and that includes 
> > PNP_DRIVER_RES_DO_NOT_CHANGE bit.
> 
> Ehm. Isn't that a bit unexpected:
> 
> #define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
> of the device */
> #define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
> disabled */
> 
> I'd say that disabling is changing, so isn't this just a braino where 
> someone meant to write 2 instead of 3?

It's irrelevant. I think that condition in driver.c in suspend and resume 
callbacks is invalid, because PNP_DRIVER_RES_DO_NOT_CHANGE means that 
resources should not be changed in the pnp core but only in the driver, 
but in suspend/resume process are resources preserved, so the condition 
should be removed.

Author of this code is:

author Pierre Ossman <drzeus-list@drzeus.cx> Tue, 29 Nov 2005 09:09:32 +0100
committer Jaroslav Kysela <perex@suse.cz> Tue, 03 Jan 2006 12:31:30 +0100

    [ALSA] [PATCH] alsa: Improved PnP suspend support

    Also use the PnP functions to start/stop the devices during the suspend so
    that drivers will not have to duplicate this code.

    Cc: Adam Belay <ambx1@neo.rr.com>
    Cc: Jaroslav Kysela <perex@suse.cz>
    Cc: Takashi Iwai <tiwai@suse.de>

    Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Takashi Iwai <tiwai@suse.de>


						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rene Herman]

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

On 10-01-08 08:58, Jaroslav Kysela wrote:

> On Thu, 10 Jan 2008, Rene Herman wrote:
> 
>> On 09-01-08 23:43, Ondrej Zary wrote:
>>
>> Jaroslav -- in your role as ISA-PnP maintainer and Bjorn, in yours as 
>> having been foollish enough to touch PnP recently:
>>
>>> as hibernation (swsusp) started to work with my CPU, I found that my Turtle 
>>> Beach Malibu stops working after resume from hibernation. It's caused by fact 
>>> that the card is not enabled on the pnp layer during resume - and thus card 
>>> registers are inaccessible (reads return FFs, writes go nowhere).
>>>
>>> During resume, pnp_bus_resume() in drivers/pnp/driver.c is called for each pnp 
>>> device. This function calls pnp_start_dev() only when the 
>>> PNP_DRIVER_RES_DO_NOT_CHANGE bit is NOT seting pnp_drv->flags. But the cs4236 
>>> driver in sound/isa/cs423x/cs4236.c explicitly sets the .flags to 
>>> PNP_DRIVER_RES_DISABLE - it's value is 3 and that includes 
>>> PNP_DRIVER_RES_DO_NOT_CHANGE bit.
>> Ehm. Isn't that a bit unexpected:
>>
>> #define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
>> of the device */
>> #define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
>> disabled */
>>
>> I'd say that disabling is changing, so isn't this just a braino where 
>> someone meant to write 2 instead of 3?
> 
> It's irrelevant. I think that condition in driver.c in suspend and resume 
> callbacks is invalid, because PNP_DRIVER_RES_DO_NOT_CHANGE means that 
> resources should not be changed in the pnp core but only in the driver, 
> but in suspend/resume process are resources preserved, so the condition 
> should be removed.

I see a PnP resume _consists_ of setting the resources so I can see the test 
implementation wise, but yes, conceptually it seems this test shouldn't be 
present. So just like the attached then?

Pierre, what was the idea here? Added the other SOBs to the CC as well...

> Author of this code is:
> 
> author Pierre Ossman <drzeus-list@drzeus.cx> Tue, 29 Nov 2005 09:09:32 +0100
> committer Jaroslav Kysela <perex@suse.cz> Tue, 03 Jan 2006 12:31:30 +0100
> 
>     [ALSA] [PATCH] alsa: Improved PnP suspend support
> 
>     Also use the PnP functions to start/stop the devices during the suspend so
>     that drivers will not have to duplicate this code.
> 
>     Cc: Adam Belay <ambx1@neo.rr.com>
>     Cc: Jaroslav Kysela <perex@suse.cz>
>     Cc: Takashi Iwai <tiwai@suse.de>
> 
>     Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
>     Signed-off-by: Andrew Morton <akpm@osdl.org>
>     Signed-off-by: Takashi Iwai <tiwai@suse.de>

Rene.


[-- Attachment #2: pnp_driver_res_do_not_test.diff --]
[-- Type: text/plain, Size: 944 bytes --]

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t state)
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
+	if (pnp_can_disable(pnp_dev)) {
 		error = pnp_stop_dev(pnp_dev);
 		if (error)
 			return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+	if (pnp_can_write(pnp_dev)) {
 		error = pnp_start_dev(pnp_dev);
 		if (error)
 			return error;
 	}
 
-	if (pnp_drv->resume)
-		return pnp_drv->resume(pnp_dev);
+	if (pnp_drv->resume) {
+		error = pnp_drv->resume(pnp_dev);
+		if (error)
+			return error;
+	}
 
 	return 0;
 }

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Pierre Ossman]

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

On Fri, 11 Jan 2008 02:19:07 +0100
Rene Herman <rene.herman@keyaccess.nl> wrote:

> 
> I see a PnP resume _consists_ of setting the resources so I can see the test 
> implementation wise, but yes, conceptually it seems this test shouldn't be 
> present. So just like the attached then?
> 
> Pierre, what was the idea here? Added the other SOBs to the CC as well...
> 

You assume there was an idea? ;)

I don't remember why things were done the way they were. And I can't find any clues in the correspondence around the issue. So your guess is as good as mine.

Rgds
Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rene Herman]

On 11-01-08 08:01, Pierre Ossman wrote:

> On Fri, 11 Jan 2008 02:19:07 +0100
> Rene Herman <rene.herman@keyaccess.nl> wrote:
> 
>> I see a PnP resume _consists_ of setting the resources so I can see the test 
>> implementation wise, but yes, conceptually it seems this test shouldn't be 
>> present. So just like the attached then?
>>
>> Pierre, what was the idea here? Added the other SOBs to the CC as well...
>>
> 
> You assume there was an idea? ;)
> 
> I don't remember why things were done the way they were. And I can't find
> any clues in the correspondence around the issue. So your guess is as
> good as mine.

Hrmpf. Well, okay. Ondrej -- I assume this patch fixes things?

Rene.

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Ondrej Zary]

On Friday 11 January 2008 15:21:55 Rene Herman wrote:
> On 11-01-08 08:01, Pierre Ossman wrote:
> > On Fri, 11 Jan 2008 02:19:07 +0100
> >
> > Rene Herman <rene.herman@keyaccess.nl> wrote:
> >> I see a PnP resume _consists_ of setting the resources so I can see the
> >> test implementation wise, but yes, conceptually it seems this test
> >> shouldn't be present. So just like the attached then?
> >>
> >> Pierre, what was the idea here? Added the other SOBs to the CC as
> >> well...
> >
> > You assume there was an idea? ;)
> >
> > I don't remember why things were done the way they were. And I can't find
> > any clues in the correspondence around the issue. So your guess is as
> > good as mine.
>
> Hrmpf. Well, okay. Ondrej -- I assume this patch fixes things?

Yes, it works fine.
3c509 card still does not work after resume, but that looks like another 
problem.

-- 
Ondrej Zary

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rene Herman]

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

On 11-01-08 19:40, Ondrej Zary wrote:

> On Friday 11 January 2008 15:21:55 Rene Herman wrote:

>> Hrmpf. Well, okay. Ondrej -- I assume this patch fixes things?
> 
> Yes, it works fine. 3c509 card still does not work after resume, but that
> looks like another problem.

Okay. Would now only still like to know why the test in resume() means 
trouble for you while it seems the same test in suspend() should've 
triggered as well and not have stopped the device in the first place.

Know absolutely nothing about hibernation so added the listed maintainers to 
the CC.

Pavel, Rafael -- the attached fixes snd-cs4236 not coming back to life for 
Ondrej after hibernation due to the PNP_DRIVER_RES_DO_NOT_CHANGE test 
triggering in pnp_bus_resume() and keeping the card in a suspended state.

There's issues on wether or not the flag _should_ be set (that is, be part 
of PNP_DRIVER_RES_DISABLE) and that it shouldn't be tested by these code 
patchs in the first place, but is it in fact expected that this would be 
neccessary?

That is, is it expected/designed that the same test in pnp_bus_suspend() 
didn't cause the device to not be disabled in the first place?

Rene.

[-- Attachment #2: pnp_driver_res_do_not_test.diff --]
[-- Type: text/plain, Size: 2275 bytes --]

commit 7d16e8b3e7739599d32c8006f9e84fadb86b8296
Author: Rene Herman <rene.herman@gmail.com>
Date:   Sat Jan 12 00:00:35 2008 +0100

    PNP: do not test PNP_DRIVER_RES_DO_NOT_CHANGE on suspend/resume
    
    The PNP_DRIVER_RES_DO_NOT_CHANGE flag is meant to signify that
    the PNP core should not change resources for the device -- not
    that it shouldn't disable/enable the device on suspend/resume.
    
    ALSA ISAPnP drivers set PNP_DRIVER_RES_DO_NOT_CHANAGE (0x0001)
    through setting PNP_DRIVER_RES_DISABLE (0x0003). The latter
    including the former may in itself be considered rather
    unexpected but doesn't change that suspend/resume wouldn't seem
    to have any business testing the flag.
    
    As reported by Ondrej Zary for snd-cs4236, ALSA driven ISAPnP
    cards don't survive swsusp hibernation with the resume skipping
    setting the resources due to testing the flag -- the same test
    in the suspend path isn't enough to keep hibernation from
    disabling the card it seems.
    
    These tests were added (in 2005) by Piere Ossman in commit
    68094e3251a664ee1389fcf179497237cbf78331, "alsa: Improved PnP
    suspend support" who doesn't remember why. This deletes them.
    
    Signed-off-by: Rene Herman <rene.herman@gmail.com>
    Tested-by: Ondrej Zary <linux@rainbow-software.org>

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t state)
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
+	if (pnp_can_disable(pnp_dev)) {
 		error = pnp_stop_dev(pnp_dev);
 		if (error)
 			return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+	if (pnp_can_write(pnp_dev)) {
 		error = pnp_start_dev(pnp_dev);
 		if (error)
 			return error;
 	}
 
-	if (pnp_drv->resume)
-		return pnp_drv->resume(pnp_dev);
+	if (pnp_drv->resume) {
+		error = pnp_drv->resume(pnp_dev);
+		if (error)
+			return error;
+	}
 
 	return 0;
 }

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Pierre Ossman]

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

On Sat, 12 Jan 2008 02:23:27 +0100
Rene Herman <rene.herman@keyaccess.nl> wrote:

> 
> Pavel, Rafael -- the attached fixes snd-cs4236 not coming back to life for 
> Ondrej after hibernation due to the PNP_DRIVER_RES_DO_NOT_CHANGE test 
> triggering in pnp_bus_resume() and keeping the card in a suspended state.
> 

I'm a bit confused here. Bjorn Helgaas wanted to remove the pnp_start/stop_dev() calls completely, and you want them called all the time. :)

Rgds
Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rene Herman]

On 12-01-08 12:12, Pierre Ossman wrote:

> On Sat, 12 Jan 2008 02:23:27 +0100
> Rene Herman <rene.herman@keyaccess.nl> wrote:
> 
>> Pavel, Rafael -- the attached fixes snd-cs4236 not coming back to life for 
>> Ondrej after hibernation due to the PNP_DRIVER_RES_DO_NOT_CHANGE test 
>> triggering in pnp_bus_resume() and keeping the card in a suspended state.
>>
> 
> I'm a bit confused here. Bjorn Helgaas wanted to remove the pnp_start/stop_dev() calls completely, and you want them called all the time. :)

Wanted where? Haven't seen a coment from Bjorn? But -- while removing them 
both looks (as) sensible from a mirror-image viewpoint, this wouldn't fix 
the problem.

As fas as I understand things, "hibernation" is basically "save state, shut 
down machine" where the latter step is going to disable the card inevitably. 
On resume, you're going to need to restore the resources (that were saved in 
pnp_dev->res) that it was using to the card, which is what pnp_start_dev() 
does -- it not being called is the current problem of the card not coming 
back to life.

pnp_stop_dev() could go in this specific situation. Not much point in first 
disabling the thing it seems if a subsequent shutdown is going to take care 
of that anyway. However, suspend/resume isn't just called in the case of 
complete hibernation I guess and I know nothing about it all -- hence my 
request to the hiberhation people for how this is designed to be.

But we certainly need the pnp_start_dev() in the current flow of things. It 
not being called is the problem this fixes...

Rene.

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Pierre Ossman]

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

On Sat, 12 Jan 2008 14:39:47 +0100
Rene Herman <rene.herman@keyaccess.nl> wrote:

> On 12-01-08 12:12, Pierre Ossman wrote:
> 
> > I'm a bit confused here. Bjorn Helgaas wanted to remove the pnp_start/stop_dev() calls completely, and you want them called all the time. :)
> 
> Wanted where? Haven't seen a coment from Bjorn? But -- while removing them 
> both looks (as) sensible from a mirror-image viewpoint, this wouldn't fix 
> the problem.
> 

Ah, sorry. It was a different thread. Look for a mail with the subject "PNP: do not stop/start devices in suspend/resume path" in the LKML och linux-pm archives.

> 
> But we certainly need the pnp_start_dev() in the current flow of things. It 
> not being called is the problem this fixes...
> 

I think the previous suggestion was that the drivers should call this, not the core, so that it behaved more like other parts of the kernel (e.g. PCI).

Rgds
Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Ondrej Zary]

On Saturday 12 January 2008 16:21:50 Pierre Ossman wrote:
> On Sat, 12 Jan 2008 14:39:47 +0100
>
> Rene Herman <rene.herman@keyaccess.nl> wrote:
> > On 12-01-08 12:12, Pierre Ossman wrote:
> > > I'm a bit confused here. Bjorn Helgaas wanted to remove the
> > > pnp_start/stop_dev() calls completely, and you want them called all the
> > > time. :)
> >
> > Wanted where? Haven't seen a coment from Bjorn? But -- while removing
> > them both looks (as) sensible from a mirror-image viewpoint, this
> > wouldn't fix the problem.
>
> Ah, sorry. It was a different thread. Look for a mail with the subject
> "PNP: do not stop/start devices in suspend/resume path" in the LKML och
> linux-pm archives.
>
> > But we certainly need the pnp_start_dev() in the current flow of things.
> > It not being called is the problem this fixes...
>
> I think the previous suggestion was that the drivers should call this, not
> the core, so that it behaved more like other parts of the kernel (e.g.
> PCI).

I don't think that drivers should call pnp_start_dev() on resume. All drivers 
would need to call it as all PnP cards are disabled after boot. No driver 
does that currently.

3c509 driver doesn't seem to register as pnp_card_driver so that's probably 
why it's not enable after resume. I guess that more ISA PnP drivers have this 
problem. I have some other PnP network and sound cards so I'll test them.

-- 
Ondrej Zary

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rene Herman]

On 12-01-08 16:21, Pierre Ossman wrote:

> Ah, sorry. It was a different thread. Look for a mail with the subject 
> "PNP: do not stop/start devices in suspend/resume path" in the LKML och 
> linux-pm archives.

Right, and I see that the removal of start/stop is already in -mm. That's 
not going to work. Something (such as removing power) disabled Ondrej's 
CS4236 and the pnp_start_dev() is needed to re-enable it upon resume.

>> But we certainly need the pnp_start_dev() in the current flow of
>> things. It not being called is the problem this fixes...
> 
> I think the previous suggestion was that the drivers should call this,
> not the core, so that it behaved more like other parts of the kernel
> (e.g. PCI).

It seems all PnP drivers would need to stick a pnp_start_dev in their resume 
method then which means it really belongs in core. One important point where 
PnP and PCI differ is that PnP allows to change the resources on a protocol 
level and I don't see how it could ever not be necessary to restore the 
state a user may have set if power has been removed. Hibernate is just that, 
isn't it?

Rene.

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rafael J. Wysocki]

On Saturday, 12 of January 2008, Rene Herman wrote:
> On 12-01-08 12:12, Pierre Ossman wrote:
> 
> > On Sat, 12 Jan 2008 02:23:27 +0100
> > Rene Herman <rene.herman@keyaccess.nl> wrote:
> > 
> >> Pavel, Rafael -- the attached fixes snd-cs4236 not coming back to life for 
> >> Ondrej after hibernation due to the PNP_DRIVER_RES_DO_NOT_CHANGE test 
> >> triggering in pnp_bus_resume() and keeping the card in a suspended state.
> >>
> > 
> > I'm a bit confused here. Bjorn Helgaas wanted to remove the pnp_start/stop_dev() calls completely, and you want them called all the time. :)
> 
> Wanted where? Haven't seen a coment from Bjorn? But -- while removing them 
> both looks (as) sensible from a mirror-image viewpoint, this wouldn't fix 
> the problem.
> 
> As fas as I understand things, "hibernation" is basically "save state, shut 
> down machine" where the latter step is going to disable the card inevitably.

Not exactly.  In the ACPI (aka platform) mode it's:
- pretend we're suspending and put everything into low power states
- snapshot memory
- power everything up
- save image
- suspend (ie. enter S4, suspending devices before).
 
> On resume, you're going to need to restore the resources (that were saved in 
> pnp_dev->res) that it was using to the card, which is what pnp_start_dev() 
> does -- it not being called is the current problem of the card not coming 
> back to life.

First of all, the card need not be initialized at all before .resume() is called.

> pnp_stop_dev() could go in this specific situation. Not much point in first 
> disabling the thing it seems if a subsequent shutdown is going to take care 
> of that anyway. However, suspend/resume isn't just called in the case of 
> complete hibernation I guess and I know nothing about it all -- hence my 
> request to the hiberhation people for how this is designed to be.

.suspend()/.resume() are used during suspend to RAM too.  As far as hibernation
is concerned, .resume() is used for two different purposes:
(a) to bring the device up after it's been put into a low power state for
    snapshotting memory
(b) to bring the device up (or reconfigure it if brought up already) after the
    hibernation image has been loaded and we're restoring the previous
    system state.

> But we certainly need the pnp_start_dev() in the current flow of things. It 
> not being called is the problem this fixes...

Yes, pnp_start_dev() is generally needed in .resume().

Thanks,
Rafael

Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236

[Rafael J. Wysocki]

On Saturday, 12 of January 2008, Rene Herman wrote:
> On 12-01-08 16:21, Pierre Ossman wrote:
> 
> > Ah, sorry. It was a different thread. Look for a mail with the subject 
> > "PNP: do not stop/start devices in suspend/resume path" in the LKML och 
> > linux-pm archives.
> 
> Right, and I see that the removal of start/stop is already in -mm. That's 
> not going to work. Something (such as removing power) disabled Ondrej's 
> CS4236 and the pnp_start_dev() is needed to re-enable it upon resume.
> 
> >> But we certainly need the pnp_start_dev() in the current flow of
> >> things. It not being called is the problem this fixes...
> > 
> > I think the previous suggestion was that the drivers should call this,
> > not the core, so that it behaved more like other parts of the kernel
> > (e.g. PCI).
> 
> It seems all PnP drivers would need to stick a pnp_start_dev in their resume 
> method

Yes.

> then which means it really belongs in core.

Yes, if practical.

> One important point where PnP and PCI differ is that PnP allows to change the
> resources on a protocol level and I don't see how it could ever not be
> necessary to restore the state a user may have set if power has been
> removed. Hibernate is just that, isn't it?

Basically, yes, it is.

Thanks,
Rafael

-mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Rene Herman]

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

Hi Andrew.

pnp-do-not-stop-start-devices-in-suspend-resume-path.patch in current -mm 
breaks resuming isapnp cards from hibernation. They need the pnp_start_dev 
to enable the device again after hibernation.

They don't really need the pnp_stop_dev() which the above mentioned patch 
also removes but with the pnp_start_dev() restored it seems pnp_stop_dev() 
should also stay. Bjorn Helgaas should decide  -- currently the patch as you 
have it breaks drivers though. Could you drop it?

Then, if so and after you do that, could you apply the attached? That's also 
needed to resume (ALSA) ISA-PnP cards from hibernation due to the 
RES_DO_NOT_CHANGE test triggering for ALSA drivers and the pnp_start_dev() 
still not happening. More in the changelog...

On 12-01-08 20:08, Rafael J. Wysocki wrote:

> On Saturday, 12 of January 2008, Rene Herman wrote:

>> It seems all PnP drivers would need to stick a pnp_start_dev in their resume 
>> method
> 
> Yes.
> 
>> then which means it really belongs in core.
> 
> Yes, if practical.
> 
>> One important point where PnP and PCI differ is that PnP allows to change the
>> resources on a protocol level and I don't see how it could ever not be
>> necessary to restore the state a user may have set if power has been
>> removed. Hibernate is just that, isn't it?
> 
> Basically, yes, it is.

Rene.


[-- Attachment #2: pnp_driver_res_do_not_test.diff --]
[-- Type: text/plain, Size: 2275 bytes --]

commit 7d16e8b3e7739599d32c8006f9e84fadb86b8296
Author: Rene Herman <rene.herman@gmail.com>
Date:   Sat Jan 12 00:00:35 2008 +0100

    PNP: do not test PNP_DRIVER_RES_DO_NOT_CHANGE on suspend/resume
    
    The PNP_DRIVER_RES_DO_NOT_CHANGE flag is meant to signify that
    the PNP core should not change resources for the device -- not
    that it shouldn't disable/enable the device on suspend/resume.
    
    ALSA ISAPnP drivers set PNP_DRIVER_RES_DO_NOT_CHANAGE (0x0001)
    through setting PNP_DRIVER_RES_DISABLE (0x0003). The latter
    including the former may in itself be considered rather
    unexpected but doesn't change that suspend/resume wouldn't seem
    to have any business testing the flag.
    
    As reported by Ondrej Zary for snd-cs4236, ALSA driven ISAPnP
    cards don't survive swsusp hibernation with the resume skipping
    setting the resources due to testing the flag -- the same test
    in the suspend path isn't enough to keep hibernation from
    disabling the card it seems.
    
    These tests were added (in 2005) by Piere Ossman in commit
    68094e3251a664ee1389fcf179497237cbf78331, "alsa: Improved PnP
    suspend support" who doesn't remember why. This deletes them.
    
    Signed-off-by: Rene Herman <rene.herman@gmail.com>
    Tested-by: Ondrej Zary <linux@rainbow-software.org>

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t state)
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
+	if (pnp_can_disable(pnp_dev)) {
 		error = pnp_stop_dev(pnp_dev);
 		if (error)
 			return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+	if (pnp_can_write(pnp_dev)) {
 		error = pnp_start_dev(pnp_dev);
 		if (error)
 			return error;
 	}
 
-	if (pnp_drv->resume)
-		return pnp_drv->resume(pnp_dev);
+	if (pnp_drv->resume) {
+		error = pnp_drv->resume(pnp_dev);
+		if (error)
+			return error;
+	}
 
 	return 0;
 }

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Bjorn Helgaas]

On Saturday 12 January 2008 1:08:01 pm Rene Herman wrote:
> pnp-do-not-stop-start-devices-in-suspend-resume-path.patch in current -mm
> breaks resuming isapnp cards from hibernation. They need the pnp_start_dev
> to enable the device again after hibernation.
>
> They don't really need the pnp_stop_dev() which the above mentioned patch
> also removes but with the pnp_start_dev() restored it seems pnp_stop_dev()
> should also stay. Bjorn Helgaas should decide  -- currently the patch as
> you have it breaks drivers though. Could you drop it?

Yes, please drop pnp-do-not-stop-start-devices-in-suspend-resume-path.patch
for now.

When the PNP core requested resources for active devices, the
pnp_stop_dev() in the suspend path released the PNP core resources,
leaving the underlying driver resources orphaned.  But the patch
that made the PNP core request those resource is also gone, so
we can leave the start/stop alone.

> On 12-01-08 20:08, Rafael J. Wysocki wrote:
> > On Saturday, 12 of January 2008, Rene Herman wrote:
> >> It seems all PnP drivers would need to stick a pnp_start_dev in their
> >> resume method
> >
> > Yes.
> >
> >> then which means it really belongs in core.
> >
> > Yes, if practical.
> >
> >> One important point where PnP and PCI differ is that PnP allows to
> >> change the resources on a protocol level and I don't see how it could
> >> ever not be necessary to restore the state a user may have set if power
> >> has been removed. Hibernate is just that, isn't it?

That's a good point, thanks for pointing that out.

Bjorn

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Rene Herman]

On 13-01-08 06:50, Bjorn Helgaas wrote:

> On Saturday 12 January 2008 1:08:01 pm Rene Herman wrote:

>> pnp-do-not-stop-start-devices-in-suspend-resume-path.patch in current -mm
>> breaks resuming isapnp cards from hibernation. They need the pnp_start_dev
>> to enable the device again after hibernation.
>>
>> They don't really need the pnp_stop_dev() which the above mentioned patch
>> also removes but with the pnp_start_dev() restored it seems pnp_stop_dev()
>> should also stay. Bjorn Helgaas should decide  -- currently the patch as
>> you have it breaks drivers though. Could you drop it?
> 
> Yes, please drop pnp-do-not-stop-start-devices-in-suspend-resume-path.patch
> for now.

Okay, thanks for the reply. And, now that I have your attention, while it's 
not important to the issue anymore with the tests removed as the submitted 
patch did, do you have an opinion on (include/linux/pnp.h):

/* pnp driver flags */
#define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
of the device */
#define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
disabled */

I find DISABLE including DO_NOT_CHANGE rather unexpected...

By the way, I also still have this next one outstanding for you... :-/

http://lkml.org/lkml/2008/1/9/168

Rene.

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Bjorn Helgaas]

On Saturday 12 January 2008 11:13:35 pm Rene Herman wrote:
> ... And, now that I have your attention, while it's 
> not important to the issue anymore with the tests removed as the submitted 
> patch did, do you have an opinion on (include/linux/pnp.h):
> 
> /* pnp driver flags */
> #define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
> of the device */
> #define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
> disabled */
> 
> I find DISABLE including DO_NOT_CHANGE rather unexpected...

I don't know the history of those flags, but I wish they didn't exist.
They really look like warts in the PNP core code.  They're used so
infrequently and without obvious rationale, that it seems like it'd
be better if there were a way to deal with them inside the driver.

But to answer your question, I don't know enough to have an opinion
on whether DISABLE should include DO_NOT_CHANGE.

> By the way, I also still have this next one outstanding for you... :-/
> 
> http://lkml.org/lkml/2008/1/9/168

This had to do with the excessive warnings about exceeding the maximum
number of resources for a PNP device.  This should be resolved by Len's
patch here:

  http://bugzilla.kernel.org/show_bug.cgi?id=9535#c10

We all agree this is a stop-gap, and for 2.6.25, we need the real
solution of making PNP resources fully dynamic.

Bjorn

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Rene Herman]

On 14-01-08 23:26, Bjorn Helgaas wrote:

> On Saturday 12 January 2008 11:13:35 pm Rene Herman wrote:

>> I find DISABLE including DO_NOT_CHANGE rather unexpected...
> 
> I don't know the history of those flags, but I wish they didn't exist.
> They really look like warts in the PNP core code.  They're used so
> infrequently and without obvious rationale, that it seems like it'd
> be better if there were a way to deal with them inside the driver.

I see, thanks for the comment. PNP_DRIVER_RES_DISABLE is used by ALSA only 
and used by _all_ ALSA ISA-PnP drivers (snd-sscape uses RES_DO_NOT_CHANGE 
instead but we should consider that one a consistency bug).

RES_DO_NOT_CHANGE is used by drivers/pnp/system.c and rtc-cmos.c as well. 
I'll look at this. Getting rid of DISABLE as a first step should not be 
overly problematic. This might again be a left-over from days where no easy 
to use interface to PnP existed which it now does in echoing things into sysfs.

Takashi: which reminds me -- crap, I promised to document more of that for 
ALSA use following up the recent pnp driver-side resource setting removal. 
Sorry, forgot, will do.

> This had to do with the excessive warnings about exceeding the maximum
> number of resources for a PNP device.  This should be resolved by Len's
> patch here:
> 
>   http://bugzilla.kernel.org/show_bug.cgi?id=9535#c10
> 
> We all agree this is a stop-gap, and for 2.6.25, we need the real
> solution of making PNP resources fully dynamic.

Thank you. Just pulled and see that's now indeed in. Wasn't in -rc7 yet...

Rene.

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Jaroslav Kysela]

On Mon, 14 Jan 2008, Bjorn Helgaas wrote:

> On Saturday 12 January 2008 11:13:35 pm Rene Herman wrote:
> > ... And, now that I have your attention, while it's 
> > not important to the issue anymore with the tests removed as the submitted 
> > patch did, do you have an opinion on (include/linux/pnp.h):
> > 
> > /* pnp driver flags */
> > #define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
> > of the device */
> > #define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
> > disabled */
> > 
> > I find DISABLE including DO_NOT_CHANGE rather unexpected...
> 
> I don't know the history of those flags, but I wish they didn't exist.

Ok, something to explain. These flags exists to allow drivers to 
manually configure (override) PnP resources at init time - we know - for 
example in ALSA - that some combinations simply does not work for all 
soundcards.

The DISABLE flags simply tells core PnP layer - driver will handle 
resource allocation itself, don't do anything, just disable hw physically 
and do not change (allocate) any resources. Value 0x03 is valid in this 
semantics.

Unfortunately, suspend / resume complicates things a bit, but PnP core can 
handle DO_NOT_CHANGE flag. But it will just mean - _preserve_ resource 
allocation from last suspend state for this device and enable hw 
physically before calling resume() callback.

					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Bjorn Helgaas]

On Tuesday 15 January 2008 12:51:35 am Jaroslav Kysela wrote:
> On Mon, 14 Jan 2008, Bjorn Helgaas wrote:
> > On Saturday 12 January 2008 11:13:35 pm Rene Herman wrote:
> > > ... And, now that I have your attention, while it's 
> > > not important to the issue anymore with the tests removed as the submitted 
> > > patch did, do you have an opinion on (include/linux/pnp.h):
> > > 
> > > /* pnp driver flags */
> > > #define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
> > > of the device */
> > > #define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
> > > disabled */
> > > 
> > > I find DISABLE including DO_NOT_CHANGE rather unexpected...
> > 
> > I don't know the history of those flags, but I wish they didn't exist.
> 
> Ok, something to explain. These flags exists to allow drivers to 
> manually configure (override) PnP resources at init time - we know - for 
> example in ALSA - that some combinations simply does not work for all 
> soundcards.
>
> The DISABLE flags simply tells core PnP layer - driver will handle 
> resource allocation itself, don't do anything, just disable hw physically 
> and do not change (allocate) any resources. Value 0x03 is valid in this 
> semantics.

It looks like sound drivers use PNP_DRIVER_RES_DISABLE to say "ignore
what PNP tells us about resource usage and we'll just use the compiled-
in or command-line-specified resources".

The main reason to do that would be to work around BIOS defects or
to work around deficiencies in the Linux PNP infrastructure (e.g.,
maybe we erroneously place another device on top of the sound card
or something).

I'm just suspicious because PNP_DRIVER_RES_DISABLE is only used in
sound drivers.  If it's to work around BIOS defects, why wouldn't
other PNP drivers need it sometimes, too?  And wouldn't it be better
to use PNP quirks for BIOS workarounds?

> Unfortunately, suspend / resume complicates things a bit, but PnP core can 
> handle DO_NOT_CHANGE flag. But it will just mean - _preserve_ resource 
> allocation from last suspend state for this device and enable hw 
> physically before calling resume() callback.

When resuming, wouldn't we *always* want to preserve the resource
allocation from the last suspend, regardless of whether
PNP_DRIVER_RES_DO_NOT_CHANGE is specified?

Linux PNP definitely has issues with suspend/resume, and I suspect
this is one of them.

Bjorn

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Ondrej Zary]

On Wednesday 16 January 2008 18:46:03 Bjorn Helgaas wrote:
> On Tuesday 15 January 2008 12:51:35 am Jaroslav Kysela wrote:
> > On Mon, 14 Jan 2008, Bjorn Helgaas wrote:
> > > On Saturday 12 January 2008 11:13:35 pm Rene Herman wrote:
> > > > ... And, now that I have your attention, while it's
> > > > not important to the issue anymore with the tests removed as the
> > > > submitted patch did, do you have an opinion on (include/linux/pnp.h):
> > > >
> > > > /* pnp driver flags */
> > > > #define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the
> > > > state of the device */
> > > > #define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device
> > > > is disabled */
> > > >
> > > > I find DISABLE including DO_NOT_CHANGE rather unexpected...
> > >
> > > I don't know the history of those flags, but I wish they didn't exist.
> >
> > Ok, something to explain. These flags exists to allow drivers to
> > manually configure (override) PnP resources at init time - we know - for
> > example in ALSA - that some combinations simply does not work for all
> > soundcards.
> >
> > The DISABLE flags simply tells core PnP layer - driver will handle
> > resource allocation itself, don't do anything, just disable hw physically
> > and do not change (allocate) any resources. Value 0x03 is valid in this
> > semantics.
>
> It looks like sound drivers use PNP_DRIVER_RES_DISABLE to say "ignore
> what PNP tells us about resource usage and we'll just use the compiled-
> in or command-line-specified resources".
>
> The main reason to do that would be to work around BIOS defects or
> to work around deficiencies in the Linux PNP infrastructure (e.g.,
> maybe we erroneously place another device on top of the sound card
> or something).
>
> I'm just suspicious because PNP_DRIVER_RES_DISABLE is only used in
> sound drivers.  If it's to work around BIOS defects, why wouldn't
> other PNP drivers need it sometimes, too?  And wouldn't it be better
> to use PNP quirks for BIOS workarounds?
>
> > Unfortunately, suspend / resume complicates things a bit, but PnP core
> > can handle DO_NOT_CHANGE flag. But it will just mean - _preserve_
> > resource allocation from last suspend state for this device and enable hw
> > physically before calling resume() callback.
>
> When resuming, wouldn't we *always* want to preserve the resource
> allocation from the last suspend, regardless of whether
> PNP_DRIVER_RES_DO_NOT_CHANGE is specified?

I'd say yes. If base address of a card changes, driver will break.

I grepped drivers for these flags too - to find out what they do (or should 
do?) - but failed to find out anything useful.

> Linux PNP definitely has issues with suspend/resume, and I suspect
> this is one of them.
>
> Bjorn
> --
> 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/



-- 
Ondrej Zary

Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards

[Rene Herman]

On 16-01-08 18:46, Bjorn Helgaas wrote:

> On Tuesday 15 January 2008 12:51:35 am Jaroslav Kysela wrote:

>> Ok, something to explain. These flags exists to allow drivers to 
>> manually configure (override) PnP resources at init time - we know - for 
>> example in ALSA - that some combinations simply does not work for all 
>> soundcards.
>>
>> The DISABLE flags simply tells core PnP layer - driver will handle 
>> resource allocation itself, don't do anything, just disable hw physically 
>> and do not change (allocate) any resources. Value 0x03 is valid in this 
>> semantics.
> 
> It looks like sound drivers use PNP_DRIVER_RES_DISABLE to say "ignore
> what PNP tells us about resource usage and we'll just use the compiled-
> in or command-line-specified resources".
> 
> The main reason to do that would be to work around BIOS defects or
> to work around deficiencies in the Linux PNP infrastructure (e.g.,
> maybe we erroneously place another device on top of the sound card
> or something).
> 
> I'm just suspicious because PNP_DRIVER_RES_DISABLE is only used in
> sound drivers.  If it's to work around BIOS defects, why wouldn't
> other PNP drivers need it sometimes, too?  And wouldn't it be better
> to use PNP quirks for BIOS workarounds?

Yes. The manual resource setting was recently removed from the ALSA drivers 
and I'd expect this can now go as a package-deal.

>> Unfortunately, suspend / resume complicates things a bit, but PnP core can 
>> handle DO_NOT_CHANGE flag. But it will just mean - _preserve_ resource 
>> allocation from last suspend state for this device and enable hw 
>> physically before calling resume() callback.
> 
> When resuming, wouldn't we *always* want to preserve the resource
> allocation from the last suspend, regardless of whether
> PNP_DRIVER_RES_DO_NOT_CHANGE is specified?

Yes.

> Linux PNP definitely has issues with suspend/resume, and I suspect
> this is one of them.

Rene.