PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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?
Signature · 1 line
Ondrej Zary
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:
Quoted 11 lines
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()indrivers/pnp/driver.cis called for each pnp device. This function callspnp_start_dev()only when thePNP_DRIVER_RES_DO_NOT_CHANGEbit is NOT setingpnp_drv->flags. But the cs4236 driver insound/isa/cs423x/cs4236.cexplicitly sets the .flags toPNP_DRIVER_RES_DISABLE- it's value is 3 and that includesPNP_DRIVER_RES_DO_NOT_CHANGEbit.
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?
Quoted 8 lines
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_cs4236module:
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
Quoted 5 lines
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.
Quoted 19 lines · 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:
Quoted 11 lines
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()indrivers/pnp/driver.cis called for each pnp device. This function callspnp_start_dev()only when thePNP_DRIVER_RES_DO_NOT_CHANGEbit is NOT setingpnp_drv->flags. But the cs4236 driver insound/isa/cs423x/cs4236.cexplicitly sets the .flags toPNP_DRIVER_RES_DISABLE- it's value is 3 and that includesPNP_DRIVER_RES_DO_NOT_CHANGEbit.Ehm. Isn't that a bit unexpected:
Quoted 7 lines
#define
PNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the state
of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* 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 <[email protected]> Tue, 29 Nov 2005 09:09:32 +0100 committer Jaroslav Kysela <[email protected]> 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.
Jaroslav
-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
Quoted 9 lines · Jaroslav Kysela
Quoted 7 lines · 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:
Quoted 3 lines
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()indrivers/pnp/driver.cis called for each pnp device. This function callspnp_start_dev()only when thePNP_DRIVER_RES_DO_NOT_CHANGEbit is NOT setingpnp_drv->flags. But the cs4236 driver insound/isa/cs423x/cs4236.cexplicitly sets the .flags toPNP_DRIVER_RES_DISABLE- it's value is 3 and that includesPNP_DRIVER_RES_DO_NOT_CHANGEbit.
Quoted 8 lines
Quoted 6 lines
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.cin suspend and resume callbacks is invalid, becausePNP_DRIVER_RES_DO_NOT_CHANGEmeans 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...
Quoted 10 lines
Author of this code is:
author Pierre Ossman <[email protected]> Tue, 29 Nov 2005 09:09:32 +0100 committer Jaroslav Kysela <[email protected]> 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.
Quoted 7 lines
Cc Adam Belay <[email protected]>Cc Jaroslav Kysela <[email protected]>Cc Takashi Iwai <[email protected]>Signed-off-by Pierre Ossman <[email protected]>Signed-off-by Andrew Morton <[email protected]>Signed-off-by Takashi Iwai <[email protected]>
Rene.
drivers/pnp/driver.c +7 -5
@@ -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 ; }
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 7 lines · On Fri, 11 Jan 2008 02:19:07 +0100 Rene Herman <[email protected]> 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
Quoted 7 lines · Pierre Ossman
Quoted 4 lines · On Fri, 11 Jan 2008 02:19:07 +0100 Rene Herman <[email protected]> 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?
Quoted 8 lines
Quoted 2 lines
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.
Quoted 11 lines · On Friday 11 January 2008 15:21:55 Rene Herman wrote:
Quoted 10 lines · Pierre Ossman
On Fri, 11 Jan 2008 02:19:07 +0100
Quoted 6 lines · Rene Herman
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...
Quoted 7 lines
Quoted 5 lines
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.
Signature · 1 line
Ondrej Zary
Rene Herman
Tested-by Ondrej ZaryQuoted 6 lines · Ondrej Zary
Quoted 1 line · 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.
commit 7d16e8b3e7739599d32c8006f9e84fadb86b8296
Author: Rene Herman <[email protected]> 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.
drivers/pnp/driver.c +7 -5
@@ -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 ; }
Rene Herman
Tested-by Ondrej ZaryQuoted 6 lines · Ondrej Zary
Quoted 1 line · 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.
commit 7d16e8b3e7739599d32c8006f9e84fadb86b8296
Author: Rene Herman <[email protected]> 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.
drivers/pnp/driver.c +7 -5
@@ -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 ; }
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 5 lines · On Sat, 12 Jan 2008 02:23:27 +0100 Rene Herman <[email protected]> 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_CHANGEtest triggering inpnp_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
Quoted 5 lines · On Sat, 12 Jan 2008 02:23:27 +0100 Rene Herman <[email protected]> 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_CHANGEtest triggering inpnp_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
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 7 lines · Pierre Ossman
Quoted 2 lines · On Sat, 12 Jan 2008 02:23:27 +0100 Rene Herman <[email protected]> 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_CHANGEtest triggering inpnp_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.
Quoted 9 lines · Pierre Ossman
Quoted 4 lines · On Sat, 12 Jan 2008 02:23:27 +0100 Rene Herman <[email protected]> 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_CHANGEtest triggering inpnp_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.
Quoted 8 lines · On Sat, 12 Jan 2008 14:39:47 +0100 Rene Herman <[email protected]> wrote:
Quoted 1 line · Pierre Ossman
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.
Quoted 4 lines
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
Quoted 8 lines · On Sat, 12 Jan 2008 14:39:47 +0100 Rene Herman <[email protected]> wrote:
Quoted 1 line · Pierre Ossman
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.
Quoted 4 lines
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
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 16 lines · On Saturday 12 January 2008 16:21:50 Pierre Ossman wrote:
On Sat, 12 Jan 2008 14:39:47 +0100
Quoted 8 lines · Rene Herman
Quoted 3 lines · Pierre Ossman
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.
Quoted 6 lines
Quoted 2 lines
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.
Signature · 1 line
Ondrej Zary
Quoted 16 lines · On Saturday 12 January 2008 16:21:50 Pierre Ossman wrote:
On Sat, 12 Jan 2008 14:39:47 +0100
Quoted 8 lines · Rene Herman
Quoted 3 lines · Pierre Ossman
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.
Quoted 6 lines
Quoted 2 lines
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.
Signature · 1 line
Ondrej Zary
Quoted 1 line · Pierre Ossman
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 3 lines · Pierre Ossman
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 13 lines · Rene Herman
Quoted 3 lines · Pierre Ossman
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.Quoted 3 lines
Quoted 2 lines
But we certainly need the
pnp_start_dev()in the current flow of things. It not being called is the problem this fixes...
Quoted 6 lines
Quoted 3 lines
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_devin their resume method
Yes.
Quoted 1 line
then which means it really belongs in core.
Yes, if practical.
Quoted 4 lines
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
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...
Quoted 8 lines · Rafael J. Wysocki
Quoted 1 line · Rene Herman
It seems all PnP drivers would need to stick a
pnp_start_devin their resume methodYes.
Quoted 1 line
then which means it really belongs in core.
Quoted 8 lines
Yes, if practical.
Quoted 4 lines
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.
commit 7d16e8b3e7739599d32c8006f9e84fadb86b8296
Author: Rene Herman <[email protected]> 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.
drivers/pnp/driver.c +7 -5
@@ -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 ; }
-mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards
In reply to Rafael J. WysockiHi 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...
Quoted 8 lines · Rafael J. Wysocki
Quoted 1 line · Rene Herman
It seems all PnP drivers would need to stick a
pnp_start_devin their resume methodYes.
Quoted 1 line
then which means it really belongs in core.
Quoted 8 lines
Yes, if practical.
Quoted 4 lines
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.
commit 7d16e8b3e7739599d32c8006f9e84fadb86b8296
Author: Rene Herman <[email protected]> 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.
drivers/pnp/driver.c +7 -5
@@ -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 ; }
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards
Quoted 8 lines · Rene Herman
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_devto enable the device again after hibernation.They don't really need the
pnp_stop_dev()which the above mentioned patch also removes but with thepnp_start_dev()restored it seemspnp_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.
Quoted 7 lines · Rafael J. Wysocki
Quoted 6 lines · Rene Herman
Quoted 2 lines
It seems all PnP drivers would need to stick a
pnp_start_devin their resume methodYes.
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
then which means it really belongs in core.
Yes, if practical.
Quoted 4 lines
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
In reply to Rene HermanQuoted 8 lines · Rene Herman
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_devto enable the device again after hibernation.They don't really need the
pnp_stop_dev()which the above mentioned patch also removes but with thepnp_start_dev()restored it seemspnp_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.
Quoted 7 lines · Rafael J. Wysocki
Quoted 6 lines · Rene Herman
Quoted 2 lines
It seems all PnP drivers would need to stick a
pnp_start_devin their resume methodYes.
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
then which means it really belongs in core.
Yes, if practical.
Quoted 4 lines
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
Quoted 6 lines · Bjorn Helgaas
Quoted 4 lines · Rene Herman
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_devto enable the device again after hibernation.
Quoted 7 lines
Quoted 4 lines
They don't really need the
pnp_stop_dev()which the above mentioned patch also removes but with thepnp_start_dev()restored it seemspnp_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
In reply to Bjorn HelgaasQuoted 6 lines · Bjorn Helgaas
Quoted 4 lines · Rene Herman
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_devto enable the device again after hibernation.
Quoted 7 lines
Quoted 4 lines
They don't really need the
pnp_stop_dev()which the above mentioned patch also removes but with thepnp_start_dev()restored it seemspnp_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
Quoted 11 lines · Rene Herman
... 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 */
#definePNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the state
of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* ensure the device is
disabled */I find DISABLE including
DO_NOT_CHANGErather 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.
Quoted 3 lines
By the way, I also still have this next one outstanding for you... :-/
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
Quoted 8 lines · Bjorn Helgaas
Quoted 1 line · Rene Herman
I find DISABLE including
DO_NOT_CHANGErather 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.
Quoted 8 lines
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
In reply to Bjorn HelgaasQuoted 8 lines · Bjorn Helgaas
Quoted 1 line · Rene Herman
I find DISABLE including
DO_NOT_CHANGErather 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.
Quoted 8 lines
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
In reply to Bjorn HelgaasQuoted 11 lines · Bjorn Helgaas
Quoted 10 lines · Rene Herman
... 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 */
#definePNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the state
of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* ensure the device is
disabled */
Quoted 3 lines
Quoted 1 line
I find DISABLE including
DO_NOT_CHANGErather 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 <[email protected]>
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
Quoted 21 lines · Jaroslav Kysela
Quoted 11 lines · Bjorn Helgaas
Quoted 10 lines · Rene Herman
... 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 */
#definePNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the state
of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* ensure the device is
disabled */Quoted 3 lines
Quoted 1 line
I find DISABLE including
DO_NOT_CHANGErather 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.
Quoted 4 lines
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
0x03is 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?
Quoted 4 lines
Unfortunately, suspend / resume complicates things a bit, but PnP core can handle
DO_NOT_CHANGEflag. But it will just mean - _preserve_ resource allocation from last suspend state for this device and enable hw physically before callingresume()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
Quoted 41 lines · On Wednesday 16 January 2008 18:46:03 Bjorn Helgaas wrote:
Quoted 21 lines · Jaroslav Kysela
Quoted 11 lines · Bjorn Helgaas
Quoted 10 lines · Rene Herman
... 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 */
#definePNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the
state of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* ensure the device
is disabled */Quoted 3 lines
Quoted 1 line
I find DISABLE including
DO_NOT_CHANGErather 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.
Quoted 4 lines
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
0x03is valid in this semantics.It looks like sound drivers use
PNP_DRIVER_RES_DISABLEto 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_DISABLEis 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?
Quoted 8 lines
Quoted 4 lines
Unfortunately, suspend / resume complicates things a bit, but PnP core can handle
DO_NOT_CHANGEflag. But it will just mean - _preserve_ resource allocation from last suspend state for this device and enable hw physically before callingresume()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_CHANGEis 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.
Quoted 9 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 1 line
Ondrej Zary
Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards
In reply to Bjorn HelgaasQuoted 15 lines · Bjorn Helgaas
Quoted 3 lines · Jaroslav Kysela
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
0x03is valid in this semantics.It looks like sound drivers use
PNP_DRIVER_RES_DISABLEto 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).
Quoted 4 lines
I'm just suspicious because
PNP_DRIVER_RES_DISABLEis 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.
Quoted 5 lines
Quoted 1 line
Unfortunately, suspend / resume complicates things a bit, but PnP core can handle
DO_NOT_CHANGEflag. But it will just mean - _preserve_ resource allocation from last suspend state for this device and enable hw physically before callingresume()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_CHANGEis specified?
Yes.
Quoted 2 lines
Linux PNP definitely has issues with suspend/resume, and I suspect this is one of them.
Rene.
Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards
In reply to Bjorn HelgaasQuoted 21 lines · Bjorn Helgaas
Quoted 9 lines · Jaroslav Kysela
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
0x03is valid in this semantics.It looks like sound drivers use
PNP_DRIVER_RES_DISABLEto 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).
Quoted 4 lines
I'm just suspicious because
PNP_DRIVER_RES_DISABLEis 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.
Quoted 8 lines
Quoted 4 lines
Unfortunately, suspend / resume complicates things a bit, but PnP core can handle
DO_NOT_CHANGEflag. But it will just mean - _preserve_ resource allocation from last suspend state for this device and enable hw physically before callingresume()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_CHANGEis specified?
Yes.
Quoted 2 lines
Linux PNP definitely has issues with suspend/resume, and I suspect this is one of them.
Rene.
Re: -mm: pnp-do-not-stop-start-devices-in-suspend-resume-path.patch breaks resuming isapnp cards
In reply to Jaroslav KyselaQuoted 21 lines · Jaroslav Kysela
Quoted 11 lines · Bjorn Helgaas
Quoted 10 lines · Rene Herman
... 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 */
#definePNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the state
of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* ensure the device is
disabled */Quoted 3 lines
Quoted 1 line
I find DISABLE including
DO_NOT_CHANGErather 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.
Quoted 4 lines
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
0x03is 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?
Quoted 4 lines
Unfortunately, suspend / resume complicates things a bit, but PnP core can handle
DO_NOT_CHANGEflag. But it will just mean - _preserve_ resource allocation from last suspend state for this device and enable hw physically before callingresume()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
In reply to Bjorn HelgaasQuoted 11 lines · Bjorn Helgaas
Quoted 10 lines · Rene Herman
... 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 */
#definePNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the state
of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* ensure the device is
disabled */
Quoted 3 lines
Quoted 1 line
I find DISABLE including
DO_NOT_CHANGErather 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 <[email protected]>
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
In reply to Rene HermanQuoted 11 lines · Rene Herman
... 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 */
#definePNP_DRIVER_RES_DO_NOT_CHANGE0x0001/* do not change the state
of the device */
#definePNP_DRIVER_RES_DISABLE0x0003/* ensure the device is
disabled */I find DISABLE including
DO_NOT_CHANGErather 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.
Quoted 3 lines
By the way, I also still have this next one outstanding for you... :-/
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
Quoted 10 lines · Rene Herman
Quoted 8 lines · Pierre Ossman
Quoted 4 lines · On Sat, 12 Jan 2008 02:23:27 +0100 Rene Herman <[email protected]> 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_CHANGEtest triggering inpnp_bus_resume()and keeping the card in a suspended state.
Quoted 8 lines
Quoted 1 line
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).
Quoted 4 lines
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 whatpnp_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.
Quoted 5 lines
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.
Quoted 2 lines
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
Quoted 10 lines · Rene Herman
Quoted 8 lines · Pierre Ossman
Quoted 4 lines · On Sat, 12 Jan 2008 02:23:27 +0100 Rene Herman <[email protected]> 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_CHANGEtest triggering inpnp_bus_resume()and keeping the card in a suspended state.
Quoted 8 lines
Quoted 1 line
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).
Quoted 4 lines
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 whatpnp_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.
Quoted 5 lines
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.
Quoted 2 lines
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