[patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

I've noticed a bunch of problem reports that go like this:

 - boot system with some USB devices attached
 - echo disk > /sys/power/state
 - ... later resume ...
 - now those USB devices don't work right
 - unplug them/replug them, all is OK

I recently observed this myself and tracked down one problem.  The solution
involves what kexec() does in much the same situation:  before starting a
new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
instead, which is the root cause of the problem.

This seems like something that might need to sit in the MM tree for
a while, as it's a longstanding bug and the fix could shake loose driver
goofs.  Luckily there's a partial workaround already in wide use:  link
drivers as modules if they use true suspend states (like all USB HCDs),
don't link them statically into the kernel image.

That workaround is partial because when BIOS takes over a controller, the
resume() method has to handle that nasty case too!  Last I checked, all
the USB HCDs do handle that case; and few drivers other than those HCDs
would ever need to worry about such issues.

- Dave



[-- Attachment #2: swsusp.patch --]
[-- Type: text/x-diff, Size: 1712 bytes --]

Currently the swsusp resume path puts devices into a bogus suspend state
before switching to the new kernel.  It's neither the suspend state that
the driver left the device in, nor a "device lost power" reset state.

Instead, it's a semi-functional state from the kernel which loaded the
snapshot.  There's no way that a driver can properly resume from that.
Some drivers issue resets in their resume methods, but that's wrong for
every driver which uses more power states than just "on" and "reset".

This patch shuts down (resets) the devices, just like kexec() does, so
that non-modular drivers will know they must fully re-initialize even
when they wouldn't ordinarily do so during resume().


--- linux.orig/kernel/power/swsusp.c	2006-04-23 08:41:22.000000000 -0700
+++ linux/kernel/power/swsusp.c	2006-04-24 13:20:17.000000000 -0700
@@ -49,6 +49,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/highmem.h>
+#include <linux/reboot.h>
 
 #include "power.h"
 
@@ -249,10 +250,16 @@
 int swsusp_resume(void)
 {
 	int error;
-	local_irq_disable();
-	if (device_power_down(PMSG_FREEZE))
-		printk(KERN_ERR "Some devices failed to power down, very bad\n");
+
+	/* We need to shutdown/reset devices so that drivers which use
+	 * non-powerdown suspend() states will see sane states in their
+	 * resume() methods.  At this point "reset" is the only option,
+	 * since we clobbered any valid suspend state a long time ago.
+	 */
+	kernel_restart_prepare(NULL);
+
 	/* We'll ignore saved state, but this gets preempt count (etc) right */
+	local_irq_disable();
 	save_processor_state();
 	error = swsusp_arch_resume();
 	/* Code below is only ever reached in case of failure. Otherwise

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

Hi,

On Monday 24 April 2006 23:29, David Brownell wrote:
> I've noticed a bunch of problem reports that go like this:
> 
>  - boot system with some USB devices attached
>  - echo disk > /sys/power/state
>  - ... later resume ...
>  - now those USB devices don't work right
>  - unplug them/replug them, all is OK
> 
> I recently observed this myself and tracked down one problem.  The solution
> involves what kexec() does in much the same situation:  before starting a
> new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
> instead, which is the root cause of the problem.
> 
> This seems like something that might need to sit in the MM tree for
> a while, as it's a longstanding bug and the fix could shake loose driver
> goofs.  Luckily there's a partial workaround already in wide use:  link
> drivers as modules if they use true suspend states (like all USB HCDs),
> don't link them statically into the kernel image.
> 
> That workaround is partial because when BIOS takes over a controller, the
> resume() method has to handle that nasty case too!  Last I checked, all
> the USB HCDs do handle that case; and few drivers other than those HCDs
> would ever need to worry about such issues.

I agree with this change but I think it'll cause some strange things to happen
for some time.

Greetings,
Rafael


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

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

Hi David.

On Tuesday 25 April 2006 07:29, David Brownell wrote:
> I've noticed a bunch of problem reports that go like this:
>
>  - boot system with some USB devices attached
>  - echo disk > /sys/power/state
>  - ... later resume ...
>  - now those USB devices don't work right
>  - unplug them/replug them, all is OK
>
> I recently observed this myself and tracked down one problem.  The solution
> involves what kexec() does in much the same situation:  before starting a
> new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
> instead, which is the root cause of the problem.

I'm not sure that it is. If we switched to not freezing devices, we'd then 
cause issues with hardware such as hard drives. They need to know that we 
just want things quiesced, mainly because we don't want to spin down drives. 
It seems to me that the right solution might be for these usb devices to 
treat a resume from a freeze as an indication that hardware should be reset.

Regards,

Nigel

-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Monday 24 April 2006 2:47 pm, Rafael J. Wysocki wrote:
> 
> I agree with this change but I think it'll cause some strange things to happen
> for some time.

Let's see.  :)  I think it's less likely to be drivers with broken
shutdown() callbacks than drivers which are missing them.

Plus, one of the main reasons that driver suspend/resume handling
is _still_ so goofy is that most drivers are stupid, and just reinit
devices from scratch in their resume() methods.  Drivers like that
should not notice any difference.  :)

- Dave



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

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

Hi,

On Tuesday 25 April 2006 00:31, Nigel Cunningham wrote:
> On Tuesday 25 April 2006 07:29, David Brownell wrote:
> > I've noticed a bunch of problem reports that go like this:
> >
> >  - boot system with some USB devices attached
> >  - echo disk > /sys/power/state
> >  - ... later resume ...
> >  - now those USB devices don't work right
> >  - unplug them/replug them, all is OK
> >
> > I recently observed this myself and tracked down one problem.  The solution
> > involves what kexec() does in much the same situation:  before starting a
> > new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
> > instead, which is the root cause of the problem.
> 
> I'm not sure that it is. If we switched to not freezing devices, we'd then 
> cause issues with hardware such as hard drives. They need to know that we 
> just want things quiesced, mainly because we don't want to spin down drives.

That's right.  And kernel_restart_prepare(NULL) will make them spin down?
If so, they have to be treated in a special way.

OTOH I think at least some device driver writers assume that .resume() will
always be called after .suspend() which only is true for non-modular drivers
(or for modular drivers loaded from an initrd before resume).  The David's
change makes this assumptions explicitly invalid and that's why I said
I agreed with it.  However you're absolutely right about hard disks.

Greetings,
Rafael

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

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

Hi.

On Tuesday 25 April 2006 08:47, David Brownell wrote:
> On Monday 24 April 2006 2:47 pm, Rafael J. Wysocki wrote:
> > I agree with this change but I think it'll cause some strange things to
> > happen for some time.
>
> Let's see.  :)  I think it's less likely to be drivers with broken
> shutdown() callbacks than drivers which are missing them.
>
> Plus, one of the main reasons that driver suspend/resume handling
> is _still_ so goofy is that most drivers are stupid, and just reinit
> devices from scratch in their resume() methods.  Drivers like that
> should not notice any difference.  :)

No!

The only reason that driver suspend/resume handling is still far from perfect 
is that a good number of drivers still don't have any suspend/resume 
handling. Getting rid of the instrastructure because it isn't completely 
implemented is the wrong approach. We should instead complete implementing 
the support so that drivers understand the difference between freezing, 
suspending, powering down, powering up and resuming. If we dumb things down, 
we'll only create problems.

Rafael raised the issue in another email of code built as modules that is 
suspended and not resumed or vice versa because in the case of suspend to 
disk, the module is loaded at boot time but not in the suspended kernel (or 
v.v). It seems to me that the right way to deal with this is to extend the 
use of __nosave so that information about what was loaded in the boot kernel 
is available when resuming drivers after the atomic restore.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-usb-devel] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1485 bytes --]

On Mon, 24 Apr 2006, David Brownell wrote:

> I've noticed a bunch of problem reports that go like this:
> 
>  - boot system with some USB devices attached
>  - echo disk > /sys/power/state
>  - ... later resume ...
>  - now those USB devices don't work right
>  - unplug them/replug them, all is OK
> 
> I recently observed this myself and tracked down one problem.  The solution
> involves what kexec() does in much the same situation:  before starting a
> new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
> instead, which is the root cause of the problem.
> 
> This seems like something that might need to sit in the MM tree for
> a while, as it's a longstanding bug and the fix could shake loose driver
> goofs.  Luckily there's a partial workaround already in wide use:  link
> drivers as modules if they use true suspend states (like all USB HCDs),
> don't link them statically into the kernel image.
> 
> That workaround is partial because when BIOS takes over a controller, the
> resume() method has to handle that nasty case too!  Last I checked, all
> the USB HCDs do handle that case; and few drivers other than those HCDs
> would ever need to worry about such issues.

We might have been able to avoid some of these problems with USB devices
if we had been a bit more careful.  In particular, it wouldn't hurt to
have usb_resume_device() verify that the upstream port is enabled and not
suspended in the non-CONFIG_USB_SUSPEND case.

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4829 bytes --]

On Tue, 25 Apr 2006, Nigel Cunningham wrote:

> It seems to me that the right solution might be for these usb devices to
> treat a resume from a freeze as an indication that hardware should be reset.

That can't be right.  For example, we issue resume from freeze during the 
suspend portion of swsusp (to write out the image).

> The only reason that driver suspend/resume handling is still far from perfect 
> is that a good number of drivers still don't have any suspend/resume 
> handling. Getting rid of the instrastructure because it isn't completely 
> implemented is the wrong approach. We should instead complete implementing 
> the support so that drivers understand the difference between freezing, 
> suspending, powering down, powering up and resuming. If we dumb things down, 
> we'll only create problems.

I agree that this is a major reason.  It's not the _only_ reason.  But 
never mind that now...

> Rafael raised the issue in another email of code built as modules that is 
> suspended and not resumed or vice versa because in the case of suspend to 
> disk, the module is loaded at boot time but not in the suspended kernel (or 
> v.v). It seems to me that the right way to deal with this is to extend the 
> use of __nosave so that information about what was loaded in the boot kernel 
> is available when resuming drivers after the atomic restore.

It's not clear that you're getting the point.  (In fact, it's not entirely
clear that I understand it perfectly either, because this is fairly
subtle.)

When the USB subsystem resumes a device, it relies on the fact that a
certain amount of state has been preserved _in the device_.  At a minimum,
that state includes the device's bus address, which is assigned
dynamically and obviously is necessary for communicating with the device.  
This state will be lost whenever the "power session" is interrupted, which
means that USB devices cannot remain suspended when VBUS power is lost --
and many systems do not provide VBUS suspend current while in various
sleep states, although some systems do.

Regardless, the problem is that the resuming kernel doesn't have any
terribly good ways of telling whether or not the power session has
remained intact.  It can test whether the host controller is still in the
same state that a suspend would leave it in.  It can test whether the
device's port is still enabled and whether anything responds at the
device's bus address.  (We don't currently try to tell whether the
responder actually _is_ the device we're trying to resume as opposed to
some other device!)

The real difficulty comes when the USB drivers are compiled into the boot 
kernel or loaded by an initrd.  They will take over the host controllers 
and any devices they can find, destroying the preserved state.  Then when 
they are told to prepare for the upcoming atomic restore, they will try to 
freeze or reset (or whatever) the devices.

Now the resuming kernel starts up, and it has to guess whether everything
is still properly suspended with all the necessary state intact.  Suppose
the boot kernel has destroyed the state and left the devices frozen, as
happens now (without David's patch).  It will appear to the resuming
kernel that the devices are still suspended, but unbeknownst to the kernel 
the state information is gone.  That's why the devices don't work after 
resuming until the user unplugs and replugs them.

The right way to solve this is to make sure that the resuming kernel can
correctly determine whether the power session (way back from the original
sw-suspend) is still intact.  It's expected that in many cases it won't
be, because most systems won't provide suspend current while the machine
is off.  We have to guarantee that the boot kernel's actions won't end up
fooling the resuming kernel into thinking that the power sessions are
intact when in fact they aren't.  (Furthermore, in an ideal world, we
would also make sure that the boot kernel won't destroy any power sessions
that still _are_ intact.  Right now we have no way to do this, because the
drivers in the boot kernel don't know that it _is_ a boot kernel.)

Making the boot kernel shut down the host controllers instead of freezing
or suspending them will indeed destroy all existing power sessions, as
well as making it clear to the resuming kernel that that are gone.  Under
certain circumstances, freezing the host controllers will fool the
resuming kernel.

Propagating some kind of information from one kernel to the next is the
solution.  Maybe that's what you meant.  Trying to store that information
in the host controller's state is a poor-man's way of doing it, but at the
moment it's the only way we have.  That's why David doesn't want the state 
during the atomic reload to be the same as the state during a regular 
resume.

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Tuesday 25 April 2006 6:50 am, Alan Stern wrote:
> 
> We might have been able to avoid some of these problems with USB devices
> if we had been a bit more careful.  In particular, it wouldn't hurt to
> have usb_resume_device() verify that the upstream port is enabled and not
> suspended in the non-CONFIG_USB_SUSPEND case.

Nope ... remember, the USB device enumerated from scratch both times.
Identically, reproducibly.  So the first device will again be given
address 2, and will even pass the "is it still there" sanity check in
finish_device_resume().  (I was testing with USB_SUSPEND of course.)

But that device wasn't in the _right_ suspend state, the one in which
it had been left before swsusp turned the power off.  Only the low level
setup had been (could have been) done -- USB enumeration, that's all.

The reason such devices can't work after this fakey "resume" is that
the system (specifically swsusp) is lying about things.  It brought
all the hardware part way up, obliterating all the true state that
drivers rely on to make resume() behave correctly.

Making swsusp do like kexec -- resetting the devices that need it -- is
just making that PM code stop its lies.

HCDs (and similar drivers) can handle it OK if there was a power loss
after suspend().  By calling device_suspend() not device_shutdown(),
swsusp is going out of its way to prevent drivers from knowing .

- Dave



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

On Monday 24 April 2006 3:31 pm, Nigel Cunningham wrote:
> >
> > I recently observed this myself and tracked down one problem.  The solution
> > involves what kexec() does in much the same situation:  before starting a
> > new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
> > instead, which is the root cause of the problem.
> 
> I'm not sure that it is. If we switched to not freezing devices, we'd then 
> cause issues with hardware such as hard drives. They need to know that we 
> just want things quiesced, mainly because we don't want to spin down drives.

Ignore potential performance issue for a minute -- it didn't spin anything
down in my testing so far, anyway! -- and focus on correctness instead.

How could it be possible to hand a device to driver with resume() and not
have it in one of the two defined states for that?  Either (a) powerup reset,
just like during resume from STR with especially aggressive hardware; or
else (b) the state suspend() left it in, ready for no-data-lossage resume()?


> It seems to me that the right solution might be for these usb devices to 
> treat a resume from a freeze as an indication that hardware should be reset.

I don't much like that model.  It creates more special cases in drivers (not
just USB), and redefines the "freeze" thing to be more than just "quiescede".
Every special case that gets added is a breeding ground for more bugs.

Also, what about the resume-from-freeze that _every_ device does during swsusp
suspend?  The kernel is still resuming every device, not just the ones needed
to write the suspend partition.

- Dave



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

On Tuesday 25 April 2006 1:32 am, Rafael J. Wysocki wrote:

> > just want things quiesced, mainly because we don't want to spin down drives.
> 
> That's right.  And kernel_restart_prepare(NULL) will make them spin down?
> If so, they have to be treated in a special way.

Seems every time that someone turns up one of the problems with this "freeze"
thing, the same responses come back:  "we can't fix that because we don't want
to spin down drives".

I've begun thinking that calls like pm_should_I_spin_down_drives() would be a
better structural approach than continually redefining this "freeze" thing so
it makes less and less sense to all other drivers ... who nonethless need to
clutter themselves up with a growing list of special cases, to accomodate
rotating media that may not even exist in the target system.


> OTOH I think at least some device driver writers assume that .resume() will
> always be called after .suspend() which only is true for non-modular drivers
> (or for modular drivers loaded from an initrd before resume). 

Say what?  Of _course_ resume() should only be called after suspend().  If
that's not true in any case, the code wrongly issuing the resume() is buggy.

I don't see where you're collecting that list of special cases.  It doesn't
match what I've observed ... and even if it were correct, I'd say that's yet
another case where special casing reflects bugs.

- Dave

  

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

On Tuesday 25 April 2006 3:34 am, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 25 April 2006 08:47, David Brownell wrote:
> > On Monday 24 April 2006 2:47 pm, Rafael J. Wysocki wrote:
> > > I agree with this change but I think it'll cause some strange things to
> > > happen for some time.
> >
> > Let's see.  :)  I think it's less likely to be drivers with broken
> > shutdown() callbacks than drivers which are missing them.
> >
> > Plus, one of the main reasons that driver suspend/resume handling
> > is _still_ so goofy is that most drivers are stupid, and just reinit
> > devices from scratch in their resume() methods.  Drivers like that
> > should not notice any difference.  :)
> 
> No!

To what in particular?  The US version of fascism?  Everything in general?
One specific comment above?  All of them?


> The only reason that driver suspend/resume handling is still far from perfect 
> is that a good number of drivers still don't have any suspend/resume 
> handling.

That's a separate issue.  I was referring to the structural issues,
the "handling".  Specifically, the ones which make it hard to use
more device states than just the "on" and "off" required by swsusp.

For example, there's the "drivers can't detect the target system state"
issue ... the pm_message_t conversion replaced a value which had originally
been used to indicate that state (back in 2.4) with something that's all
but useless for any purpose other than telling IDE when not to spin down.
(I have one proposal to help there, which I'll make at some point in a
separate thread.)

It's those structural issues that make it hard to do things like handling
both swsusp _and_ real system suspend states like STR ... and to make
wakeup events work.  I do understand that ACPI is sometimes in the way,
but the structural issues are what keep things from acting sane even on
nice clean embedded hardware where not even a bootloader gets in the way
of having Linux do proper suspend/wakeup/resume handling.


> Getting rid of the instrastructure because it isn't completely  
> implemented is the wrong approach.

I didn't advocate getting rid of any infrastructure.  How could you even
read what I wrote as recommending that?  There wasn't even a mention of
anything to be removed.


> We should instead complete implementing  
> the support so that drivers understand the difference between freezing, 
> suspending, powering down, powering up and resuming. If we dumb things down, 
> we'll only create problems.

Another of the reasons system suspend/resume is goofy is that there's
still confusion about what that support is, and what those differences are.

For example, you were the one who recently (yesterday!!) advocated redefining
FREEZE so it's more than just driver quiescence.

And for that matter, there are no driver power down/up APIs; all we have are
suspend/resume.  That actually makes sense to me, but you seem to think there
should be some difference between the two.


How could you read what I wrote as recommending we "dumb things down"?
All I did was observe that swsusp was violating the existing definition
of device state during resume() callbacks, and provide a fix for it.

If you refer to my dislike of special cases ... that's not dumbing down,
that's experience.  You can't evolve complex systems **THAT WORK** only by
adding special cases whenever another inconvenient truth comes up.  At some
point you either succumb to chaos, or replace most of those special cases
with simple rules.


> Rafael raised the issue in another email of code built as modules that is 
> suspended and not resumed or vice versa because in the case of suspend to 
> disk, the module is loaded at boot time but not in the suspended kernel (or 
> v.v).

Which issue made no sense to me when he tried to write it up either.


> It seems to me that the right way to deal with this is to extend the  
> use of __nosave so that information about what was loaded in the boot kernel 
> is available when resuming drivers after the atomic restore.

I seriously doubt you'll be able to get all the hardware designers to
cooperate with you.  Not all the hardware state is directly visible to
software even in normal operation, so there's no way to turn all the
relevant information into driver state to which __nosave (etc) could
be applied ...

- Dave


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Tuesday 25 April 2006 7:41 am, Alan Stern wrote:
> 
> When the USB subsystem resumes a device, it relies on the fact that a
> certain amount of state has been preserved _in the device_.  At a minimum,
> that state includes the device's bus address, which is assigned
> dynamically and obviously is necessary for communicating with the device.  

It also includes other state ... for example the device's driver may
have initialized it in a particular way, and external events may also
have changed things in ways that the kernel cares about.  Not all of
that state can be interrogated by Linux, and drivers are allowed to
rely on that information being unchanged after a true resume().


> This state will be lost whenever the "power session" is interrupted, which
> means that USB devices cannot remain suspended when VBUS power is lost --
> and many systems do not provide VBUS suspend current while in various
> sleep states, although some systems do.

Most systems _do_ provide VBUS power during true suspend states.  But the
swsusp approach precludes VBUS; it's "system off" not "system suspend".


> Regardless, the problem is that the resuming kernel doesn't have any
> terribly good ways of telling whether or not the power session has
> remained intact.

Actually almost all systems do have ways to report that.  The issue in
this case is that swsusp both trashed that report and replaced it with
an incorrect and troublesome report that the sessions DID remain.


> It can test whether the host controller is still in the 
> same state that a suspend would leave it in.  It can test whether the
> device's port is still enabled and whether anything responds at the
> device's bus address.  (We don't currently try to tell whether the
> responder actually _is_ the device we're trying to resume as opposed to
> some other device!)

True.  Sometimes the port is managed with the help of an external
transceive that can monitor things more closely than the controller,
so the "controller" isn't the only relevant device.

 
> The real difficulty comes when the USB drivers are compiled into the boot 
> kernel or loaded by an initrd.  They will take over the host controllers 
> and any devices they can find, destroying the preserved state.  Then when 
> they are told to prepare for the upcoming atomic restore, they will try to 
> freeze or reset (or whatever) the devices.

That's what the patch changed:  to reset, not (wrongly) freeze/suspend.

And note that it also makes the behavior uniform, so that the snapshot
will always see the reset even if that driver had initialized inside
the kernel that restored the snapshot.


> Now the resuming kernel starts up, and it has to guess whether everything
> is still properly suspended with all the necessary state intact.  Suppose
> the boot kernel has destroyed the state and left the devices frozen, as
> happens now (without David's patch).  It will appear to the resuming
> kernel that the devices are still suspended, but unbeknownst to the kernel 
> the state information is gone.  That's why the devices don't work after 
> resuming until the user unplugs and replugs them.

Yes:  swsusp trashed the correct status report.


> The right way to solve this is to make sure that the resuming kernel can
> correctly determine whether the power session (way back from the original
> sw-suspend) is still intact.  It's expected that in many cases it won't
> be, because most systems won't provide suspend current while the machine
> is off.

True for swsusp, but generally not for true system suspend states.


> We have to guarantee that the boot kernel's actions won't end up 
> fooling the resuming kernel into thinking that the power sessions are
> intact when in fact they aren't.  (Furthermore, in an ideal world, we
> would also make sure that the boot kernel won't destroy any power sessions
> that still _are_ intact.  Right now we have no way to do this, because the
> drivers in the boot kernel don't know that it _is_ a boot kernel.)

Again, true system suspend states don't have this issue.  With swsusp
the sessions will always be broken.

 
> Making the boot kernel shut down the host controllers instead of freezing
> or suspending them will indeed destroy all existing power sessions, as
> well as making it clear to the resuming kernel that that are gone.  Under
> certain circumstances, freezing the host controllers will fool the
> resuming kernel.

Right.

 
> Propagating some kind of information from one kernel to the next is the
> solution. 

True system suspend states propagate that information in the hardware,
and I see no reason to define a second mechanism.


> Maybe that's what you meant.  Trying to store that information 
> in the host controller's state is a poor-man's way of doing it, but at the
> moment it's the only way we have.  That's why David doesn't want the state 
> during the atomic reload to be the same as the state during a regular 
> resume.

I'd put it differently.  It's not a poor-man's way at all; passing that
information in hardware is the correct solution.  A regular resume would
keep the information there too (from STR or standby).  We don't want to
have swsusp piling on special cases.

- Dave


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

Hi,

On Tuesday 25 April 2006 18:11, David Brownell wrote:
> On Tuesday 25 April 2006 1:32 am, Rafael J. Wysocki wrote:
> 
> > > just want things quiesced, mainly because we don't want to spin down drives.
> > 
> > That's right.  And kernel_restart_prepare(NULL) will make them spin down?
> > If so, they have to be treated in a special way.
> 
> Seems every time that someone turns up one of the problems with this "freeze"
> thing, the same responses come back:  "we can't fix that because we don't want
> to spin down drives".

I didn't say we couldn't fix that, did I?

> I've begun thinking that calls like pm_should_I_spin_down_drives() would be a
> better structural approach than continually redefining this "freeze" thing so
> it makes less and less sense to all other drivers ... who nonethless need to
> clutter themselves up with a growing list of special cases, to accomodate
> rotating media that may not even exist in the target system.

I think we should do something different to device_power_down(PMSG_FREEZE)
there, but I'm not sure it should be kernel_restart_prepare(NULL).

Actually spinning down disks during resume is a problem for some users (yes,
we've had such bug reports recently), so it's better to avoid this.

> > OTOH I think at least some device driver writers assume that .resume() will
> > always be called after .suspend() which only is true for non-modular drivers
> > (or for modular drivers loaded from an initrd before resume). 
> 
> Say what?  Of _course_ resume() should only be called after suspend().  If
> that's not true in any case, the code wrongly issuing the resume() is buggy.

Well, suppose we have a modular driver that's not loaded before resume.
Then it goes like that (approximately):
(1) We activate swsusp which calls .suspend() for all devices including our
driver (this is a real suspend).
(2) swsusp snapshots the system and creates the image.
(3) swsusp calls .resume() for all devices in order to be able to save the
image (.resume() for our driver is also called which is OK).
(4) swsusp turns off the system.
(5) (some time later) We start a new kernel and tell it to resume.
(6) It activates swsusp which reads the image.
(7) (without your change) swsusp calls .suspend() for all device drivers that
are present at that time, but our driver is not there, so its .suspend()
_won't_ be called.  [Of course with your change .suspend() won't be called
for any driver.]
(8) swsusp restores the image.
(9) swsusp calls .resume() for all devices _including_ our driver, because it
was in memory before suspend.  For our driver this .resume() is not
called after .suspend(), is it?

You're saying that (9) is wrong, so could you please suggest what to do
instead of it?
 
> I don't see where you're collecting that list of special cases.

I have no such list ...

> It doesn't match what I've observed ... and even if it were correct, I'd say
> that's yet another case where special casing reflects bugs.

... and I'm against special casing as far as it's avoidable.

Greetings,
Rafael

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 1694 bytes --]

Hi.

On Wednesday 26 April 2006 04:56, Rafael J. Wysocki wrote:
> You're saying that (9) is wrong, so could you please suggest what to do
> instead of it?

'scuse me for butting in, but here's my suggested modified version:

> Well, suppose we have a modular driver that's not loaded before resume.
> Then it goes like that (approximately):
> (1) We activate swsusp which calls .suspend() for all devices including our
> driver (this is a real suspend).
> (2) swsusp snapshots the system and creates the image.
> (3) swsusp calls .resume() for all devices in order to be able to save the
> image (.resume() for our driver is also called which is OK).
> (4) swsusp turns off the system.
> (5) (some time later) We start a new kernel and tell it to resume.
> (6) It activates swsusp which reads the image.
> (7) (without your change) swsusp calls .suspend() for all device drivers
> that are present at that time, but our driver is not there, so its
> .suspend() _won't_ be called.  [Of course with your change .suspend() won't
> be called for any driver.]

We also make a list that is safely available after the atomic restore of 
drivers that have had .suspend methods called.

> (8) swsusp restores the image.

(9) suspend to disk calls .resume() for each device that is found in the list 
generated above, thereby excluding our problem driver. Drivers which are 
present but not in the list above (such as the problem driver) have a 
different routine called, perhaps the normal initialisation one?

Regards,

Nigel
-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4084 bytes --]

On Tue, 25 Apr 2006, David Brownell wrote:

> On Tuesday 25 April 2006 7:41 am, Alan Stern wrote:
> > 
> > When the USB subsystem resumes a device, it relies on the fact that a
> > certain amount of state has been preserved _in the device_.  At a minimum,
> > that state includes the device's bus address, which is assigned
> > dynamically and obviously is necessary for communicating with the device.  
> 
> It also includes other state

Maybe.  If the device has been configured and a driver loaded.

> ... for example the device's driver may
> have initialized it in a particular way, and external events may also
> have changed things in ways that the kernel cares about.  Not all of
> that state can be interrogated by Linux, and drivers are allowed to
> rely on that information being unchanged after a true resume().

Yes, certainly.

> > This state will be lost whenever the "power session" is interrupted, which
> > means that USB devices cannot remain suspended when VBUS power is lost --
> > and many systems do not provide VBUS suspend current while in various
> > sleep states, although some systems do.
> 
> Most systems _do_ provide VBUS power during true suspend states.  But the
> swsusp approach precludes VBUS;

I don't know how you justify that "Most".  At the least it would depend on
the particular state.  For instance, I've got a PCI USB controller card
(VIA) on which the UHCI controllers support only D0 and D3cold.  And
(although I'm not certain of this) I may have seen a report from someone
whose system _does_ retain power during swsusp.  IIRC it was a powerbook
of some sort.

> it's "system off" not "system suspend".

Well, there's "off" and there's "off".  You're the one who's been 
complaining about generic terms like "on" and "off" being too imprecise.

> > Regardless, the problem is that the resuming kernel doesn't have any
> > terribly good ways of telling whether or not the power session has
> > remained intact.
> 
> Actually almost all systems do have ways to report that.

Again, I'm not so sure about that "almost all".  There have been plenty of 
reports of controller settings getting messed up by BIOS firmware during a 
suspend/resume cycle.  Basically we have to assume that if the controller 
isn't exactly the way we left it then the whole thing needs to be reset.  
There should be a better approach...

> > The right way to solve this is to make sure that the resuming kernel can
> > correctly determine whether the power session (way back from the original
> > sw-suspend) is still intact.  It's expected that in many cases it won't
> > be, because most systems won't provide suspend current while the machine
> > is off.
> 
> True for swsusp, but generally not for true system suspend states.

As mentioned above, there's lots of variability.

> Again, true system suspend states don't have this issue.  With swsusp
> the sessions will always be broken.

If that is correct, then your patch is the right thing to do.  If it's not
correct, your patch still isn't wrong -- it's just not quite optimal.

> > Maybe that's what you meant.  Trying to store that information 
> > in the host controller's state is a poor-man's way of doing it, but at the
> > moment it's the only way we have.  That's why David doesn't want the state 
> > during the atomic reload to be the same as the state during a regular 
> > resume.
> 
> I'd put it differently.  It's not a poor-man's way at all; passing that
> information in hardware is the correct solution.  A regular resume would
> keep the information there too (from STR or standby).  We don't want to
> have swsusp piling on special cases.

Okay, I take it back.  Yes, putting the hardware into the appropriate 
state is the right thing to do.

On the other hand, it might not be so easy to know what that state is.  
Perhaps the decision should be left up to the device driver.  If drivers
were told the difference between "FREEZE to prepare for creating a memory
snapshot" and "FREEZE to prepare for restoring a memory image" then they 
could do the right thing always.

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

Hi,

On Tuesday 25 April 2006 22:28, Nigel Cunningham wrote:
> On Wednesday 26 April 2006 04:56, Rafael J. Wysocki wrote:
> > You're saying that (9) is wrong, so could you please suggest what to do
> > instead of it?
> 
> 'scuse me for butting in, but here's my suggested modified version:
> 
> > Well, suppose we have a modular driver that's not loaded before resume.
> > Then it goes like that (approximately):
> > (1) We activate swsusp which calls .suspend() for all devices including our
> > driver (this is a real suspend).
> > (2) swsusp snapshots the system and creates the image.
> > (3) swsusp calls .resume() for all devices in order to be able to save the
> > image (.resume() for our driver is also called which is OK).
> > (4) swsusp turns off the system.
> > (5) (some time later) We start a new kernel and tell it to resume.
> > (6) It activates swsusp which reads the image.
> > (7) (without your change) swsusp calls .suspend() for all device drivers
> > that are present at that time, but our driver is not there, so its
> > .suspend() _won't_ be called.  [Of course with your change .suspend() won't
> > be called for any driver.]
> 
> We also make a list that is safely available after the atomic restore of 
> drivers that have had .suspend methods called.

Do you mean we place the list in a __nosave area?

Rafael


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

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

Hi.

On Wednesday 26 April 2006 06:53, Rafael J. Wysocki wrote:
> Hi,
>
> On Tuesday 25 April 2006 22:28, Nigel Cunningham wrote:
> > On Wednesday 26 April 2006 04:56, Rafael J. Wysocki wrote:
> > > You're saying that (9) is wrong, so could you please suggest what to do
> > > instead of it?
> >
> > 'scuse me for butting in, but here's my suggested modified version:
> > > Well, suppose we have a modular driver that's not loaded before resume.
> > > Then it goes like that (approximately):
> > > (1) We activate swsusp which calls .suspend() for all devices including
> > > our driver (this is a real suspend).
> > > (2) swsusp snapshots the system and creates the image.
> > > (3) swsusp calls .resume() for all devices in order to be able to save
> > > the image (.resume() for our driver is also called which is OK).
> > > (4) swsusp turns off the system.
> > > (5) (some time later) We start a new kernel and tell it to resume.
> > > (6) It activates swsusp which reads the image.
> > > (7) (without your change) swsusp calls .suspend() for all device
> > > drivers that are present at that time, but our driver is not there, so
> > > its .suspend() _won't_ be called.  [Of course with your change
> > > .suspend() won't be called for any driver.]
> >
> > We also make a list that is safely available after the atomic restore of
> > drivers that have had .suspend methods called.
>
> Do you mean we place the list in a __nosave area?

Well, I wasn't wanting to get bogged down in the details yet, but let's see 
what we can come up with. How about this:

At the point where we do the drivers resume at resume time, we're still 
atomic, right? Since that's the case, we can just use get_safe_page() prior 
to the restore to store the list and a __nosave pointer to retain the 
location across the atomic restore. If we are concerned about resuming 
drivers allocating memory and overwriting our list (and I think that's a 
valid concern), space could be allocated and the list copied between doing 
the atomic restore and the device pwoerup/resume. Another approach might be 
to allocate space for the list while suspending (prior to the atomic copy), 
but that could cause problems since you don't know how many devices will be 
in the list ahead of time. Are there other possibilities I haven't thought 
of?

Regards,

Nigel

-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

On Tuesday 25 April 2006 11:56 am, Rafael J. Wysocki wrote:
> 
> > I've begun thinking that calls like pm_should_I_spin_down_drives() would be a
> > better structural approach than continually redefining this "freeze" thing so
> > it makes less and less sense to all other drivers ... who nonethless need to
> > clutter themselves up with a growing list of special cases, to accomodate
> > rotating media that may not even exist in the target system.
> 
> I think we should do something different to device_power_down(PMSG_FREEZE)
> there, but I'm not sure it should be kernel_restart_prepare(NULL).
> 
> Actually spinning down disks during resume is a problem for some users (yes,
> we've had such bug reports recently), so it's better to avoid this.

Well, if we had a pm_should_I_spin_down_drives() it would make sense to me
that it return FALSE during kernel_restart_prepare() too ... surely kexec
users have the same issues!


If you currently have users who object to spindown-during-resume, then it'd
seem that my patch couldn't change anything except maybe details.  And that
switching over to a call like pm_should_I_spin_down_drives() should fix it all.


> > > OTOH I think at least some device driver writers assume that .resume() will
> > > always be called after .suspend() which only is true for non-modular drivers
> > > (or for modular drivers loaded from an initrd before resume). 
> > 
> > Say what?  Of _course_ resume() should only be called after suspend().  If
> > that's not true in any case, the code wrongly issuing the resume() is buggy.
> 
> Well, suppose we have a modular driver that's not loaded before resume.

That's not the problem case though; it works correctly, since the device
hardware is already being left in an appropriate (RESET) state.


> Then it goes like that (approximately):
> (1) We activate swsusp which calls .suspend() for all devices including our
> driver (this is a real suspend).
> (2) swsusp snapshots the system and creates the image.
> (3) swsusp calls .resume() for all devices in order to be able to save the
> image (.resume() for our driver is also called which is OK).
> (4) swsusp turns off the system.
> (5) (some time later) We start a new kernel and tell it to resume.
> (6) It activates swsusp which reads the image.

And assuming this is an x86 PC, at this point every device is in one of three states:

  - initialized by BIOS.  This is a particular PITA for USB, but one that's
    handled OK (mostly) except when BIOS bugs kick in.  There's some nasty
    code that kicks in along with PCI quirk handling, which ensures that by
    the time Linux-USB  driver could see this state (or the input subsystem
    needs to care about it), the state has morphed to reset.  Video cards
    have funky issues here too.

  - (powerup) reset.  This is the ideal state, in terms of "truth" to convey
    to the image we're about to restore ... no ambiguity, every driver will
    need to re-init.  As if there were (thank you!!) no BIOS.

  - initialized by Linux ... which leads to the case my patch addresses.

Those first two states are legit for any resume() call, and they apply in
your scenario restriction.

The third state is the problem scenario, kicking in when the driver was
statically linked (or modprobed from initramfs, etc), but not during
your scenario.


> (7) (without your change) swsusp calls .suspend() for all device drivers that
> are present at that time,

... the current troublesome consequence of that third state ...

> but our driver is not there, so its .suspend() 
> _won't_ be called.  [Of course with your change .suspend() won't be called
> for any driver.]

Right:  the first two "safe" cases kick in.  This is the partial workaround I
had identified:  dodging the code paths for that third state, where suspend()
is being used to put the hardware into a broken suspend state.


Note that with that third state, there are actually two suspend() calls, but
only one resume() call.  (Suspend before snapshot, suspend before resume
snapshot, resume after activating snapshot.)  Such an extra suspend() call is
a small hint that something's odd, and maybe wrong.


> (8) swsusp restores the image.
> (9) swsusp calls .resume() for all devices _including_ our driver, because it
> was in memory before suspend.  For our driver this .resume() is not
> called after .suspend(), is it?

The suspend() was called from the kernel being resumed ... and the device hardware
is in one of the states (reset) that it's allowed to be in when calling its
matching resume().  No problem there.


> You're saying that (9) is wrong, so could you please suggest what to do
> instead of it?

The case in which (9) is wrong is the case you excluded:  where the pre-resume
kernel loaded the driver and used the third state listed above, and then trashed
the correct device hardware state (reset) and replaced it with a suspend state.

It may help to think of two distinct types of device hardware suspend states
(only the first is real, the second is just a software bug):

 - Correct, with internal state corresponding to what the driver suspend() did;
   what a normal hardware suspend/resume cycle (not powercycle!) could do.

 - Broken, with any other internal state (except reset).  This is what swsusp
   currently forces, by adding **AND HIDING** a reset and reinit cycle, because
   of the extra suspend() call in (7).

My patch/suggestion just ensures that instead of that broken state, reset is used.
in all cases ... not just the "driver not initialized before snapshot resume" case.

- Dave


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

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

(aha, I see the mail now)

> > > I've begun thinking that calls like pm_should_I_spin_down_drives() would be a
> > > better structural approach than continually redefining this "freeze" thing so
> > > it makes less and less sense to all other drivers ... who nonethless need to
> > > clutter themselves up with a growing list of special cases, to accomodate
> > > rotating media that may not even exist in the target system.
> > 
> > I think we should do something different to device_power_down(PMSG_FREEZE)
> > there, but I'm not sure it should be kernel_restart_prepare(NULL).
> > 
> > Actually spinning down disks during resume is a problem for some users (yes,
> > we've had such bug reports recently), so it's better to avoid this.
> 
> Well, if we had a pm_should_I_spin_down_drives() it would make sense to me
> that it return FALSE during kernel_restart_prepare() too ... surely kexec
> users have the same issues!

pm_should_I_spin_down_drives() is incredibly ugly hack, sorry.

> > Then it goes like that (approximately):
> > (1) We activate swsusp which calls .suspend() for all devices including our
> > driver (this is a real suspend).
> > (2) swsusp snapshots the system and creates the image.
> > (3) swsusp calls .resume() for all devices in order to be able to save the
> > image (.resume() for our driver is also called which is OK).
> > (4) swsusp turns off the system.
> > (5) (some time later) We start a new kernel and tell it to resume.
> > (6) It activates swsusp which reads the image.
> 
> And assuming this is an x86 PC, at this point every device is in one of three states:
> 
>   - initialized by BIOS.  This is a particular PITA for USB, but one that's
>     handled OK (mostly) except when BIOS bugs kick in.  There's some nasty
>     code that kicks in along with PCI quirk handling, which ensures that by
>     the time Linux-USB  driver could see this state (or the input subsystem
>     needs to care about it), the state has morphed to reset.  Video cards
>     have funky issues here too.
> 
>   - (powerup) reset.  This is the ideal state, in terms of "truth" to convey
>     to the image we're about to restore ... no ambiguity, every driver will
>     need to re-init.  As if there were (thank you!!) no BIOS.
> 
>   - initialized by Linux ... which leads to the case my patch addresses.
> 
> Those first two states are legit for any resume() call, and they apply in
> your scenario restriction.
> 
> The third state is the problem scenario, kicking in when the driver was
> statically linked (or modprobed from initramfs, etc), but not during
> your scenario.

Driver should be able to resume() from state #3. If driver is broken
and can't do that, modify suspend() call to power down hardware.

> My patch/suggestion just ensures that instead of that broken state, reset is used.
> in all cases ... not just the "driver not initialized before snapshot resume" case.

I guess I partially see what you are saying. Behaviour in
driver-modular and driver-non-modular is indeed different and that is
not nice.

But it would be nice to have less intrusive solution than call reboot
notifiers...
								Pavel
-- 
Thanks for all the (sleeping) penguins.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

On Tuesday 25 April 2006 23:04, David Brownell wrote:
> On Tuesday 25 April 2006 11:56 am, Rafael J. Wysocki wrote:
> > 
> > > I've begun thinking that calls like pm_should_I_spin_down_drives() would be a
> > > better structural approach than continually redefining this "freeze" thing so
> > > it makes less and less sense to all other drivers ... who nonethless need to
> > > clutter themselves up with a growing list of special cases, to accomodate
> > > rotating media that may not even exist in the target system.
> > 
> > I think we should do something different to device_power_down(PMSG_FREEZE)
> > there, but I'm not sure it should be kernel_restart_prepare(NULL).
> > 
> > Actually spinning down disks during resume is a problem for some users (yes,
> > we've had such bug reports recently), so it's better to avoid this.
> 
> Well, if we had a pm_should_I_spin_down_drives() it would make sense to me
> that it return FALSE during kernel_restart_prepare() too ... surely kexec
> users have the same issues!
> 
> If you currently have users who object to spindown-during-resume, then it'd
> seem that my patch couldn't change anything except maybe details.

Fortunately this particular problem has been fixed in the driver. ;-)

> And that switching over to a call like pm_should_I_spin_down_drives() should
> fix it all.

Agreed, but I have to learn quite a bit to implement such a thing.

> > > > OTOH I think at least some device driver writers assume that .resume() will
> > > > always be called after .suspend() which only is true for non-modular drivers
> > > > (or for modular drivers loaded from an initrd before resume). 
> > > 
> > > Say what?  Of _course_ resume() should only be called after suspend().  If
> > > that's not true in any case, the code wrongly issuing the resume() is buggy.
> > 
> > Well, suppose we have a modular driver that's not loaded before resume.
> 
> That's not the problem case though; it works correctly, since the device
> hardware is already being left in an appropriate (RESET) state.
> 
> 
> > Then it goes like that (approximately):
> > (1) We activate swsusp which calls .suspend() for all devices including our
> > driver (this is a real suspend).
> > (2) swsusp snapshots the system and creates the image.
> > (3) swsusp calls .resume() for all devices in order to be able to save the
> > image (.resume() for our driver is also called which is OK).
> > (4) swsusp turns off the system.
> > (5) (some time later) We start a new kernel and tell it to resume.
> > (6) It activates swsusp which reads the image.
> 
> And assuming this is an x86 PC, at this point every device is in one of three states:
> 
>   - initialized by BIOS.  This is a particular PITA for USB, but one that's
>     handled OK (mostly) except when BIOS bugs kick in.  There's some nasty
>     code that kicks in along with PCI quirk handling, which ensures that by
>     the time Linux-USB  driver could see this state (or the input subsystem
>     needs to care about it), the state has morphed to reset.  Video cards
>     have funky issues here too.
> 
>   - (powerup) reset.  This is the ideal state, in terms of "truth" to convey
>     to the image we're about to restore ... no ambiguity, every driver will
>     need to re-init.  As if there were (thank you!!) no BIOS.
> 
>   - initialized by Linux ... which leads to the case my patch addresses.
> 
> Those first two states are legit for any resume() call, and they apply in
> your scenario restriction.

IIRC, there were some ALSA problems with .resume() called on a reseted
device, but they seem to be fixed now.
 
> The third state is the problem scenario, kicking in when the driver was
> statically linked (or modprobed from initramfs, etc), but not during
> your scenario.

The problem, as I see it, is that too many devices may be initialized at the
kernel startup.  I think we _can_ reset some of them before the image
is restored, but at least some of them need to be treated more carefully.

> > (7) (without your change) swsusp calls .suspend() for all device drivers that
> > are present at that time,
> 
> ... the current troublesome consequence of that third state ...
> 
> > but our driver is not there, so its .suspend() 
> > _won't_ be called.  [Of course with your change .suspend() won't be called
> > for any driver.]
> 
> Right:  the first two "safe" cases kick in.  This is the partial workaround I
> had identified:  dodging the code paths for that third state, where suspend()
> is being used to put the hardware into a broken suspend state.

So perhaps we should just make them enter a state that's not broken?
That may be reset for some devices (eg. USB) and something else for some
others (eg. storage).

> Note that with that third state, there are actually two suspend() calls, but
> only one resume() call.  (Suspend before snapshot, suspend before resume
> snapshot, resume after activating snapshot.)  Such an extra suspend() call is
> a small hint that something's odd, and maybe wrong.

Agreed.

> > (8) swsusp restores the image.
> > (9) swsusp calls .resume() for all devices _including_ our driver, because it
> > was in memory before suspend.  For our driver this .resume() is not
> > called after .suspend(), is it?
> 
> The suspend() was called from the kernel being resumed ... and the device hardware
> is in one of the states (reset) that it's allowed to be in when calling its
> matching resume().  No problem there.
> 
> 
> > You're saying that (9) is wrong, so could you please suggest what to do
> > instead of it?
> 
> The case in which (9) is wrong is the case you excluded:  where the pre-resume
> kernel loaded the driver and used the third state listed above, and then trashed
> the correct device hardware state (reset) and replaced it with a suspend state.
> 
> It may help to think of two distinct types of device hardware suspend states
> (only the first is real, the second is just a software bug):
> 
>  - Correct, with internal state corresponding to what the driver suspend() did;
>    what a normal hardware suspend/resume cycle (not powercycle!) could do.
> 
>  - Broken, with any other internal state (except reset).  This is what swsusp
>    currently forces, by adding **AND HIDING** a reset and reinit cycle, because
>    of the extra suspend() call in (7).

Still there are drivers that have no problems with it, so why we should we
forcibly reset their devices?

> My patch/suggestion just ensures that instead of that broken state, reset is used.
> in all cases ... not just the "driver not initialized before snapshot resume" case.

As I said before I generally agree with this except I think some more fine
grained approach is needed in this case.

Basically it seems we need something like a .prepare_for_resume() routine,
that will be called before restoring the swsusp's image, apart from .suspend()
and .resume() for each driver.  Your patch just assumes that
.prepare_for_resume() should be "reset" for all devices.

Greetings,
Rafael

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

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

Hi,

On Tuesday 25 April 2006 23:03, Nigel Cunningham wrote:
> On Wednesday 26 April 2006 06:53, Rafael J. Wysocki wrote:
> > On Tuesday 25 April 2006 22:28, Nigel Cunningham wrote:
> > > On Wednesday 26 April 2006 04:56, Rafael J. Wysocki wrote:
> > > > You're saying that (9) is wrong, so could you please suggest what to do
> > > > instead of it?
> > >
> > > 'scuse me for butting in, but here's my suggested modified version:
> > > > Well, suppose we have a modular driver that's not loaded before resume.
> > > > Then it goes like that (approximately):
> > > > (1) We activate swsusp which calls .suspend() for all devices including
> > > > our driver (this is a real suspend).
> > > > (2) swsusp snapshots the system and creates the image.
> > > > (3) swsusp calls .resume() for all devices in order to be able to save
> > > > the image (.resume() for our driver is also called which is OK).
> > > > (4) swsusp turns off the system.
> > > > (5) (some time later) We start a new kernel and tell it to resume.
> > > > (6) It activates swsusp which reads the image.
> > > > (7) (without your change) swsusp calls .suspend() for all device
> > > > drivers that are present at that time, but our driver is not there, so
> > > > its .suspend() _won't_ be called.  [Of course with your change
> > > > .suspend() won't be called for any driver.]
> > >
> > > We also make a list that is safely available after the atomic restore of
> > > drivers that have had .suspend methods called.
> >
> > Do you mean we place the list in a __nosave area?
> 
> Well, I wasn't wanting to get bogged down in the details yet, but let's see 
> what we can come up with. How about this:
> 
> At the point where we do the drivers resume at resume time, we're still 
> atomic, right? Since that's the case, we can just use get_safe_page() prior 
> to the restore to store the list and a __nosave pointer to retain the 
> location across the atomic restore. If we are concerned about resuming 
> drivers allocating memory and overwriting our list (and I think that's a 
> valid concern), space could be allocated and the list copied between doing 
> the atomic restore and the device pwoerup/resume.

This one seems to look better.

Still the problem is we need to know what to do with the devices that have had
.suspend() routines called.  Some of them would have to be reset, some of
them might be left in the current state, and for some of them we'd have to
do something special.  Frankly, only the driver writer will know what's
appropriate.

Greetings,
Rafael

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 3388 bytes --]

Hi.

On Wednesday 26 April 2006 08:06, Rafael J. Wysocki wrote:
> On Tuesday 25 April 2006 23:03, Nigel Cunningham wrote:
> > On Wednesday 26 April 2006 06:53, Rafael J. Wysocki wrote:
> > > On Tuesday 25 April 2006 22:28, Nigel Cunningham wrote:
> > > > On Wednesday 26 April 2006 04:56, Rafael J. Wysocki wrote:
> > > > > You're saying that (9) is wrong, so could you please suggest what
> > > > > to do instead of it?
> > > >
> > > > 'scuse me for butting in, but here's my suggested modified version:
> > > > > Well, suppose we have a modular driver that's not loaded before
> > > > > resume. Then it goes like that (approximately):
> > > > > (1) We activate swsusp which calls .suspend() for all devices
> > > > > including our driver (this is a real suspend).
> > > > > (2) swsusp snapshots the system and creates the image.
> > > > > (3) swsusp calls .resume() for all devices in order to be able to
> > > > > save the image (.resume() for our driver is also called which is
> > > > > OK). (4) swsusp turns off the system.
> > > > > (5) (some time later) We start a new kernel and tell it to resume.
> > > > > (6) It activates swsusp which reads the image.
> > > > > (7) (without your change) swsusp calls .suspend() for all device
> > > > > drivers that are present at that time, but our driver is not there,
> > > > > so its .suspend() _won't_ be called.  [Of course with your change
> > > > > .suspend() won't be called for any driver.]
> > > >
> > > > We also make a list that is safely available after the atomic restore
> > > > of drivers that have had .suspend methods called.
> > >
> > > Do you mean we place the list in a __nosave area?
> >
> > Well, I wasn't wanting to get bogged down in the details yet, but let's
> > see what we can come up with. How about this:
> >
> > At the point where we do the drivers resume at resume time, we're still
> > atomic, right? Since that's the case, we can just use get_safe_page()
> > prior to the restore to store the list and a __nosave pointer to retain
> > the location across the atomic restore. If we are concerned about
> > resuming drivers allocating memory and overwriting our list (and I think
> > that's a valid concern), space could be allocated and the list copied
> > between doing the atomic restore and the device pwoerup/resume.
>
> This one seems to look better.
>
> Still the problem is we need to know what to do with the devices that have
> had .suspend() routines called.  Some of them would have to be reset, some
> of them might be left in the current state, and for some of them we'd have
> to do something special.  Frankly, only the driver writer will know what's
> appropriate.

I saw the 2 suspends, 1 resume comment in another part of the thread, but 
don't believe a driver would be able to detect that 2 suspends and 1 resume 
call had been made - at least not without some non volatile ram. This is the 
problem I'm mainly seeking to address. I agree that the driver is the only 
thing that will know what to do with the information. That presents another 
issue - we would then have to pass on to the drivers the information - I 
guess by modified a field in their struct device?

Regards,

Nigel
-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

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

Hi,

On Wednesday 26 April 2006 00:18, Nigel Cunningham wrote:
> On Wednesday 26 April 2006 08:06, Rafael J. Wysocki wrote:
> > On Tuesday 25 April 2006 23:03, Nigel Cunningham wrote:
> > > On Wednesday 26 April 2006 06:53, Rafael J. Wysocki wrote:
> > > > On Tuesday 25 April 2006 22:28, Nigel Cunningham wrote:
> > > > > On Wednesday 26 April 2006 04:56, Rafael J. Wysocki wrote:
> > > > > > You're saying that (9) is wrong, so could you please suggest what
> > > > > > to do instead of it?
> > > > >
> > > > > 'scuse me for butting in, but here's my suggested modified version:
> > > > > > Well, suppose we have a modular driver that's not loaded before
> > > > > > resume. Then it goes like that (approximately):
> > > > > > (1) We activate swsusp which calls .suspend() for all devices
> > > > > > including our driver (this is a real suspend).
> > > > > > (2) swsusp snapshots the system and creates the image.
> > > > > > (3) swsusp calls .resume() for all devices in order to be able to
> > > > > > save the image (.resume() for our driver is also called which is
> > > > > > OK). (4) swsusp turns off the system.
> > > > > > (5) (some time later) We start a new kernel and tell it to resume.
> > > > > > (6) It activates swsusp which reads the image.
> > > > > > (7) (without your change) swsusp calls .suspend() for all device
> > > > > > drivers that are present at that time, but our driver is not there,
> > > > > > so its .suspend() _won't_ be called.  [Of course with your change
> > > > > > .suspend() won't be called for any driver.]
> > > > >
> > > > > We also make a list that is safely available after the atomic restore
> > > > > of drivers that have had .suspend methods called.
> > > >
> > > > Do you mean we place the list in a __nosave area?
> > >
> > > Well, I wasn't wanting to get bogged down in the details yet, but let's
> > > see what we can come up with. How about this:
> > >
> > > At the point where we do the drivers resume at resume time, we're still
> > > atomic, right? Since that's the case, we can just use get_safe_page()
> > > prior to the restore to store the list and a __nosave pointer to retain
> > > the location across the atomic restore. If we are concerned about
> > > resuming drivers allocating memory and overwriting our list (and I think
> > > that's a valid concern), space could be allocated and the list copied
> > > between doing the atomic restore and the device pwoerup/resume.
> >
> > This one seems to look better.
> >
> > Still the problem is we need to know what to do with the devices that have
> > had .suspend() routines called.  Some of them would have to be reset, some
> > of them might be left in the current state, and for some of them we'd have
> > to do something special.  Frankly, only the driver writer will know what's
> > appropriate.
> 
> I saw the 2 suspends, 1 resume comment in another part of the thread, but 
> don't believe a driver would be able to detect that 2 suspends and 1 resume 
> call had been made - at least not without some non volatile ram.

In fact we know when the second .suspend() is going to be called from the
driver, so we can tell it to take care, for example by passing a special
value to .suspend().  If that value is passed to .suspend(), the driver should
prepare the device for .resume() being called after the snapshot image is
restored (eg. reset it).

Greetings,
Rafael

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

On Tuesday 25 April 2006 2:55 pm, Rafael J. Wysocki wrote:
> On Tuesday 25 April 2006 23:04, David Brownell wrote:
>  
> > The third state is the problem scenario, kicking in when the driver was
> > statically linked (or modprobed from initramfs, etc), but not during
> > your scenario.
> 
> The problem, as I see it, is that too many devices may be initialized at the
> kernel startup.

That's a fair observation.  In the same way, swsusp resumes too many devices
before writing the snapshot.  In both cases, it only needs the resume partition,
not every random bit of hardware in the system.

But it's not the root cause of the problem either.  The same problem appears if
the device holding the resume partition gets forced into this "broken suspend"
state.


> I think we _can_ reset some of them before the image 
> is restored, but at least some of them need to be treated more carefully.

Maybe; but remember they _were_ just reset ... and that if a driver doesn't
know how to re-init a device that it has just reset, then it has other problems!
It's a common practice to reset on entrance to probe() ... if that works, it's
"just" a simple matter of code cleanup/reorg to let the init code be used if
resume() detects the device hardware has been reset.

For example, rmmod/modprobe cycles wouldn't behave sanely ... and it'd likely
break with kexec().  If they "need" careful treatment, that'd seem to me most
like a driver bug ...

Now, if you have specific examples of things that shouldn't be reset, that
could be interesting.  I don't seem to have tripped over any, and from first
principles it's clear to me there _shouldn't_ be such drivers (modulo bugs).

If you're going to argue that there are enough buggy drivers around that
we can't rely on them to behave correctly here ... well, we don't have an
example of even one such driver, much less a flood of them.  And the thing
to do with bugs is fix them, not coddle them.  ;)


> > Right:  the first two "safe" cases kick in.  This is the partial workaround I
> > had identified:  dodging the code paths for that third state, where suspend()
> > is being used to put the hardware into a broken suspend state.
> 
> So perhaps we should just make them enter a state that's not broken?
> That may be reset for some devices (eg. USB) and something else for some
> others (eg. storage).

Sure, but I'm not sure what "something else" would ever be.  Examples?
Not of cases where something else _could_ be done, but where it _must_ be.


> > It may help to think of two distinct types of device hardware suspend states
> > (only the first is real, the second is just a software bug):
> > 
> >  - Correct, with internal state corresponding to what the driver suspend() did;
> >    what a normal hardware suspend/resume cycle (not powercycle!) could do.
> > 
> >  - Broken, with any other internal state (except reset).  This is what swsusp
> >    currently forces, by adding **AND HIDING** a reset and reinit cycle, because
> >    of the extra suspend() call in (7).
> 
> Still there are drivers that have no problems with it, so why we should we
> forcibly reset their devices?

Can any driver can really justify not handling the case where resume() sees
hardware that's been reset?  That is, _exactly_ as would be the case if its
driver were only loaded as a module after the kernel booted?  I think not...


> > My patch/suggestion just ensures that instead of that broken state, reset is used.
> > in all cases ... not just the "driver not initialized before snapshot resume" case.
> 
> As I said before I generally agree with this except I think some more fine
> grained approach is needed in this case.

You've not convinced me of that; might be right, but nothing proves it to me.


> Basically it seems we need something like a .prepare_for_resume() routine,
> that will be called before restoring the swsusp's image, apart from .suspend()
> and .resume() for each driver.  Your patch just assumes that
> .prepare_for_resume() should be "reset" for all devices.

I want to avoid piling on more special cases for swsusp, and that would appear
to be the only potential user for a prepare_for_resume().

The patch assumes that kexec() and the swsusp snapshot resume should be subject
to the same basic constraints.  While that might be a bit aggressive, it sure
seems like it's the right approach to take:  not adding special cases just for
swsusp, and making sure two very similar problems don't needlessly diverge
their solutions.  Plus, it's a mechanism that's been in place for some time now,
known to work.

- Dave


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Tuesday 25 April 2006 2:41 pm, Pavel Machek wrote:

> > Well, if we had a pm_should_I_spin_down_drives() it would make sense to me
> > that it return FALSE during kernel_restart_prepare() too ... surely kexec
> > users have the same issues!
> 
> pm_should_I_spin_down_drives() is incredibly ugly hack, sorry.

The name was chosen to be ugly.  I'd expect someone to make it prettier;
maybe draw ponies around the name or somesuch.  ;)


However, by the way your pm_message_t changes removed all information about
system states, you've forced that family of approach in every driver that needs
to know about such states in order to behave correctly.  For example, "don't
turn off clock X in PM_STANDBY" ... since no driver has a standard way to know
about PM_STANDBY, it needs some function accessing state set up by pm_ops.prepare
or else it's not going to be able to behave right.



> > The third state is the problem scenario, kicking in when the driver was
> > statically linked (or modprobed from initramfs, etc), but not during
> > your scenario.
> 
> Driver should be able to resume() from state #3. If driver is broken
> and can't do that, modify suspend() call to power down hardware.

You conveniently edited out the elaboration there.  State #3 is no problem
by itself ... it's later states, only accessible through door #3.

The problem comes from then calling suspend() -- as you advise! -- to force
broken/invalid/bogus suspend state into the hardware.  It's NOT a driver
issue ... it's the fact that calling suspend() there will trash hardware
state (needlessly).  Drivers should not need to cope with such trashing.


> > My patch/suggestion just ensures that instead of that broken state, reset is used.
> > in all cases ... not just the "driver not initialized before snapshot resume" case.
> 
> I guess I partially see what you are saying. Behaviour in
> driver-modular and driver-non-modular is indeed different and that is
> not nice.

That's part of it; the other part is that swsusp should never force the
hardware to reflect an invalid state (the way it does without my patch).

Different behaviours wouldn't be much trouble if they were both correct.


> But it would be nice to have less intrusive solution than call reboot
> notifiers...

Actually, we'd need the notifiers to be called as much as the driver.shutdown()
methods ... they are both used to reset hardware.


You still haven't come up with a response to my point that starting up a
new kernel via swsusp snapshot resume is a system restart in exactly the
same way as it is for kexec() ... at this point, there is no better fix
for the problem than my original patch, AND you haven't actually identified
a single real problem with it.

- Dave



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

On Tuesday 25 April 2006 3:18 pm, Nigel Cunningham wrote:

> I saw the 2 suspends, 1 resume comment in another part of the thread, but 
> don't believe a driver would be able to detect that 2 suspends and 1 resume 
> call had been made - at least not without some non volatile ram.

The extra suspend is just IMO a symptom of sloppiness, like a "here may be bugs"
sign.  Not necessarily an issue in its own right.

In fact if you count carefully, it's three suspends and one resume:  one
suspend before calling swsusp_resume, one inside swsusp_resume -- replaced
by my patch -- and a correctly matched pair in the kernel being resumed.

Raising two points:  (1) for those who think suspend is right, you can
think of it as replacing one of the extra ones with kernel_restart_prepare();
and (2) not very much code interacts with that restart prep, that's a
sane audit problem.


Also, about non-volatile RAM.  Not necessary ... the device hardware holds all
the relevant state.  The problem is that swsusp is now trashing it with those
suspend calls before resuming the real kernel.  On the plus side, we already
have code being used to resolve the identical issue for kexec(), and all my
patch does is to use it.

- Dave



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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


> > > This state will be lost whenever the "power session" is interrupted, which
> > > means that USB devices cannot remain suspended when VBUS power is lost --
> > > and many systems do not provide VBUS suspend current while in various
> > > sleep states, although some systems do.
> > 
> > Most systems _do_ provide VBUS power during true suspend states.  But the
> > swsusp approach precludes VBUS;
> 
> I don't know how you justify that "Most".  

Most PC motherboards -- more than half for sure, every one I've seen or
heard of -- maintain it.  I've seen one ultralight laptop that powers
off VBUS during suspend-to-RAM, to save battery power.  Ditto for every
embedded board I've worked with; a few have a GPIO that can be used to
switch off the VBUS supply.


> At the least it would depend on 
> the particular state.  For instance, I've got a PCI USB controller card
> (VIA) on which the UHCI controllers support only D0 and D3cold.  And
> (although I'm not certain of this) I may have seen a report from someone
> whose system _does_ retain power during swsusp.  IIRC it was a powerbook
> of some sort.

I know that Macintoshes are good about maintaining power during STR, and
suspect that some systems running Apple's OS will do so during "hibernate";
they have a more complete/powerful power management framework than Linux.

But last time the topic came up, I recall Pavel saying he'd never seen or
heard of hardware that did things like that with swsusp, even when using
"platform" (or "firmware") suspend mode.


> > it's "system off" not "system suspend".
> 
> Well, there's "off" and there's "off".  You're the one who's been 
> complaining about generic terms like "on" and "off" being too imprecise.

In that context I meant real power off.  Sorry, didn't mean to confuse.

 
> > > Regardless, the problem is that the resuming kernel doesn't have any
> > > terribly good ways of telling whether or not the power session has
> > > remained intact.
> > 
> > Actually almost all systems do have ways to report that.
> 
> Again, I'm not so sure about that "almost all".  There have been plenty of 
> reports of controller settings getting messed up by BIOS firmware during a 
> suspend/resume cycle. 

Well, EHCI certainly has such ways, and for the past few years it's been
hard to find motherboards that don't include that.  OHCI also has ways,
which counts for all non-Intel/VIA motherboard.   I think you've explained
that UHCI still has some issues there ... although in that case, ISTR you
can at least detect that the controller was reset during suspend, and thus
deduce that the poser session was broken.


> Basically we have to assume that if the controller  
> isn't exactly the way we left it then the whole thing needs to be reset.  
> There should be a better approach...

Because for example that approach wouldn't detect the scenario that
led to my patch.  :)


> > Again, true system suspend states don't have this issue.  With swsusp
> > the sessions will always be broken.
> 
> If that is correct, then your patch is the right thing to do.  If it's not
> correct, your patch still isn't wrong -- it's just not quite optimal.

I still think it's correct.


> > > Maybe that's what you meant.  Trying to store that information 
> > > in the host controller's state is a poor-man's way of doing it, but at the
> > > moment it's the only way we have.  That's why David doesn't want the state 
> > > during the atomic reload to be the same as the state during a regular 
> > > resume.
> > 
> > I'd put it differently.  It's not a poor-man's way at all; passing that
> > information in hardware is the correct solution.  A regular resume would
> > keep the information there too (from STR or standby).  We don't want to
> > have swsusp piling on special cases.
> 
> Okay, I take it back.  Yes, putting the hardware into the appropriate 
> state is the right thing to do.
> 
> On the other hand, it might not be so easy to know what that state is.  

That was the point of my comment above (you responded "if that is correct...").
That's how to know.  Hardware is already correct in all cases except the annoying
one where the kernel resuming a swsusp snapshot initializes that USB driver; and
in that case, it's always correct to reset the USB controller.


> Perhaps the decision should be left up to the device driver.  If drivers
> were told the difference between "FREEZE to prepare for creating a memory
> snapshot" and "FREEZE to prepare for restoring a memory image" then they 
> could do the right thing always.

Or better yet, just do what kexec() does.  :)

- Dave


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 2045 bytes --]

Hi.

On Wednesday 26 April 2006 09:55, David Brownell wrote:
> On Tuesday 25 April 2006 3:18 pm, Nigel Cunningham wrote:
> > I saw the 2 suspends, 1 resume comment in another part of the thread, but
> > don't believe a driver would be able to detect that 2 suspends and 1
> > resume call had been made - at least not without some non volatile ram.
>
> The extra suspend is just IMO a symptom of sloppiness, like a "here may be
> bugs" sign.  Not necessarily an issue in its own right.
>
> In fact if you count carefully, it's three suspends and one resume:  one
> suspend before calling swsusp_resume, one inside swsusp_resume -- replaced
> by my patch -- and a correctly matched pair in the kernel being resumed.

That doesn't sound right. Let' see - where's this third one?

			Real				Effective
SUSPEND

Pre-copy		suspend(freeze)		suspend(freeze)
			powerdown(freeze)	powerdown(freeze)

Post-copy		powerup
			resume

Powerdown	shutdown_prepare/kernel_power_off/...

RESUME

			bios & kernel init

Pre-restore		suspend(freeze)
			powerdown(freeze)

Post-restore	powerup			powerup
			resume			resume


> Raising two points:  (1) for those who think suspend is right, you can
> think of it as replacing one of the extra ones with
> kernel_restart_prepare(); and (2) not very much code interacts with that
> restart prep, that's a sane audit problem.
>
>
> Also, about non-volatile RAM.  Not necessary ... the device hardware holds
> all the relevant state.  The problem is that swsusp is now trashing it with
> those suspend calls before resuming the real kernel.  On the plus side, we
> already have code being used to resolve the identical issue for kexec(),
> and all my patch does is to use it.

If devices really do get powered down, then they won't retain the state. If 
they don't actually powerdown, then I see your point.

Regards,

Nigel
-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Tuesday 25 April 2006 6:16 pm, Nigel Cunningham wrote:
> Hi.
> 
> On Wednesday 26 April 2006 09:55, David Brownell wrote:
> > On Tuesday 25 April 2006 3:18 pm, Nigel Cunningham wrote:
> > > I saw the 2 suspends, 1 resume comment in another part of the thread, but
> > > don't believe a driver would be able to detect that 2 suspends and 1
> > > resume call had been made - at least not without some non volatile ram.
> >
> > The extra suspend is just IMO a symptom of sloppiness, like a "here may be
> > bugs" sign.  Not necessarily an issue in its own right.
> >
> > In fact if you count carefully, it's three suspends and one resume:  one
> > suspend before calling swsusp_resume, one inside swsusp_resume -- replaced
> > by my patch -- and a correctly matched pair in the kernel being resumed.
> 
> That doesn't sound right. Let' see - where's this third one?

I see your point ... device_suspend() and device_power_down() really
do the same thing, but to different sets of devices.  A nastily deceptive
naming convention that I may want to fix someday.

So it's not quite fair counting those separately.  Three suspend calls,
sure ... but only two per device.


> > Also, about non-volatile RAM.  Not necessary ... the device hardware holds
> > all the relevant state.  The problem is that swsusp is now trashing it with
> > those suspend calls before resuming the real kernel.  On the plus side, we
> > already have code being used to resolve the identical issue for kexec(),
> > and all my patch does is to use it.
> 
> If devices really do get powered down, then they won't retain the state. If 
> they don't actually powerdown, then I see your point.

While the system is restarting into the snapshot, there's no powerdown.
(Because device_power_down doesn't actually power down devices.)  Q.E.D.

- Dave



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

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

Hi.

On Wednesday 26 April 2006 13:32, David Brownell wrote:
> On Tuesday 25 April 2006 6:16 pm, Nigel Cunningham wrote:
> > Hi.
> >
> > On Wednesday 26 April 2006 09:55, David Brownell wrote:
> > > On Tuesday 25 April 2006 3:18 pm, Nigel Cunningham wrote:
> > > > I saw the 2 suspends, 1 resume comment in another part of the thread,
> > > > but don't believe a driver would be able to detect that 2 suspends
> > > > and 1 resume call had been made - at least not without some non
> > > > volatile ram.
> > >
> > > The extra suspend is just IMO a symptom of sloppiness, like a "here may
> > > be bugs" sign.  Not necessarily an issue in its own right.
> > >
> > > In fact if you count carefully, it's three suspends and one resume: 
> > > one suspend before calling swsusp_resume, one inside swsusp_resume --
> > > replaced by my patch -- and a correctly matched pair in the kernel
> > > being resumed.
> >
> > That doesn't sound right. Let' see - where's this third one?
>
> I see your point ... device_suspend() and device_power_down() really
> do the same thing, but to different sets of devices.  A nastily deceptive
> naming convention that I may want to fix someday.

:)

> So it's not quite fair counting those separately.  Three suspend calls,
> sure ... but only two per device.
>
> > > Also, about non-volatile RAM.  Not necessary ... the device hardware
> > > holds all the relevant state.  The problem is that swsusp is now
> > > trashing it with those suspend calls before resuming the real kernel. 
> > > On the plus side, we already have code being used to resolve the
> > > identical issue for kexec(), and all my patch does is to use it.
> >
> > If devices really do get powered down, then they won't retain the state.
> > If they don't actually powerdown, then I see your point.
>
> While the system is restarting into the snapshot, there's no powerdown.
> (Because device_power_down doesn't actually power down devices.)  Q.E.D.

I thought that at least some devices might go D3cold, or is it always hot? I'm 
perfectly happy to admit being an ignoramus here - I'm just beginning to work 
on drivers, so still have a lot to learn.

Regards,

Nigel
-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

On Út 25-04-06 16:13:41, David Brownell wrote:
> On Tuesday 25 April 2006 2:41 pm, Pavel Machek wrote:
> 
> > > Well, if we had a pm_should_I_spin_down_drives() it would make sense to me
> > > that it return FALSE during kernel_restart_prepare() too ... surely kexec
> > > users have the same issues!
> > 
> > pm_should_I_spin_down_drives() is incredibly ugly hack, sorry.
> 
> The name was chosen to be ugly.  I'd expect someone to make it prettier;
> maybe draw ponies around the name or somesuch.  ;)

:-).

> However, by the way your pm_message_t changes removed all information about
> system states, you've forced that family of approach in every driver that needs
> to know about such states in order to behave correctly.  For example, "don't
> turn off clock X in PM_STANDBY" ... since no driver has a standard way to know
> about PM_STANDBY, it needs some function accessing state set up by pm_ops.prepare
> or else it's not going to be able to behave right.

Or you can simply adds flags field to pm_message_t.

> You still haven't come up with a response to my point that starting up a
> new kernel via swsusp snapshot resume is a system restart in exactly
> the

After kexec, you hit device-init path, while after swsusp resume, you
hit... well resume(). That's quite different.

> same way as it is for kexec() ... at this point, there is no better fix
> for the problem than my original patch, AND you haven't actually identified
> a single real problem with it.

You identified that problem yourselves: it will break drivers.

Add flags field and be done with it... please?
								Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

On Wednesday 26 April 2006 00:56, David Brownell wrote:
> On Tuesday 25 April 2006 2:55 pm, Rafael J. Wysocki wrote:
> > On Tuesday 25 April 2006 23:04, David Brownell wrote:
> >  
> > > The third state is the problem scenario, kicking in when the driver was
> > > statically linked (or modprobed from initramfs, etc), but not during
> > > your scenario.
> > 
> > The problem, as I see it, is that too many devices may be initialized at the
> > kernel startup.
> 
> That's a fair observation.  In the same way, swsusp resumes too many devices
> before writing the snapshot.  In both cases, it only needs the resume partition,
> not every random bit of hardware in the system.

That's right, and I think we should make it resume only what's needed at some
point in the future.

> But it's not the root cause of the problem either.  The same problem appears if
> the device holding the resume partition gets forced into this "broken suspend"
> state.

Well, IMO the state may or may not be broken depending on the device,
so we should not assume it will always be broken.

> > I think we _can_ reset some of them before the image 
> > is restored, but at least some of them need to be treated more carefully.
> 
> Maybe; but remember they _were_ just reset ... and that if a driver doesn't
> know how to re-init a device that it has just reset, then it has other problems!
> It's a common practice to reset on entrance to probe() ... if that works, it's
> "just" a simple matter of code cleanup/reorg to let the init code be used if
> resume() detects the device hardware has been reset.

Agreed.

> For example, rmmod/modprobe cycles wouldn't behave sanely ... and it'd likely
> break with kexec().  If they "need" careful treatment, that'd seem to me most
> like a driver bug ...
> 
> Now, if you have specific examples of things that shouldn't be reset, that
> could be interesting.

The resume device and friends (ie. controller, bus, etc.).

> I don't seem to have tripped over any, and from first principles it's clear to
> me there _shouldn't_ be such drivers (modulo bugs). 
> 
> If you're going to argue that there are enough buggy drivers around that
> we can't rely on them to behave correctly here ... well, we don't have an
> example of even one such driver, much less a flood of them.  And the thing
> to do with bugs is fix them, not coddle them.  ;)

Agreed.

> > > Right:  the first two "safe" cases kick in.  This is the partial workaround I
> > > had identified:  dodging the code paths for that third state, where suspend()
> > > is being used to put the hardware into a broken suspend state.
> > 
> > So perhaps we should just make them enter a state that's not broken?
> > That may be reset for some devices (eg. USB) and something else for some
> > others (eg. storage).
> 
> Sure, but I'm not sure what "something else" would ever be.  Examples?
> Not of cases where something else _could_ be done, but where it _must_ be.

My point is that it need not be _necessary_ to reset all devices.  We should
reset only those which need to be reset.

> > > It may help to think of two distinct types of device hardware suspend states
> > > (only the first is real, the second is just a software bug):
> > > 
> > >  - Correct, with internal state corresponding to what the driver suspend() did;
> > >    what a normal hardware suspend/resume cycle (not powercycle!) could do.
> > > 
> > >  - Broken, with any other internal state (except reset).  This is what swsusp
> > >    currently forces, by adding **AND HIDING** a reset and reinit cycle, because
> > >    of the extra suspend() call in (7).
> > 
> > Still there are drivers that have no problems with it, so why we should we
> > forcibly reset their devices?
> 
> Can any driver can really justify not handling the case where resume() sees
> hardware that's been reset?  That is, _exactly_ as would be the case if its
> driver were only loaded as a module after the kernel booted?  I think not...

Of course every driver should handle this but that's not the point.  The point
is whether we should _force_ the reset on every device.
 
> > > My patch/suggestion just ensures that instead of that broken state, reset is used.
> > > in all cases ... not just the "driver not initialized before snapshot resume" case.
> > 
> > As I said before I generally agree with this except I think some more fine
> > grained approach is needed in this case.
> 
> You've not convinced me of that; might be right, but nothing proves it to me.

Fair enough. :-)

Greetings,
Rafael

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1401 bytes --]

A little behind the current head of the thread, but better late than
never...

On Wed, 26 Apr 2006, Nigel Cunningham wrote:

> > (7) (without your change) swsusp calls .suspend() for all device drivers
> > that are present at that time, but our driver is not there, so its
> > .suspend() _won't_ be called.  [Of course with your change .suspend() won't
> > be called for any driver.]
> 
> We also make a list that is safely available after the atomic restore of 
> drivers that have had .suspend methods called.
> 
> > (8) swsusp restores the image.
> 
> (9) suspend to disk calls .resume() for each device that is found in the list 
> generated above, thereby excluding our problem driver. Drivers which are 
> present but not in the list above (such as the problem driver) have a 
> different routine called, perhaps the normal initialisation one?

First, is this a list of drivers or a list of devices?  Under (7) you said 
drivers, and under (9) you said devices.

Second, how do you tell whether a device or driver in the resuming kernel 
is in the list or not?  Do you store the driver's name in the list and 
compare names?

Third, suppose a driver was present in the original swsusp kernel (and 
hence also in the resuming kernel) but not in the boot kernel.  Then it 
won't be in the list.  Apparently that means it doesn't get resumed during 
(9).  So when does it get resumed?

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

On Wed, 26 Apr 2006, Rafael J. Wysocki wrote:

> > Now, if you have specific examples of things that shouldn't be reset, that
> > could be interesting.
> 
> The resume device and friends (ie. controller, bus, etc.).

I presume the freeze/reset we're talking about occurs immediately after 
the memory snapshot has been read from the swap partition and immediately 
before it is installed/activated.  (If occurs before the snapshot is read 
in, then how would you actually manage to read it?)

So under these circumstances, how does it hurt anything to reset the 
resume device rather than to freeze it?

> My point is that it need not be _necessary_ to reset all devices.  We should
> reset only those which need to be reset.

> Of course every driver should handle this but that's not the point.  The point
> is whether we should _force_ the reset on every device.

I'm not convinced that it hurts anything.  But in any case, it wouldn't
hurt to follow Pavel's advice and add a flags field to pm_message_t.

Alan Stern



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

On Wednesday 26 April 2006 16:38, Alan Stern wrote:
> On Wed, 26 Apr 2006, Rafael J. Wysocki wrote:
> 
> > > Now, if you have specific examples of things that shouldn't be reset, that
> > > could be interesting.
> > 
> > The resume device and friends (ie. controller, bus, etc.).
> 
> I presume the freeze/reset we're talking about occurs immediately after 
> the memory snapshot has been read from the swap partition and immediately 
> before it is installed/activated.  (If occurs before the snapshot is read 
> in, then how would you actually manage to read it?)
> 
> So under these circumstances, how does it hurt anything to reset the 
> resume device rather than to freeze it?

It just shouldn't be necessary.  Actually I think the resume device shouldn't
be frozen too.

> > My point is that it need not be _necessary_ to reset all devices.  We should
> > reset only those which need to be reset.
> 
> > Of course every driver should handle this but that's not the point.  The point
> > is whether we should _force_ the reset on every device.
> 
> I'm not convinced that it hurts anything.  But in any case, it wouldn't
> hurt to follow Pavel's advice and add a flags field to pm_message_t.

Exactly.

Greetings,
Rafael


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

On Wed, 26 Apr 2006, Rafael J. Wysocki wrote:

> > So under these circumstances, how does it hurt anything to reset the 
> > resume device rather than to freeze it?
> 
> It just shouldn't be necessary.  Actually I think the resume device shouldn't
> be frozen too.

Well, you wouldn't want it doing DMA to unknown memory areas while you're
trying to place the image data in those same areas, would you?  And when
the image is reactivated, you wouldn't want the device generating
interrupt requests while its driver still thinks it is suspended.  (Or if 
it doesn't even have a driver in the image.)

And don't say that no resume device would ever do DMA or generate IRQs
while it was idle!  What if it was a remote disk accessible via a network 
interface?

Alan Stern



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

On Wednesday 26 April 2006 17:38, Alan Stern wrote:
> On Wed, 26 Apr 2006, Rafael J. Wysocki wrote:
> 
> > > So under these circumstances, how does it hurt anything to reset the 
> > > resume device rather than to freeze it?
> > 
> > It just shouldn't be necessary.  Actually I think the resume device shouldn't
> > be frozen too.
>
> Well, you wouldn't want it doing DMA to unknown memory areas while you're
> trying to place the image data in those same areas, would you?  And when
> the image is reactivated, you wouldn't want the device generating
> interrupt requests while its driver still thinks it is suspended.  (Or if 
> it doesn't even have a driver in the image.)

I think it'll always have a driver in the image, because we use it on suspend
to save the image.  Also IMHO, theoretically, the driver need not think the
device is suspended.

> And don't say that no resume device would ever do DMA or generate IRQs
> while it was idle!  What if it was a remote disk accessible via a network 
> interface?

I think at least some devices may be told not to do DMA and/or generate IRQs
without being reset or put into a low(er) power state.

Ayway, as of today, we have no infrastructure allowing us to handle resume
devices in a special way.  However, if we decide to reset all devices before
restoring the image, we'll probably make such things harder to implement
in the future.

Greetings,
Rafael

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1717 bytes --]

On Wed, 26 Apr 2006, Rafael J. Wysocki wrote:

> > > It just shouldn't be necessary.  Actually I think the resume device shouldn't
> > > be frozen too.
> >
> > Well, you wouldn't want it doing DMA to unknown memory areas while you're
> > trying to place the image data in those same areas, would you?  And when
> > the image is reactivated, you wouldn't want the device generating
> > interrupt requests while its driver still thinks it is suspended.  (Or if 
> > it doesn't even have a driver in the image.)
> 
> I think it'll always have a driver in the image, because we use it on suspend
> to save the image.

True.

>  Also IMHO, theoretically, the driver need not think the
> device is suspended.

It does have to think the device is frozen, however.  The device _has_ to 
be frozen while the memory image is created, for the same reasons as 
mentioned before: it mustn't do DMA or generate IRQs.

> I think at least some devices may be told not to do DMA and/or generate IRQs
> without being reset or put into a low(er) power state.

That is in fact the very definition of FREEZE.  From 
Documentation/power/devices.txt:

	FREEZE -- stop DMA and interrupts, and be prepared to reinit HW from
	scratch.

Agreed, general devices do not need to be reset or put into a lower power 
state when they enter FREEZE.

> Ayway, as of today, we have no infrastructure allowing us to handle resume
> devices in a special way.  However, if we decide to reset all devices before
> restoring the image, we'll probably make such things harder to implement
> in the future.

Given the definition above, if a driver that has problems resuming from a 
FREEZE when the device has been reset then the driver is wrong.

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

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

Hi.

On Thursday 27 April 2006 00:24, Alan Stern wrote:
> A little behind the current head of the thread, but better late than
> never...

:)

> On Wed, 26 Apr 2006, Nigel Cunningham wrote:
> > > (7) (without your change) swsusp calls .suspend() for all device
> > > drivers that are present at that time, but our driver is not there, so
> > > its .suspend() _won't_ be called.  [Of course with your change
> > > .suspend() won't be called for any driver.]
> >
> > We also make a list that is safely available after the atomic restore of
> > drivers that have had .suspend methods called.
> >
> > > (8) swsusp restores the image.
> >
> > (9) suspend to disk calls .resume() for each device that is found in the
> > list generated above, thereby excluding our problem driver. Drivers which
> > are present but not in the list above (such as the problem driver) have a
> > different routine called, perhaps the normal initialisation one?
>
> First, is this a list of drivers or a list of devices?  Under (7) you said
> drivers, and under (9) you said devices.

Sorry. I was using them interchangably. I meant devices, since the same driver 
might be handling more than one instance.

> Second, how do you tell whether a device or driver in the resuming kernel
> is in the list or not?  Do you store the driver's name in the list and
> compare names?

I was deliberately not worrying about that at the moment, and just assuming 
that we'd come up with some way to match the entries. My initial guess would 
be that it would involve whatever per instance ids were available, or 
something as close to that as we can manage.

> Third, suppose a driver was present in the original swsusp kernel (and
> hence also in the resuming kernel) but not in the boot kernel.  Then it
> won't be in the list.  Apparently that means it doesn't get resumed during
> (9).  So when does it get resumed?

I would think it should still be resumed at that stage. It just gets told 
(somehow) at that point that it wasn't suspended in the boot kernel, and it 
might therefore perform different actions, based on that info.

Regards,

Nigel

-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

On Wednesday 26 April 2006 21:06, Alan Stern wrote:
> On Wed, 26 Apr 2006, Rafael J. Wysocki wrote:
> 
> > > > It just shouldn't be necessary.  Actually I think the resume device shouldn't
> > > > be frozen too.
> > >
> > > Well, you wouldn't want it doing DMA to unknown memory areas while you're
> > > trying to place the image data in those same areas, would you?  And when
> > > the image is reactivated, you wouldn't want the device generating
> > > interrupt requests while its driver still thinks it is suspended.  (Or if 
> > > it doesn't even have a driver in the image.)
> > 
> > I think it'll always have a driver in the image, because we use it on suspend
> > to save the image.
> 
> True.
> 
> >  Also IMHO, theoretically, the driver need not think the
> > device is suspended.
> 
> It does have to think the device is frozen, however.  The device _has_ to 
> be frozen while the memory image is created, for the same reasons as 
> mentioned before: it mustn't do DMA or generate IRQs.
> 
> > I think at least some devices may be told not to do DMA and/or generate IRQs
> > without being reset or put into a low(er) power state.
> 
> That is in fact the very definition of FREEZE.  From 
> Documentation/power/devices.txt:
> 
> 	FREEZE -- stop DMA and interrupts, and be prepared to reinit HW from
> 	scratch.
> 
> Agreed, general devices do not need to be reset or put into a lower power 
> state when they enter FREEZE.

Ah, my bad.  I wasn't referring to this definition, sorry for the confusion.

> > Ayway, as of today, we have no infrastructure allowing us to handle resume
> > devices in a special way.  However, if we decide to reset all devices before
> > restoring the image, we'll probably make such things harder to implement
> > in the future.
> 
> Given the definition above, if a driver that has problems resuming from a 
> FREEZE when the device has been reset then the driver is wrong.

Agreed.

Greetings,
Rafael

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

On Wednesday 26 April 2006 4:26 am, Rafael J. Wysocki wrote:
> On Wednesday 26 April 2006 00:56, David Brownell wrote:

> > But it's not the root cause of the problem either.  The same problem appears if
> > the device holding the resume partition gets forced into this "broken suspend"
> > state.
> 
> Well, IMO the state may or may not be broken depending on the device,
> so we should not assume it will always be broken.

Not so.  See my previous emails.  The "broken suspend" state is broken
by definition.  Maybe you're referring to a different issue ... whether
or not its driver would notice that bug.


> > Now, if you have specific examples of things that shouldn't be reset, that
> > could be interesting.
> 
> The resume device and friends (ie. controller, bus, etc.).

Bad answer.  If its driver would notice, then it must be reset.
And if the driver wouldn't notice, resetting won't matter.

There's still no example of a device that should not be reset
(a second time).

- Dave

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

On Wednesday 26 April 2006 23:31, David Brownell wrote:
> On Wednesday 26 April 2006 4:26 am, Rafael J. Wysocki wrote:
> > On Wednesday 26 April 2006 00:56, David Brownell wrote:
> 
> > > But it's not the root cause of the problem either.  The same problem appears if
> > > the device holding the resume partition gets forced into this "broken suspend"
> > > state.
> > 
> > Well, IMO the state may or may not be broken depending on the device,
> > so we should not assume it will always be broken.
> 
> Not so.  See my previous emails.  The "broken suspend" state is broken
> by definition.  Maybe you're referring to a different issue ... whether
> or not its driver would notice that bug.

It is a bug from your point of view, and I was referring to the fact that it
apparently doesn't matter for many drivers.

> > > Now, if you have specific examples of things that shouldn't be reset, that
> > > could be interesting.
> > 
> > The resume device and friends (ie. controller, bus, etc.).
> 
> Bad answer.  If its driver would notice, then it must be reset.
> And if the driver wouldn't notice, resetting won't matter.
> 
> There's still no example of a device that should not be reset
> (a second time).

OK, so I have no any.  Which doesn't seem to matter as far as your patch is
concerned, as Pavel doesn't like it anyway. ;-)

Greetings,
Rafael

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Patrick Mochel]

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

On Mon, Apr 24, 2006 at 02:29:51PM -0700, David Brownell wrote:

> I recently observed this myself and tracked down one problem.  The solution
> involves what kexec() does in much the same situation:  before starting a
> new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
> instead, which is the root cause of the problem.

I'm not sure that is a good thing to do for a lot of video chipsets on x86 
platforms. For many, we do not have a way to reinitialize them from scratch,
so if we do a reset on them then we shoot ourselves in the foot. 

Now, maybe some/most/all video chipsets wouldn't do anything when it gets
the reset call. But, if you have one driver that does reset the hardware
(e.g. because the hardware is buggy and needs it before system reset), then
this patch will prevent that one chipset from working, so you'll have to 
work around that, possibly by passing a flag to the reset method. 

At that point, you have something very similar to just calling suspend with
a different flag than FREEZE (or an additional flag). 


I don't disagree with the intent or justification of the patch, but ultimately
it is up to the drivers to decide what to do for a particular piece of 
hardware in a particular context. Maybe the driver does the same thing for
all hardware in all situations, like reset the device. Or, maybe all
drivers do the same thing in a particular context. 

It's very difficult to know what every single device needs, so we should just
be honest with the drivers - tell them exactly what we're about to do - and
let them handle it as appropriate..


	Pat

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

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

Hi!

> The right way to solve this is to make sure that the resuming kernel can
> correctly determine whether the power session (way back from the original
> sw-suspend) is still intact.  It's expected that in many cases it won't
> be, because most systems won't provide suspend current while the machine
> is off.  We have to guarantee that the boot kernel's actions won't end up
> fooling the resuming kernel into thinking that the power sessions are
> intact when in fact they aren't.  (Furthermore, in an ideal world, we
> would also make sure that the boot kernel won't destroy any power sessions
> that still _are_ intact.  Right now we have no way to do this, because the
> drivers in the boot kernel don't know that it _is_ a boot kernel.)

You don't know if it is boot or resume until you read the disk, sorry.

							Pavel
-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

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

Hi!

> > I'd put it differently.  It's not a poor-man's way at all; passing that
> > information in hardware is the correct solution.  A regular resume would
> > keep the information there too (from STR or standby).  We don't want to
> > have swsusp piling on special cases.
> 
> Okay, I take it back.  Yes, putting the hardware into the appropriate 
> state is the right thing to do.
> 
> On the other hand, it might not be so easy to know what that state is.  
> Perhaps the decision should be left up to the device driver.  If drivers
> were told the difference between "FREEZE to prepare for creating a memory
> snapshot" and "FREEZE to prepare for restoring a memory image" then they 
> could do the right thing always.

Add flags field to pm_message_t, please. There, you have way to pass
down that ino while staying back-compatible.

-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

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

Hi!

> > At the least it would depend on 
> > the particular state.  For instance, I've got a PCI USB controller card
> > (VIA) on which the UHCI controllers support only D0 and D3cold.  And
> > (although I'm not certain of this) I may have seen a report from someone
> > whose system _does_ retain power during swsusp.  IIRC it was a powerbook
> > of some sort.
> 
> I know that Macintoshes are good about maintaining power during STR, and
> suspect that some systems running Apple's OS will do so during "hibernate";
> they have a more complete/powerful power management framework than Linux.
> 
> But last time the topic came up, I recall Pavel saying he'd never seen or
> heard of hardware that did things like that with swsusp, even when using
> "platform" (or "firmware") suspend mode.

I have seen such macine since that. NAPA seems to keep usb powered
even when off. I think I'll just use it as cellphone charger :-).

							Pavel

-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

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

Hi!

> > It seems to me that the right solution might be for these usb devices to 
> > treat a resume from a freeze as an indication that hardware should be reset.
> 
> I don't much like that model.  It creates more special cases in drivers (not
> just USB), and redefines the "freeze" thing to be more than just "quiescede".
> Every special case that gets added is a breeding ground for more bugs.
> 
> Also, what about the resume-from-freeze that _every_ device does during swsusp
> suspend?  The kernel is still resuming every device, not just the ones needed
> to write the suspend partition.

It is quite hard to tell which devices are needed for write... Plus
only resuming some drivers would only hide the bugs in drivers not
needed for write, and speedup suspend a tiny bit. Too little benefit
for more complex code, I'd say.
							Pavel
-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

On Thu, 27 Apr 2006, Pavel Machek wrote:

> Hi!
> 
> > The right way to solve this is to make sure that the resuming kernel can
> > correctly determine whether the power session (way back from the original
> > sw-suspend) is still intact.  It's expected that in many cases it won't
> > be, because most systems won't provide suspend current while the machine
> > is off.  We have to guarantee that the boot kernel's actions won't end up
> > fooling the resuming kernel into thinking that the power sessions are
> > intact when in fact they aren't.  (Furthermore, in an ideal world, we
> > would also make sure that the boot kernel won't destroy any power sessions
> > that still _are_ intact.  Right now we have no way to do this, because the
> > drivers in the boot kernel don't know that it _is_ a boot kernel.)
> 
> You don't know if it is boot or resume until you read the disk, sorry.

According to David this shouldn't matter.  During swsusp the system is
supposed to be completely off, with no suspend power available.  Hence all
the power sessions are guaranteed to be interrupted, and the boot kernel
doesn't have to worry about destroying any of them.

Alan Stern



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Patrick Mochel]

On Thu, Apr 27, 2006 at 10:34:16AM -0400, Alan Stern wrote:
> On Thu, 27 Apr 2006, Pavel Machek wrote:
> 
> > Hi!
> > 
> > > The right way to solve this is to make sure that the resuming kernel can
> > > correctly determine whether the power session (way back from the original
> > > sw-suspend) is still intact.  It's expected that in many cases it won't
> > > be, because most systems won't provide suspend current while the machine
> > > is off.  We have to guarantee that the boot kernel's actions won't end up
> > > fooling the resuming kernel into thinking that the power sessions are
> > > intact when in fact they aren't.  (Furthermore, in an ideal world, we
> > > would also make sure that the boot kernel won't destroy any power sessions
> > > that still _are_ intact.  Right now we have no way to do this, because the
> > > drivers in the boot kernel don't know that it _is_ a boot kernel.)
> > 
> > You don't know if it is boot or resume until you read the disk, sorry.
> 
> According to David this shouldn't matter.  During swsusp the system is
> supposed to be completely off, with no suspend power available.  Hence all
> the power sessions are guaranteed to be interrupted, and the boot kernel
> doesn't have to worry about destroying any of them.

Not necessarily. x86 hardware implementations of suspend-to-disk retain some 
power during suspend. Not many (if any) devices will retain context, but the
system is definitely not completely "off". Actually, the same is true for
soft-off on x86 (aka ACPI S5). Some power may be drawn to support various power
on events. The only time a system is truly off is when it is unplugged from
the wall (and/or the battery taken out, if applicable). 

I don't think it matters much in this particular context, but it's important
to keep in mind when making assumptions about "off-ness".

Thanks,


	Patrick


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Alan Stern]

On Thu, 27 Apr 2006, Patrick Mochel wrote:

> > According to David this shouldn't matter.  During swsusp the system is
> > supposed to be completely off, with no suspend power available.  Hence all
> > the power sessions are guaranteed to be interrupted, and the boot kernel
> > doesn't have to worry about destroying any of them.
> 
> Not necessarily. x86 hardware implementations of suspend-to-disk retain some 
> power during suspend. Not many (if any) devices will retain context, but the
> system is definitely not completely "off". Actually, the same is true for
> soft-off on x86 (aka ACPI S5). Some power may be drawn to support various power
> on events. The only time a system is truly off is when it is unplugged from
> the wall (and/or the battery taken out, if applicable). 
> 
> I don't think it matters much in this particular context, but it's important
> to keep in mind when making assumptions about "off-ness".

I'm perfectly happy to let you duke this out with David!  :-)

Either way, there is one important observation: You can't use swsusp if 
your root filesystem is on a USB disk.  Not until some sort of persistent 
storage manager is available...

Alan Stern



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Thursday 27 April 2006 9:55 am, Patrick Mochel wrote:
> On Thu, Apr 27, 2006 at 10:34:16AM -0400, Alan Stern wrote:
> > 
> > 		During swsusp the system is
> > supposed to be completely off, with no suspend power available.  Hence all
> > the power sessions are guaranteed to be interrupted, and the boot kernel
> > doesn't have to worry about destroying any of them.
> 
> Not necessarily. x86 hardware implementations of suspend-to-disk retain some 
> power during suspend. Not many (if any) devices will retain context, but the
> system is definitely not completely "off".

As a rule swsusp (or firmware suspend-to-disk) power off everything except
what's needed to power up the motherboard ... or to provide "5 AM wakeup"
type events using a battery-backed realtime clock.  Maintaining VBUS power
sessions from USB host controllers is one of those "theoretically allowed,
but never observed in the wild" cases.

Right, not "completely" off ... but certainly nowhere as close to "on" as
would be true of suspend-to-RAM.  And regardless, the problem in $SUBJECT is
when Linux trashes the state which the limited "on" is there to maintain.

- Dave



-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Wednesday 26 April 2006 6:22 pm, Patrick Mochel wrote:
> On Mon, Apr 24, 2006 at 02:29:51PM -0700, David Brownell wrote:
> 
> > I recently observed this myself and tracked down one problem.  The solution
> > involves what kexec() does in much the same situation:  before starting a
> > new kernel, most hardware needs to be reset.  Today, swsusp will suspend it
> > instead, which is the root cause of the problem.
> 
> I'm not sure that is a good thing to do for a lot of video chipsets on x86 
> platforms. For many, we do not have a way to reinitialize them from scratch,
> so if we do a reset on them then we shoot ourselves in the foot. 

I was waiting for someone else to bring up video; it wasn't exactly the
elephant-in-the-room-that-nobody-wants-to-talk-about, but it was clearly
a trouble spot missing from the discussion.

Agreed that it'd be good not to reset video hardware.  Of the handful of
drivers that try to do anything specific with FREEZE, Mr. Grep reports
that we have USB, IDE ... and a bunch of video drivers.


> It's very difficult to know what every single device needs, so we should just
> be honest with the drivers - tell them exactly what we're about to do - and
> let them handle it as appropriate..

There does seem to be agreement that the current FREEE invocation is not
sufficient.  I'm looking at a slightly different solution now ... one which
unfortunately involves changing drivers, but can indeed allow swsusp resume
paths to do the right thing (instead of what it does now).

- Dave


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

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

> > > > But it's not the root cause of the problem either.  The same problem appears if
> > > > the device holding the resume partition gets forced into this "broken suspend"
> > > > state.
> > > 
> > > Well, IMO the state may or may not be broken depending on the device,
> > > so we should not assume it will always be broken.
> > 
> > Not so.  See my previous emails.  The "broken suspend" state is broken
> > by definition.  Maybe you're referring to a different issue ... whether
> > or not its driver would notice that bug.
> 
> It is a bug from your point of view, and I was referring to the fact that it
> apparently doesn't matter for many drivers.

That is, to "whether driver notices".  It's clearly a bug with respect to
how the FREEZE state is defined.  Not all drivers need to notice all bugs.

- Dave


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Nigel Cunningham]

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

Hi.

On Friday 28 April 2006 05:21, David Brownell wrote:
> On Thursday 27 April 2006 9:55 am, Patrick Mochel wrote:
> > On Thu, Apr 27, 2006 at 10:34:16AM -0400, Alan Stern wrote:
> > > 		During swsusp the system is
> > > supposed to be completely off, with no suspend power available.  Hence
> > > all the power sessions are guaranteed to be interrupted, and the boot
> > > kernel doesn't have to worry about destroying any of them.
> >
> > Not necessarily. x86 hardware implementations of suspend-to-disk retain
> > some power during suspend. Not many (if any) devices will retain context,
> > but the system is definitely not completely "off".
>
> As a rule swsusp (or firmware suspend-to-disk) power off everything except
> what's needed to power up the motherboard ... or to provide "5 AM wakeup"
> type events using a battery-backed realtime clock.  Maintaining VBUS power
> sessions from USB host controllers is one of those "theoretically allowed,
> but never observed in the wild" cases.
>
> Right, not "completely" off ... but certainly nowhere as close to "on" as
> would be true of suspend-to-RAM.  And regardless, the problem in $SUBJECT
> is when Linux trashes the state which the limited "on" is there to
> maintain.

This isn't necessarily true either. Suspend2 has supported writing the image, 
then suspending to ram for a year or two. uswsusp has just gained the same 
functionality. In this state, if you don't pull the plug/drain the battery, 
you never actually power down. Having said that, this might be a different 
kettle of fish though, because there's no boot kernel in that case. For 
Suspend2 (and uswsusp, I assume), it's more akin to backing out of the cycle 
at the last possible moment before powering down.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

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

Hi!

> > > > 		During swsusp the system is
> > > > supposed to be completely off, with no suspend power available.  Hence
> > > > all the power sessions are guaranteed to be interrupted, and the boot
> > > > kernel doesn't have to worry about destroying any of them.
> > >
> > > Not necessarily. x86 hardware implementations of suspend-to-disk retain
> > > some power during suspend. Not many (if any) devices will retain context,
> > > but the system is definitely not completely "off".
> >
> > As a rule swsusp (or firmware suspend-to-disk) power off everything except
> > what's needed to power up the motherboard ... or to provide "5 AM wakeup"
> > type events using a battery-backed realtime clock.  Maintaining VBUS power
> > sessions from USB host controllers is one of those "theoretically allowed,
> > but never observed in the wild" cases.

Napa machine seems to maintain USB power even while turned off.

> > Right, not "completely" off ... but certainly nowhere as close to "on" as
> > would be true of suspend-to-RAM.  And regardless, the problem in $SUBJECT
> > is when Linux trashes the state which the limited "on" is there to
> > maintain.
> 
> This isn't necessarily true either. Suspend2 has supported writing the image, 
> then suspending to ram for a year or two. uswsusp has just gained the same 
> functionality. In this state, if you don't pull the plug/drain the battery, 
> you never actually power down. Having said that, this might be a different 
> kettle of fish though, because there's no boot kernel in that case. For 
> Suspend2 (and uswsusp, I assume), it's more akin to backing out of the cycle 
> at the last possible moment before powering down.

Yep it is same for uswsusp, and probably irrelevant here.
									Pavel



-- 
Thanks for all the (sleeping) penguins.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Patrick Mochel]

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

On Thu, Apr 27, 2006 at 12:41:28PM -0700, David Brownell wrote:

> There does seem to be agreement that the current FREEE invocation is not
> sufficient.  I'm looking at a slightly different solution now ... one which
> unfortunately involves changing drivers, but can indeed allow swsusp resume
> paths to do the right thing (instead of what it does now).

It's Ok if it involves a drive change, so long as its an optional change, which
means that it shouldn't affect the interface very much (i.e. the calling 
convention). That's why it'd be good to augment and/or modify pm_message_t
to implement the changes, so we wouldn't have to change every single driver
again.. 


	Patrick

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Tuesday 02 May 2006 9:12 am, Patrick Mochel wrote:
> On Thu, Apr 27, 2006 at 12:41:28PM -0700, David Brownell wrote:
> 
> > There does seem to be agreement that the current FREEZE invocation is not
> > sufficient.  I'm looking at a slightly different solution now ... one which
> > unfortunately involves changing drivers, but can indeed allow swsusp resume
> > paths to do the right thing (instead of what it does now).
> 
> It's Ok if it involves a drive change, so long as its an optional change, which
> means that it shouldn't affect the interface very much (i.e. the calling 
> convention). That's why it'd be good to augment and/or modify pm_message_t
> to implement the changes, so we wouldn't have to change every single driver
> again.. 

I'll post more patches after I sort out some oddness -- why is swsusp_suspend()
leaving preempt_count() == 1, code I was nowhere near? -- but the patch appended
here shows what I'm pursuing.  Same calling convention, new PRETHAW message
that "pm-naive" drivers (most of them!) can handle just like FREEZE.

Other than this, it affects about 20 files with about 2/3 being drivers; say
that maybe 5% of all in-tree drivers needed trivial changes, most of which
could count as bugfixes _before_ defining the new message.

- Dave

p.s. Note the minor nuance in definition of the "ON" state:  that platforms
     may have "ON" operational states with relevant I/O clocks unavailable,
     so that (by implication!) a driver may not be able to resume() into a
     fully functional state. 



This adds a new pm_message_t event type to use when preparing to switch
into a swsusp snapshot.  Devices that have been initialized by Linux
after resume (rather than left in power-up-reset state) may need to be
reset; this new event type give drivers the chance to do that.

The drivers that will care about this are those which understand more
hardware states than just "on" and "reset", and read the hardware state
during resume().   Hardware state during resume() should be either the
state left by the preceding suspend(), or a power-lost reset.  When
the swsusp freeze/thaw mechanism kicks in, a troublesome third state
could exist:  something set up by a different kernel instance, before
a snapshot image is resumed.  This mechanism helps eliminate that state.

(Later patches start to use these new messages.)


Index: g26/include/linux/pm.h
===================================================================
--- g26.orig/include/linux/pm.h	2006-04-23 00:34:46.000000000 -0700
+++ g26/include/linux/pm.h	2006-05-21 09:19:40.000000000 -0700
@@ -143,29 +143,61 @@ typedef struct pm_message {
 } pm_message_t;
 
 /*
- * There are 4 important states driver can be in:
- * ON     -- driver is working
- * FREEZE -- stop operations and apply whatever policy is applicable to a
- *           suspended driver of that class, freeze queues for block like IDE
- *           does, drop packets for ethernet, etc... stop DMA engine too etc...
- *           so a consistent image can be saved; but do not power any hardware
- *           down.
- * SUSPEND - like FREEZE, but hardware is doing as much powersaving as
- *           possible. Roughly pci D3.
- *
- * Unfortunately, current drivers only recognize numeric values 0 (ON) and 3
- * (SUSPEND).  We'll need to fix the drivers. So yes, putting 3 to all different
- * defines is intentional, and will go away as soon as drivers are fixed.  Also
- * note that typedef is neccessary, we'll probably want to switch to
- *   typedef struct pm_message_t { int event; int flags; } pm_message_t
- * or something similar soon.
+ * Several driver power state transitions are externally visible, affecting
+ * the state of pending I/O queues and (for drivers that touch hardware)
+ * interrupts, wakeups, and other hardware state.  There may also be
+ * internal transitions to various low power modes, which are transparent
+ * to the rest of the driver stack (such as a driver that's ON gating off
+ * clocks which are not in active use).
+ *
+ * One transition is triggered by resume(), after a suspend() call; the
+ * message is implicit:
+ *
+ * ON		Driver starts working again, responding to hardware events
+ * 		and software requests.  The hardware may have gone through
+ * 		a power-off reset, or it may have maintained state from the
+ * 		previous suspend() which the driver will rely on while
+ * 		resuming.  On most platforms, there are no restrictions on
+ * 		availability of resources like clocks during resume().
+ *
+ * Other transitions are triggered by messages sent using suspend().  All
+ * these transitions quiesce the driver, so that I/O queues are inactive.
+ * That commonly entails turning off IRQs and DMA; there may be rules
+ * about how to quiesce that are specific to the bus or the device's type.
+ * (For example, network drivers mark the link state.)  Other details may
+ * differ according to the message:
+ *
+ * SUSPEND	Quiesce, enter a low power device state appropriate for
+ * 		the upcoming system state (such as PCI_D3hot), and enable
+ * 		wakeup events as appropriate.
+ *
+ * FREEZE	Quiesce operations so that a consistent image can be saved;
+ * 		but do NOT otherwise enter a low power device state, and do
+ * 		NOT emit system wakeup events.
+ *
+ * PRETHAW	Quiesce as if for FREEZE; additionally, prepare for restoring
+ * 		the system from a snapshot taken after an earlier FREEZE.
+ * 		Some drivers will need to reset their hardware state instead
+ * 		of preserving it, to ensure that it's never mistaken for the
+ * 		state which that earlier snapshot had set up.
+ *
+ * A minimally power-aware driver treats all messages as SUSPEND, fully
+ * reinitializes its device during resume() -- whether or not it was reset
+ * during the suspend/resume cycle -- and can't issue wakeup events.
+ *
+ * More power-aware drivers may also use low power states at runtime as
+ * well as during system sleep states like PM_SUSPEND_STANDBY.  They may
+ * be able to use wakeup events to exit from runtime low-power states,
+ * or from system low-power states such as standby or suspend-to-RAM.
  */
 
 #define PM_EVENT_ON 0
 #define PM_EVENT_FREEZE 1
 #define PM_EVENT_SUSPEND 2
+#define PM_EVENT_PRETHAW 3
 
 #define PMSG_FREEZE	((struct pm_message){ .event = PM_EVENT_FREEZE, })
+#define PMSG_PRETHAW	((struct pm_message){ .event = PM_EVENT_PRETHAW, })
 #define PMSG_SUSPEND	((struct pm_message){ .event = PM_EVENT_SUSPEND, })
 #define PMSG_ON		((struct pm_message){ .event = PM_EVENT_ON, })
 

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Rafael J. Wysocki]

On Friday 26 May 2006 05:06, David Brownell wrote:
> On Tuesday 02 May 2006 9:12 am, Patrick Mochel wrote:
> > On Thu, Apr 27, 2006 at 12:41:28PM -0700, David Brownell wrote:
> > 
> > > There does seem to be agreement that the current FREEZE invocation is not
> > > sufficient.  I'm looking at a slightly different solution now ... one which
> > > unfortunately involves changing drivers, but can indeed allow swsusp resume
> > > paths to do the right thing (instead of what it does now).
> > 
> > It's Ok if it involves a drive change, so long as its an optional change, which
> > means that it shouldn't affect the interface very much (i.e. the calling 
> > convention). That's why it'd be good to augment and/or modify pm_message_t
> > to implement the changes, so we wouldn't have to change every single driver
> > again.. 
> 
> I'll post more patches after I sort out some oddness -- why is swsusp_suspend()
> leaving preempt_count() == 1, code I was nowhere near? -- but the patch appended
> here shows what I'm pursuing.  Same calling convention, new PRETHAW message
> that "pm-naive" drivers (most of them!) can handle just like FREEZE.

Frankly I thought you'd add a new member to pm_message_t, to be ignored by the
drivers that didn't care.

That said I also see the point in what you're doing. :-)

Greetings,
Rafael

Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

Hi!

> > It's Ok if it involves a drive change, so long as its an optional change, which
> > means that it shouldn't affect the interface very much (i.e. the calling 
> > convention). That's why it'd be good to augment and/or modify pm_message_t
> > to implement the changes, so we wouldn't have to change every single driver
> > again.. 
> 
> I'll post more patches after I sort out some oddness -- why is swsusp_suspend()
> leaving preempt_count() == 1, code I was nowhere near? -- but the patch appended
> here shows what I'm pursuing.  Same calling convention, new PRETHAW message
> that "pm-naive" drivers (most of them!) can handle just like FREEZE.
> 
> Other than this, it affects about 20 files with about 2/3 being drivers; say
> that maybe 5% of all in-tree drivers needed trivial changes, most of which
> could count as bugfixes _before_ defining the new message.

Okay, so you changed the interface, and it needed fixing at 16 places.


> @@ -143,29 +143,61 @@ typedef struct pm_message {
>  } pm_message_t;
>  
>  /*
> - * There are 4 important states driver can be in:
> - * ON     -- driver is working
> - * FREEZE -- stop operations and apply whatever policy is applicable to a
> - *           suspended driver of that class, freeze queues for block like IDE
> - *           does, drop packets for ethernet, etc... stop DMA engine too etc...
> - *           so a consistent image can be saved; but do not power any hardware
> - *           down.
> - * SUSPEND - like FREEZE, but hardware is doing as much powersaving as
> - *           possible. Roughly pci D3.
> - *
> - * Unfortunately, current drivers only recognize numeric values 0 (ON) and 3
> - * (SUSPEND).  We'll need to fix the drivers. So yes, putting 3 to all different
> - * defines is intentional, and will go away as soon as drivers are fixed.  Also
> - * note that typedef is neccessary, we'll probably want to switch to
> - *   typedef struct pm_message_t { int event; int flags; } pm_message_t
> - * or something similar soon.
> + * Several driver power state transitions are externally visible, affecting
> + * the state of pending I/O queues and (for drivers that touch hardware)
> + * interrupts, wakeups, and other hardware state.  There may also be
> + * internal transitions to various low power modes, which are transparent
> + * to the rest of the driver stack (such as a driver that's ON gating off
> + * clocks which are not in active use).
> + *
> + * One transition is triggered by resume(), after a suspend() call; the
> + * message is implicit:
> + *
> + * ON		Driver starts working again, responding to hardware events
> + * 		and software requests.  The hardware may have gone through
> + * 		a power-off reset, or it may have maintained state from the
> + * 		previous suspend() which the driver will rely on while
> + * 		resuming.  On most platforms, there are no restrictions on
> + * 		availability of resources like clocks during resume().
> + *
> + * Other transitions are triggered by messages sent using suspend().  All
> + * these transitions quiesce the driver, so that I/O queues are inactive.
> + * That commonly entails turning off IRQs and DMA; there may be rules
> + * about how to quiesce that are specific to the bus or the device's type.
> + * (For example, network drivers mark the link state.)  Other details may
> + * differ according to the message:
> + *
> + * SUSPEND	Quiesce, enter a low power device state appropriate for
> + * 		the upcoming system state (such as PCI_D3hot), and enable
> + * 		wakeup events as appropriate.
> + *
> + * FREEZE	Quiesce operations so that a consistent image can be saved;
> + * 		but do NOT otherwise enter a low power device state, and do
> + * 		NOT emit system wakeup events.
> + *
> + * PRETHAW	Quiesce as if for FREEZE; additionally, prepare for restoring
> + * 		the system from a snapshot taken after an earlier FREEZE.
> + * 		Some drivers will need to reset their hardware state instead
> + * 		of preserving it, to ensure that it's never mistaken for the
> + * 		state which that earlier snapshot had set up.

And you *do* realize that PRETHAW is like a FREEZE... So... can we use
FREEZE, and add aditional flag field that says it is preTHAW?

This will result in if (message == FREEZE || message == PRETHAW) that
is kind-of ugly.

Now, you are right that you only needed to fix 16 places, so it is not
_that_ bad...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-usb-devel] Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Friday 26 May 2006 4:16 pm, Pavel Machek wrote:
> 
> > 		 the patch appended
> > here shows what I'm pursuing.  Same calling convention, new PRETHAW message
> > that "pm-naive" drivers (most of them!) can handle just like FREEZE.
> > 
> > Other than this, it affects about 20 files with about 2/3 being drivers; say
> > that maybe 5% of all in-tree drivers needed trivial changes, most of which
> > could count as bugfixes _before_ defining the new message.
> 
> Okay, so you changed the interface, and it needed fixing at 16 places.

Not what I said ... in many of those places, the driver code was already dubious.
Making the event code __bitwise would have helped catch those errors.


> > + * Other transitions are triggered by messages sent using suspend().  All
> > + * these transitions quiesce the driver, so that I/O queues are inactive.
> > + * That commonly entails turning off IRQs and DMA; there may be rules
> > + * about how to quiesce that are specific to the bus or the device's type.
> > + * (For example, network drivers mark the link state.)  Other details may
> > + * differ according to the message:
> > + *
> > + * SUSPEND	Quiesce, enter a low power device state appropriate for
> > + * 		the upcoming system state (such as PCI_D3hot), and enable
> > + * 		wakeup events as appropriate.
> > + *
> > + * FREEZE	Quiesce operations so that a consistent image can be saved;
> > + * 		but do NOT otherwise enter a low power device state, and do
> > + * 		NOT emit system wakeup events.
> > + *
> > + * PRETHAW	Quiesce as if for FREEZE; additionally, prepare for restoring
> > + * 		the system from a snapshot taken after an earlier FREEZE.
> > + * 		Some drivers will need to reset their hardware state instead
> > + * 		of preserving it, to ensure that it's never mistaken for the
> > + * 		state which that earlier snapshot had set up.
> 
> And you *do* realize that PRETHAW is like a FREEZE... So... can we use
> FREEZE, and add aditional flag field that says it is preTHAW?

Of course, FREEZE is like a SUSPEND, and SUSPEND is the only behavior that's
required of all drivers, or which most drivers understand.  So why did you not
implement FREEZE as a flag in the first place, if you like that model?  :)

Most drivers treat all suspend() messages as SUSPEND anyway; mesg.event was
already little more than a fancy flag (SUSPEND vs FREEZE).  We only need
three transition types (so far), not four (flag x flag ==> 4 states).

Plus, I'd never add flags to that structure.  I've elaborated some of the reasons
why not in the past, and I won't repeat that here; it's basically a bad idea,
wastes data and code space, is an error prone and ugly state machine design
practice, and so forth.


> This will result in if (message == FREEZE || message == PRETHAW) that
> is kind-of ugly.

switch (mesg.event) { ... } for the few drivers that actually care about
more than one of the transitions.  Only a handful need to care about more
than whether it's a real SUSPEND or not.

- Dave

Re: [linux-usb-devel] Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek]

Hi!

> > > 		 the patch appended
> > > here shows what I'm pursuing.  Same calling convention, new PRETHAW message
> > > that "pm-naive" drivers (most of them!) can handle just like FREEZE.
> > > 
> > > Other than this, it affects about 20 files with about 2/3 being drivers; say
> > > that maybe 5% of all in-tree drivers needed trivial changes, most of which
> > > could count as bugfixes _before_ defining the new message.
> > 
> > Okay, so you changed the interface, and it needed fixing at 16 places.
> 
> Not what I said ... in many of those places, the driver code was already dubious.
> Making the event code __bitwise would have helped catch those errors.

Yep, __bitwise (+ someone using sparse :-) would be nice.

> > > + * PRETHAW	Quiesce as if for FREEZE; additionally, prepare for restoring
> > > + * 		the system from a snapshot taken after an earlier FREEZE.
> > > + * 		Some drivers will need to reset their hardware state instead
> > > + * 		of preserving it, to ensure that it's never mistaken for the
> > > + * 		state which that earlier snapshot had set up.
> > 
> > And you *do* realize that PRETHAW is like a FREEZE... So... can we use
> > FREEZE, and add aditional flag field that says it is preTHAW?
> 
> Of course, FREEZE is like a SUSPEND, and SUSPEND is the only behavior that's
> required of all drivers, or which most drivers understand.  So why did you not
> implement FREEZE as a flag in the first place, if you like that model?  :)
> 
> Most drivers treat all suspend() messages as SUSPEND anyway; mesg.event was
> already little more than a fancy flag (SUSPEND vs FREEZE).  We only need
> three transition types (so far), not four (flag x flag ==> 4 states).

Okay, I still don't 100% like it, but it makes some sense and I think
we can live with PREFREEZE.

> > This will result in if (message == FREEZE || message == PRETHAW) that
> > is kind-of ugly.
> 
> switch (mesg.event) { ... } for the few drivers that actually care about
> more than one of the transitions.  Only a handful need to care about more
> than whether it's a real SUSPEND or not.

Okay, you are right that suspend and freeze are similar, too. Can you
retransmit the patch?
							Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: [patch/rft 2.6.17-rc5-git 2/6] add PM_EVENT_PRETHAW

[Pavel Machek]

Hi!

Looks mostly good...

> This adds a new pm_message_t event type to use when preparing to switch
> into a swsusp snapshot.  Devices that have been initialized by Linux
> after resume (rather than left in power-up-reset state) may need to be
> reset; this new event type gives drivers the chance to do that.
> 
> The drivers that will care about this are those which understand more
> hardware states than just "on" and "reset", and read the hardware state
> during resume().   Hardware state during resume() should be either the
> state left by the preceding suspend(), or a power-lost reset.  When
> the swsusp freeze/thaw mechanism kicks in, a troublesome third state
> could exist:  one state set up by a different kernel instance, before
> a snapshot image is resumed.  This mechanism helps eliminate that state.

...should this go to Documentation/ somewhere?

> + * ON		Driver starts working again, responding to hardware events
> + * 		and software requests.  The hardware may have gone through
> + * 		a power-off reset, or it may have maintained state from the
> + * 		previous suspend() which the driver will rely on while
> + * 		resuming.  On most platforms, there are no restrictions on
> + * 		availability of resources like clocks during resume().
> + *
> + * Other transitions are triggered by messages sent using suspend().  All
> + * these transitions quiesce the driver, so that I/O queues are inactive.
> + * That commonly entails turning off IRQs and DMA; there may be rules

Actually all DMAs must always be turned off. Otherwise swsusp can't
get consistent image. IRQs can be disabled or not (they usually are), however driver
prefers... but DMA must be off.
						Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: [patch/rft 2.6.17-rc5-git 3/6] PM_EVENT_PRETHAW, handle in IDE and PCI

[Pavel Machek]

Hi!

> @@ -1227,7 +1227,9 @@ static int generic_ide_suspend(struct de
>  	rq.special = &args;
>  	rq.pm = &rqpm;
>  	rqpm.pm_step = ide_pm_state_start_suspend;
> -	rqpm.pm_state = state.event;
> +	if (mesg.event == PM_EVENT_PRETHAW)
> +		mesg.event = PM_EVENT_FREEZE;
> +	rqpm.pm_state = mesg.event;
> 

Actually it would be nicer not to modify mesg.event... And perhaps I'd
move this check lower -- to be consistent with 'low level driver gets
full info' idea. (Or is it checked at too many places?)

-- 
Thanks for all the (sleeping) penguins.

Re: [patch/rft 2.6.17-rc5-git 6/6] PM_EVENT_PRETHAW, issue from PM core

[Pavel Machek]

On Mon 05-06-06 09:38:49, David Brownell wrote:
> This converts the PM core to issue the new PRETHAW message.  The rest of
> the kernel is now ready to receive these, courtesy of preceding patches.


ACK.

-- 
Thanks for all the (sleeping) penguins.

Re: [patch/rft 2.6.17-rc5-git 4/6] PM_EVENT_PRETHAW for various graphics cards

[Pavel Machek]

Hi!

>  #ifdef CONFIG_PM
> -static int nvidiafb_suspend(struct pci_dev *dev, pm_message_t state)
> +static int nvidiafb_suspend(struct pci_dev *dev, pm_message_t mesg)
>  {
>  	struct fb_info *info = pci_get_drvdata(dev);
>  	struct nvidia_par *par = info->par;
>  
> +	if (mesg.event == PM_EVENT_PRETHAW)
> +		mesg.event = PM_EVENT_FREEZE;
>  	acquire_console_sem();
> -	par->pm_state = state.event;
> +	par->pm_state = mesg.event;
>  
> -	if (state.event == PM_EVENT_FREEZE) {
> -		dev->dev.power.power_state = state;
> -	} else {
> +	if (mesg.event == PM_EVENT_SUSPEND) {
>  		fb_set_suspend(info, 1);
>  		nvidiafb_blank(FB_BLANK_POWERDOWN, info);
>  		nvidia_write_regs(par, &par->SavedReg);
>  		pci_save_state(dev);
>  		pci_disable_device(dev);
> -		pci_set_power_state(dev, pci_choose_state(dev, state));
> +		pci_set_power_state(dev, pci_choose_state(dev, mesg));
>  	}
> +	dev->dev.power.power_state = mesg;

If PRETHAW event is sent, we still store FREEZE into power_state. That
is not consistent with other drivers.

> Index: g26/drivers/video/savage/savagefb_driver.c
> ===================================================================
> --- g26.orig/drivers/video/savage/savagefb_driver.c	2006-06-02 18:08:30.000000000 -0700
> +++ g26/drivers/video/savage/savagefb_driver.c	2006-06-02 18:11:27.000000000 -0700
> @@ -2151,24 +2151,24 @@ static void __devexit savagefb_remove (s
>  	}
>  }
>  
> -static int savagefb_suspend (struct pci_dev* dev, pm_message_t state)
> +static int savagefb_suspend (struct pci_dev* dev, pm_message_t mesg)
>  {
>  	struct fb_info *info = pci_get_drvdata(dev);
>  	struct savagefb_par *par = info->par;
>  
>  	DBG("savagefb_suspend");
>  
> -
> -	par->pm_state = state.event;
> +	if (mesg.event == PM_EVENT_PRETHAW)
> +		mesg.event = PM_EVENT_FREEZE;
> +	par->pm_state = mesg.event;
> +	dev->dev.power.power_state = mesg;

Same here.
						Pavel

-- 
Thanks for all the (sleeping) penguins.

Re: [linux-usb-devel] Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[David Brownell]

On Saturday 27 May 2006 9:38 am, Pavel Machek wrote:

> Okay, you are right that suspend and freeze are similar, too. Can you
> retransmit the patch?

What I'll do is post the whole series, of which that one interface
patch was just the interface change that I thought was worth a bit
of separate discussion.  They'll be in the next messages from me,
against 2.6.17-rc5-git11 or so ... going only to linux-pm, but I'll
send a note with archive URLs to linux-usb-devel.

- Dave

[patch/rft 2.6.17-rc5-git 0/6] PM_EVENT_PRETHAW

[David Brownell]

Following this message will be patches adding the new PRETHAW event,
so that drivers can make sure that swsusp doesn't put hardware into
bogus states before resuming.

As previously discussed, this is needed by drivers which examine the
hardware state during resume() methods ... notably, USB controller
drivers, which expect to see true suspend states in order to handle
remote wakeup, but currently break with swsusp.  Only two states are
valid in resume():  after hardware reset, or else the state that
suspend() left it in.  PRETHAW allows drivers to force the former.

In sequence, the patches are:

 - prethaw-misc.patch ... fixes code that's currently broken/dubious
   so that it won't care about the new message (and is thus more or
   less mergeable regardless of the rest of these patches)

 - prethaw-header.patch ... defines the new event and its message

 - prethaw-fw.patch ... IDE and (dumb) PCI can handle it simplistically

 - prethaw-video.patch ... likewise, but this is per-driver not at the
   framework level

 - prethaw-usb.patch ... fixing various "swsusp resume fails if the
   HCD is statically linked" bugs

 - prethaw-core.patch ... updating the pm core to issue PRETHAW
   events, handling for which which the previous patches added.

Yes, it might be worth splitting some of those driver patches out into
patch-per-driver format.  Yes, I _did_ look at a couple hundred drivers
(grr) to see what needed changing ... darn few drivers treat suspend() as
anything other than PM_EVENT_SUSPEND.

- Dave

[patch/rft 2.6.17-rc5-git 1/6] fix broken/dubious driver suspend() methods

[David Brownell]

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

Small driver suspend() updates in preparation for the upcoming FREEZE message;

 - Only compare message events for equality against PM_EVENT_* codes;
   not against integers, or using greater/less-than comparisons.
   (PM_EVENT_* should really become a __bitwise thing.)

 - Explicitly test for SUSPEND events (rather than not-something-else)
   before suspending devices.

 - Removes more of the confusion between a pm_message_t (wraps event code)
   and a "state" ... suspend() originally took a target system state.

These updates are correct and appropriate even without new PM_EVENT codes.
---
 drivers/ide/ppc/pmac.c                  |   14 ++++++++------
 drivers/media/dvb/cinergyT2/cinergyT2.c |    2 +-
 drivers/scsi/libata-core.c              |    6 +++---
 drivers/scsi/mesh.c                     |    7 ++++---
 4 files changed, 16 insertions(+), 13 deletions(-)


[-- Attachment #2: prethaw-misc.patch --]
[-- Type: text/x-diff, Size: 5121 bytes --]

Small driver suspend() updates in preparation for the upcoming FREEZE message;

 - Only compare message events for equality against PM_EVENT_* codes;
   not against integers, or using greater/less-than comparisons.
   (PM_EVENT_* should really become a __bitwise thing.)

 - Explicitly test for SUSPEND events (rather than not-something-else)
   before suspending devices.
 
 - Removes more of the confusion between a pm_message_t (wraps event code)
   and a "state" ... suspend() originally took a target system state.

These updates are correct and appropriate even without new PM_EVENT codes.

---
 drivers/ide/ppc/pmac.c                  |   14 ++++++++------
 drivers/media/dvb/cinergyT2/cinergyT2.c |    2 +-
 drivers/scsi/libata-core.c              |    6 +++---
 drivers/scsi/mesh.c                     |    7 ++++---
 4 files changed, 16 insertions(+), 13 deletions(-)

Index: scratch/drivers/ide/ppc/pmac.c
===================================================================
--- scratch.orig/drivers/ide/ppc/pmac.c	2006-06-05 09:06:00.000000000 -0700
+++ scratch/drivers/ide/ppc/pmac.c	2006-06-05 09:19:13.000000000 -0700
@@ -1495,15 +1495,16 @@ pmac_ide_macio_attach(struct macio_dev *
 }
 
 static int
-pmac_ide_macio_suspend(struct macio_dev *mdev, pm_message_t state)
+pmac_ide_macio_suspend(struct macio_dev *mdev, pm_message_t mesg)
 {
 	ide_hwif_t	*hwif = (ide_hwif_t *)dev_get_drvdata(&mdev->ofdev.dev);
 	int		rc = 0;
 
-	if (state.event != mdev->ofdev.dev.power.power_state.event && state.event >= PM_EVENT_SUSPEND) {
+	if (mesg.event != mdev->ofdev.dev.power.power_state.event
+			&& mesg.event == PM_EVENT_SUSPEND) {
 		rc = pmac_ide_do_suspend(hwif);
 		if (rc == 0)
-			mdev->ofdev.dev.power.power_state = state;
+			mdev->ofdev.dev.power.power_state = mesg;
 	}
 
 	return rc;
@@ -1599,15 +1600,16 @@ pmac_ide_pci_attach(struct pci_dev *pdev
 }
 
 static int
-pmac_ide_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+pmac_ide_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
 	ide_hwif_t	*hwif = (ide_hwif_t *)pci_get_drvdata(pdev);
 	int		rc = 0;
 	
-	if (state.event != pdev->dev.power.power_state.event && state.event >= 2) {
+	if (mesg.event != pdev->dev.power.power_state.event
+			&& mesg.event == PM_EVENT_SUSPEND) {
 		rc = pmac_ide_do_suspend(hwif);
 		if (rc == 0)
-			pdev->dev.power.power_state = state;
+			pdev->dev.power.power_state = mesg;
 	}
 
 	return rc;
Index: scratch/drivers/scsi/libata-core.c
===================================================================
--- scratch.orig/drivers/scsi/libata-core.c	2006-06-05 09:06:16.000000000 -0700
+++ scratch/drivers/scsi/libata-core.c	2006-06-05 09:19:13.000000000 -0700
@@ -4313,19 +4313,19 @@ int ata_device_resume(struct ata_port *a
  *	ata_device_suspend - prepare a device for suspend
  *	@ap: port the device is connected to
  *	@dev: the device to suspend
- *	@state: target power management state
+ *	@mesg: power management request
  *
  *	Flush the cache on the drive, if appropriate, then issue a
  *	standbynow command.
  */
-int ata_device_suspend(struct ata_port *ap, struct ata_device *dev, pm_message_t state)
+int ata_device_suspend(struct ata_port *ap, struct ata_device *dev, pm_message_t mesg)
 {
 	if (!ata_dev_present(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
 		ata_flush_cache(ap, dev);
 
-	if (state.event != PM_EVENT_FREEZE)
+	if (mesg.event == PM_EVENT_SUSPEND)
 		ata_standby_drive(ap, dev);
 	ap->flags |= ATA_FLAG_SUSPENDED;
 	return 0;
Index: scratch/drivers/scsi/mesh.c
===================================================================
--- scratch.orig/drivers/scsi/mesh.c	2006-06-05 09:06:16.000000000 -0700
+++ scratch/drivers/scsi/mesh.c	2006-06-05 09:19:13.000000000 -0700
@@ -1761,12 +1761,13 @@ static void set_mesh_power(struct mesh_s
 
 
 #ifdef CONFIG_PM
-static int mesh_suspend(struct macio_dev *mdev, pm_message_t state)
+static int mesh_suspend(struct macio_dev *mdev, pm_message_t mesg)
 {
 	struct mesh_state *ms = (struct mesh_state *)macio_get_drvdata(mdev);
 	unsigned long flags;
 
-	if (state.event == mdev->ofdev.dev.power.power_state.event || state.event < 2)
+	if (mesg.event == mdev->ofdev.dev.power.power_state.event
+			|| mesg.event != PM_EVENT_SUSPEND)
 		return 0;
 
 	scsi_block_requests(ms->host);
@@ -1781,7 +1782,7 @@ static int mesh_suspend(struct macio_dev
 	disable_irq(ms->meshintr);
 	set_mesh_power(ms, 0);
 
-	mdev->ofdev.dev.power.power_state = state;
+	mdev->ofdev.dev.power.power_state = mesg;
 
 	return 0;
 }
Index: scratch/drivers/media/dvb/cinergyT2/cinergyT2.c
===================================================================
--- scratch.orig/drivers/media/dvb/cinergyT2/cinergyT2.c	2006-06-05 09:06:01.000000000 -0700
+++ scratch/drivers/media/dvb/cinergyT2/cinergyT2.c	2006-06-05 09:19:13.000000000 -0700
@@ -978,7 +978,7 @@ static int cinergyt2_suspend (struct usb
 	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
 		return -ERESTARTSYS;
 
-	if (state.event > PM_EVENT_ON) {
+	if (1) {
 		struct cinergyt2 *cinergyt2 = usb_get_intfdata (intf);
 
 		cinergyt2_suspend_rc(cinergyt2);

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch/rft 2.6.17-rc5-git 2/6] add PM_EVENT_PRETHAW

[David Brownell]

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

This adds a new pm_message_t event type to use when preparing to switch
into a swsusp snapshot.  Devices that have been initialized by Linux
after resume (rather than left in power-up-reset state) may need to be
reset; this new event type gives drivers the chance to do that.

The drivers that will care about this are those which understand more
hardware states than just "on" and "reset", and read the hardware state
during resume().   Hardware state during resume() should be either the
state left by the preceding suspend(), or a power-lost reset.  When
the swsusp freeze/thaw mechanism kicks in, a troublesome third state
could exist:  one state set up by a different kernel instance, before
a snapshot image is resumed.  This mechanism helps eliminate that state.

(In particular, without the mechanism provided by this patch series,
USB host controller drivers which are statically linked will misbehave
badly during swsusp resume, since swsusp now forces those controllers
into that bogus third state.)
---
 include/linux/pm.h |   64 +++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 48 insertions(+), 16 deletions(-)


[-- Attachment #2: prethaw-header.patch --]
[-- Type: text/x-diff, Size: 5314 bytes --]

This adds a new pm_message_t event type to use when preparing to switch
into a swsusp snapshot.  Devices that have been initialized by Linux
after resume (rather than left in power-up-reset state) may need to be
reset; this new event type gives drivers the chance to do that.

The drivers that will care about this are those which understand more
hardware states than just "on" and "reset", and read the hardware state
during resume().   Hardware state during resume() should be either the
state left by the preceding suspend(), or a power-lost reset.  When
the swsusp freeze/thaw mechanism kicks in, a troublesome third state
could exist:  one state set up by a different kernel instance, before
a snapshot image is resumed.  This mechanism helps eliminate that state.

(In particular, without the mechanism provided by this patch series,
USB host controller drivers which are statically linked will misbehave
badly during swsusp resume, since swsusp now forces those controllers
into that bogus third state.)

---
 include/linux/pm.h |   64 +++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 48 insertions(+), 16 deletions(-)

Index: g26/include/linux/pm.h
===================================================================
--- g26.orig/include/linux/pm.h	2006-06-02 18:08:30.000000000 -0700
+++ g26/include/linux/pm.h	2006-06-02 18:11:26.000000000 -0700
@@ -143,29 +143,61 @@ typedef struct pm_message {
 } pm_message_t;
 
 /*
- * There are 4 important states driver can be in:
- * ON     -- driver is working
- * FREEZE -- stop operations and apply whatever policy is applicable to a
- *           suspended driver of that class, freeze queues for block like IDE
- *           does, drop packets for ethernet, etc... stop DMA engine too etc...
- *           so a consistent image can be saved; but do not power any hardware
- *           down.
- * SUSPEND - like FREEZE, but hardware is doing as much powersaving as
- *           possible. Roughly pci D3.
- *
- * Unfortunately, current drivers only recognize numeric values 0 (ON) and 3
- * (SUSPEND).  We'll need to fix the drivers. So yes, putting 3 to all different
- * defines is intentional, and will go away as soon as drivers are fixed.  Also
- * note that typedef is neccessary, we'll probably want to switch to
- *   typedef struct pm_message_t { int event; int flags; } pm_message_t
- * or something similar soon.
+ * Several driver power state transitions are externally visible, affecting
+ * the state of pending I/O queues and (for drivers that touch hardware)
+ * interrupts, wakeups, and other hardware state.  There may also be
+ * internal transitions to various low power modes, which are transparent
+ * to the rest of the driver stack (such as a driver that's ON gating off
+ * clocks which are not in active use).
+ *
+ * One transition is triggered by resume(), after a suspend() call; the
+ * message is implicit:
+ *
+ * ON		Driver starts working again, responding to hardware events
+ * 		and software requests.  The hardware may have gone through
+ * 		a power-off reset, or it may have maintained state from the
+ * 		previous suspend() which the driver will rely on while
+ * 		resuming.  On most platforms, there are no restrictions on
+ * 		availability of resources like clocks during resume().
+ *
+ * Other transitions are triggered by messages sent using suspend().  All
+ * these transitions quiesce the driver, so that I/O queues are inactive.
+ * That commonly entails turning off IRQs and DMA; there may be rules
+ * about how to quiesce that are specific to the bus or the device's type.
+ * (For example, network drivers mark the link state.)  Other details may
+ * differ according to the message:
+ *
+ * SUSPEND	Quiesce, enter a low power device state appropriate for
+ * 		the upcoming system state (such as PCI_D3hot), and enable
+ * 		wakeup events as appropriate.
+ *
+ * FREEZE	Quiesce operations so that a consistent image can be saved;
+ * 		but do NOT otherwise enter a low power device state, and do
+ * 		NOT emit system wakeup events.
+ *
+ * PRETHAW	Quiesce as if for FREEZE; additionally, prepare for restoring
+ * 		the system from a snapshot taken after an earlier FREEZE.
+ * 		Some drivers will need to reset their hardware state instead
+ * 		of preserving it, to ensure that it's never mistaken for the
+ * 		state which that earlier snapshot had set up.
+ *
+ * A minimally power-aware driver treats all messages as SUSPEND, fully
+ * reinitializes its device during resume() -- whether or not it was reset
+ * during the suspend/resume cycle -- and can't issue wakeup events.
+ *
+ * More power-aware drivers may also use low power states at runtime as
+ * well as during system sleep states like PM_SUSPEND_STANDBY.  They may
+ * be able to use wakeup events to exit from runtime low-power states,
+ * or from system low-power states such as standby or suspend-to-RAM.
  */
 
 #define PM_EVENT_ON 0
 #define PM_EVENT_FREEZE 1
 #define PM_EVENT_SUSPEND 2
+#define PM_EVENT_PRETHAW 3
 
 #define PMSG_FREEZE	((struct pm_message){ .event = PM_EVENT_FREEZE, })
+#define PMSG_PRETHAW	((struct pm_message){ .event = PM_EVENT_PRETHAW, })
 #define PMSG_SUSPEND	((struct pm_message){ .event = PM_EVENT_SUSPEND, })
 #define PMSG_ON		((struct pm_message){ .event = PM_EVENT_ON, })
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch/rft 2.6.17-rc5-git 3/6] PM_EVENT_PRETHAW, handle in IDE and PCI

[David Brownell]

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

Convert some framework code to handle the new PRETHAW message.

  - IDE just treats it like a FREEZE.

  - The pci_choose_state() thingie still doesn't use PCI_D0 when it gets a
    FREEZE (and now PRETHAW) event, which seems rather buglike but wasn't
    something to change with this patch.

Of course PCI drivers don't need to use pci_choose_state().
---
 drivers/ide/ide.c |    6 ++++--
 drivers/pci/pci.c |    4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)


[-- Attachment #2: prethaw-fw.patch --]
[-- Type: text/x-diff, Size: 1905 bytes --]

Convert some framework code to handle the new PRETHAW message.

  - IDE just treats it like a FREEZE.

  - The pci_choose_state() thingie still doesn't use PCI_D0 when it gets a
    FREEZE (and now PRETHAW) event, which seems rather buglike but wasn't
    something to change with this patch.

---
 drivers/ide/ide.c |    6 ++++--
 drivers/pci/pci.c |    4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

Index: g26/drivers/ide/ide.c
===================================================================
--- g26.orig/drivers/ide/ide.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/ide/ide.c	2006-06-02 18:11:27.000000000 -0700
@@ -1213,7 +1213,7 @@ int system_bus_clock (void)
 
 EXPORT_SYMBOL(system_bus_clock);
 
-static int generic_ide_suspend(struct device *dev, pm_message_t state)
+static int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 {
 	ide_drive_t *drive = dev->driver_data;
 	struct request rq;
@@ -1227,7 +1227,9 @@ static int generic_ide_suspend(struct de
 	rq.special = &args;
 	rq.pm = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_suspend;
-	rqpm.pm_state = state.event;
+	if (mesg.event == PM_EVENT_PRETHAW)
+		mesg.event = PM_EVENT_FREEZE;
+	rqpm.pm_state = mesg.event;
 
 	return ide_do_drive_cmd(drive, &rq, ide_wait);
 }
Index: g26/drivers/pci/pci.c
===================================================================
--- g26.orig/drivers/pci/pci.c	2006-06-02 18:11:20.000000000 -0700
+++ g26/drivers/pci/pci.c	2006-06-02 18:11:27.000000000 -0700
@@ -424,10 +424,12 @@ pci_power_t pci_choose_state(struct pci_
 	case PM_EVENT_ON:
 		return PCI_D0;
 	case PM_EVENT_FREEZE:
+	case PM_EVENT_PRETHAW:
+		/* REVISIT both freeze and pre-thaw "should" use D0 */
 	case PM_EVENT_SUSPEND:
 		return PCI_D3hot;
 	default:
-		printk("They asked me for state %d\n", state.event);
+		printk("Unrecognized suspend event %d\n", state.event);
 		BUG();
 	}
 	return PCI_D0;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch/rft 2.6.17-rc5-git 4/6] PM_EVENT_PRETHAW for various graphics cards

[David Brownell]

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

Video drivers which explicitly test for messages reporting
PM_EVENT_FREEZE will now handle PM_EVENT_PRETHAW the same way.
---
 drivers/video/aty/radeon_pm.c          |   15 +++++++++------
 drivers/video/i810/i810_main.c         |   12 +++++++-----
 drivers/video/nvidia/nvidia.c          |   13 +++++++------
 drivers/video/savage/savagefb_driver.c |   14 +++++++-------
 4 files changed, 30 insertions(+), 24 deletions(-)



[-- Attachment #2: prethaw-video.patch --]
[-- Type: text/x-diff, Size: 5973 bytes --]

Video drivers which explicitly test for messages reporting
PM_EVENT_FREEZE will now handle PM_EVENT_PRETHAW the same way.

---
 drivers/video/aty/radeon_pm.c          |   15 +++++++++------
 drivers/video/i810/i810_main.c         |   12 +++++++-----
 drivers/video/nvidia/nvidia.c          |   13 +++++++------
 drivers/video/savage/savagefb_driver.c |   14 +++++++-------
 4 files changed, 30 insertions(+), 24 deletions(-)


Index: g26/drivers/video/aty/radeon_pm.c
===================================================================
--- g26.orig/drivers/video/aty/radeon_pm.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/video/aty/radeon_pm.c	2006-06-02 18:11:27.000000000 -0700
@@ -2520,25 +2520,28 @@ static int radeon_restore_pci_cfg(struct
 }
 
 
-int radeonfb_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+int radeonfb_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
         struct fb_info *info = pci_get_drvdata(pdev);
         struct radeonfb_info *rinfo = info->par;
 	int i;
 
-	if (state.event == pdev->dev.power.power_state.event)
+	if (mesg.event == pdev->dev.power.power_state.event)
 		return 0;
 
-	printk(KERN_DEBUG "radeonfb (%s): suspending to state: %d...\n",
-	       pci_name(pdev), state.event);
+	printk(KERN_DEBUG "radeonfb (%s): suspending for event: %d...\n",
+	       pci_name(pdev), mesg.event);
 
 	/* For suspend-to-disk, we cheat here. We don't suspend anything and
 	 * let fbcon continue drawing until we are all set. That shouldn't
 	 * really cause any problem at this point, provided that the wakeup
 	 * code knows that any state in memory may not match the HW
 	 */
-	if (state.event == PM_EVENT_FREEZE)
+	switch (mesg.event) {
+	case PM_EVENT_FREEZE:		/* about to take snapshot */
+	case PM_EVENT_PRETHAW:		/* before restoring snapshot */
 		goto done;
+	}
 
 	acquire_console_sem();
 
@@ -2605,7 +2608,7 @@ int radeonfb_pci_suspend(struct pci_dev 
 	release_console_sem();
 
  done:
-	pdev->dev.power.power_state = state;
+	pdev->dev.power.power_state = mesg;
 
 	return 0;
 }
Index: g26/drivers/video/i810/i810_main.c
===================================================================
--- g26.orig/drivers/video/i810/i810_main.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/video/i810/i810_main.c	2006-06-02 18:11:27.000000000 -0700
@@ -1556,15 +1556,17 @@ static struct fb_ops i810fb_ops __devini
 /***********************************************************************
  *                         Power Management                            *
  ***********************************************************************/
-static int i810fb_suspend(struct pci_dev *dev, pm_message_t state)
+static int i810fb_suspend(struct pci_dev *dev, pm_message_t mesg)
 {
 	struct fb_info *info = pci_get_drvdata(dev);
 	struct i810fb_par *par = info->par;
 
-	par->cur_state = state.event;
+	par->cur_state = mesg.event;
 
-	if (state.event == PM_EVENT_FREEZE) {
-		dev->dev.power.power_state = state;
+	switch (mesg.event) {
+	case PM_EVENT_FREEZE:
+	case PM_EVENT_PRETHAW:
+		dev->dev.power.power_state = mesg;
 		return 0;
 	}
 
@@ -1580,7 +1582,7 @@ static int i810fb_suspend(struct pci_dev
 
 	pci_save_state(dev);
 	pci_disable_device(dev);
-	pci_set_power_state(dev, pci_choose_state(dev, state));
+	pci_set_power_state(dev, pci_choose_state(dev, mesg));
 	release_console_sem();
 
 	return 0;
Index: g26/drivers/video/nvidia/nvidia.c
===================================================================
--- g26.orig/drivers/video/nvidia/nvidia.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/video/nvidia/nvidia.c	2006-06-02 18:11:27.000000000 -0700
@@ -1381,24 +1381,25 @@ static struct fb_ops nvidia_fb_ops = {
 };
 
 #ifdef CONFIG_PM
-static int nvidiafb_suspend(struct pci_dev *dev, pm_message_t state)
+static int nvidiafb_suspend(struct pci_dev *dev, pm_message_t mesg)
 {
 	struct fb_info *info = pci_get_drvdata(dev);
 	struct nvidia_par *par = info->par;
 
+	if (mesg.event == PM_EVENT_PRETHAW)
+		mesg.event = PM_EVENT_FREEZE;
 	acquire_console_sem();
-	par->pm_state = state.event;
+	par->pm_state = mesg.event;
 
-	if (state.event == PM_EVENT_FREEZE) {
-		dev->dev.power.power_state = state;
-	} else {
+	if (mesg.event == PM_EVENT_SUSPEND) {
 		fb_set_suspend(info, 1);
 		nvidiafb_blank(FB_BLANK_POWERDOWN, info);
 		nvidia_write_regs(par, &par->SavedReg);
 		pci_save_state(dev);
 		pci_disable_device(dev);
-		pci_set_power_state(dev, pci_choose_state(dev, state));
+		pci_set_power_state(dev, pci_choose_state(dev, mesg));
 	}
+	dev->dev.power.power_state = mesg;
 
 	release_console_sem();
 	return 0;
Index: g26/drivers/video/savage/savagefb_driver.c
===================================================================
--- g26.orig/drivers/video/savage/savagefb_driver.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/video/savage/savagefb_driver.c	2006-06-02 18:11:27.000000000 -0700
@@ -2151,24 +2151,24 @@ static void __devexit savagefb_remove (s
 	}
 }
 
-static int savagefb_suspend (struct pci_dev* dev, pm_message_t state)
+static int savagefb_suspend (struct pci_dev* dev, pm_message_t mesg)
 {
 	struct fb_info *info = pci_get_drvdata(dev);
 	struct savagefb_par *par = info->par;
 
 	DBG("savagefb_suspend");
 
-
-	par->pm_state = state.event;
+	if (mesg.event == PM_EVENT_PRETHAW)
+		mesg.event = PM_EVENT_FREEZE;
+	par->pm_state = mesg.event;
+	dev->dev.power.power_state = mesg;
 
 	/*
 	 * For PM_EVENT_FREEZE, do not power down so the console
 	 * can remain active.
 	 */
-	if (state.event == PM_EVENT_FREEZE) {
-		dev->dev.power.power_state = state;
+	if (mesg.event == PM_EVENT_FREEZE)
 		return 0;
-	}
 
 	acquire_console_sem();
 	fb_set_suspend(info, 1);
@@ -2180,7 +2180,7 @@ static int savagefb_suspend (struct pci_
 	savage_disable_mmio(par);
 	pci_save_state(dev);
 	pci_disable_device(dev);
-	pci_set_power_state(dev, pci_choose_state(dev, state));
+	pci_set_power_state(dev, pci_choose_state(dev, mesg));
 	release_console_sem();
 
 	return 0;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch/rft 2.6.17-rc5-git 5/6] PM_EVENT_PRETHAW, handle for USB

[David Brownell]

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

This teaches several USB host controller drivers to treat PRETHAW as a
chip reset since the controller, and all devices connected to it, are no
longer in states compatible with how the snapshotted suspend() left them.
---
 drivers/usb/core/hcd-pci.c   |    2 +-
 drivers/usb/core/usb.c       |    2 +-
 drivers/usb/host/ehci-pci.c  |    6 ++++++
 drivers/usb/host/ohci-pci.c  |    5 +++++
 drivers/usb/host/sl811-hcd.c |    9 +++++++--
 drivers/usb/host/uhci-hcd.c  |    4 ++++
 6 files changed, 24 insertions(+), 4 deletions(-)


[-- Attachment #2: prethaw-usb.patch --]
[-- Type: text/x-diff, Size: 4357 bytes --]

This teaches several USB host controller drivers to treat PRETHAW as a
chip reset since the controller, and all devices connected to it, are no
longer in states compatible with how the snapshotted suspend() left them.

---
 drivers/usb/core/hcd-pci.c   |    2 +-
 drivers/usb/core/usb.c       |    2 +-
 drivers/usb/host/ehci-pci.c  |    6 ++++++
 drivers/usb/host/ohci-pci.c  |    5 +++++
 drivers/usb/host/sl811-hcd.c |    9 +++++++--
 drivers/usb/host/uhci-hcd.c  |    4 ++++
 6 files changed, 24 insertions(+), 4 deletions(-)

Index: g26/drivers/usb/core/hcd-pci.c
===================================================================
--- g26.orig/drivers/usb/core/hcd-pci.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/usb/core/hcd-pci.c	2006-06-02 18:11:28.000000000 -0700
@@ -282,7 +282,7 @@ int usb_hcd_pci_suspend (struct pci_dev 
 			(void) usb_hcd_pci_resume (dev);
 		}
 
-	} else {
+	} else if (hcd->state != HC_STATE_HALT) {
 		dev_dbg (hcd->self.controller, "hcd state %d; not suspended\n",
 			hcd->state);
 		WARN_ON(1);
Index: g26/drivers/usb/core/usb.c
===================================================================
--- g26.orig/drivers/usb/core/usb.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/usb/core/usb.c	2006-06-02 18:11:28.000000000 -0700
@@ -1005,7 +1005,7 @@ static int usb_generic_suspend(struct de
 	 * But those semantics are useless, so we equate the two (sigh).
 	 */
 	if (dev->driver == &usb_generic_driver) {
-		if (dev->power.power_state.event == message.event)
+		if (dev->power.power_state.event != PM_EVENT_ON)
 			return 0;
 		/* we need to rule out bogus requests through sysfs */
 		status = device_for_each_child(dev, NULL, verify_suspended);
Index: g26/drivers/usb/host/sl811-hcd.c
===================================================================
--- g26.orig/drivers/usb/host/sl811-hcd.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/usb/host/sl811-hcd.c	2006-06-02 18:11:28.000000000 -0700
@@ -1780,10 +1780,15 @@ sl811h_suspend(struct platform_device *d
 	struct sl811	*sl811 = hcd_to_sl811(hcd);
 	int		retval = 0;
 
-	if (state.event == PM_EVENT_FREEZE)
+	switch (state.event) {
+	case PM_EVENT_FREEZE:
 		retval = sl811h_bus_suspend(hcd);
-	else if (state.event == PM_EVENT_SUSPEND)
+		break;
+	case PM_EVENT_SUSPEND:
+	case PM_EVENT_PRETHAW:		/* explicitly discard hw state */
 		port_power(sl811, 0);
+		break;
+	}
 	if (retval == 0)
 		dev->dev.power.power_state = state;
 	return retval;
Index: g26/drivers/usb/host/ehci-pci.c
===================================================================
--- g26.orig/drivers/usb/host/ehci-pci.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/usb/host/ehci-pci.c	2006-06-02 18:11:28.000000000 -0700
@@ -229,6 +229,12 @@ static int ehci_pci_suspend(struct usb_h
 	writel (0, &ehci->regs->intr_enable);
 	(void)readl(&ehci->regs->intr_enable);
 
+	/* make sure snapshot being resumed re-enumerates everything */
+	if (message.event == PM_EVENT_PRETHAW) {
+		ehci_halt(ehci);
+		ehci_reset(ehci);
+	}
+
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
  bail:
 	spin_unlock_irqrestore (&ehci->lock, flags);
Index: g26/drivers/usb/host/ohci-pci.c
===================================================================
--- g26.orig/drivers/usb/host/ohci-pci.c	2006-06-02 18:11:22.000000000 -0700
+++ g26/drivers/usb/host/ohci-pci.c	2006-06-02 18:11:28.000000000 -0700
@@ -135,6 +135,11 @@ static int ohci_pci_suspend (struct usb_
 	}
 	ohci_writel(ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);
 	(void)ohci_readl(ohci, &ohci->regs->intrdisable);
+
+	/* make sure snapshot being resumed re-enumerates everything */
+	if (message.event == PM_EVENT_PRETHAW)
+		ohci_usb_reset(ohci);
+
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
  bail:
 	spin_unlock_irqrestore (&ohci->lock, flags);
Index: g26/drivers/usb/host/uhci-hcd.c
===================================================================
--- g26.orig/drivers/usb/host/uhci-hcd.c	2006-06-02 18:08:30.000000000 -0700
+++ g26/drivers/usb/host/uhci-hcd.c	2006-06-02 18:11:28.000000000 -0700
@@ -731,6 +731,10 @@ static int uhci_suspend(struct usb_hcd *
 
 	/* FIXME: Enable non-PME# remote wakeup? */
 
+	/* make sure snapshot being resumed re-enumerates everything */
+	if (message.event == PM_EVENT_PRETHAW)
+		hc_died(uhci);
+
 done:
 	spin_unlock_irq(&uhci->lock);
 	return rc;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch/rft 2.6.17-rc5-git 6/6] PM_EVENT_PRETHAW, issue from PM core

[David Brownell]

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

This converts the PM core to issue the new PRETHAW message.  The rest of
the kernel is now ready to receive these, courtesy of preceding patches.
---
 kernel/power/disk.c   |    4 ++--
 kernel/power/swsusp.c |    9 ++++++++-
 kernel/power/user.c   |    2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)


[-- Attachment #2: prethaw-core.patch --]
[-- Type: text/x-diff, Size: 2628 bytes --]

This converts the PM core to issue the new PRETHAW message.  The rest of
the kernel is now ready to receive these, courtesy of preceding patches.

---
 kernel/power/disk.c   |    4 ++--
 kernel/power/swsusp.c |    9 ++++++++-
 kernel/power/user.c   |    2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

Index: scratch/kernel/power/swsusp.c
===================================================================
--- scratch.orig/kernel/power/swsusp.c	2006-06-05 09:06:46.000000000 -0700
+++ scratch/kernel/power/swsusp.c	2006-06-05 09:20:01.000000000 -0700
@@ -240,6 +240,9 @@ int swsusp_suspend(void)
 	restore_processor_state();
 Restore_highmem:
 	restore_highmem();
+	/* NOTE:  device_power_up() is just a resume() for devices
+	 * that suspended with irqs off ... no overall powerup.
+	 */
 	device_power_up();
 Enable_irqs:
 	local_irq_enable();
@@ -249,8 +252,12 @@ Enable_irqs:
 int swsusp_resume(void)
 {
 	int error;
+
 	local_irq_disable();
-	if (device_power_down(PMSG_FREEZE))
+	/* NOTE:  device_power_down() is just a suspend() with irqs off;
+	 * it has no special "power things down" semantics
+	 */
+	if (device_power_down(PMSG_PRETHAW))
 		printk(KERN_ERR "Some devices failed to power down, very bad\n");
 	/* We'll ignore saved state, but this gets preempt count (etc) right */
 	save_processor_state();
Index: scratch/kernel/power/user.c
===================================================================
--- scratch.orig/kernel/power/user.c	2006-06-05 09:06:46.000000000 -0700
+++ scratch/kernel/power/user.c	2006-06-05 09:20:01.000000000 -0700
@@ -191,7 +191,7 @@ static int snapshot_ioctl(struct inode *
 		}
 		down(&pm_sem);
 		pm_prepare_console();
-		error = device_suspend(PMSG_FREEZE);
+		error = device_suspend(PMSG_PRETHAW);
 		if (!error) {
 			error = swsusp_resume();
 			device_resume();
Index: scratch/kernel/power/disk.c
===================================================================
--- scratch.orig/kernel/power/disk.c	2006-06-05 09:06:46.000000000 -0700
+++ scratch/kernel/power/disk.c	2006-06-05 09:20:01.000000000 -0700
@@ -98,7 +98,7 @@ static void unprepare_processes(void)
 }
 
 /**
- *	pm_suspend_disk - The granpappy of power management.
+ *	pm_suspend_disk - The granpappy of hibernation power management.
  *
  *	If we're going through the firmware, then get it over with quickly.
  *
@@ -207,7 +207,7 @@ static int software_resume(void)
 
 	pr_debug("PM: Preparing devices for restore.\n");
 
-	if ((error = device_suspend(PMSG_FREEZE))) {
+	if ((error = device_suspend(PMSG_PRETHAW))) {
 		printk("Some devices failed to suspend\n");
 		swsusp_free();
 		goto Thaw;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch/rft 2.6.17-rc5-git 3/6] PM_EVENT_PRETHAW, handle in IDE and PCI

[David Brownell]

On Tuesday 30 May 2006 12:21 pm, Pavel Machek wrote:
> Hi!
> 
> > @@ -1227,7 +1227,9 @@ static int generic_ide_suspend(struct de
> >  	rq.special = &args;
> >  	rq.pm = &rqpm;
> >  	rqpm.pm_step = ide_pm_state_start_suspend;
> > -	rqpm.pm_state = state.event;
> > +	if (mesg.event == PM_EVENT_PRETHAW)
> > +		mesg.event = PM_EVENT_FREEZE;
> > +	rqpm.pm_state = mesg.event;
> > 
> 
> Actually it would be nicer not to modify mesg.event... And perhaps I'd 
> move this check lower -- to be consistent with 'low level driver gets
> full info' idea.

Instead, it's consistent with the "smallest obviously correct patch"
principle.  Didn't want to end up overhauling IDE, especially since it
doesn't seem to be a class  where resume() does anything interesting.


> (Or is it checked at too many places?) 

Notice that "rqpm.pm_state" confusion ... it's checked indirectly
from there.  Event codes are not states. 

Re: [patch/rft 2.6.17-rc5-git 1/6] fix broken/dubious driver suspend() methods

[David Brownell]

On Tuesday 30 May 2006 12:11 pm, Pavel Machek wrote:
> Hi!
> 
> > Small driver suspend() updates in preparation for the upcoming FREEZE message;
> > 
> >  - Only compare message events for equality against PM_EVENT_* codes;
> >    not against integers, or using greater/less-than comparisons.
> >    (PM_EVENT_* should really become a __bitwise thing.)
> > 
> >  - Explicitly test for SUSPEND events (rather than not-something-else)
> >    before suspending devices.
> > 
> >  - Removes more of the confusion between a pm_message_t (wraps event code)
> >    and a "state" ... suspend() originally took a target system state.
> > 
> > These updates are correct and appropriate even without new PM_EVENT codes.
> > ---
> >  drivers/ide/ppc/pmac.c                  |   14 ++++++++------
> >  drivers/media/dvb/cinergyT2/cinergyT2.c |    2 +-
> >  drivers/scsi/libata-core.c              |    6 +++---
> >  drivers/scsi/mesh.c                     |    7 ++++---
> >  4 files changed, 16 insertions(+), 13 deletions(-)
> 
> ACK. (You probably want to add s-o-b: header, and not send changelog
> twice.)

I'll forward this to Andrew for 2.6.18 then ... was holding off
on the signed-off-by pending feedback.  Changelog sent twice to
make sure that the linux-pm archives are readable.  :)

- Dave

Re: [patch/rft 2.6.17-rc5-git 2/6] add PM_EVENT_PRETHAW

[David Brownell]

On Tuesday 30 May 2006 12:17 pm, Pavel Machek wrote:
> Hi!
> 
> Looks mostly good...
> 
> > This adds a new pm_message_t event type to use when preparing to switch
> > into a swsusp snapshot.  Devices that have been initialized by Linux
> > after resume (rather than left in power-up-reset state) may need to be
> > reset; this new event type gives drivers the chance to do that.
> > 
> > The drivers that will care about this are those which understand more
> > hardware states than just "on" and "reset", and read the hardware state
> > during resume().   ...
> 
> ...should this go to Documentation/ somewhere?

I have a separate doc patch, which I will send once the code passes
sufficient muster.


> > + * Other transitions are triggered by messages sent using suspend().  All
> > + * these transitions quiesce the driver, so that I/O queues are inactive.
> > + * That commonly entails turning off IRQs and DMA; there may be rules
> 
> Actually all DMAs must always be turned off. Otherwise swsusp can't
> get consistent image. IRQs can be disabled or not (they usually are), however driver
> prefers... but DMA must be off.

I think there are cases where DMA should still be active, but didn't want to
go into them.  For FREEZE, DMA should stop, yes.

Surely there'd be no point in **disllowing** system states where almost
everything is idled (CPU, most peripherals, etc) but the I/O is flowing?
Normally it's felt to be a _good_ thing to get the system into modes
like that.

- Dave

Re: [patch/rft 2.6.17-rc5-git 4/6] PM_EVENT_PRETHAW for various graphics cards

[David Brownell]

On Tuesday 30 May 2006 12:30 pm, Pavel Machek wrote:

> > +	if (mesg.event == PM_EVENT_PRETHAW)
> > +		mesg.event = PM_EVENT_FREEZE;
> >  ...
> > +	dev->dev.power.power_state = mesg;
> 
> If PRETHAW event is sent, we still store FREEZE into power_state. That
> is not consistent with other drivers.

Doesn't matter.  By definition, it's OK for drivers to treat PRETHAW
as FREEZE, when they don't care about that distinction ... plus ISTR
there's no code that _relies_ on that power_state.   Few drivers
even bother setting it ... it's not exactly a useful record.

- Dave

PM docs and API?

[bsmith]

Where can I find the latest documentation for the PM APIs in Linux?
Specifically, where can I read about the API that a device driver
should offer to make it "power manageable".

A Google search returns a blizzard of stuff but much of it is out
of date, and part of the kernel docs are marked as obsolete.  (Still,
I'm sorry if this question has been asked here 'bout a million times.)


thanks
Bob Smith

Re: PM docs and API?

[David Brownell]

On Wednesday 07 June 2006 11:57 am, bsmith wrote:
> Where can I find the latest documentation for the PM APIs in Linux?

AFAIK there's nothing worth reading just now.  I have a patch that
may make some of the linux/Documentation stuff more correct and
comprehensible, but it's not quite ready yet.


> Specifically, where can I read about the API that a device driver
> should offer to make it "power manageable".

The fundamental device driver power management hook is to participate
in system-wide suspend and resume operations ... the stuff that happens
with "echo standby > /sys/power/state", as one example, which triggers
platform-specific implementations of the "struct pm_ops" calls.

Drivers based on the driver model all connect to some bus and will
implement suspend() and resume() methods.  Those methods will put
devices into low power states using hardware-specific mechanisms.

For example, with PCI PM support the pci_set_power_state() routine
is used.  SOC based systems routinely use <linux/clk.h> APIs to
gate clocks to the relevant portion of silicon.  The resume() call
generally reverses the effect of suspend(); sometimes it needs to
recover from a hardware reset, or force one, and reinitialize.

Drivers based on the driver model may also be able to make the
hardware issue wakeup events.  For wakeup-capable hardware, the
sysfs /sys/devices/.../power/wakeup flag can be changed from
user mode code.  Drivers are responsible for looking at that flag
during suspend().

There's also just being "power aware", for example by drivers
turning off power supplies and clocks they're not actively using,
and otherwise staying in the state with the lowest power drain
that's practical for the current level of device usage.


IMO the most significant hole in device driver support for PM is
that the system doesn't expose relevant aspects of the upcoming
power state that suspend() is there to support.  You can see one
fix for that in the clk_must_disable() API that I sent to this
list for comment a while back ... I think I'll just submit it for
general merge, it's impossible for embedded USB controllers to
support wakeup events without that.

Another hole in device driver support for PM is a basic platform
neutral API for managing power domains ... like "turn on the 3.0V
supply going to MMC slot 2" (or maybe that needs 1.8V instead).
Lacking such an API, all those calls are board-specific:  this
board uses a native GPIO call, that one uses an external power
switch, another one has programmable output voltage, etc.


For platform PM support, there are several things that apply:

  - Providing pm_ops through a pm_set_ops() call.  When the
    system is asked to go into e.g. STANDBY or STR state, the
    drivers are all asked to suspend and then pm_ops methods
    are used to do things like put memory into self-refresh
    mode and entering appropriate low-power modes if the drivers
    didn't misbehave (e.g. all necessary clocks got disabled)
    and other constraints are met (e.g. switch from 1.6V for
    main power supply down to 1.1V).

  - On ARM (and at some point, more generically) implementing
    dynamic tick (CONFIG_NO_IDLE_HZ) in the system timer driver
    so that the system doesn't needlessly waste energy issuing
    and responding to timer IRQs.  It's common that this puts
    the tick rate down to 6-8 HZ ... with timer precision still
    being arbitrarily good.

  - Where the system supports it, cpufreq essentially provides
    a class of driver for CPUs.  There's discussion about growing
    this to provide better support for voltage scaling, rather
    than purely CPU scaling.

  - There's also a potential "pm_idle" mechanism, whereby the
    system idle process can kick in extra power savings.

So without the pm_set_ops() stuff, the only driver-level PM stuff
that matters is automatically throttling down power usage.

- Dave