Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 11:13 -0400, Alan Stern wrote:
> On Mon, 14 Apr 2008, Rafael J. Wysocki wrote:
> 
> >     "you can assume that the user space is there while ->prepare() is running,
> >     but you are supposed to prevent new children of the device from being
> >     registered from that point on _and_ you have to make sure that freezable
> >     tasks will be able to freeze after ->prepare() has run" (but why on Earth a
> >     driver writer is now required to know what's a freezable task etc.?)
> 
> This reminds me...  We're going to need a way to make certain
> activities mutually exclusive with system sleep.  The simplest example
> is loading a kernel module; init and probe routines often end up
> causing new child devices to be registered.
> 
> The most straightforward approach is to use an rwsem like the one we 
> used to have.  However I'm concerned that under some circumstances 
> there might be recursive read-locking.  (For example, the init routine 
> in a newly-loaded module decides to load yet another module.  Can this 
> actually happen?  libusual does something much like it.)
> 
> So it's quite possible we'll end up needing a mechanism that resembles 
> an rwsem but allows recursive (properly nested) read-locking.  Does 
> such a thing exist already, or would it have to be invented?

Despite what Oliver says, that's a perfect example where the module load
syscalls should return an error. Maybe something like -EAGAIN would do
tho... that might need a minor update of the module init tools so they
retry instead of failing.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Oliver Neukum]

Am Montag, 14. April 2008 16:49:26 schrieb Alan Stern:
> Now, I don't see what the big deal is here.  We all agree that it would 
> be nice and appropriate if drivers could do things like 
> request_firmware() during prepare().  But for the moment they can't.  
> It's not like this is some tremendous hardship -- you aren't going to 
> have to rip out all the existing prepare() implementations and rewrite 
> them, because they don't exist yet!

True. You cannot request firmware in the current suspend() call either.
Rafael's code is a clear improvement over the existing code. Removal
of the freezer will need a lot of rewriting in any case.

	Regards
		Oliver

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Alan Stern]

On Mon, 14 Apr 2008, Rafael J. Wysocki wrote:

>     "you can assume that the user space is there while ->prepare() is running,
>     but you are supposed to prevent new children of the device from being
>     registered from that point on _and_ you have to make sure that freezable
>     tasks will be able to freeze after ->prepare() has run" (but why on Earth a
>     driver writer is now required to know what's a freezable task etc.?)

This reminds me...  We're going to need a way to make certain
activities mutually exclusive with system sleep.  The simplest example
is loading a kernel module; init and probe routines often end up
causing new child devices to be registered.

The most straightforward approach is to use an rwsem like the one we 
used to have.  However I'm concerned that under some circumstances 
there might be recursive read-locking.  (For example, the init routine 
in a newly-loaded module decides to load yet another module.  Can this 
actually happen?  libusual does something much like it.)

So it's quite possible we'll end up needing a mechanism that resembles 
an rwsem but allows recursive (properly nested) read-locking.  Does 
such a thing exist already, or would it have to be invented?

Alan Stern

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Alan Stern wrote:
> On Mon, 14 Apr 2008, Nigel Cunningham wrote:
> 
> > > The ability for prepare() callbacks to sync with userland,
> > > request_firmware, etc... is an important feature that's been needed for
> > > some time imho.
> > > 
> > > Ben.
> > 
> > I agree. These calls have already been changing far too often in
> > mainline. I know it's all been necessary but please, can we try to make
> > one set of changes and just get it right this time?
> 
> In practical terms, it will be easier to keep the freezer where it is 
> for now.  This is because prepare() requires drivers to change their 
> behavior; they aren't allowed to register new children any more.
> 
> But if userspace isn't frozen then user programs can interact with
> drivers in a way that does cause new children to be created.  For
> example this happens in USB, where opening an audio device and
> selecting its bitrate causes a new set of endpoints to be realized,
> along with their representations in sysfs.
> 
> Thus, in addition to making all the interface changes implied by the 
> new callbacks, drivers would also have to change the way they interact 
> with userspace.  Yes, this will have to be done eventually in any case, 
> as the freezer goes away -- but it shouldn't have to be done right now.

That's exactly my point.

Plus in my opinion, while with ->prepare() after the freezer it's possible
to write ->prepare() and ->suspend() that will work just fine when we remove
the freezer, with ->prepare() before the freezer it would be hard to do that,
unless ->prepare() was trivial.

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Sun, 2008-04-13 at 19:23 -0400, Alan Stern wrote:
> 
> Thus, in addition to making all the interface changes implied by the 
> new callbacks, drivers would also have to change the way they
> interact 
> with userspace.  Yes, this will have to be done eventually in any
> case, 
> as the freezer goes away -- but it shouldn't have to be done right
> now.

I disagree. The freezer can already be compiled out and is already not
used on some architectures.

Cheers,
Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 01:33 +0200, Rafael J. Wysocki wrote:
> 
> Plus in my opinion, while with ->prepare() after the freezer it's
> possible
> to write ->prepare() and ->suspend() that will work just fine when we
> remove
> the freezer, with ->prepare() before the freezer it would be hard to
> do that,
> unless ->prepare() was trivial.

And removing a good half of the usage scenario where prepare is useful..

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> On Sun, 2008-04-13 at 19:23 -0400, Alan Stern wrote:
> > 
> > Thus, in addition to making all the interface changes implied by the 
> > new callbacks, drivers would also have to change the way they
> > interact 
> > with userspace.  Yes, this will have to be done eventually in any
> > case, 
> > as the freezer goes away -- but it shouldn't have to be done right
> > now.
> 
> I disagree. The freezer can already be compiled out and is already not
> used on some architectures.

Please have a look at this thread:
http://lkml.org/lkml/2008/3/21/322
(in short, the reporter sees APM suspend breakage under stress, occuring
because APM uses our suspending of devices without the freezer).

It mostly appears to work without the freezer, but that's bacuse no one
actually does things that might break it.  I don't think we can rely on users
being so kind to us forever. :-)

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

> Please have a look at this thread:
> http://lkml.org/lkml/2008/3/21/322
> (in short, the reporter sees APM suspend breakage under stress, occuring
> because APM uses our suspending of devices without the freezer).
> 
> It mostly appears to work without the freezer, but that's bacuse no one
> actually does things that might break it.  I don't think we can rely on users
> being so kind to us forever. :-)

As far as I'm concerned, it's yet another case of the freezer papering
over a problem rather than fixing it properly.

If we're going to introduce new callbacks, we should have the right
semantic from day 1 -and- fix those problems, rather than going to the
same old recursive nonsensical arguments and do things to paper over
problems.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

> Please have a look at this thread:
> http://lkml.org/lkml/2008/3/21/322
> (in short, the reporter sees APM suspend breakage under stress, occuring
> because APM uses our suspending of devices without the freezer).

Note that the above seems to lack any useful information (as usual) such
as what block driver is involved etc...

We fixed IDE to be robust vs. pending IOs a while ago. It's possible
that libata isn't as solid yet, I don't know.

This needs to be done regardless of feezer vs. not freezer. There are
thins in the kernel that can trigger BIOs at any time pretty much
regardless of user space being frozen or not, again, it's a case of
sticking our head in the sand and hoping the freezer hides all our
design bugs.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> > Please have a look at this thread:
> > http://lkml.org/lkml/2008/3/21/322
> > (in short, the reporter sees APM suspend breakage under stress, occuring
> > because APM uses our suspending of devices without the freezer).
> 
> Note that the above seems to lack any useful information (as usual) such
> as what block driver is involved etc...
> 
> We fixed IDE to be robust vs. pending IOs a while ago. It's possible
> that libata isn't as solid yet, I don't know.
> 
> This needs to be done regardless of feezer vs. not freezer. There are
> thins in the kernel that can trigger BIOs at any time pretty much
> regardless of user space being frozen or not, again, it's a case of
> sticking our head in the sand and hoping the freezer hides all our
> design bugs.

Now, you are talking about a completely different thing I agree with.

I gave this example just to show that some drivers break without the freezer
if suspend is carried out under stress, nothing else.

My point is (and has always been) that we can't just drop the freezer right now
without causing functional regressions to appear, at least for some users.

If there had not been _any_ other way to do things, we could do that, but IMO
there is a way (that I described in the previous message).

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> > Please have a look at this thread:
> > http://lkml.org/lkml/2008/3/21/322
> > (in short, the reporter sees APM suspend breakage under stress, occuring
> > because APM uses our suspending of devices without the freezer).
> > 
> > It mostly appears to work without the freezer, but that's bacuse no one
> > actually does things that might break it.  I don't think we can rely on users
> > being so kind to us forever. :-)
> 
> As far as I'm concerned, it's yet another case of the freezer papering
> over a problem rather than fixing it properly.

Well, this is not a user's point of view.

> If we're going to introduce new callbacks, we should have the right
> semantic from day 1 -and- fix those problems, rather than going to the
> same old recursive nonsensical arguments and do things to paper over
> problems.

Still, we're not supposed to break things, as far as the functionality is
concerned, and that's important, because it means we _have_ _to_ make changes
in steps.

To be more precise, what you suggest (move ->prepare() before the freezer
right now) means a patch with _functional_ changes (it's impossible to register
new children of dev after ->prepare(dev) has run which can affect the user
space in the window before ->prepare() and the freezer), whereas what I'd like
to do is the (present) patch without functional changes.

Thanks,
Rafael

[PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Introduce 'struct pm_ops' and 'struct pm_ext_ops' ('ext' meaning
'extended') representing suspend and hibernation operations for bus
types, device classes, device types and device drivers.

Modify the PM core to use 'struct pm_ops' and 'struct pm_ext_ops'
objects, if defined, instead of the ->suspend(), ->resume(),
->suspend_late(), and ->resume_early() callbacks (the old callbacks
will be considered as legacy and gradually phased out).

The main purpose of doing this is to separate suspend (aka S2RAM and
standby) callbacks from hibernation callbacks in such a way that the
new callbacks won't take arguments and the semantics of each of them
will be clearly specified.  This has been requested for multiple
times by many people, including Linus himself, and the reason is that
within the current scheme if ->resume() is called, for example, it's
difficult to say why it's been called (ie. is it a resume from RAM or
from hibernation or a suspend/hibernation failure etc.?).

The second purpose is to make the suspend/hibernation callbacks more
flexible so that device drivers can handle more than they can within
the current scheme.  For example, some drivers may need to prevent
new children of the device from being registered before their
->suspend() callbacks are executed or they may want to carry out some
operations requiring the availability of some other devices, not
directly bound via the parent-child relationship, in order to prepare
for the execution of ->suspend(), etc.

Ultimately, we'd like to stop using the freezing of tasks for suspend
and therefore the drivers' suspend/hibernation code will have to take
care of the handling of the user space during suspend/hibernation.
That, in turn, would be difficult within the current scheme, without
the new ->prepare() and ->complete() callbacks.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---

 arch/x86/kernel/apm_32.c   |    8 
 drivers/base/power/main.c  |  693 ++++++++++++++++++++++++++++++++++-----------
 drivers/base/power/power.h |    2 
 drivers/base/power/trace.c |    4 
 include/linux/device.h     |    9 
 include/linux/pm.h         |  314 ++++++++++++++++++--
 kernel/power/disk.c        |   20 -
 kernel/power/main.c        |    6 
 8 files changed, 852 insertions(+), 204 deletions(-)

Index: linux-next/include/linux/pm.h
===================================================================
--- linux-next.orig/include/linux/pm.h
+++ linux-next/include/linux/pm.h
@@ -114,7 +114,9 @@ typedef struct pm_message {
 	int event;
 } pm_message_t;
 
-/*
+/**
+ * struct pm_ops - device PM callbacks
+ *
  * Several driver power state transitions are externally visible, affecting
  * the state of pending I/O queues and (for drivers that touch hardware)
  * interrupts, wakeups, DMA, and other hardware state.  There may also be
@@ -122,6 +124,284 @@ typedef struct pm_message {
  * to the rest of the driver stack (such as a driver that's ON gating off
  * clocks which are not in active use).
  *
+ * The externally visible transitions are handled with the help of the following
+ * callbacks included in this structure:
+ *
+ * @prepare: Prepare the device for the upcoming transition, but do NOT change
+ *	its hardware state.  Prevent new children of the device from being
+ *	registered after @prepare() returns (the driver's subsystem and
+ *	generally the rest of the kernel is supposed to prevent new calls to the
+ *	probe method from being made too once @prepare() has succeeded).  If
+ *	@prepare() detects a situation it cannot handle (e.g. registration of a
+ *	child already in progress), it may return -EAGAIN, so that the PM core
+ *	can execute it once again (e.g. after the new child has been registered)
+ *	to recover from the race condition.  This method is executed for all
+ *	kinds of suspend transitions and is followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), or @poweroff().
+ *	The PM core executes @prepare() for all devices before starting to
+ *	execute suspend callbacks for any of them, so drivers may assume all of
+ *	the other devices to be present and functional while @prepare() is being
+ *	executed.  In particular, it is safe to make GFP_KERNEL memory
+ *	allocations from within @prepare().  However, drivers may NOT assume
+ *	anything about the availability of the user space at that time and it
+ *	is not correct to request firmware from within @prepare() (it's too
+ *	late to do that).  [To work around this limitation, drivers may
+ *	register suspend and hibernation notifiers that are executed before the
+ *	freezing of tasks.]
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, following one of the resume callbacks:
+ *	@resume(), @thaw(), @restore().  Also called if the state transition
+ *	fails before the driver's suspend callback (@suspend(), @freeze(),
+ *	@poweroff()) can be executed (e.g. if the suspend callback fails for one
+ *	of the other devices that the PM core has unsuccessfully attempted to
+ *	suspend earlier).
+ *	The PM core executes @complete() after it has executed the appropriate
+ *	resume callback for all devices.
+ *
+ * @suspend: Executed before putting the system into a sleep state in which the
+ *	contents of main memory are preserved.  Quiesce the device, put it into
+ *	a low power state appropriate for the upcoming system state (such as
+ *	PCI_D3hot), and enable wakeup events as appropriate.
+ *
+ * @resume: Executed after waking the system up from a sleep state in which the
+ *	contents of main memory were preserved.  Put the device into the
+ *	appropriate state, according to the information saved in memory by the
+ *	preceding @suspend().  The 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 may rely on while resuming.  On most
+ *	platforms, there are no restrictions on availability of resources like
+ *	clocks during @resume().
+ *
+ * @freeze: Hibernation-specific, executed before creating a hibernation image.
+ *	Quiesce operations so that a consistent image can be created, but do NOT
+ *	otherwise put the device into a low power device state and do NOT emit
+ *	system wakeup events.  Save in main memory the device settings to be
+ *	used by @restore() during the subsequent resume from hibernation or by
+ *	the subsequent @thaw(), if the creation of the image or the restoration
+ *	of main memory contents from it fails.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation image OR
+ *	if the creation of the image fails.  Also executed after a failing
+ *	attempt to restore the contents of main memory from such an image.
+ *	Undo the changes made by the preceding @freeze(), so the device can be
+ *	operated in the same way as immediately before the call to @freeze().
+ *
+ * @poweroff: Hibernation-specific, executed after saving a hibernation image.
+ *	Quiesce the device, put it into a low power state appropriate for the
+ *	upcoming system state (such as PCI_D3hot), and enable wakeup events as
+ *	appropriate.
+ *
+ * @restore: Hibernation-specific, executed after restoring the contents of main
+ *	memory from a hibernation image.  Driver starts working again,
+ *	responding to hardware events and software requests.  Drivers may NOT
+ *	make ANY assumptions about the hardware state right prior to @restore().
+ *	On most platforms, there are no restrictions on availability of
+ *	resources like clocks during @restore().
+ *
+ * All of the above callbacks, except for @complete(), return error codes.
+ * However, the error codes returned by the resume operations, @resume(),
+ * @thaw(), and @restore(), do not cause the PM core to abort the resume
+ * transition during which they are returned.  The error codes returned in
+ * that cases are only printed by the PM core to the system logs for debugging
+ * purposes.  Still, it is recommended that drivers only return error codes
+ * from their resume methods in case of an unrecoverable failure (i.e. when the
+ * device being handled refuses to resume and becomes unusable) to allow us to
+ * modify the PM core in the future, so that it can avoid attempting to handle
+ * devices that failed to resume and their children.
+ *
+ * It is allowed to unregister devices while the above callbacks are being
+ * executed.  However, it is not allowed to unregister a device from within any
+ * of its own callbacks.
+ */
+
+struct pm_ops {
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*restore)(struct device *dev);
+};
+
+/**
+ * struct pm_ext_ops - extended device PM callbacks
+ *
+ * Some devices require certain operations related to suspend and hibernation
+ * to be carried out with interrupts disabled.  Thus, 'struct pm_ext_ops' below
+ * is defined, adding callbacks to be executed with interrupts disabled to
+ * 'struct pm_ops'.
+ *
+ * The following callbacks included in 'struct pm_ext_ops' are executed with
+ * the nonboot CPUs switched off and with interrupts disabled on the only
+ * functional CPU.  They also are executed with the PM core list of devices
+ * locked, so they must NOT unregister any devices.
+ *
+ * @suspend_noirq: Complete the operations of ->suspend() by carrying out any
+ *	actions required for suspending the device that need interrupts to be
+ *	disabled
+ *
+ * @resume_noirq: Prepare for the execution of ->resume() by carrying out any
+ *	actions required for resuming the device that need interrupts to be
+ *	disabled
+ *
+ * @freeze_noirq: Complete the operations of ->freeze() by carrying out any
+ *	actions required for freezing the device that need interrupts to be
+ *	disabled
+ *
+ * @thaw_noirq: Prepare for the execution of ->thaw() by carrying out any
+ *	actions required for thawing the device that need interrupts to be
+ *	disabled
+ *
+ * @poweroff_noirq: Complete the operations of ->poweroff() by carrying out any
+ *	actions required for handling the device that need interrupts to be
+ *	disabled
+ *
+ * @restore_noirq: Prepare for the execution of ->restore() by carrying out any
+ *	actions required for restoring the operations of the device that need
+ *	interrupts to be disabled
+ *
+ * All of the above callbacks return error codes, but the error codes returned
+ * by the resume operations, @resume_noirq(), @thaw_noirq(), and
+ * @restore_noirq(), do not cause the PM core to abort the resume transition
+ * during which they are returned.  The error codes returned in that cases are
+ * only printed by the PM core to the system logs for debugging purposes.
+ * Still, as stated above, it is recommended that drivers only return error
+ * codes from their resume methods if the device being handled fails to resume
+ * and is not usable any more.
+ */
+
+struct pm_ext_ops {
+	struct pm_ops base;
+	int (*suspend_noirq)(struct device *dev);
+	int (*resume_noirq)(struct device *dev);
+	int (*freeze_noirq)(struct device *dev);
+	int (*thaw_noirq)(struct device *dev);
+	int (*poweroff_noirq)(struct device *dev);
+	int (*restore_noirq)(struct device *dev);
+};
+
+/**
+ * PM_EVENT_ messages
+ *
+ * The following PM_EVENT_ messages are defined for the internal use of the PM
+ * core, in order to provide a mechanism allowing the high level suspend and
+ * hibernation code to convey the necessary information to the device PM core
+ * code:
+ *
+ * ON		No transition.
+ *
+ * FREEZE 	System is going to hibernate, call ->prepare() and ->freeze()
+ *		for all devices.
+ *
+ * SUSPEND	System is going to suspend, call ->prepare() and ->suspend()
+ *		for all devices.
+ *
+ * HIBERNATE	Hibernation image has been saved, call ->prepare() and
+ *		->poweroff() for all devices.
+ *
+ * QUIESCE	Contents of main memory are going to be restored from a (loaded)
+ *		hibernation image, call ->prepare() and ->freeze() for all
+ *		devices.
+ *
+ * RESUME	System is resuming, call ->resume() and ->complete() for all
+ *		devices.
+ *
+ * THAW		Hibernation image has been created, call ->thaw() and
+ *		->complete() for all devices.
+ *
+ * RESTORE	Contents of main memory have been restored from a hibernation
+ *		image, call ->restore() and ->complete() for all devices.
+ *
+ * RECOVER	Creation of a hibernation image or restoration of the main
+ *		memory contents from a hibernation image has failed, call
+ *		->thaw() and ->complete() for all devices.
+ */
+
+#define PM_EVENT_ON		0x0000
+#define PM_EVENT_FREEZE 	0x0001
+#define PM_EVENT_SUSPEND	0x0002
+#define PM_EVENT_HIBERNATE	0x0004
+#define PM_EVENT_QUIESCE	0x0008
+#define PM_EVENT_RESUME		0x0010
+#define PM_EVENT_THAW		0x0020
+#define PM_EVENT_RESTORE	0x0040
+#define PM_EVENT_RECOVER	0x0080
+
+#define PM_EVENT_SLEEP	(PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE)
+
+#define PMSG_FREEZE	((struct pm_message){ .event = PM_EVENT_FREEZE, })
+#define PMSG_QUIESCE	((struct pm_message){ .event = PM_EVENT_QUIESCE, })
+#define PMSG_SUSPEND	((struct pm_message){ .event = PM_EVENT_SUSPEND, })
+#define PMSG_HIBERNATE	((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
+#define PMSG_RESUME	((struct pm_message){ .event = PM_EVENT_RESUME, })
+#define PMSG_THAW	((struct pm_message){ .event = PM_EVENT_THAW, })
+#define PMSG_RESTORE	((struct pm_message){ .event = PM_EVENT_RESTORE, })
+#define PMSG_RECOVER	((struct pm_message){ .event = PM_EVENT_RECOVER, })
+#define PMSG_ON		((struct pm_message){ .event = PM_EVENT_ON, })
+
+/**
+ * Device power management states
+ *
+ * These state labels are used internally by the PM core to indicate the current
+ * status of a device with respect to the PM core operations.
+ *
+ * DPM_ON		Device is regarded as operational.  Set this way
+ *			initially and when ->complete() is about to be called.
+ *			Also set when ->prepare() fails.
+ *
+ * DPM_PREPARING	Device is going to be prepared for a PM transition.  Set
+ *			when ->prepare() is about to be called.
+ *
+ * DPM_RESUMING		Device is going to be resumed.  Set when ->resume(),
+ *			->thaw(), or ->restore() is about to be called.
+ *
+ * DPM_SUSPENDING	Device has been prepared for a power transition.  Set
+ *			when ->prepare() has just succeeded.
+ *
+ * DPM_OFF		Device is regarded as inactive.  Set immediately after
+ *			->suspend(), ->freeze(), or ->poweroff() has succeeded.
+ *			Also set when ->resume()_noirq, ->thaw_noirq(), or
+ *			->restore_noirq() is about to be called.
+ *
+ * DPM_OFF_IRQ		Device is in a "deep sleep".  Set immediately after
+ *			->suspend_noirq(), ->freeze_noirq(), or
+ *			->poweroff_noirq() has just succeeded.
+ */
+
+enum dpm_state {
+	DPM_INVALID,
+	DPM_ON,
+	DPM_PREPARING,
+	DPM_RESUMING,
+	DPM_SUSPENDING,
+	DPM_OFF,
+	DPM_OFF_IRQ,
+};
+
+struct dev_pm_info {
+	pm_message_t		power_state;
+	unsigned		can_wakeup:1;
+	unsigned		should_wakeup:1;
+	enum dpm_state		status;		/* Owned by the PM core */
+#ifdef	CONFIG_PM_SLEEP
+	struct list_head	entry;
+#endif
+};
+
+/*
+ * The PM_EVENT_ messages are also used by drivers implementing the legacy
+ * suspend framework, based on the ->suspend() and ->resume() callbacks common
+ * for suspend and hibernation transitions, according to the rules below.
+ */
+
+/* Necessary, because several drivers use PM_EVENT_PRETHAW */
+#define PM_EVENT_PRETHAW PM_EVENT_QUIESCE
+
+/*
  * One transition is triggered by resume(), after a suspend() call; the
  * message is implicit:
  *
@@ -166,35 +446,13 @@ typedef struct pm_message {
  * 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_HIBERNATE 4
-#define PM_EVENT_PRETHAW 8
-
-#define PM_EVENT_SLEEP	(PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE)
-
-#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_HIBERNATE	((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
-#define PMSG_ON		((struct pm_message){ .event = PM_EVENT_ON, })
-
-struct dev_pm_info {
-	pm_message_t		power_state;
-	unsigned		can_wakeup:1;
-	unsigned		should_wakeup:1;
-	bool			sleeping:1;	/* Owned by the PM core */
-#ifdef	CONFIG_PM_SLEEP
-	struct list_head	entry;
-#endif
-};
+#ifdef CONFIG_PM_SLEEP
+extern void device_pm_lock(void);
+extern void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
 
+extern void device_pm_unlock(void);
 extern int device_power_down(pm_message_t state);
-extern void device_power_up(void);
-extern void device_resume(void);
-
-#ifdef CONFIG_PM_SLEEP
 extern int device_suspend(pm_message_t state);
 extern int device_prepare_suspend(pm_message_t state);
 
Index: linux-next/drivers/base/power/main.c
===================================================================
--- linux-next.orig/drivers/base/power/main.c
+++ linux-next/drivers/base/power/main.c
@@ -12,11 +12,9 @@
  * and add it to the list of power-controlled devices. sysfs entries for
  * controlling device power management will also be added.
  *
- * A different set of lists than the global subsystem list are used to
- * keep track of power info because we use different lists to hold
- * devices based on what stage of the power management process they
- * are in. The power domain dependencies may also differ from the
- * ancestral dependencies that the subsystem list maintains.
+ * A separate list is used for keeping track of power info, because the power
+ * domain dependencies may differ from the ancestral dependencies that the
+ * subsystem list maintains.
  */
 
 #include <linux/device.h>
@@ -30,31 +28,40 @@
 #include "power.h"
 
 /*
- * The entries in the dpm_active list are in a depth first order, simply
+ * The entries in the dpm_list list are in a depth first order, simply
  * because children are guaranteed to be discovered after parents, and
  * are inserted at the back of the list on discovery.
  *
- * All the other lists are kept in the same order, for consistency.
- * However the lists aren't always traversed in the same order.
- * Semaphores must be acquired from the top (i.e., front) down
- * and released in the opposite order.  Devices must be suspended
- * from the bottom (i.e., end) up and resumed in the opposite order.
- * That way no parent will be suspended while it still has an active
- * child.
- *
  * Since device_pm_add() may be called with a device semaphore held,
  * we must never try to acquire a device semaphore while holding
  * dpm_list_mutex.
  */
 
-LIST_HEAD(dpm_active);
-static LIST_HEAD(dpm_off);
-static LIST_HEAD(dpm_off_irq);
+LIST_HEAD(dpm_list);
 
 static DEFINE_MUTEX(dpm_list_mtx);
 
-/* 'true' if all devices have been suspended, protected by dpm_list_mtx */
-static bool all_sleeping;
+/*
+ * Set once the preparation of devices for a PM transition has started, reset
+ * before starting to resume devices.  Protected by dpm_list_mtx.
+ */
+static bool transition_started;
+
+/**
+ *	device_pm_lock - lock the list of active devices used by the PM core
+ */
+void device_pm_lock(void)
+{
+	mutex_lock(&dpm_list_mtx);
+}
+
+/**
+ *	device_pm_unlock - unlock the list of active devices used by the PM core
+ */
+void device_pm_unlock(void)
+{
+	mutex_unlock(&dpm_list_mtx);
+}
 
 /**
  *	device_pm_add - add a device to the list of active devices
@@ -68,22 +75,32 @@ int device_pm_add(struct device *dev)
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
-	if ((dev->parent && dev->parent->power.sleeping) || all_sleeping) {
-		if (dev->parent->power.sleeping)
-			dev_warn(dev,
-				"parent %s is sleeping, will not add\n",
+	if (dev->parent) {
+		if (dev->parent->power.status >= DPM_SUSPENDING) {
+			dev_warn(dev, "parent %s is sleeping, will not add\n",
 				dev->parent->bus_id);
-		else
-			dev_warn(dev, "devices are sleeping, will not add\n");
-		WARN_ON(true);
-		error = -EBUSY;
-	} else {
-		error = dpm_sysfs_add(dev);
-		if (!error)
-			list_add_tail(&dev->power.entry, &dpm_active);
+			goto Refuse;
+		}
+	} else if (transition_started) {
+		/*
+		 * We refuse to register parentless devices while a PM
+		 * transition is in progress in order to avoid leaving them
+		 * unhandled down the road
+		 */
+		goto Refuse;
+	}
+	error = dpm_sysfs_add(dev);
+	if (!error) {
+		dev->power.status = DPM_ON;
+		list_add_tail(&dev->power.entry, &dpm_list);
 	}
+ End:
 	mutex_unlock(&dpm_list_mtx);
 	return error;
+ Refuse:
+	WARN_ON(true);
+	error = -EBUSY;
+	goto End;
 }
 
 /**
@@ -103,73 +120,241 @@ void device_pm_remove(struct device *dev
 	mutex_unlock(&dpm_list_mtx);
 }
 
+/**
+ *	pm_op - execute the PM operation appropiate for given PM event
+ *	@dev:	Device.
+ *	@ops:	PM operations to choose from.
+ *	@state:	PM transition of the system being carried out.
+ */
+static int pm_op(struct device *dev, struct pm_ops *ops, pm_message_t state)
+{
+	int error = 0;
+
+	switch (state.event) {
+#ifdef CONFIG_SUSPEND
+	case PM_EVENT_SUSPEND:
+		if (ops->suspend) {
+			error = ops->suspend(dev);
+			suspend_report_result(ops->suspend, error);
+		}
+		break;
+	case PM_EVENT_RESUME:
+		if (ops->resume) {
+			error = ops->resume(dev);
+			suspend_report_result(ops->resume, error);
+		}
+		break;
+#endif /* CONFIG_SUSPEND */
+#ifdef CONFIG_HIBERNATION
+	case PM_EVENT_FREEZE:
+	case PM_EVENT_QUIESCE:
+		if (ops->freeze) {
+			error = ops->freeze(dev);
+			suspend_report_result(ops->freeze, error);
+		}
+		break;
+	case PM_EVENT_HIBERNATE:
+		if (ops->poweroff) {
+			error = ops->poweroff(dev);
+			suspend_report_result(ops->poweroff, error);
+		}
+		break;
+	case PM_EVENT_THAW:
+	case PM_EVENT_RECOVER:
+		if (ops->thaw) {
+			error = ops->thaw(dev);
+			suspend_report_result(ops->thaw, error);
+		}
+		break;
+	case PM_EVENT_RESTORE:
+		if (ops->restore) {
+			error = ops->restore(dev);
+			suspend_report_result(ops->restore, error);
+		}
+		break;
+#endif /* CONFIG_HIBERNATION */
+	default:
+		error = -EINVAL;
+	}
+	return error;
+}
+
+/**
+ *	pm_noirq_op - execute the PM operation appropiate for given PM event
+ *	@dev:	Device.
+ *	@ops:	PM operations to choose from.
+ *	@state: PM transition of the system being carried out.
+ *
+ *	The operation is executed with interrupts disabled by the only remaining
+ *	functional CPU in the system.
+ */
+static int pm_noirq_op(struct device *dev, struct pm_ext_ops *ops,
+			pm_message_t state)
+{
+	int error = 0;
+
+	switch (state.event) {
+#ifdef CONFIG_SUSPEND
+	case PM_EVENT_SUSPEND:
+		if (ops->suspend_noirq) {
+			error = ops->suspend_noirq(dev);
+			suspend_report_result(ops->suspend_noirq, error);
+		}
+		break;
+	case PM_EVENT_RESUME:
+		if (ops->resume_noirq) {
+			error = ops->resume_noirq(dev);
+			suspend_report_result(ops->resume_noirq, error);
+		}
+		break;
+#endif /* CONFIG_SUSPEND */
+#ifdef CONFIG_HIBERNATION
+	case PM_EVENT_FREEZE:
+	case PM_EVENT_QUIESCE:
+		if (ops->freeze_noirq) {
+			error = ops->freeze_noirq(dev);
+			suspend_report_result(ops->freeze_noirq, error);
+		}
+		break;
+	case PM_EVENT_HIBERNATE:
+		if (ops->poweroff_noirq) {
+			error = ops->poweroff_noirq(dev);
+			suspend_report_result(ops->poweroff_noirq, error);
+		}
+		break;
+	case PM_EVENT_THAW:
+	case PM_EVENT_RECOVER:
+		if (ops->thaw_noirq) {
+			error = ops->thaw_noirq(dev);
+			suspend_report_result(ops->thaw_noirq, error);
+		}
+		break;
+	case PM_EVENT_RESTORE:
+		if (ops->restore_noirq) {
+			error = ops->restore_noirq(dev);
+			suspend_report_result(ops->restore_noirq, error);
+		}
+		break;
+#endif /* CONFIG_HIBERNATION */
+	default:
+		error = -EINVAL;
+	}
+	return error;
+}
+
+static char *pm_verb(int event)
+{
+	switch (event) {
+	case PM_EVENT_SUSPEND:
+		return "suspend";
+	case PM_EVENT_RESUME:
+		return "resume";
+	case PM_EVENT_FREEZE:
+		return "freeze";
+	case PM_EVENT_QUIESCE:
+		return "quiesce";
+	case PM_EVENT_HIBERNATE:
+		return "hibernate";
+	case PM_EVENT_THAW:
+		return "thaw";
+	case PM_EVENT_RESTORE:
+		return "restore";
+	default:
+		return "(unknown PM event)";
+	}
+}
+
+static void pm_dev_dbg(struct device *dev, pm_message_t state, char *info)
+{
+	dev_dbg(dev, "%s%s%s\n", info, pm_verb(state.event),
+		((state.event & PM_EVENT_SLEEP) && device_may_wakeup(dev)) ?
+		", may wakeup" : "");
+}
+
+static void pm_dev_err(struct device *dev, pm_message_t state, char *info,
+			int error)
+{
+	printk(KERN_ERR "PM: Device %s failed to %s%s: error %d\n",
+		kobject_name(&dev->kobj), pm_verb(state.event), info, error);
+}
+
 /*------------------------- Resume routines -------------------------*/
 
 /**
- *	resume_device_early - Power on one device (early resume).
+ *	resume_device_noirq - Power on one device (early resume).
  *	@dev:	Device.
+ *	@state: PM transition of the system being carried out.
  *
  *	Must be called with interrupts disabled.
  */
-static int resume_device_early(struct device *dev)
+static int resume_device_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->bus && dev->bus->resume_early) {
-		dev_dbg(dev, "EARLY resume\n");
+	if (!dev->bus)
+		goto End;
+
+	if (dev->bus->pm) {
+		pm_dev_dbg(dev, state, "EARLY ");
+		error = pm_noirq_op(dev, dev->bus->pm, state);
+	} else if (dev->bus->resume_early) {
+		pm_dev_dbg(dev, state, "legacy EARLY ");
 		error = dev->bus->resume_early(dev);
 	}
-
+ End:
 	TRACE_RESUME(error);
 	return error;
 }
 
 /**
  *	dpm_power_up - Power on all regular (non-sysdev) devices.
+ *	@state: PM transition of the system being carried out.
  *
- *	Walk the dpm_off_irq list and power each device up. This
- *	is used for devices that required they be powered down with
- *	interrupts disabled. As devices are powered on, they are moved
- *	to the dpm_off list.
+ *	Execute the appropriate "noirq resume" callback for all devices marked
+ *	as DPM_OFF_IRQ.
  *
  *	Must be called with interrupts disabled and only one CPU running.
  */
-static void dpm_power_up(void)
+static void dpm_power_up(pm_message_t state)
 {
+	struct device *dev;
 
-	while (!list_empty(&dpm_off_irq)) {
-		struct list_head *entry = dpm_off_irq.next;
-		struct device *dev = to_device(entry);
-
-		list_move_tail(entry, &dpm_off);
-		resume_device_early(dev);
-	}
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		if (dev->power.status > DPM_OFF) {
+			int error;
+
+			dev->power.status = DPM_OFF;
+			error = resume_device_noirq(dev, state);
+			if (error)
+				pm_dev_err(dev, state, " early", error);
+		}
 }
 
 /**
  *	device_power_up - Turn on all devices that need special attention.
+ *	@state: PM transition of the system being carried out.
  *
  *	Power on system devices, then devices that required we shut them down
  *	with interrupts disabled.
  *
  *	Must be called with interrupts disabled.
  */
-void device_power_up(void)
+void device_power_up(pm_message_t state)
 {
 	sysdev_resume();
-	dpm_power_up();
+	dpm_power_up(state);
 }
 EXPORT_SYMBOL_GPL(device_power_up);
 
 /**
  *	resume_device - Restore state for one device.
  *	@dev:	Device.
- *
+ *	@state: PM transition of the system being carried out.
  */
-static int resume_device(struct device *dev)
+static int resume_device(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
@@ -178,21 +363,40 @@ static int resume_device(struct device *
 
 	down(&dev->sem);
 
-	if (dev->bus && dev->bus->resume) {
-		dev_dbg(dev,"resuming\n");
-		error = dev->bus->resume(dev);
+	if (dev->bus) {
+		if (dev->bus->pm) {
+			pm_dev_dbg(dev, state, "");
+			error = pm_op(dev, &dev->bus->pm->base, state);
+		} else if (dev->bus->resume) {
+			pm_dev_dbg(dev, state, "legacy ");
+			error = dev->bus->resume(dev);
+		}
+		if (error)
+			goto End;
 	}
 
-	if (!error && dev->type && dev->type->resume) {
-		dev_dbg(dev,"resuming\n");
-		error = dev->type->resume(dev);
+	if (dev->type) {
+		if (dev->type->pm) {
+			pm_dev_dbg(dev, state, "type ");
+			error = pm_op(dev, dev->type->pm, state);
+		} else if (dev->type->resume) {
+			pm_dev_dbg(dev, state, "legacy type ");
+			error = dev->type->resume(dev);
+		}
+		if (error)
+			goto End;
 	}
 
-	if (!error && dev->class && dev->class->resume) {
-		dev_dbg(dev,"class resume\n");
-		error = dev->class->resume(dev);
+	if (dev->class) {
+		if (dev->class->pm) {
+			pm_dev_dbg(dev, state, "class ");
+			error = pm_op(dev, dev->class->pm, state);
+		} else if (dev->class->resume) {
+			pm_dev_dbg(dev, state, "legacy class ");
+			error = dev->class->resume(dev);
+		}
 	}
-
+ End:
 	up(&dev->sem);
 
 	TRACE_RESUME(error);
@@ -201,78 +405,161 @@ static int resume_device(struct device *
 
 /**
  *	dpm_resume - Resume every device.
+ *	@state: PM transition of the system being carried out.
  *
- *	Resume the devices that have either not gone through
- *	the late suspend, or that did go through it but also
- *	went through the early resume.
+ *	Execute the appropriate "resume" callback for all devices the status of
+ *	which indicates that they are inactive.
+ */
+static void dpm_resume(pm_message_t state)
+{
+	struct list_head list;
+
+	INIT_LIST_HEAD(&list);
+	mutex_lock(&dpm_list_mtx);
+	transition_started = false;
+	while (!list_empty(&dpm_list)) {
+		struct device *dev = to_device(dpm_list.next);
+
+		get_device(dev);
+		if (dev->power.status >= DPM_OFF) {
+			int error;
+
+			dev->power.status = DPM_RESUMING;
+			mutex_unlock(&dpm_list_mtx);
+
+			error = resume_device(dev, state);
+
+			mutex_lock(&dpm_list_mtx);
+			if (error)
+				pm_dev_err(dev, state, "", error);
+		} else if (dev->power.status == DPM_SUSPENDING) {
+			/* Allow new children of the device to be registered */
+			dev->power.status = DPM_RESUMING;
+		}
+		if (!list_empty(&dev->power.entry))
+			list_move_tail(&dev->power.entry, &list);
+		put_device(dev);
+	}
+	list_splice(&list, &dpm_list);
+	mutex_unlock(&dpm_list_mtx);
+}
+
+/**
+ *	complete_device - Complete a PM transition for given device
+ *	@dev:	Device.
+ *	@state: PM transition of the system being carried out.
+ */
+static void complete_device(struct device *dev, pm_message_t state)
+{
+	down(&dev->sem);
+
+	if (dev->class && dev->class->pm && dev->class->pm->complete) {
+		pm_dev_dbg(dev, state, "completing class ");
+		dev->class->pm->complete(dev);
+	}
+
+	if (dev->type && dev->type->pm && dev->type->pm->complete) {
+		pm_dev_dbg(dev, state, "completing type ");
+		dev->type->pm->complete(dev);
+	}
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->base.complete) {
+		pm_dev_dbg(dev, state, "completing ");
+		dev->bus->pm->base.complete(dev);
+	}
+
+	up(&dev->sem);
+}
+
+/**
+ *	dpm_complete - Complete a PM transition for all devices.
+ *	@state: PM transition of the system being carried out.
  *
- *	Take devices from the dpm_off_list, resume them,
- *	and put them on the dpm_locked list.
+ *	Execute the ->complete() callbacks for all devices that are not marked
+ *	as DPM_ON.
  */
-static void dpm_resume(void)
+static void dpm_complete(pm_message_t state)
 {
+	struct list_head list;
+
+	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
-	all_sleeping = false;
-	while(!list_empty(&dpm_off)) {
-		struct list_head *entry = dpm_off.next;
-		struct device *dev = to_device(entry);
+	while (!list_empty(&dpm_list)) {
+		struct device *dev = to_device(dpm_list.prev);
 
-		list_move_tail(entry, &dpm_active);
-		dev->power.sleeping = false;
-		mutex_unlock(&dpm_list_mtx);
-		resume_device(dev);
-		mutex_lock(&dpm_list_mtx);
+		get_device(dev);
+		if (dev->power.status > DPM_ON) {
+			dev->power.status = DPM_ON;
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, state);
+
+			mutex_lock(&dpm_list_mtx);
+		}
+		if (!list_empty(&dev->power.entry))
+			list_move(&dev->power.entry, &list);
+		put_device(dev);
 	}
+	list_splice(&list, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
 }
 
 /**
  *	device_resume - Restore state of each device in system.
+ *	@state: PM transition of the system being carried out.
  *
  *	Resume all the devices, unlock them all, and allow new
  *	devices to be registered once again.
  */
-void device_resume(void)
+void device_resume(pm_message_t state)
 {
 	might_sleep();
-	dpm_resume();
+	dpm_resume(state);
+	dpm_complete(state);
 }
 EXPORT_SYMBOL_GPL(device_resume);
 
 
 /*------------------------- Suspend routines -------------------------*/
 
-static inline char *suspend_verb(u32 event)
+/**
+ *	resume_event - return a PM message representing the resume event
+ *	               corresponding to given sleep state.
+ *	@sleep_state: PM message representing a sleep state.
+ */
+static pm_message_t resume_event(pm_message_t sleep_state)
 {
-	switch (event) {
-	case PM_EVENT_SUSPEND:	return "suspend";
-	case PM_EVENT_FREEZE:	return "freeze";
-	case PM_EVENT_PRETHAW:	return "prethaw";
-	default:		return "(unknown suspend event)";
+	switch (sleep_state.event) {
+	case PM_EVENT_SUSPEND:
+		return PMSG_RESUME;
+	case PM_EVENT_FREEZE:
+	case PM_EVENT_QUIESCE:
+		return PMSG_RECOVER;
+	case PM_EVENT_HIBERNATE:
+		return PMSG_RESTORE;
 	}
-}
-
-static void
-suspend_device_dbg(struct device *dev, pm_message_t state, char *info)
-{
-	dev_dbg(dev, "%s%s%s\n", info, suspend_verb(state.event),
-		((state.event == PM_EVENT_SUSPEND) && device_may_wakeup(dev)) ?
-		", may wakeup" : "");
+	return PMSG_ON;
 }
 
 /**
- *	suspend_device_late - Shut down one device (late suspend).
+ *	suspend_device_noirq - Shut down one device (late suspend).
  *	@dev:	Device.
- *	@state:	Power state device is entering.
+ *	@state: PM transition of the system being carried out.
  *
  *	This is called with interrupts off and only a single CPU running.
  */
-static int suspend_device_late(struct device *dev, pm_message_t state)
+static int suspend_device_noirq(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
-	if (dev->bus && dev->bus->suspend_late) {
-		suspend_device_dbg(dev, state, "LATE ");
+	if (!dev->bus)
+		return 0;
+
+	if (dev->bus->pm) {
+		pm_dev_dbg(dev, state, "LATE ");
+		error = pm_noirq_op(dev, dev->bus->pm, state);
+	} else if (dev->bus->suspend_late) {
+		pm_dev_dbg(dev, state, "legacy LATE ");
 		error = dev->bus->suspend_late(dev, state);
 		suspend_report_result(dev->bus->suspend_late, error);
 	}
@@ -281,37 +568,30 @@ static int suspend_device_late(struct de
 
 /**
  *	device_power_down - Shut down special devices.
- *	@state:		Power state to enter.
+ *	@state: PM transition of the system being carried out.
  *
- *	Power down devices that require interrupts to be disabled
- *	and move them from the dpm_off list to the dpm_off_irq list.
+ *	Power down devices that require interrupts to be disabled.
  *	Then power down system devices.
  *
  *	Must be called with interrupts disabled and only one CPU running.
  */
 int device_power_down(pm_message_t state)
 {
+	struct device *dev;
 	int error = 0;
 
-	while (!list_empty(&dpm_off)) {
-		struct list_head *entry = dpm_off.prev;
-		struct device *dev = to_device(entry);
-
-		error = suspend_device_late(dev, state);
+	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+		error = suspend_device_noirq(dev, state);
 		if (error) {
-			printk(KERN_ERR "Could not power down device %s: "
-					"error %d\n",
-					kobject_name(&dev->kobj), error);
+			pm_dev_err(dev, state, " late", error);
 			break;
 		}
-		if (!list_empty(&dev->power.entry))
-			list_move(&dev->power.entry, &dpm_off_irq);
+		dev->power.status = DPM_OFF_IRQ;
 	}
-
 	if (!error)
 		error = sysdev_suspend(state);
 	if (error)
-		dpm_power_up();
+		dpm_power_up(resume_event(state));
 	return error;
 }
 EXPORT_SYMBOL_GPL(device_power_down);
@@ -319,7 +599,7 @@ EXPORT_SYMBOL_GPL(device_power_down);
 /**
  *	suspend_device - Save state of one device.
  *	@dev:	Device.
- *	@state:	Power state device is entering.
+ *	@state: PM transition of the system being carried out.
  */
 static int suspend_device(struct device *dev, pm_message_t state)
 {
@@ -327,24 +607,43 @@ static int suspend_device(struct device 
 
 	down(&dev->sem);
 
-	if (dev->class && dev->class->suspend) {
-		suspend_device_dbg(dev, state, "class ");
-		error = dev->class->suspend(dev, state);
-		suspend_report_result(dev->class->suspend, error);
-	}
-
-	if (!error && dev->type && dev->type->suspend) {
-		suspend_device_dbg(dev, state, "type ");
-		error = dev->type->suspend(dev, state);
-		suspend_report_result(dev->type->suspend, error);
-	}
-
-	if (!error && dev->bus && dev->bus->suspend) {
-		suspend_device_dbg(dev, state, "");
-		error = dev->bus->suspend(dev, state);
-		suspend_report_result(dev->bus->suspend, error);
+	if (dev->class) {
+		if (dev->class->pm) {
+			pm_dev_dbg(dev, state, "class ");
+			error = pm_op(dev, dev->class->pm, state);
+		} else if (dev->class->suspend) {
+			pm_dev_dbg(dev, state, "legacy class ");
+			error = dev->class->suspend(dev, state);
+			suspend_report_result(dev->class->suspend, error);
+		}
+		if (error)
+			goto End;
+	}
+
+	if (dev->type) {
+		if (dev->type->pm) {
+			pm_dev_dbg(dev, state, "type ");
+			error = pm_op(dev, dev->type->pm, state);
+		} else if (dev->type->suspend) {
+			pm_dev_dbg(dev, state, "legacy type ");
+			error = dev->type->suspend(dev, state);
+			suspend_report_result(dev->type->suspend, error);
+		}
+		if (error)
+			goto End;
 	}
 
+	if (dev->bus) {
+		if (dev->bus->pm) {
+			pm_dev_dbg(dev, state, "");
+			error = pm_op(dev, &dev->bus->pm->base, state);
+		} else if (dev->bus->suspend) {
+			pm_dev_dbg(dev, state, "legacy ");
+			error = dev->bus->suspend(dev, state);
+			suspend_report_result(dev->bus->suspend, error);
+		}
+	}
+ End:
 	up(&dev->sem);
 
 	return error;
@@ -352,67 +651,141 @@ static int suspend_device(struct device 
 
 /**
  *	dpm_suspend - Suspend every device.
- *	@state:	Power state to put each device in.
+ *	@state: PM transition of the system being carried out.
  *
- *	Walk the dpm_locked list.  Suspend each device and move it
- *	to the dpm_off list.
- *
- *	(For historical reasons, if it returns -EAGAIN, that used to mean
- *	that the device would be called again with interrupts disabled.
- *	These days, we use the "suspend_late()" callback for that, so we
- *	print a warning and consider it an error).
+ *	Execute the appropriate "suspend" callbacks for all devices.
  */
 static int dpm_suspend(pm_message_t state)
 {
+	struct list_head list;
 	int error = 0;
 
+	INIT_LIST_HEAD(&list);
 	mutex_lock(&dpm_list_mtx);
-	while (!list_empty(&dpm_active)) {
-		struct list_head *entry = dpm_active.prev;
-		struct device *dev = to_device(entry);
-
-		WARN_ON(dev->parent && dev->parent->power.sleeping);
+	while (!list_empty(&dpm_list)) {
+		struct device *dev = to_device(dpm_list.prev);
 
-		dev->power.sleeping = true;
+		get_device(dev);
 		mutex_unlock(&dpm_list_mtx);
+
 		error = suspend_device(dev, state);
+
 		mutex_lock(&dpm_list_mtx);
 		if (error) {
-			printk(KERN_ERR "Could not suspend device %s: "
-					"error %d%s\n",
-					kobject_name(&dev->kobj),
-					error,
-					(error == -EAGAIN ?
-					" (please convert to suspend_late)" :
-					""));
-			dev->power.sleeping = false;
+			pm_dev_err(dev, state, "", error);
+			put_device(dev);
 			break;
 		}
+		dev->power.status = DPM_OFF;
 		if (!list_empty(&dev->power.entry))
-			list_move(&dev->power.entry, &dpm_off);
+			list_move(&dev->power.entry, &list);
+		put_device(dev);
 	}
-	if (!error)
-		all_sleeping = true;
+	list_splice(&list, dpm_list.prev);
 	mutex_unlock(&dpm_list_mtx);
+	return error;
+}
+
+/**
+ *	prepare_device - Execute the ->prepare() callback(s) for given device.
+ *	@dev:	Device.
+ *	@state: PM transition of the system being carried out.
+ */
+static int prepare_device(struct device *dev, pm_message_t state)
+{
+	int error = 0;
+
+	down(&dev->sem);
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->base.prepare) {
+		pm_dev_dbg(dev, state, "preparing ");
+		error = dev->bus->pm->base.prepare(dev);
+		suspend_report_result(dev->bus->pm->base.prepare, error);
+		if (error)
+			goto End;
+	}
+
+	if (dev->type && dev->type->pm && dev->type->pm->prepare) {
+		pm_dev_dbg(dev, state, "preparing type ");
+		error = dev->type->pm->prepare(dev);
+		suspend_report_result(dev->type->pm->prepare, error);
+		if (error)
+			goto End;
+	}
+
+	if (dev->class && dev->class->pm && dev->class->pm->prepare) {
+		pm_dev_dbg(dev, state, "preparing class ");
+		error = dev->class->pm->prepare(dev);
+		suspend_report_result(dev->class->pm->prepare, error);
+	}
+ End:
+	up(&dev->sem);
+
+	return error;
+}
+
+/**
+ *	dpm_prepare - Prepare all devices for a PM transition.
+ *	@state: PM transition of the system being carried out.
+ *
+ *	Execute the ->prepare() callback for all devices.
+ */
+static int dpm_prepare(pm_message_t state)
+{
+	struct list_head list;
+	int error = 0;
+
+	INIT_LIST_HEAD(&list);
+	mutex_lock(&dpm_list_mtx);
+	transition_started = true;
+	while (!list_empty(&dpm_list)) {
+		struct device *dev = to_device(dpm_list.next);
+
+		get_device(dev);
+		dev->power.status = DPM_PREPARING;
+		mutex_unlock(&dpm_list_mtx);
+
+		error = prepare_device(dev, state);
 
+		mutex_lock(&dpm_list_mtx);
+		if (error) {
+			dev->power.status = DPM_ON;
+			if (error == -EAGAIN) {
+				put_device(dev);
+				continue;
+			}
+			printk(KERN_ERR "PM: Failed to prepare device %s "
+				"for power transition: error %d\n",
+				kobject_name(&dev->kobj), error);
+			put_device(dev);
+			break;
+		}
+		dev->power.status = DPM_SUSPENDING;
+		if (!list_empty(&dev->power.entry))
+			list_move_tail(&dev->power.entry, &list);
+		put_device(dev);
+	}
+	list_splice(&list, &dpm_list);
+	mutex_unlock(&dpm_list_mtx);
 	return error;
 }
 
 /**
  *	device_suspend - Save state and stop all devices in system.
- *	@state: new power management state
+ *	@state: PM transition of the system being carried out.
  *
- *	Prevent new devices from being registered, then lock all devices
- *	and suspend them.
+ *	Prepare and suspend all devices.
  */
 int device_suspend(pm_message_t state)
 {
 	int error;
 
 	might_sleep();
-	error = dpm_suspend(state);
+	error = dpm_prepare(state);
+	if (!error)
+		error = dpm_suspend(state);
 	if (error)
-		device_resume();
+		device_resume(resume_event(state));
 	return error;
 }
 EXPORT_SYMBOL_GPL(device_suspend);
Index: linux-next/include/linux/device.h
===================================================================
--- linux-next.orig/include/linux/device.h
+++ linux-next/include/linux/device.h
@@ -69,6 +69,8 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ext_ops *pm;
+
 	struct bus_type_private *p;
 };
 
@@ -132,6 +134,8 @@ struct device_driver {
 	int (*resume) (struct device *dev);
 	struct attribute_group **groups;
 
+	struct pm_ops *pm;
+
 	struct driver_private *p;
 };
 
@@ -202,6 +206,8 @@ struct class {
 
 	int (*suspend)(struct device *dev, pm_message_t state);
 	int (*resume)(struct device *dev);
+
+	struct pm_ops *pm;
 };
 
 extern int __must_check class_register(struct class *class);
@@ -345,8 +351,11 @@ struct device_type {
 	struct attribute_group **groups;
 	int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
 	void (*release)(struct device *dev);
+
 	int (*suspend)(struct device *dev, pm_message_t state);
 	int (*resume)(struct device *dev);
+
+	struct pm_ops *pm;
 };
 
 /* interface for exporting device attributes */
Index: linux-next/kernel/power/disk.c
===================================================================
--- linux-next.orig/kernel/power/disk.c
+++ linux-next/kernel/power/disk.c
@@ -193,6 +193,7 @@ static int create_image(int platform_mod
 	if (error)
 		return error;
 
+	device_pm_lock();
 	local_irq_disable();
 	/* At this point, device_suspend() has been called, but *not*
 	 * device_power_down(). We *must* call device_power_down() now.
@@ -224,9 +225,10 @@ static int create_image(int platform_mod
 	/* NOTE:  device_power_up() is just a resume() for devices
 	 * that suspended with irqs off ... no overall powerup.
 	 */
-	device_power_up();
+	device_power_up(in_suspend ? PMSG_RECOVER : PMSG_RESTORE);
  Enable_irqs:
 	local_irq_enable();
+	device_pm_unlock();
 	return error;
 }
 
@@ -280,7 +282,7 @@ int hibernation_snapshot(int platform_mo
  Finish:
 	platform_finish(platform_mode);
  Resume_devices:
-	device_resume();
+	device_resume(in_suspend ? PMSG_RECOVER : PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
@@ -300,8 +302,9 @@ static int resume_target_kernel(void)
 {
 	int error;
 
+	device_pm_lock();
 	local_irq_disable();
-	error = device_power_down(PMSG_PRETHAW);
+	error = device_power_down(PMSG_QUIESCE);
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to power down, "
 			"aborting resume\n");
@@ -329,9 +332,10 @@ static int resume_target_kernel(void)
 	swsusp_free();
 	restore_processor_state();
 	touch_softlockup_watchdog();
-	device_power_up();
+	device_power_up(PMSG_THAW);
  Enable_irqs:
 	local_irq_enable();
+	device_pm_unlock();
 	return error;
 }
 
@@ -350,7 +354,7 @@ int hibernation_restore(int platform_mod
 
 	pm_prepare_console();
 	suspend_console();
-	error = device_suspend(PMSG_PRETHAW);
+	error = device_suspend(PMSG_QUIESCE);
 	if (error)
 		goto Finish;
 
@@ -362,7 +366,7 @@ int hibernation_restore(int platform_mod
 		enable_nonboot_cpus();
 	}
 	platform_restore_cleanup(platform_mode);
-	device_resume();
+	device_resume(PMSG_RECOVER);
  Finish:
 	resume_console();
 	pm_restore_console();
@@ -403,6 +407,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Finish;
 
+	device_pm_lock();
 	local_irq_disable();
 	error = device_power_down(PMSG_HIBERNATE);
 	if (!error) {
@@ -411,6 +416,7 @@ int hibernation_platform_enter(void)
 		while (1);
 	}
 	local_irq_enable();
+	device_pm_unlock();
 
 	/*
 	 * We don't need to reenable the nonboot CPUs or resume consoles, since
@@ -419,7 +425,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
Index: linux-next/kernel/power/main.c
===================================================================
--- linux-next.orig/kernel/power/main.c
+++ linux-next/kernel/power/main.c
@@ -228,6 +228,7 @@ static int suspend_enter(suspend_state_t
 {
 	int error = 0;
 
+	device_pm_lock();
 	arch_suspend_disable_irqs();
 	BUG_ON(!irqs_disabled());
 
@@ -239,10 +240,11 @@ static int suspend_enter(suspend_state_t
 	if (!suspend_test(TEST_CORE))
 		error = suspend_ops->enter(state);
 
-	device_power_up();
+	device_power_up(PMSG_RESUME);
  Done:
 	arch_suspend_enable_irqs();
 	BUG_ON(irqs_disabled());
+	device_pm_unlock();
 	return error;
 }
 
@@ -291,7 +293,7 @@ int suspend_devices_and_enter(suspend_st
 	if (suspend_ops->finish)
 		suspend_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESUME);
  Resume_console:
 	resume_console();
  Close:
Index: linux-next/arch/x86/kernel/apm_32.c
===================================================================
--- linux-next.orig/arch/x86/kernel/apm_32.c
+++ linux-next/arch/x86/kernel/apm_32.c
@@ -1208,9 +1208,9 @@ static int suspend(int vetoable)
 	if (err != APM_SUCCESS)
 		apm_error("suspend", err);
 	err = (err == APM_SUCCESS) ? 0 : -EIO;
-	device_power_up();
+	device_power_up(PMSG_RESUME);
 	local_irq_enable();
-	device_resume();
+	device_resume(PMSG_RESUME);
 	queue_event(APM_NORMAL_RESUME, NULL);
 	spin_lock(&user_list_lock);
 	for (as = user_list; as != NULL; as = as->next) {
@@ -1235,7 +1235,7 @@ static void standby(void)
 		apm_error("standby", err);
 
 	local_irq_disable();
-	device_power_up();
+	device_power_up(PMSG_RESUME);
 	local_irq_enable();
 }
 
@@ -1321,7 +1321,7 @@ static void check_events(void)
 			ignore_bounce = 1;
 			if ((event != APM_NORMAL_RESUME)
 			    || (ignore_normal_resume == 0)) {
-				device_resume();
+				device_resume(PMSG_RESUME);
 				queue_event(event, NULL);
 			}
 			ignore_normal_resume = 0;
Index: linux-next/drivers/base/power/power.h
===================================================================
--- linux-next.orig/drivers/base/power/power.h
+++ linux-next/drivers/base/power/power.h
@@ -4,7 +4,7 @@
  * main.c
  */
 
-extern struct list_head dpm_active;	/* The active device list */
+extern struct list_head dpm_list;	/* The active device list */
 
 static inline struct device *to_device(struct list_head *entry)
 {
Index: linux-next/drivers/base/power/trace.c
===================================================================
--- linux-next.orig/drivers/base/power/trace.c
+++ linux-next/drivers/base/power/trace.c
@@ -188,9 +188,9 @@ static int show_file_hash(unsigned int v
 static int show_dev_hash(unsigned int value)
 {
 	int match = 0;
-	struct list_head * entry = dpm_active.prev;
+	struct list_head *entry = dpm_list.prev;
 
-	while (entry != &dpm_active) {
+	while (entry != &dpm_list) {
 		struct device * dev = to_device(entry);
 		unsigned int hash = hash_string(DEVSEED, dev->bus_id, DEVHASH);
 		if (hash == value) {

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Sun, 2008-04-13 at 15:33 +0200, Rafael J. Wysocki wrote:
> + *     The PM core executes @prepare() for all devices before starting to
> + *     execute suspend callbacks for any of them, so drivers may assume all of
> + *     the other devices to be present and functional while @prepare() is being
> + *     executed.  In particular, it is safe to make GFP_KERNEL memory
> + *     allocations from within @prepare().  However, drivers may NOT assume
> + *     anything about the availability of the user space at that time and it
> + *     is not correct to request firmware from within @prepare() (it's too
> + *     late to do that).  [To work around this limitation, drivers may
> + *     register suspend and hibernation notifiers that are executed before the
> + *     freezing of tasks.]

Can you tell me why you kept that limitation with user space ?

I don't see the point... On the contrary, prepare() is the pefect place
to implement handshaking with userspace for drivers that need to do so,
such as the DRM.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Sunday, 13 of April 2008, Benjamin Herrenschmidt wrote:
> 
> On Sun, 2008-04-13 at 15:33 +0200, Rafael J. Wysocki wrote:
> > + *     The PM core executes @prepare() for all devices before starting to
> > + *     execute suspend callbacks for any of them, so drivers may assume all of
> > + *     the other devices to be present and functional while @prepare() is being
> > + *     executed.  In particular, it is safe to make GFP_KERNEL memory
> > + *     allocations from within @prepare().  However, drivers may NOT assume
> > + *     anything about the availability of the user space at that time and it
> > + *     is not correct to request firmware from within @prepare() (it's too
> > + *     late to do that).  [To work around this limitation, drivers may
> > + *     register suspend and hibernation notifiers that are executed before the
> > + *     freezing of tasks.]
> 
> Can you tell me why you kept that limitation with user space ?
> 
> I don't see the point... On the contrary, prepare() is the pefect place
> to implement handshaking with userspace for drivers that need to do so,
> such as the DRM.

This _comment_ reflects the current situation, which is that we freeze tasks
before a suspend.  When it's no longer necessary to do that, I'll be happy to
change this comment.  For now, however, that's not the case.

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

> > I don't see the point... On the contrary, prepare() is the pefect place
> > to implement handshaking with userspace for drivers that need to do so,
> > such as the DRM.
> 
> This _comment_ reflects the current situation, which is that we freeze tasks
> before a suspend.  When it's no longer necessary to do that, I'll be happy to
> change this comment.  For now, however, that's not the case.

Can't we run the freezer after prepare() instead ?

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> > > I don't see the point... On the contrary, prepare() is the pefect place
> > > to implement handshaking with userspace for drivers that need to do so,
> > > such as the DRM.
> > 
> > This _comment_ reflects the current situation, which is that we freeze tasks
> > before a suspend.  When it's no longer necessary to do that, I'll be happy to
> > change this comment.  For now, however, that's not the case.
> 
> Can't we run the freezer after prepare() instead ?

Well, I'm not sure and I'm not going to introduce the change right now, after
the paches have been included in -mm.

That would require quite some changes in the core code that I'd prefer to
avoid for now.  We can do something like this in a separate patch series after
the present one settles down a bit.

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

> Well, I'm not sure and I'm not going to introduce the change right now, after
> the paches have been included in -mm.
> 
> That would require quite some changes in the core code that I'd prefer to
> avoid for now.  We can do something like this in a separate patch series after
> the present one settles down a bit.

I disagree.

Doing it later would introduce yet another major semantic change.

I think we should get it right now. There's no hurry in pushing things
especially if they aren't quite right.

The ability for prepare() callbacks to sync with userland,
request_firmware, etc... is an important feature that's been needed for
some time imho.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> > Well, I'm not sure and I'm not going to introduce the change right now, after
> > the paches have been included in -mm.
> > 
> > That would require quite some changes in the core code that I'd prefer to
> > avoid for now.  We can do something like this in a separate patch series after
> > the present one settles down a bit.
> 
> I disagree.

Okay, so we have different opinions. :-)
 
> Doing it later would introduce yet another major semantic change.
>
> I think we should get it right now. There's no hurry in pushing things
> especially if they aren't quite right.

>From my point of view, they are as right as they can be at the moment.

Besides, maintainig a set of patches like this so that it always applies to
a tree that's continuously changing under it is far from funny.  I'm not going
to do it much longer, that's for sure.

> The ability for prepare() callbacks to sync with userland,
> request_firmware, etc... is an important feature that's been needed for
> some time imho.

Well, if we put ->prepare() before the freezer, what can it actually do?
Certainly nothing that will block any (user space) task, because the freezer
won't work after that.  So, all of the significant suspend work, like blocking
tasks using the device etc., will have to be done by ->suspend().  Will that be
convenient?  I'm not sure.  I'm not even sure that would be _doable_ at all.

The point of view depends on what you think ->prepare() should be used for
and I'm sure you have some specific cases in mind.  However, are they generic
enough?

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Nigel Cunningham]

Hi Rafael etc.

On Mon, 2008-04-14 at 08:47 +1000, Benjamin Herrenschmidt wrote:
> > Well, I'm not sure and I'm not going to introduce the change right now, after
> > the paches have been included in -mm.
> > 
> > That would require quite some changes in the core code that I'd prefer to
> > avoid for now.  We can do something like this in a separate patch series after
> > the present one settles down a bit.
> 
> I disagree.
> 
> Doing it later would introduce yet another major semantic change.
> 
> I think we should get it right now. There's no hurry in pushing things
> especially if they aren't quite right.
> 
> The ability for prepare() callbacks to sync with userland,
> request_firmware, etc... is an important feature that's been needed for
> some time imho.
> 
> Ben.

I agree. These calls have already been changing far too often in
mainline. I know it's all been necessary but please, can we try to make
one set of changes and just get it right this time?

Regards,

Nigel

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Nigel Cunningham wrote:
> Hi Rafael etc.

Hi,
 
> On Mon, 2008-04-14 at 08:47 +1000, Benjamin Herrenschmidt wrote:
> > > Well, I'm not sure and I'm not going to introduce the change right now, after
> > > the paches have been included in -mm.
> > > 
> > > That would require quite some changes in the core code that I'd prefer to
> > > avoid for now.  We can do something like this in a separate patch series after
> > > the present one settles down a bit.
> > 
> > I disagree.
> > 
> > Doing it later would introduce yet another major semantic change.
> > 
> > I think we should get it right now. There's no hurry in pushing things
> > especially if they aren't quite right.
> > 
> > The ability for prepare() callbacks to sync with userland,
> > request_firmware, etc... is an important feature that's been needed for
> > some time imho.
> > 
> > Ben.
> 
> I agree. These calls have already been changing far too often in
> mainline. I know it's all been necessary but please, can we try to make
> one set of changes and just get it right this time?

Sorry, what exactly has been changing too often?  Device suspend callbacks??
They haven't changed since pm_message_t was introduced.

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Alan Stern]

On Mon, 14 Apr 2008, Nigel Cunningham wrote:

> > The ability for prepare() callbacks to sync with userland,
> > request_firmware, etc... is an important feature that's been needed for
> > some time imho.
> > 
> > Ben.
> 
> I agree. These calls have already been changing far too often in
> mainline. I know it's all been necessary but please, can we try to make
> one set of changes and just get it right this time?

In practical terms, it will be easier to keep the freezer where it is 
for now.  This is because prepare() requires drivers to change their 
behavior; they aren't allowed to register new children any more.

But if userspace isn't frozen then user programs can interact with
drivers in a way that does cause new children to be created.  For
example this happens in USB, where opening an audio device and
selecting its bitrate causes a new set of endpoints to be realized,
along with their representations in sysfs.

Thus, in addition to making all the interface changes implied by the 
new callbacks, drivers would also have to change the way they interact 
with userspace.  Yes, this will have to be done eventually in any case, 
as the freezer goes away -- but it shouldn't have to be done right now.

Alan Stern

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Nigel Cunningham]

Hi.

On Mon, 2008-04-14 at 01:17 +0200, Rafael J. Wysocki wrote:
> On Monday, 14 of April 2008, Nigel Cunningham wrote:
> > Hi Rafael etc.
> 
> Hi,
>  
> > On Mon, 2008-04-14 at 08:47 +1000, Benjamin Herrenschmidt wrote:
> > > > Well, I'm not sure and I'm not going to introduce the change right now, after
> > > > the paches have been included in -mm.
> > > > 
> > > > That would require quite some changes in the core code that I'd prefer to
> > > > avoid for now.  We can do something like this in a separate patch series after
> > > > the present one settles down a bit.
> > > 
> > > I disagree.
> > > 
> > > Doing it later would introduce yet another major semantic change.
> > > 
> > > I think we should get it right now. There's no hurry in pushing things
> > > especially if they aren't quite right.
> > > 
> > > The ability for prepare() callbacks to sync with userland,
> > > request_firmware, etc... is an important feature that's been needed for
> > > some time imho.
> > > 
> > > Ben.
> > 
> > I agree. These calls have already been changing far too often in
> > mainline. I know it's all been necessary but please, can we try to make
> > one set of changes and just get it right this time?
> 
> Sorry, what exactly has been changing too often?  Device suspend callbacks??
> They haven't changed since pm_message_t was introduced.

I'm thinking of driver models calls for both hibernation and suspend to
ram, both names and semantics as to what is called when.

Regards,

Nigel

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 01:08 +0200, Rafael J. Wysocki wrote:
> 
> Well, if we put ->prepare() before the freezer, what can it actually do?
> Certainly nothing that will block any (user space) task, because the freezer
> won't work after that.  So, all of the significant suspend work, like blocking
> tasks using the device etc., will have to be done by ->suspend().  Will that be
> convenient?  I'm not sure.  I'm not even sure that would be _doable_ at all.

Most of the actual suspending of requests queues etc... has to be done
in suspend. Nothing changed here. I don't see why it would change.
prepare() is a way to preallocate things when needed, cache things when
needed, etc... and possibly interact user space. At least that how I see
it.

It -is- acceptable to stop servicing user space after prepare() in some
well defined cases such as the DRM, as long as in-kernel users aren't
affected. But that's not necessarily what I have in mind. In the case us
userspace for example, the idea here is to perform the migration of all
the dirty data in the VRAM (that will be lost during suspend) out to
main memory (or AGP memory). That doesn't mean necessarily that the DRM
will stop servicing anything from there, it can still perform things. It
might mean that user space would have to disable some accelerations that
rely on dirty data in VRAM though until complete() is called.

> The point of view depends on what you think ->prepare() should be used for
> and I'm sure you have some specific cases in mind.  However, are they generic
> enough?

request_firmware() & caching the resulting firmware so that the driver
can resume & start operating right away is an example (think about NFS
over wireless here for example, or iSCSI). the DRM example above. In
general, any driver that needs to interact with user space, perform
large allocations, muck around with the VM, etc...

prepare() isn't intended to stop operations, but to
preload/cache/prepare and have userspace available for drivers where
this is needed. In some case, that might need the driver will continue
operating in some degraded mode (for example, a multiqueue driver can
degrade to a single queue with one pre-allocated request to avoid
relying on allocations).

I would expect most simple drivers not to need it of course.

Cheers,
Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> On Mon, 2008-04-14 at 01:08 +0200, Rafael J. Wysocki wrote:
> > 
> > Well, if we put ->prepare() before the freezer, what can it actually do?
> > Certainly nothing that will block any (user space) task, because the freezer
> > won't work after that.  So, all of the significant suspend work, like blocking
> > tasks using the device etc., will have to be done by ->suspend().  Will that be
> > convenient?  I'm not sure.  I'm not even sure that would be _doable_ at all.
> 
> Most of the actual suspending of requests queues etc... has to be done
> in suspend. Nothing changed here. I don't see why it would change.
> prepare() is a way to preallocate things when needed, cache things when
> needed, etc... and possibly interact user space. At least that how I see
> it.
> 
> It -is- acceptable to stop servicing user space after prepare() in some
> well defined cases such as the DRM, as long as in-kernel users aren't
> affected. But that's not necessarily what I have in mind. In the case us
> userspace for example, the idea here is to perform the migration of all
> the dirty data in the VRAM (that will be lost during suspend) out to
> main memory (or AGP memory). That doesn't mean necessarily that the DRM
> will stop servicing anything from there, it can still perform things. It
> might mean that user space would have to disable some accelerations that
> rely on dirty data in VRAM though until complete() is called.
> 
> > The point of view depends on what you think ->prepare() should be used for
> > and I'm sure you have some specific cases in mind.  However, are they generic
> > enough?
> 
> request_firmware() & caching the resulting firmware so that the driver
> can resume & start operating right away is an example (think about NFS
> over wireless here for example, or iSCSI). the DRM example above. In
> general, any driver that needs to interact with user space, perform
> large allocations, muck around with the VM, etc...
> 
> prepare() isn't intended to stop operations, but to
> preload/cache/prepare and have userspace available for drivers where
> this is needed. In some case, that might need the driver will continue
> operating in some degraded mode (for example, a multiqueue driver can
> degrade to a single queue with one pre-allocated request to avoid
> relying on allocations).
> 
> I would expect most simple drivers not to need it of course.

Well, in our discussions with Alan Stern ->prepare() turned out to be necessary
for exactly one reason, preventing new children of the device from being
registered (by threads concurrent wrt the suspend thread).  For this reason,
it doesn't really seem a good idea to run it before the freezer (seemingly, it
would be difficult to avoid situations in which the freezer would fail as a
result of ->prepare()).

It looks like you'd like to have a third callback executed before the freezer,
but OTOH I don't see the reason not to use a notifier for such things.

I have imagined that while we have the freezer, the operations that need to
be carried out with the user space available will be done using notifiers
and the rest will be done by ->prepare() and ->suspend().  Next, when we
finally drop the freezer, it will be possible to move the code from the
notifiers into ->prepare() and drop the notifiers altogether.

Since, as you said, there aren't too many drivers that will need anything like
that, it seems perfectly doable to me.

Thanks,
Rafael


> 
> Cheers,
> Ben.
> 
> 
> 
> 
> 



-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 02:31 +0200, Rafael J. Wysocki wrote:
> 
> Well, in our discussions with Alan Stern ->prepare() turned out to be necessary
> for exactly one reason, preventing new children of the device from being
> registered (by threads concurrent wrt the suspend thread).  For this reason,
> it doesn't really seem a good idea to run it before the freezer (seemingly, it
> would be difficult to avoid situations in which the freezer would fail as a
> result of ->prepare()).

I'm opposed to designing something around the freezer since we know it
will ultimately go away.

If things like USB have issues with userland doing nasty things after
prepare(), then those things need to be fixed. The freezer will only
hide bugs and not even always or properly and not on all archs.

> It looks like you'd like to have a third callback executed before the freezer,
> but OTOH I don't see the reason not to use a notifier for such things.

That's just gratuituous complication imho. We can add callbacks every
week and no driver will every find out what to use and when.

prepare() has quite well defined and nice semantics if you ignore your
freezer trickery. It matches well with the needs of things like
request_firmware or the DRM, and possibly a few others, in addition to
matching well the need to block bus discovery.

If some drivers have issue because of what userland might do after
prepare(), then those drivers need to be fixed. We all know the freezer
is not a proper solution. It just hides problems and not always
correctly.

> I have imagined that while we have the freezer, the operations that need to
> be carried out with the user space available will be done using notifiers
> and the rest will be done by ->prepare() and ->suspend().  Next, when we
> finally drop the freezer, it will be possible to move the code from the
> notifiers into ->prepare() and drop the notifiers altogether.

Why do this two steps ? What is the point ?

> Since, as you said, there aren't too many drivers that will need anything like
> that, it seems perfectly doable to me.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> On Mon, 2008-04-14 at 02:31 +0200, Rafael J. Wysocki wrote:
> > 
> > Well, in our discussions with Alan Stern ->prepare() turned out to be necessary
> > for exactly one reason, preventing new children of the device from being
> > registered (by threads concurrent wrt the suspend thread).  For this reason,
> > it doesn't really seem a good idea to run it before the freezer (seemingly, it
> > would be difficult to avoid situations in which the freezer would fail as a
> > result of ->prepare()).
> 
> I'm opposed to designing something around the freezer since we know it
> will ultimately go away.

Well, I think the freezer is not anything we can remove overnight.  It's there
and does something our users rely on, whether we like it or not.  That means
we have to provide the users with other things to rely on and _then_
remove the freezer, not the other way around.  Hence, we need to design
around it, otherwise the users will suffer.
 
> If things like USB have issues with userland doing nasty things after
> prepare(), then those things need to be fixed. The freezer will only
> hide bugs and not even always or properly and not on all archs.
> 
> > It looks like you'd like to have a third callback executed before the freezer,
> > but OTOH I don't see the reason not to use a notifier for such things.
> 
> That's just gratuituous complication imho. We can add callbacks every
> week and no driver will every find out what to use and when.
> 
> prepare() has quite well defined and nice semantics if you ignore your
> freezer trickery. It matches well with the needs of things like
> request_firmware or the DRM, and possibly a few others, in addition to
> matching well the need to block bus discovery.
> 
> If some drivers have issue because of what userland might do after
> prepare(), then those drivers need to be fixed. We all know the freezer
> is not a proper solution. It just hides problems and not always
> correctly.
> 
> > I have imagined that while we have the freezer, the operations that need to
> > be carried out with the user space available will be done using notifiers
> > and the rest will be done by ->prepare() and ->suspend().  Next, when we
> > finally drop the freezer, it will be possible to move the code from the
> > notifiers into ->prepare() and drop the notifiers altogether.
> 
> Why do this two steps ? What is the point ?

To avoid breaking things (from the functional point of view) unnecessarily.

In short, I don't really see the difference between moving ->prepare() before
the freezer and droppig the freezer, which I'm not going to do right now.

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Nigel Cunningham]

Hi.

On Mon, 2008-04-14 at 10:46 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2008-04-14 at 02:31 +0200, Rafael J. Wysocki wrote:
> > 
> > Well, in our discussions with Alan Stern ->prepare() turned out to be necessary
> > for exactly one reason, preventing new children of the device from being
> > registered (by threads concurrent wrt the suspend thread).  For this reason,
> > it doesn't really seem a good idea to run it before the freezer (seemingly, it
> > would be difficult to avoid situations in which the freezer would fail as a
> > result of ->prepare()).
> 
> I'm opposed to designing something around the freezer since we know it
> will ultimately go away.

I don't think we should be assuming that the freezer will ultimately go
away. Aiming for that is one thing, assuming that we know what will
happen in the future is another. Frankly, I hope it doesn't go away,
because right now it's the only way I can see that we can capture as
close as possible to a complete image of memory for hibernation. I know
that some people don't care about that option, but others of us find it
extremely useful.

> If things like USB have issues with userland doing nasty things after
> prepare(), then those things need to be fixed. The freezer will only
> hide bugs and not even always or properly and not on all archs.

Having said the above, I also agree with this. I want to see things
being done properly too. I believe the freezer still has a place when it
comes to providing the environment in which we can capture a consistent
image for hibernation, but that shouldn't get in the way of kernel
drivers (or even well designed userspace drivers) doing their thing,
including reloading firmware post-atomic restore or post-suspend-to-ram.

> > It looks like you'd like to have a third callback executed before the freezer,
> > but OTOH I don't see the reason not to use a notifier for such things.
> 
> That's just gratuituous complication imho. We can add callbacks every
> week and no driver will every find out what to use and when.
> 
> prepare() has quite well defined and nice semantics if you ignore your
> freezer trickery. It matches well with the needs of things like
> request_firmware or the DRM, and possibly a few others, in addition to
> matching well the need to block bus discovery.
> 
> If some drivers have issue because of what userland might do after
> prepare(), then those drivers need to be fixed. We all know the freezer
> is not a proper solution. It just hides problems and not always
> correctly.

Not a proper/complete solution to every problem.

> > I have imagined that while we have the freezer, the operations that need to
> > be carried out with the user space available will be done using notifiers
> > and the rest will be done by ->prepare() and ->suspend().  Next, when we
> > finally drop the freezer, it will be possible to move the code from the
> > notifiers into ->prepare() and drop the notifiers altogether.
> 
> Why do this two steps ? What is the point ?

1 step for pre-freezing prepare (load firmware you might want to restore
later, allocate memory) and 1 step for post-freezing prepare (stop
driver discovery)? I'm not sure that this is what Rafael has in mind,
though.

Nigel

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

> To avoid breaking things (from the functional point of view) unnecessarily.
> 
> In short, I don't really see the difference between moving ->prepare() before
> the freezer and droppig the freezer, which I'm not going to do right now.

I believe the use of prepare for things like request_firmware etc... is
worth the effort of fixing the known breakage of not having the freezer
while preventing insertion of new devices (mostly USB). In fact, it
won't be such a big issue as the core should/will return an error from
attempting to add the device in that case.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Oliver Neukum]

Am Montag, 14. April 2008 05:23:00 schrieb Benjamin Herrenschmidt:
> 
> > To avoid breaking things (from the functional point of view) unnecessarily.
> > 
> > In short, I don't really see the difference between moving ->prepare() before
> > the freezer and droppig the freezer, which I'm not going to do right now.
> 
> I believe the use of prepare for things like request_firmware etc... is
> worth the effort of fixing the known breakage of not having the freezer
> while preventing insertion of new devices (mostly USB). In fact, it
> won't be such a big issue as the core should/will return an error from
> attempting to add the device in that case.

Which is the problem. Suspension is supposed to be transparent.
We cannot start returning error codes for operations which never
failed in practice (eg. switching configurations in USB), just because
the system is about to be suspended.

If you want to request firmware in a PM callback, which makes a certain
sense, as we should move to a comprehensive API, if we change the API
at all, we need a model with 3 callbacks.

	Regards
		Oliver

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 08:43 +0200, Oliver Neukum wrote:
> 
> Which is the problem. Suspension is supposed to be transparent.
> We cannot start returning error codes for operations which never
> failed in practice (eg. switching configurations in USB), just because
> the system is about to be suspended.

Returning errors is better than crashing in any way. So what I meant
here is that the problem is not as bad as it sounds.

> If you want to request firmware in a PM callback, which makes a
> certain
> sense, as we should move to a comprehensive API, if we change the API
> at all, we need a model with 3 callbacks.

No. At this pace, we'll find reasons to have 98213674 callbacks and will
still not be happy.

Prepare() should be the right place to call request_firmware() and if
that is a problem because of bugs in some USB things, then those bugs
should be fixed.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Oliver Neukum]

Am Montag, 14. April 2008 09:23:36 schrieb Benjamin Herrenschmidt:
> 
> On Mon, 2008-04-14 at 08:43 +0200, Oliver Neukum wrote:

> > If you want to request firmware in a PM callback, which makes a
> > certain
> > sense, as we should move to a comprehensive API, if we change the API
> > at all, we need a model with 3 callbacks.
> 
> No. At this pace, we'll find reasons to have 98213674 callbacks and will
> still not be happy.
> 
> Prepare() should be the right place to call request_firmware() and if
> that is a problem because of bugs in some USB things, then those bugs
> should be fixed.

This isn't a bug. USB simply needs to be able to register (and deregister)
children to be fully operative. You cannot expect a subsystem to work
while some core services are not available.

	Regards
		Oliver

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 09:37 +0200, Oliver Neukum wrote:
> > Prepare() should be the right place to call request_firmware() and if
> > that is a problem because of bugs in some USB things, then those bugs
> > should be fixed.
> 
> This isn't a bug. USB simply needs to be able to register (and deregister)
> children to be fully operative. You cannot expect a subsystem to work
> while some core services are not available.

No, and that's why the subsystem in question needs to nicely defer the
operations that it cannot do immediately to until resume. Nothing new
here.

In the case of USB, there are plenty of ways it could be done, from
blocking in whatever ioctl is causing trouble, to putting to-be-added
objects in a list that gets processed later...

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> > To avoid breaking things (from the functional point of view) unnecessarily.
> > 
> > In short, I don't really see the difference between moving ->prepare() before
> > the freezer and droppig the freezer, which I'm not going to do right now.
> 
> I believe the use of prepare for things like request_firmware etc... is
> worth the effort of fixing the known breakage of not having the freezer
> while preventing insertion of new devices (mostly USB). In fact, it
> won't be such a big issue as the core should/will return an error from
> attempting to add the device in that case.

Okay, this was a shortcut.  [If we're discussing things when there's 2 am here,
I'm at a big disadvantage. :-)]

I think we agree that finally there should be no freezer and the things should
go like this:

(1) notifiers
(2) platform ->begin()
(3) ->prepare()
(4) ->suspend()
(5) platform ->prepare()
(6) ->suspend_noirq()
(7) platform ->enter()
(8) (we are in the sleep state)
(9) ->resume_noirq()
(10) platform ->finish()
(11) ->resume()
(12) ->complete()
(13) platform ->end()
(14) notifiers

Now, the question arises how to reach that status from what we have at the
moment and IMO there are two ways to go.

The first one is what I've implemented in the $subject patch:
(1) make ->prepare() be executed after the freezer with the current semantics
    (ie. you can assume the user space do be there when ->prepare() is running
    and use a notifier for things depending on that assumption)
(2) after we've dropped the freezer, change the semantics of ->prepare() (now
    you can assume that the user space is there)
(3) move code from notifiers to ->prepare() - this is entirely optional from
    the functionality point of view, the code can stay in the notifiers.
The advantage of this is that it doesn't require nontrivial modifications of
the core suspend/hibernation code.

The second one is a bit more complicated:
(1) make ->prepare() be executed before the freezer with the semantics like
    "you can assume that the user space is there while ->prepare() is running,
    but you are supposed to prevent new children of the device from being
    registered from that point on _and_ you have to make sure that freezable
    tasks will be able to freeze after ->prepare() has run" (but why on Earth a
    driver writer is now required to know what's a freezable task etc.?)
(2) after we've dropped the freezer, change the semantics of ->prepare() (now
    there are no freezable tasks to care for)
(3) remove the code necessary to make the freezer happy from ->prepare()
    (if any) - this need not be totally optional, IMO
Also, to implement this we'd have to change the core code (ie. add two
additional routines, device_prepare() and device_complete() to be called by it,
rework the error paths, move the platform ->begin()/->end() invocations
before/after the freezer etc.).  In the end, we'll end up with a more
complicated core in this case (we can simplify it afterwards, but that means
one patchset more).

[See how the semantics of ->prepare() changes in both cases, BTW.  We can't
avoid changing it in the future, we can only choose _how_ to change it.]

I prefer the first one quite a lot.

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Alan Stern]

On Mon, 14 Apr 2008, Benjamin Herrenschmidt wrote:

> 
> > To avoid breaking things (from the functional point of view) unnecessarily.
> > 
> > In short, I don't really see the difference between moving ->prepare() before
> > the freezer and droppig the freezer, which I'm not going to do right now.
> 
> I believe the use of prepare for things like request_firmware etc... is
> worth the effort of fixing the known breakage of not having the freezer
> while preventing insertion of new devices (mostly USB).

I used USB as an example simply because that's what I'm familiar with.  
Don't be so quick to assume the rest of the kernel is trouble-free.

> In fact, it
> won't be such a big issue as the core should/will return an error from
> attempting to add the device in that case.

But it _would_ be a regression -- and everyone knows how much Linus and 
Andrew dislike regressions.  :-)

Now, I don't see what the big deal is here.  We all agree that it would 
be nice and appropriate if drivers could do things like 
request_firmware() during prepare().  But for the moment they can't.  
It's not like this is some tremendous hardship -- you aren't going to 
have to rip out all the existing prepare() implementations and rewrite 
them, because they don't exist yet!

All it means is that the full conversion to the new interface will be a 
multi-step process.  Right now we add the new callbacks.  Later on we 
make it safe to move the freezer after prepare.  Later still we 
eliminate the freezer altogether from suspend.  (Hibernation is a 
separate matter...)

Alan Stern

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Nigel Cunningham wrote:
> Hi.
> 
> On Mon, 2008-04-14 at 10:46 +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2008-04-14 at 02:31 +0200, Rafael J. Wysocki wrote:
> > > 
> > > Well, in our discussions with Alan Stern ->prepare() turned out to be necessary
> > > for exactly one reason, preventing new children of the device from being
> > > registered (by threads concurrent wrt the suspend thread).  For this reason,
> > > it doesn't really seem a good idea to run it before the freezer (seemingly, it
> > > would be difficult to avoid situations in which the freezer would fail as a
> > > result of ->prepare()).
> > 
> > I'm opposed to designing something around the freezer since we know it
> > will ultimately go away.
> 
> I don't think we should be assuming that the freezer will ultimately go
> away. Aiming for that is one thing, assuming that we know what will
> happen in the future is another.

I agree.

> Frankly, I hope it doesn't go away, because right now it's the only way I
> can see that we can capture as close as possible to a complete image of
> memory for hibernation. I know that some people don't care about that option,
> but others of us find it extremely useful.

Well, I think there's another way, but that's for a separate discussion. :-)

> > If things like USB have issues with userland doing nasty things after
> > prepare(), then those things need to be fixed. The freezer will only
> > hide bugs and not even always or properly and not on all archs.
> 
> Having said the above, I also agree with this. I want to see things
> being done properly too. I believe the freezer still has a place when it
> comes to providing the environment in which we can capture a consistent
> image for hibernation, but that shouldn't get in the way of kernel
> drivers (or even well designed userspace drivers) doing their thing,
> including reloading firmware post-atomic restore or post-suspend-to-ram.

Unfortunately, if ->prepare() is to be run before the freezer, the writers of
the ->prepare() routines will have to make sure they won't block any freezable
tasks, so that the freezer doesn't fail.  This, in turn, will require them to
learn how the freezer works etc., which I'm not quite sure is really necessary.

> > > It looks like you'd like to have a third callback executed before the freezer,
> > > but OTOH I don't see the reason not to use a notifier for such things.
> > 
> > That's just gratuituous complication imho. We can add callbacks every
> > week and no driver will every find out what to use and when.
> > 
> > prepare() has quite well defined and nice semantics if you ignore your
> > freezer trickery. It matches well with the needs of things like
> > request_firmware or the DRM, and possibly a few others, in addition to
> > matching well the need to block bus discovery.
> > 
> > If some drivers have issue because of what userland might do after
> > prepare(), then those drivers need to be fixed. We all know the freezer
> > is not a proper solution. It just hides problems and not always
> > correctly.
> 
> Not a proper/complete solution to every problem.
> 
> > > I have imagined that while we have the freezer, the operations that need to
> > > be carried out with the user space available will be done using notifiers
> > > and the rest will be done by ->prepare() and ->suspend().  Next, when we
> > > finally drop the freezer, it will be possible to move the code from the
> > > notifiers into ->prepare() and drop the notifiers altogether.
> > 
> > Why do this two steps ? What is the point ?
> 
> 1 step for pre-freezing prepare (load firmware you might want to restore
> later, allocate memory) and 1 step for post-freezing prepare (stop
> driver discovery)? I'm not sure that this is what Rafael has in mind,
> though.

Yes, sort of.

I think that, as long as there is the freezer, we need two "preparation" steps
in general, one to be executed with the user space available and one to be
executed after it's been frozen.  The difference between them is quite clear
to me and trying to forcibly merge them into one routine will eventually lead
to problems.

If the freezer is not there, though, we can merge the two just fine.  This
seems to be a natural way to go to me and that's why I'm trying to use this
approach.

Thanks,
Rafael

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[David Brownell]

On Sunday 13 April 2008, Benjamin Herrenschmidt wrote:
> I think we should get it right now. There's no hurry in pushing things
> especially if they aren't quite right.

There is Rafael's patience to remember... (rev 8?)


> The ability for prepare() callbacks to sync with userland,
> request_firmware, etc... is an important feature that's been needed for
> some time imho.

I sort of agree.  Looking at it from a whole-system perspective,
suspending needs to be able to chitchat with userspace ... and I
don't think that can be done *before* writing to /sys/power/state
in an acceptably generic/portable way.  (Briefly, applications
need to have clean stopping points and be able to arrange system
wakeup.  They may well have more work to do than most drivers.)

But also, not all of that chitchat would naturally be associated
with any particular device(s).  So it's not clear to me that a
prepare() is what should oversee that... or that we've had real
discussion (yet) about the requirements there.


Rafael wrote:

> It looks like you'd like to have a third callback executed before the
> freezer, but OTOH I don't see the reason not to use a notifier for such
> things.

Neither of those is actually a userspace notification/handshake.

- Dave

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Pavel Machek]

On Mon 2008-04-14 08:47:02, Benjamin Herrenschmidt wrote:
> 
> > Well, I'm not sure and I'm not going to introduce the change right now, after
> > the paches have been included in -mm.
> > 
> > That would require quite some changes in the core code that I'd prefer to
> > avoid for now.  We can do something like this in a separate patch series after
> > the present one settles down a bit.
> 
> I disagree.
> 
> Doing it later would introduce yet another major semantic change.

'allow load_firmware from prepare that was not allowed before' is not 'major'.

If you feel strongly about that, please prepare patch at the top of
Rafael's and we can discuss it.

But it really should be separate patch.
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, David Brownell wrote:
> On Sunday 13 April 2008, Benjamin Herrenschmidt wrote:
> > I think we should get it right now. There's no hurry in pushing things
> > especially if they aren't quite right.
> 
> There is Rafael's patience to remember... (rev 8?)

Yeah.  And rev. 1 was posted something like ... a month ago.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Alan Stern]

On Sun, 13 Apr 2008, David Brownell wrote:

> I sort of agree.  Looking at it from a whole-system perspective,
> suspending needs to be able to chitchat with userspace ... and I
> don't think that can be done *before* writing to /sys/power/state
> in an acceptably generic/portable way.  (Briefly, applications
> need to have clean stopping points and be able to arrange system
> wakeup.  They may well have more work to do than most drivers.)

Pavel's recent work aside, the only way to initiate a system sleep is 
from userspace.  So it seems natural for all application notifications 
to be made by the initiating program, perhaps via dbus.

Alan Stern

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 10:51 -0400, Alan Stern wrote:
> On Sun, 13 Apr 2008, David Brownell wrote:
> 
> > I sort of agree.  Looking at it from a whole-system perspective,
> > suspending needs to be able to chitchat with userspace ... and I
> > don't think that can be done *before* writing to /sys/power/state
> > in an acceptably generic/portable way.  (Briefly, applications
> > need to have clean stopping points and be able to arrange system
> > wakeup.  They may well have more work to do than most drivers.)
> 
> Pavel's recent work aside, the only way to initiate a system sleep is 
> from userspace.  So it seems natural for all application notifications 
> to be made by the initiating program, perhaps via dbus.

No. Certainly not via dbus.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Pavel Machek]

On Mon 2008-04-14 10:51:34, Alan Stern wrote:
> On Sun, 13 Apr 2008, David Brownell wrote:
> 
> > I sort of agree.  Looking at it from a whole-system perspective,
> > suspending needs to be able to chitchat with userspace ... and I
> > don't think that can be done *before* writing to /sys/power/state
> > in an acceptably generic/portable way.  (Briefly, applications
> > need to have clean stopping points and be able to arrange system
> > wakeup.  They may well have more work to do than most drivers.)
> 
> Pavel's recent work aside, the only way to initiate a system sleep is 
> from userspace.  So it seems natural for all application notifications 
> to be made by the initiating program, perhaps via dbus.

Actually, my plan is to do without notifications. Userspace should
tell kernel "I'm prepared, you can suspend any time now"...

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

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Benjamin Herrenschmidt]

On Mon, 2008-04-14 at 12:55 +0200, Pavel Machek wrote:
> On Mon 2008-04-14 08:47:02, Benjamin Herrenschmidt wrote:
> > 
> > > Well, I'm not sure and I'm not going to introduce the change right now, after
> > > the paches have been included in -mm.
> > > 
> > > That would require quite some changes in the core code that I'd prefer to
> > > avoid for now.  We can do something like this in a separate patch series after
> > > the present one settles down a bit.
> > 
> > I disagree.
> > 
> > Doing it later would introduce yet another major semantic change.
> 
> 'allow load_firmware from prepare that was not allowed before' is not 'major'.

Having user space fully operating is major.

Ben.

Re: [PATCH 1/3] PM: Introduce new top level suspend and hibernation callbacks (rev. 8)

[Rafael J. Wysocki]

On Monday, 14 of April 2008, Benjamin Herrenschmidt wrote:
> 
> On Mon, 2008-04-14 at 12:55 +0200, Pavel Machek wrote:
> > On Mon 2008-04-14 08:47:02, Benjamin Herrenschmidt wrote:
> > > 
> > > > Well, I'm not sure and I'm not going to introduce the change right now, after
> > > > the paches have been included in -mm.
> > > > 
> > > > That would require quite some changes in the core code that I'd prefer to
> > > > avoid for now.  We can do something like this in a separate patch series after
> > > > the present one settles down a bit.
> > > 
> > > I disagree.
> > > 
> > > Doing it later would introduce yet another major semantic change.
> > 
> > 'allow load_firmware from prepare that was not allowed before' is not 'major'.
> 
> Having user space fully operating is major.

Please tell me what exactly is wrong with doing things that depend on the
user space being available from notifiers.

Thanks,
Rafael