[alsa-devel] 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?
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.
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.
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.
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; }
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
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.
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.
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.
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; }
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
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.
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
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.
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.
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
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.
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; }
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
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.
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... :-/
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
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.
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.
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
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/
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.
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
participants (6)
-
Bjorn Helgaas -
Jaroslav Kysela -
Ondrej Zary -
Pierre Ossman -
Rafael J. Wysocki -
Rene Herman