Hello,
as hibernation (swsusp) started to work with my CPU, I found that my Turtle
Beach Malibu stops working after resume from hibernation. It's caused by fact
that the card is not enabled on the pnp layer during resume - and thus card
registers are inaccessible (reads return FFs, writes go nowhere).
During resume, pnp_bus_resume() in drivers/pnp/driver.c is called for each pnp
device. This function calls pnp_start_dev() only when the
PNP_DRIVER_RES_DO_NOT_CHANGE bit is NOT seting pnp_drv->flags. But the cs4236
driver in sound/isa/cs423x/cs4236.c explicitly sets the .flags to
PNP_DRIVER_RES_DISABLE - it's value is 3 and that includes
PNP_DRIVER_RES_DO_NOT_CHANGE bit.
The same .flags value is present in many of the ALSA ISA sound drivers.
Removing that .flags line caused this to appear inlog when loading snd_cs4236
module:
CS4236+ WSS PnP manual resources are invalid, using auto config
CS4236+ CTRL PnP manual resources are invalid, using auto config
CS4236+ MPU401 PnP manual resources are invalid, using auto config
and the sound now works after resume!
So the question is: why is this line present?
Is this a bug? What's the correct fix?
--
Ondrej Zary
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 <[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.
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 <[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
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.
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
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;
}
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
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.
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
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
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 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
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
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;
}
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... :-/
>
> http://lkml.org/lkml/2008/1/9/168
This had to do with the excessive warnings about exceeding the maximum
number of resources for a PNP device. This should be resolved by Len's
patch here:
http://bugzilla.kernel.org/show_bug.cgi?id=9535#c10
We all agree this is a stop-gap, and for 2.6.25, we need the real
solution of making PNP resources fully dynamic.
Bjorn
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 <[email protected]>
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 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 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Ondrej Zary