Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 <[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_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 -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On Saturday 12 January 2008 16:21:50 Pierre Ossman wrote: > On Sat, 12 Jan 2008 14:39:47 +0100 > > Rene Herman <[EMAIL PROTECTED]> wrote: > > On 12-01-08 12:12, Pierre Ossman wrote: > > > I'm a bit confused here. Bjorn Helgaas wanted to remove the > > > pnp_start/stop_dev() calls completely, and you want them called all the > > > time. :) > > > > Wanted where? Haven't seen a coment from Bjorn? But -- while removing > > them both looks (as) sensible from a mirror-image viewpoint, this > > wouldn't fix the problem. > > Ah, sorry. It was a different thread. Look for a mail with the subject > "PNP: do not stop/start devices in suspend/resume path" in the LKML och > linux-pm archives. > > > But we certainly need the pnp_start_dev() in the current flow of things. > > It not being called is the problem this fixes... > > I think the previous suggestion was that the drivers should call this, not > the core, so that it behaved more like other parts of the kernel (e.g. > PCI). I don't think that drivers should call pnp_start_dev() on resume. All drivers would need to call it as all PnP cards are disabled after boot. No driver does that currently. 3c509 driver doesn't seem to register as pnp_card_driver so that's probably why it's not enable after resume. I guess that more ISA PnP drivers have this problem. I have some other PnP network and sound cards so I'll test them. -- Ondrej Zary -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On Sat, 12 Jan 2008 14:39:47 +0100 Rene Herman <[EMAIL PROTECTED]> 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 signature.asc Description: PGP signature
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On 12-01-08 12:12, Pierre Ossman wrote: 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_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. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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_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 signature.asc Description: PGP signature
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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_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 signature.asc Description: PGP signature
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On 12-01-08 12:12, Pierre Ossman wrote: 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_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. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On Sat, 12 Jan 2008 14:39:47 +0100 Rene Herman [EMAIL PROTECTED] 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 signature.asc Description: PGP signature
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On Saturday 12 January 2008 16:21:50 Pierre Ossman wrote: On Sat, 12 Jan 2008 14:39:47 +0100 Rene Herman [EMAIL PROTECTED] wrote: On 12-01-08 12:12, Pierre Ossman wrote: I'm a bit confused here. Bjorn Helgaas wanted to remove the pnp_start/stop_dev() calls completely, and you want them called all the time. :) Wanted where? Haven't seen a coment from Bjorn? But -- while removing them both looks (as) sensible from a mirror-image viewpoint, this wouldn't fix the problem. Ah, sorry. It was a different thread. Look for a mail with the subject PNP: do not stop/start devices in suspend/resume path in the LKML och linux-pm archives. But we certainly need the pnp_start_dev() in the current flow of things. It not being called is the problem this fixes... I think the previous suggestion was that the drivers should call this, not the core, so that it behaved more like other parts of the kernel (e.g. PCI). I don't think that drivers should call pnp_start_dev() on resume. All drivers would need to call it as all PnP cards are disabled after boot. No driver does that currently. 3c509 driver doesn't seem to register as pnp_card_driver so that's probably why it's not enable after resume. I guess that more ISA PnP drivers have this problem. I have some other PnP network and sound cards so I'll test them. -- Ondrej Zary -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 [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_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 -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 <[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.
Signed-off-by: Rene Herman <[EMAIL PROTECTED]>
Tested-by: Ondrej Zary <[EMAIL PROTECTED]>
diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t
state)
return error;
}
- if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
- pnp_can_disable(pnp_dev)) {
+ if (pnp_can_disable(pnp_dev)) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
if (pnp_dev->protocol && pnp_dev->protocol->resume)
pnp_dev->protocol->resume(pnp_dev);
- if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+ if (pnp_can_write(pnp_dev)) {
error = pnp_start_dev(pnp_dev);
if (error)
return error;
}
- if (pnp_drv->resume)
- return pnp_drv->resume(pnp_dev);
+ if (pnp_drv->resume) {
+ error = pnp_drv->resume(pnp_dev);
+ if (error)
+ return error;
+ }
return 0;
}
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 <[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. > > Hrmpf. Well, okay. Ondrej -- I assume this patch fixes things? Yes, it works fine. 3c509 card still does not work after resume, but that looks like another problem. -- Ondrej Zary -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On 11-01-08 08:01, Pierre Ossman wrote: 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. Hrmpf. Well, okay. Ondrej -- I assume this patch fixes things? Rene. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
On 11-01-08 08:01, Pierre Ossman wrote: 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. Hrmpf. Well, okay. Ondrej -- I assume this patch fixes things? Rene. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 [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. Hrmpf. Well, okay. Ondrej -- I assume this patch fixes things? Yes, it works fine. 3c509 card still does not work after resume, but that looks like another problem. -- Ondrej Zary -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 [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.
Signed-off-by: Rene Herman [EMAIL PROTECTED]
Tested-by: Ondrej Zary [EMAIL PROTECTED]
diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t
state)
return error;
}
- if (!(pnp_drv-flags PNP_DRIVER_RES_DO_NOT_CHANGE)
- pnp_can_disable(pnp_dev)) {
+ if (pnp_can_disable(pnp_dev)) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
if (pnp_dev-protocol pnp_dev-protocol-resume)
pnp_dev-protocol-resume(pnp_dev);
- if (!(pnp_drv-flags PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+ if (pnp_can_write(pnp_dev)) {
error = pnp_start_dev(pnp_dev);
if (error)
return error;
}
- if (pnp_drv-resume)
- return pnp_drv-resume(pnp_dev);
+ if (pnp_drv-resume) {
+ error = pnp_drv-resume(pnp_dev);
+ if (error)
+ return error;
+ }
return 0;
}
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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 signature.asc Description: PGP signature
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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_CHANGE0x0001 /* 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 <[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.
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.
diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t
state)
return error;
}
- if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
- pnp_can_disable(pnp_dev)) {
+ if (pnp_can_disable(pnp_dev)) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
if (pnp_dev->protocol && pnp_dev->protocol->resume)
pnp_dev->protocol->resume(pnp_dev);
- if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+ if (pnp_can_write(pnp_dev)) {
error = pnp_start_dev(pnp_dev);
if (error)
return error;
}
- if (pnp_drv->resume)
- return pnp_drv->resume(pnp_dev);
+ if (pnp_drv->resume) {
+ error = pnp_drv->resume(pnp_dev);
+ if (error)
+ return error;
+ }
return 0;
}
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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_CHANGE0x0001 /* 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 [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.
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.
diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t
state)
return error;
}
- if (!(pnp_drv-flags PNP_DRIVER_RES_DO_NOT_CHANGE)
- pnp_can_disable(pnp_dev)) {
+ if (pnp_can_disable(pnp_dev)) {
error = pnp_stop_dev(pnp_dev);
if (error)
return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
if (pnp_dev-protocol pnp_dev-protocol-resume)
pnp_dev-protocol-resume(pnp_dev);
- if (!(pnp_drv-flags PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+ if (pnp_can_write(pnp_dev)) {
error = pnp_start_dev(pnp_dev);
if (error)
return error;
}
- if (pnp_drv-resume)
- return pnp_drv-resume(pnp_dev);
+ if (pnp_drv-resume) {
+ error = pnp_drv-resume(pnp_dev);
+ if (error)
+ return error;
+ }
return 0;
}
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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_CHANGE0x0001 /* 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 <[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. 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]> Jaroslav - Jaroslav Kysela <[EMAIL PROTECTED]> Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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_CHANGE0x0001 /* 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. -- 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/
Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least with snd_cs4236
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_CHANGE0x0001 /* 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. -- 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/
