Skip to contentSkip to conversation

PM: Separate suspend and hibernation callbacks (highest level) - updated

RFC PATCH In review Rafael J. Wysocki 3 participants 19 messages Mar 2, 2008 – Mar 12, 2008

Subsystems (7)
x86 ACPI Driver core Suspend to RAM +3 more subsystems
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 PM: Separate suspend and hibernation callbacks (highest level) - updated

Hi,

Below is a new version of the patch that introduces the separation of suspend and hibernation callbacks on the highest level.

Relative to the previous version two additional callbacks have been added, ->prepare() and ->complete(), allowing to prepare the device for a "suspend" transition and to finalize the "resume" respectively.

Also, all callbacks and PM_EVENT_ messages are now described in the comments in include/linux/pm.h (the documentation in Documentation/power will be updated separately).

As previously, it is assumed that the existing (legacy) ->suspend() and ->resume() callbacks will be used for bus types, device classes and device types that don't implement the new callbacks. As a result, the patch doesn't introduce any visible functional changes.

The patch has been compilation tested on x86-64.

Comments welcome.

Thanks,
Rafael

---
Introduce 'struct pm_ops' representing a set of suspend and hibernation operations for bus types, device classes and device types.

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

---

 arch/x86/kernel/apm_32.c  |    4 
 drivers/base/power/main.c |  375 +++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h    |    8 
 include/linux/pm.h        |  193 ++++++++++++++++++++---
 kernel/power/disk.c       |   14 -
 kernel/power/main.c       |    4 
 6 files changed, 482 insertions(+), 116 deletions(-)
include/linux/pm.h +170 -23
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -110,11 +110,9 @@ extern void (*pm_power_off_prepare)(void
 
 struct device;
 
-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 +120,169 @@ 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.  Also, if a concurrent child
+ *	registration already in progress is detected by @prepare(), it should
+ *	wait for the registration to complete, block the subsequent
+ *	registrations of children and return -EAGAIN, so that the PM core can
+ *	execute it once again (after suspending the newly registered child) to
+ *	recover from the race condition.  This method is executed for all kinds
+ *	of suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, immediately following one of the resume
+ *	callbacks: @resume(), @thaw(), @restore(), or @recover().  Also executed
+ *	if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ *	immediately following a successful @prepare() fails.
+ *
+ * @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.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ *	Undo the changes made by the preceding @freeze(), so the device can be
+ *	operated in the same was 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.
+ *
+ * @quiesce: Hibernation-specific, executed after loading a hibernation image
+ *	and before restoring the contents of main memory from it.  Quiesce
+ *	operations so that the contents of main memory can be restored from the
+ *	image in a consistent way, but do NOT otherwise put the device into a
+ *	low power state and do NOT emit system wakeup events.  Do NOT save any
+ *	device settings in main memory.
+ *
+ * @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.  Do 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().
+ *
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce, so the device can be operated in the same was as
+ *	immediately before the failing transition.
+ */
+
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif
+};
+
+/**
+ * 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 ->quiesce() 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
+ *		->recover() and ->complete() for all devices.
+ */
+
+typedef struct pm_message {
+	int event;
+} pm_message_t;
+
+#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, })
+
+/*
+ * 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,20 +327,6 @@ 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;
@@ -190,11 +337,11 @@ struct dev_pm_info {
 #endif
 };
 
-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 void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
+
+extern int device_power_down(pm_message_t state);
 extern int device_suspend(pm_message_t state);
 extern int device_prepare_suspend(pm_message_t state);
 
drivers/base/power/main.c +293 -82

541 lines not on this page · raw message

include/linux/device.h +8 -0
Index: linux-2.6/include/linux/device.h
===================================================================
@@ -69,6 +69,9 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ops *pm;
+	struct pm_ops *pm_noirq;
+
 	struct bus_type_private *p;
 };
 
@@ -202,6 +205,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 +350,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 */
kernel/power/disk.c +7 -7
Index: linux-2.6/kernel/power/disk.c
===================================================================
@@ -224,7 +224,7 @@ 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();
 	return error;
@@ -280,7 +280,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:
@@ -301,7 +301,7 @@ static int resume_target_kernel(void)
 	int error;
 
 	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,7 +329,7 @@ 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();
 	return error;
@@ -350,7 +350,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 +362,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();
@@ -419,7 +419,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
kernel/power/main.c +2 -2
Index: linux-2.6/kernel/power/main.c
===================================================================
@@ -239,7 +239,7 @@ 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());
@@ -291,7 +291,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:
arch/x86/kernel/apm_32.c +2 -2
Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -1221,9 +1221,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);
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
#1 lore raw
Quoted 13 lines · Rafael J. Wysocki

Hi,

Below is a new version of the patch that introduces the separation of suspend and hibernation callbacks on the highest level.

Relative to the previous version two additional callbacks have been added, ->prepare() and ->complete(), allowing to prepare the device for a "suspend" transition and to finalize the "resume" respectively.

Also, all callbacks and PM_EVENT_ messages are now described in the comments in include/linux/pm.h (the documentation in Documentation/power will be updated separately).

Quoted 8 lines

As previously, it is assumed that the existing (legacy) ->suspend() and ->resume() callbacks will be used for bus types, device classes and device types that don't implement the new callbacks. As a result, the patch doesn't introduce any visible functional changes.

The patch has been compilation tested on x86-64.

Comments welcome.

On the whole this looks pretty good. A few comments are below.

Alan Stern

Quoted 7 lines
include/linux/pm.h +3 -0
@@ -122,6 +120,169 @@ 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:
+ *

include/linux/pm.h typedef struct pm_message { · line 135

Quoted 10 lines
include/linux/pm.h +10 -0
+ * @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.  Also, if a concurrent child
+ *	registration already in progress is detected by @prepare(), it should
+ *	wait for the registration to complete, block the subsequent
+ *	registrations of children and return -EAGAIN, so that the PM core can
+ *	execute it once again (after suspending the newly registered child) to
+ *	recover from the race condition.  This method is executed for all kinds
+ *	of suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().

Didn't we decide it would be better to call @prepare in a separate pass? In any case, drivers aren't in a position to detect concurrent registrations. The PM core shouldn't require them to return -EAGAIN; it can detect these registrations by itself.

It is also worth mentioning that subsystems should treat calls to probe methods the same as child registrations: wait for concurrent ones to complete and block subsequent ones. This will of course be handled automatically for probes initiating in the driver core.

Finally, there needs to be a new "prepared" field added to dev_pm_info, right next to the "sleeping" field. Mention that it will be set prior to calling @prepare and cleared prior to calling @complete. Even though it is read-only to drivers, they will find it useful.

include/linux/pm.h typedef struct pm_message { · line 190

Quoted 4 lines
include/linux/pm.h +4 -0
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce, so the device can be operated in the same was as

s/was/way/

Review typedef struct pm_message { · line 141

Quoted 2 lines
(quoted diff) +2 -0
+ *	immediately before the failing transition.
+ */

Don't you mean the preceding @poweroff or @quiesce? When recovering after @freeze(), you can use @thaw instead of @recover.

include/linux/pm.h typedef struct pm_message { · line 332

Quoted 3 lines
include/linux/pm.h +0 -0
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;

Add the new "prepared" field.

Quoted 10 lines
drivers/base/power/main.c +1 -3
-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");

drivers/base/power/main.c void device_pm_schedule_removal(struct d · line 248

Quoted 10 lines
drivers/base/power/main.c +7 -2
-		error = dev->bus->resume_early(dev);
-	}
+	if (!dev->bus)
+		goto End;
 
+	pm_dev_dbg(dev, state, "EARLY ");
+	if (dev->bus->pm_noirq)
+		error = pm_op(dev, dev->bus->pm_noirq, state);
+	else if (dev->bus->resume_early)
+		error = dev->bus->resume_early(dev);

Here and below, you have the pm_dev_dbg() call before the test for whether the method exists. This will generate a lot of useless debugging output. Although it makes the code a bit larger, IMO it would be to test for the existence of either the new or the legacy method before calling pm_dev_dbg(), like you do in the pm_op() routine.

drivers/base/power/main.c static void dpm_resume(void) · line 390

Quoted 11 lines
drivers/base/power/main.c +2 -2
-static void dpm_resume(void)
+static void dpm_resume(pm_message_t state)
 {
 	mutex_lock(&dpm_list_mtx);
 	all_sleeping = false;
@@ -241,7 +387,8 @@ static void dpm_resume(void)
 		list_move_tail(entry, &dpm_active);
 		dev->power.sleeping = false;
 		mutex_unlock(&dpm_list_mtx);
-		resume_device(dev);
+		resume_device(dev, state);

Clear dev->power.prepared.

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 627

Quoted 11 lines
drivers/base/power/main.c +2 -0
+		complete_device(dev, state);
 		mutex_lock(&dpm_list_mtx);
 	}
 	mutex_unlock(&dpm_list_mtx);
@@ -419,21 +623,29 @@ static int dpm_suspend(pm_message_t stat
 		struct device *dev = to_device(entry);
 
 		WARN_ON(dev->parent && dev->parent->power.sleeping);
+		mutex_unlock(&dpm_list_mtx);
 

If dev->power.prepared is already set, skip calling prepare_device(). Otherwise set dev->power.prepared and proceed.

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 631

Quoted 4 lines
drivers/base/power/main.c +4 -0
+		error = prepare_device(dev, state);
+
+		mutex_lock(&dpm_list_mtx);
+		if (error) {

Clear dev->power.prepared.

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 636

Quoted 5 lines
drivers/base/power/main.c +5 -0
+			if (error == -EAGAIN)
+				continue;
+			else
+				break;
+		}

At this point, if dev isn't still the last entry on dpm_active then start the next loop iteration. As a corollary, in case a suspend aborts partway through, after the resume code has put all the devices back onto dpm_active you will still have to run through the devices that were already on dpm_active, calling @complete if dev->power.prepared is set.

Quoted 7 lines
drivers/base/power/main.c +2 -0
 		dev->power.sleeping = true;
 		mutex_unlock(&dpm_list_mtx);
+
 		error = suspend_device(dev, state);
+
 		mutex_lock(&dpm_list_mtx);
 		if (error) {
#2 lore raw
In reply to Rafael J. Wysocki
Quoted 13 lines · Rafael J. Wysocki

Hi,

Below is a new version of the patch that introduces the separation of suspend and hibernation callbacks on the highest level.

Relative to the previous version two additional callbacks have been added, ->prepare() and ->complete(), allowing to prepare the device for a "suspend" transition and to finalize the "resume" respectively.

Also, all callbacks and PM_EVENT_ messages are now described in the comments in include/linux/pm.h (the documentation in Documentation/power will be updated separately).

Quoted 8 lines

As previously, it is assumed that the existing (legacy) ->suspend() and ->resume() callbacks will be used for bus types, device classes and device types that don't implement the new callbacks. As a result, the patch doesn't introduce any visible functional changes.

The patch has been compilation tested on x86-64.

Comments welcome.

On the whole this looks pretty good. A few comments are below.

Alan Stern

Quoted 7 lines
include/linux/pm.h +3 -0
@@ -122,6 +120,169 @@ 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:
+ *

include/linux/pm.h typedef struct pm_message { · line 135

Quoted 10 lines
include/linux/pm.h +10 -0
+ * @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.  Also, if a concurrent child
+ *	registration already in progress is detected by @prepare(), it should
+ *	wait for the registration to complete, block the subsequent
+ *	registrations of children and return -EAGAIN, so that the PM core can
+ *	execute it once again (after suspending the newly registered child) to
+ *	recover from the race condition.  This method is executed for all kinds
+ *	of suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().

Didn't we decide it would be better to call @prepare in a separate pass? In any case, drivers aren't in a position to detect concurrent registrations. The PM core shouldn't require them to return -EAGAIN; it can detect these registrations by itself.

It is also worth mentioning that subsystems should treat calls to probe methods the same as child registrations: wait for concurrent ones to complete and block subsequent ones. This will of course be handled automatically for probes initiating in the driver core.

Finally, there needs to be a new "prepared" field added to dev_pm_info, right next to the "sleeping" field. Mention that it will be set prior to calling @prepare and cleared prior to calling @complete. Even though it is read-only to drivers, they will find it useful.

include/linux/pm.h typedef struct pm_message { · line 190

Quoted 4 lines
include/linux/pm.h +4 -0
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce, so the device can be operated in the same was as

s/was/way/

Review typedef struct pm_message { · line 141

Quoted 2 lines
(quoted diff) +2 -0
+ *	immediately before the failing transition.
+ */

Don't you mean the preceding @poweroff or @quiesce? When recovering after @freeze(), you can use @thaw instead of @recover.

include/linux/pm.h typedef struct pm_message { · line 332

Quoted 3 lines
include/linux/pm.h +0 -0
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;

Add the new "prepared" field.

Quoted 10 lines
drivers/base/power/main.c +1 -3
-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");

drivers/base/power/main.c void device_pm_schedule_removal(struct d · line 248

Quoted 10 lines
drivers/base/power/main.c +7 -2
-		error = dev->bus->resume_early(dev);
-	}
+	if (!dev->bus)
+		goto End;
 
+	pm_dev_dbg(dev, state, "EARLY ");
+	if (dev->bus->pm_noirq)
+		error = pm_op(dev, dev->bus->pm_noirq, state);
+	else if (dev->bus->resume_early)
+		error = dev->bus->resume_early(dev);

Here and below, you have the pm_dev_dbg() call before the test for whether the method exists. This will generate a lot of useless debugging output. Although it makes the code a bit larger, IMO it would be to test for the existence of either the new or the legacy method before calling pm_dev_dbg(), like you do in the pm_op() routine.

drivers/base/power/main.c static void dpm_resume(void) · line 390

Quoted 11 lines
drivers/base/power/main.c +2 -2
-static void dpm_resume(void)
+static void dpm_resume(pm_message_t state)
 {
 	mutex_lock(&dpm_list_mtx);
 	all_sleeping = false;
@@ -241,7 +387,8 @@ static void dpm_resume(void)
 		list_move_tail(entry, &dpm_active);
 		dev->power.sleeping = false;
 		mutex_unlock(&dpm_list_mtx);
-		resume_device(dev);
+		resume_device(dev, state);

Clear dev->power.prepared.

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 627

Quoted 11 lines
drivers/base/power/main.c +2 -0
+		complete_device(dev, state);
 		mutex_lock(&dpm_list_mtx);
 	}
 	mutex_unlock(&dpm_list_mtx);
@@ -419,21 +623,29 @@ static int dpm_suspend(pm_message_t stat
 		struct device *dev = to_device(entry);
 
 		WARN_ON(dev->parent && dev->parent->power.sleeping);
+		mutex_unlock(&dpm_list_mtx);
 

If dev->power.prepared is already set, skip calling prepare_device(). Otherwise set dev->power.prepared and proceed.

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 631

Quoted 4 lines
drivers/base/power/main.c +4 -0
+		error = prepare_device(dev, state);
+
+		mutex_lock(&dpm_list_mtx);
+		if (error) {

Clear dev->power.prepared.

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 636

Quoted 5 lines
drivers/base/power/main.c +5 -0
+			if (error == -EAGAIN)
+				continue;
+			else
+				break;
+		}

At this point, if dev isn't still the last entry on dpm_active then start the next loop iteration. As a corollary, in case a suspend aborts partway through, after the resume code has put all the devices back onto dpm_active you will still have to run through the devices that were already on dpm_active, calling @complete if dev->power.prepared is set.

Quoted 7 lines
drivers/base/power/main.c +2 -0
 		dev->power.sleeping = true;
 		mutex_unlock(&dpm_list_mtx);
+
 		error = suspend_device(dev, state);
+
 		mutex_lock(&dpm_list_mtx);
 		if (error) {
#3 lore raw
Quoted 36 lines · Alan Stern
Quoted 11 lines · Rafael J. Wysocki

Below is a new version of the patch that introduces the separation of suspend and hibernation callbacks on the highest level.

Relative to the previous version two additional callbacks have been added, ->prepare() and ->complete(), allowing to prepare the device for a "suspend" transition and to finalize the "resume" respectively.

Also, all callbacks and PM_EVENT_ messages are now described in the comments in include/linux/pm.h (the documentation in Documentation/power will be updated separately).

Quoted 8 lines

As previously, it is assumed that the existing (legacy) ->suspend() and ->resume() callbacks will be used for bus types, device classes and device types that don't implement the new callbacks. As a result, the patch doesn't introduce any visible functional changes.

The patch has been compilation tested on x86-64.

Comments welcome.

On the whole this looks pretty good. A few comments are below.

Quoted 12 lines
(quoted diff) +8 -0
@@ -122,6 +120,169 @@ 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.  Also, if a concurrent child
+ *	registration already in progress is detected by @prepare(), it should
+ *	wait for the registration to complete, block the subsequent
Quoted 8 lines

Review typedef struct pm_message { · line 135

Quoted 5 lines
(quoted diff) +5 -0
+ *	registrations of children and return -EAGAIN, so that the PM core can
+ *	execute it once again (after suspending the newly registered child) to
+ *	recover from the race condition.  This method is executed for all kinds
+ *	of suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().

Didn't we decide it would be better to call @prepare in a separate pass?

Afterwards I realized that it would be racy. Namely, once a new device is registered, it may require ->prepare() to be called before ->suspend(), so if that happens after we've called ->prepare() for all devices, we'll be in trouble.

Quoted 3 lines

In any case, drivers aren't in a position to detect concurrent registrations. The PM core shouldn't require them to return -EAGAIN; it can detect these registrations by itself.

In the appended new version of the patch, the core does it. However, in theory there may be situations in which the core can't detect a race condition and recover from it, so I think it's reasonable to leave the possibility to signal such a condition by the driver.

Quoted 9 lines

It is also worth mentioning that subsystems should treat calls to probe methods the same as child registrations: wait for concurrent ones to complete and block subsequent ones. This will of course be handled automatically for probes initiating in the driver core.

Finally, there needs to be a new "prepared" field added to dev_pm_info, right next to the "sleeping" field. Mention that it will be set prior to calling @prepare and cleared prior to calling @complete. Even though it is read-only to drivers, they will find it useful.

Yes, I noticed that after sending the patch. I handled it in a slightly different way, by converting 'sleeping' into 'status' that may assum one of the three values: DPM_ON, DPM_SUSPENDING, DPM_OFF.

Quoted 5 lines

Review typedef struct pm_message { · line 139

Quoted 4 lines
(quoted diff) +4 -0
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce, so the device can be operated in the same was as
Quoted 7 lines

s/was/way/

Review typedef struct pm_message { · line 141

Quoted 2 lines
(quoted diff) +2 -0
+ *	immediately before the failing transition.
+ */

Don't you mean the preceding @poweroff or @quiesce? When recovering after @freeze(), you can use @thaw instead of @recover.

No, ->restore() is called after a failing ->poweroff(), because ->poweroff() may put the device into a low power state which ->recover() need not be able to handle.

Also, I'd like to reserve ->thaw() for being called only after creating the image and before saving it (in the future we may want some devices to stay quiescent while the image is being saved). At the same time, the operations necessary to recover after ->freeze() and ->quiesce() should really be the same (I don't see a reason why it may not be the case).

Quoted 5 lines

Review typedef struct pm_message { · line 144

Quoted 3 lines
(quoted diff) +0 -0
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;

Add the new "prepared" field.

Addressed by changing 'sleeping' to 'status'.

Quoted 15 lines
Quoted 15 lines
(quoted diff) +3 -5
-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");
-		error = dev->bus->resume_early(dev);
-	}
+	if (!dev->bus)
+		goto End;
Quoted 11 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+	pm_dev_dbg(dev, state, "EARLY ");
+	if (dev->bus->pm_noirq)
+		error = pm_op(dev, dev->bus->pm_noirq, state);
+	else if (dev->bus->resume_early)
+		error = dev->bus->resume_early(dev);

Here and below, you have the pm_dev_dbg() call before the test for whether the method exists. This will generate a lot of useless debugging output. Although it makes the code a bit larger, IMO it would be to test for the existence of either the new or the legacy method before calling pm_dev_dbg(), like you do in the pm_op() routine.

Okay, I'll do that in the future, although it's not yet implemented in the patch below.

The remaining comments should be addressed in the appended patch.

Thanks,
Rafael

---

 arch/x86/kernel/apm_32.c  |    4 
 drivers/base/power/main.c |  421 +++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h    |    8 
 include/linux/pm.h        |  216 +++++++++++++++++++++--
 kernel/power/disk.c       |   14 -
 kernel/power/main.c       |    4 
 6 files changed, 536 insertions(+), 131 deletions(-)
include/linux/pm.h +194 -22
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -110,11 +110,9 @@ extern void (*pm_power_off_prepare)(void
 
 struct device;
 
-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 +120,175 @@ 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 and prevent new calls to the probe method from being made
+ *	after @prepare() returns.  Also, if a concurrent child registration
+ *	or a call to the probe method already in progress is detected by
+ *	@prepare(), it may return -EAGAIN, so that the PM core can execute it
+ *	once again (e.g. after suspending the newly registered child) to recover
+ *	from the race condition. This method is executed for all kinds of
+ *	suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ *	[NOTE: The PM core is able to detect a registration of a new child of a
+ *	device concurrent with this device's @prepare() callback, in which case
+ *	@complete() will be called for the device and the core will try again to
+ *	suspend it later.  However, the core is NOT able to handle a call to
+ *	the probe method concurrent with @prepare().]
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, immediately following one of the resume
+ *	callbacks: @resume(), @thaw(), @restore(), or @recover().  Also executed
+ *	if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ *	immediately following a successful @prepare() fails OR if @prepare()
+ *	returns -EAGAIN.
+ *
+ * @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.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ *	Undo the changes made by the preceding @freeze(), so the device can be
+ *	operated in the same was 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.
+ *
+ * @quiesce: Hibernation-specific, executed after loading a hibernation image
+ *	and before restoring the contents of main memory from it.  Quiesce
+ *	operations so that the contents of main memory can be restored from the
+ *	image in a consistent way, but do NOT otherwise put the device into a
+ *	low power state and do NOT emit system wakeup events.  Do NOT save any
+ *	device settings in main memory.
+ *
+ * @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.  Do 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().
+ *
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce(), so the device can be operated in the same way
+ *	as immediately before the failing transition.
+ */
+
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif
+};
+
+/**
+ * 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 ->quiesce() 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
+ *		->recover() and ->complete() for all devices.
+ */
+
+typedef struct pm_message {
+	int event;
+} pm_message_t;
+
+#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, })
+
+/*
+ * 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 +333,40 @@ 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)
+/**
+ * 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.
+ *
+ * DPM_SUSPENDING	Device is currently being suspended.
+ *
+ * DPM_OFF		Device is regarded as suspended.
+ */
 
-#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, })
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};
 
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;
-	bool			sleeping:1;	/* Owned by the PM core */
+	enum dpm_state		status:2;	/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	unsigned		should_wakeup:1;
 	struct list_head	entry;
 #endif
 };
 
-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 void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
+
+extern int device_power_down(pm_message_t state);
 extern int device_suspend(pm_message_t state);
 extern int device_prepare_suspend(pm_message_t state);
 
drivers/base/power/main.c +323 -98

596 lines not on this page · raw message

include/linux/device.h +8 -0
Index: linux-2.6/include/linux/device.h
===================================================================
@@ -69,6 +69,9 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ops *pm;
+	struct pm_ops *pm_noirq;
+
 	struct bus_type_private *p;
 };
 
@@ -202,6 +205,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 +350,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 */
kernel/power/disk.c +7 -7
Index: linux-2.6/kernel/power/disk.c
===================================================================
@@ -224,7 +224,7 @@ 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();
 	return error;
@@ -280,7 +280,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:
@@ -301,7 +301,7 @@ static int resume_target_kernel(void)
 	int error;
 
 	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,7 +329,7 @@ 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();
 	return error;
@@ -350,7 +350,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 +362,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();
@@ -419,7 +419,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
kernel/power/main.c +2 -2
Index: linux-2.6/kernel/power/main.c
===================================================================
@@ -239,7 +239,7 @@ 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());
@@ -291,7 +291,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:
arch/x86/kernel/apm_32.c +2 -2
Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -1221,9 +1221,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);
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
#4 lore raw
In reply to Alan Stern
Quoted 36 lines · Alan Stern
Quoted 11 lines · Rafael J. Wysocki

Below is a new version of the patch that introduces the separation of suspend and hibernation callbacks on the highest level.

Relative to the previous version two additional callbacks have been added, ->prepare() and ->complete(), allowing to prepare the device for a "suspend" transition and to finalize the "resume" respectively.

Also, all callbacks and PM_EVENT_ messages are now described in the comments in include/linux/pm.h (the documentation in Documentation/power will be updated separately).

Quoted 8 lines

As previously, it is assumed that the existing (legacy) ->suspend() and ->resume() callbacks will be used for bus types, device classes and device types that don't implement the new callbacks. As a result, the patch doesn't introduce any visible functional changes.

The patch has been compilation tested on x86-64.

Comments welcome.

On the whole this looks pretty good. A few comments are below.

Quoted 12 lines
(quoted diff) +8 -0
@@ -122,6 +120,169 @@ 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.  Also, if a concurrent child
+ *	registration already in progress is detected by @prepare(), it should
+ *	wait for the registration to complete, block the subsequent
Quoted 8 lines

Review typedef struct pm_message { · line 135

Quoted 5 lines
(quoted diff) +5 -0
+ *	registrations of children and return -EAGAIN, so that the PM core can
+ *	execute it once again (after suspending the newly registered child) to
+ *	recover from the race condition.  This method is executed for all kinds
+ *	of suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().

Didn't we decide it would be better to call @prepare in a separate pass?

Afterwards I realized that it would be racy. Namely, once a new device is registered, it may require ->prepare() to be called before ->suspend(), so if that happens after we've called ->prepare() for all devices, we'll be in trouble.

Quoted 3 lines

In any case, drivers aren't in a position to detect concurrent registrations. The PM core shouldn't require them to return -EAGAIN; it can detect these registrations by itself.

In the appended new version of the patch, the core does it. However, in theory there may be situations in which the core can't detect a race condition and recover from it, so I think it's reasonable to leave the possibility to signal such a condition by the driver.

Quoted 9 lines

It is also worth mentioning that subsystems should treat calls to probe methods the same as child registrations: wait for concurrent ones to complete and block subsequent ones. This will of course be handled automatically for probes initiating in the driver core.

Finally, there needs to be a new "prepared" field added to dev_pm_info, right next to the "sleeping" field. Mention that it will be set prior to calling @prepare and cleared prior to calling @complete. Even though it is read-only to drivers, they will find it useful.

Yes, I noticed that after sending the patch. I handled it in a slightly different way, by converting 'sleeping' into 'status' that may assum one of the three values: DPM_ON, DPM_SUSPENDING, DPM_OFF.

Quoted 5 lines

Review typedef struct pm_message { · line 139

Quoted 4 lines
(quoted diff) +4 -0
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce, so the device can be operated in the same was as
Quoted 7 lines

s/was/way/

Review typedef struct pm_message { · line 141

Quoted 2 lines
(quoted diff) +2 -0
+ *	immediately before the failing transition.
+ */

Don't you mean the preceding @poweroff or @quiesce? When recovering after @freeze(), you can use @thaw instead of @recover.

No, ->restore() is called after a failing ->poweroff(), because ->poweroff() may put the device into a low power state which ->recover() need not be able to handle.

Also, I'd like to reserve ->thaw() for being called only after creating the image and before saving it (in the future we may want some devices to stay quiescent while the image is being saved). At the same time, the operations necessary to recover after ->freeze() and ->quiesce() should really be the same (I don't see a reason why it may not be the case).

Quoted 5 lines

Review typedef struct pm_message { · line 144

Quoted 3 lines
(quoted diff) +0 -0
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;

Add the new "prepared" field.

Addressed by changing 'sleeping' to 'status'.

Quoted 15 lines
Quoted 15 lines
(quoted diff) +3 -5
-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");
-		error = dev->bus->resume_early(dev);
-	}
+	if (!dev->bus)
+		goto End;
Quoted 11 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+	pm_dev_dbg(dev, state, "EARLY ");
+	if (dev->bus->pm_noirq)
+		error = pm_op(dev, dev->bus->pm_noirq, state);
+	else if (dev->bus->resume_early)
+		error = dev->bus->resume_early(dev);

Here and below, you have the pm_dev_dbg() call before the test for whether the method exists. This will generate a lot of useless debugging output. Although it makes the code a bit larger, IMO it would be to test for the existence of either the new or the legacy method before calling pm_dev_dbg(), like you do in the pm_op() routine.

Okay, I'll do that in the future, although it's not yet implemented in the patch below.

The remaining comments should be addressed in the appended patch.

Thanks,
Rafael

---

 arch/x86/kernel/apm_32.c  |    4 
 drivers/base/power/main.c |  421 +++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h    |    8 
 include/linux/pm.h        |  216 +++++++++++++++++++++--
 kernel/power/disk.c       |   14 -
 kernel/power/main.c       |    4 
 6 files changed, 536 insertions(+), 131 deletions(-)
include/linux/pm.h +194 -22
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -110,11 +110,9 @@ extern void (*pm_power_off_prepare)(void
 
 struct device;
 
-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 +120,175 @@ 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 and prevent new calls to the probe method from being made
+ *	after @prepare() returns.  Also, if a concurrent child registration
+ *	or a call to the probe method already in progress is detected by
+ *	@prepare(), it may return -EAGAIN, so that the PM core can execute it
+ *	once again (e.g. after suspending the newly registered child) to recover
+ *	from the race condition. This method is executed for all kinds of
+ *	suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ *	[NOTE: The PM core is able to detect a registration of a new child of a
+ *	device concurrent with this device's @prepare() callback, in which case
+ *	@complete() will be called for the device and the core will try again to
+ *	suspend it later.  However, the core is NOT able to handle a call to
+ *	the probe method concurrent with @prepare().]
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, immediately following one of the resume
+ *	callbacks: @resume(), @thaw(), @restore(), or @recover().  Also executed
+ *	if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ *	immediately following a successful @prepare() fails OR if @prepare()
+ *	returns -EAGAIN.
+ *
+ * @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.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ *	Undo the changes made by the preceding @freeze(), so the device can be
+ *	operated in the same was 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.
+ *
+ * @quiesce: Hibernation-specific, executed after loading a hibernation image
+ *	and before restoring the contents of main memory from it.  Quiesce
+ *	operations so that the contents of main memory can be restored from the
+ *	image in a consistent way, but do NOT otherwise put the device into a
+ *	low power state and do NOT emit system wakeup events.  Do NOT save any
+ *	device settings in main memory.
+ *
+ * @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.  Do 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().
+ *
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce(), so the device can be operated in the same way
+ *	as immediately before the failing transition.
+ */
+
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif
+};
+
+/**
+ * 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 ->quiesce() 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
+ *		->recover() and ->complete() for all devices.
+ */
+
+typedef struct pm_message {
+	int event;
+} pm_message_t;
+
+#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, })
+
+/*
+ * 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 +333,40 @@ 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)
+/**
+ * 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.
+ *
+ * DPM_SUSPENDING	Device is currently being suspended.
+ *
+ * DPM_OFF		Device is regarded as suspended.
+ */
 
-#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, })
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};
 
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;
-	bool			sleeping:1;	/* Owned by the PM core */
+	enum dpm_state		status:2;	/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	unsigned		should_wakeup:1;
 	struct list_head	entry;
 #endif
 };
 
-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 void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
+
+extern int device_power_down(pm_message_t state);
 extern int device_suspend(pm_message_t state);
 extern int device_prepare_suspend(pm_message_t state);
 
drivers/base/power/main.c +323 -98

596 lines not on this page · raw message

include/linux/device.h +8 -0
Index: linux-2.6/include/linux/device.h
===================================================================
@@ -69,6 +69,9 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ops *pm;
+	struct pm_ops *pm_noirq;
+
 	struct bus_type_private *p;
 };
 
@@ -202,6 +205,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 +350,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 */
kernel/power/disk.c +7 -7
Index: linux-2.6/kernel/power/disk.c
===================================================================
@@ -224,7 +224,7 @@ 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();
 	return error;
@@ -280,7 +280,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:
@@ -301,7 +301,7 @@ static int resume_target_kernel(void)
 	int error;
 
 	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,7 +329,7 @@ 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();
 	return error;
@@ -350,7 +350,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 +362,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();
@@ -419,7 +419,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
kernel/power/main.c +2 -2
Index: linux-2.6/kernel/power/main.c
===================================================================
@@ -239,7 +239,7 @@ 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());
@@ -291,7 +291,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:
arch/x86/kernel/apm_32.c +2 -2
Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -1221,9 +1221,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);
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
#5 lore raw
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines

Didn't we decide it would be better to call @prepare in a separate pass?

Afterwards I realized that it would be racy. Namely, once a new device is registered, it may require ->prepare() to be called before ->suspend(), so if that happens after we've called ->prepare() for all devices, we'll be in trouble.

There's nothing wrong with doing it this way. However the race you're worried about will never happen if drivers are written correctly. Once prepare has been called for all devices, no new devices should be registered.

Quoted 8 lines
Quoted 3 lines

In any case, drivers aren't in a position to detect concurrent registrations. The PM core shouldn't require them to return -EAGAIN; it can detect these registrations by itself.

In the appended new version of the patch, the core does it. However, in theory there may be situations in which the core can't detect a race condition and recover from it, so I think it's reasonable to leave the possibility to signal such a condition by the driver.

I don't mind taking special action when drivers return -EAGAIN. The point is that drivers shouldn't be _required_ to do it.

Quoted 8 lines
Quoted 4 lines

Finally, there needs to be a new "prepared" field added to dev_pm_info, right next to the "sleeping" field. Mention that it will be set prior to calling @prepare and cleared prior to calling @complete. Even though it is read-only to drivers, they will find it useful.

Yes, I noticed that after sending the patch. I handled it in a slightly different way, by converting 'sleeping' into 'status' that may assum one of the three values: DPM_ON, DPM_SUSPENDING, DPM_OFF.

Okay, that should work just as well.

Quoted 7 lines

+ * @prepare: Prepare the device for the upcoming transition, but do NOT change
+ * its hardware state. Prevent new children of the device from being
+ * registered and prevent new calls to the probe method from being made
+ * after @prepare() returns. Also, if a concurrent child registration
+ * or a call to the probe method already in progress is detected by
+ * @prepare(), it may return -EAGAIN, so that the PM core can execute it
+ * once again (e.g. after suspending the newly registered child) to recover

Quoted 8 lines

+ * from the race condition. This method is executed for all kinds of
+ * suspend transitions and is immediately followed by one of the suspend
+ * callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ * [NOTE: The PM core is able to detect a registration of a new child of a
+ * device concurrent with this device's @prepare() callback, in which case
+ * @complete() will be called for the device and the core will try again to
+ * suspend it later. However, the core is NOT able to handle a call to
+ * the probe method concurrent with @prepare().]

I think you can leave out this NOTE. Direct probe method calls by drivers are pretty rare; almost all probing is done by the driver core.

Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * returns -EAGAIN.

The last part is wrong. If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called. What you mean is that @complete will be invoked immediately following a successful @prepare if the PM core detects that a new child was registered while @prepare was running.

include/linux/pm.h typedef struct pm_message { · line 353

Quoted 5 lines
include/linux/pm.h +5 -0
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};

There should be kerneldoc for dpm_state. Drivers may very well want to check dev->power.status to see whether a new child registration should be blocked. The documentation should be very clear that dev->power.status doesn't describe the device's actual power state; rather it describes which method calls the PM core has made or is about to make.

Quoted 18 lines
drivers/base/power/main.c +7 -6
@@ -71,22 +71,29 @@ 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 (all_sleeping) {
+		dev_warn(dev, "devices are sleeping, will not add\n");
+		goto Failure;
+	}
+	if (dev->parent) {
+		if (dev->parent->power.status == DPM_OFF) {
+			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");

drivers/base/power/main.c int device_pm_add(struct device *dev) · line 85

Quoted 10 lines
drivers/base/power/main.c +4 -6
-		WARN_ON(true);
-		error = -EBUSY;
-	} else {
-		error = dpm_sysfs_add(dev);
-		if (!error)
-			list_add_tail(&dev->power.entry, &dpm_active);
+			goto Failure;
+		} else if (dev->parent->power.status == DPM_SUSPENDING) {
+			dev->parent->power.status == DPM_ON;
+		}

This part puzzled me until I read the rest of the patch. It's a good way to look specifically for new _child_ registrations as opposed to any new registrations.

Quoted 6 lines
drivers/base/power/main.c +1 -1
@@ -418,22 +629,37 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
-		WARN_ON(dev->parent && dev->parent->power.sleeping);
+		WARN_ON(dev->parent && dev->parent->power.status == DPM_OFF);

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 642

Quoted 10 lines
drivers/base/power/main.c +10 -0
+		dev->power.status = DPM_SUSPENDING;
+		mutex_unlock(&dpm_list_mtx);
+
+		error = prepare_device(dev, state);
+
+		mutex_lock(&dpm_list_mtx);
+		if (error == -EAGAIN)
+			dev->power.status = DPM_ON;
+		else if(error)
+			break;

The last four lines should be:

		if (error) {
			dev->power.status = DPM_ON;
			if (error == -EAGAIN)
				continue;
			break;
		}

Insert: /* Was a child added during prepare_device()? */

Quoted 20 lines
drivers/base/power/main.c +10 -5
+		if (dev->power.status == DPM_ON) {
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
 
-		dev->power.sleeping = true;
+			mutex_lock(&dpm_list_mtx);
+			continue;
+		}
+		dev->power.status = DPM_OFF;
 		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,

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 662

Quoted 10 lines
drivers/base/power/main.c +5 -5
-					(error == -EAGAIN ?
-					" (please convert to suspend_late)" :
-					""));
-			dev->power.sleeping = false;
-			break;
+			dev->power.status = DPM_ON;
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
+			return error;

Early return? I guess it's okay...

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 665

Quoted 3 lines
drivers/base/power/main.c +0 -0
 		}
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_off);

Looks good.

Alan Stern

#6 lore raw
In reply to Rafael J. Wysocki
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines

Didn't we decide it would be better to call @prepare in a separate pass?

Afterwards I realized that it would be racy. Namely, once a new device is registered, it may require ->prepare() to be called before ->suspend(), so if that happens after we've called ->prepare() for all devices, we'll be in trouble.

There's nothing wrong with doing it this way. However the race you're worried about will never happen if drivers are written correctly. Once prepare has been called for all devices, no new devices should be registered.

Quoted 8 lines
Quoted 3 lines

In any case, drivers aren't in a position to detect concurrent registrations. The PM core shouldn't require them to return -EAGAIN; it can detect these registrations by itself.

In the appended new version of the patch, the core does it. However, in theory there may be situations in which the core can't detect a race condition and recover from it, so I think it's reasonable to leave the possibility to signal such a condition by the driver.

I don't mind taking special action when drivers return -EAGAIN. The point is that drivers shouldn't be _required_ to do it.

Quoted 8 lines
Quoted 4 lines

Finally, there needs to be a new "prepared" field added to dev_pm_info, right next to the "sleeping" field. Mention that it will be set prior to calling @prepare and cleared prior to calling @complete. Even though it is read-only to drivers, they will find it useful.

Yes, I noticed that after sending the patch. I handled it in a slightly different way, by converting 'sleeping' into 'status' that may assum one of the three values: DPM_ON, DPM_SUSPENDING, DPM_OFF.

Okay, that should work just as well.

Quoted 7 lines

+ * @prepare: Prepare the device for the upcoming transition, but do NOT change
+ * its hardware state. Prevent new children of the device from being
+ * registered and prevent new calls to the probe method from being made
+ * after @prepare() returns. Also, if a concurrent child registration
+ * or a call to the probe method already in progress is detected by
+ * @prepare(), it may return -EAGAIN, so that the PM core can execute it
+ * once again (e.g. after suspending the newly registered child) to recover

Quoted 8 lines

+ * from the race condition. This method is executed for all kinds of
+ * suspend transitions and is immediately followed by one of the suspend
+ * callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ * [NOTE: The PM core is able to detect a registration of a new child of a
+ * device concurrent with this device's @prepare() callback, in which case
+ * @complete() will be called for the device and the core will try again to
+ * suspend it later. However, the core is NOT able to handle a call to
+ * the probe method concurrent with @prepare().]

I think you can leave out this NOTE. Direct probe method calls by drivers are pretty rare; almost all probing is done by the driver core.

Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * returns -EAGAIN.

The last part is wrong. If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called. What you mean is that @complete will be invoked immediately following a successful @prepare if the PM core detects that a new child was registered while @prepare was running.

include/linux/pm.h typedef struct pm_message { · line 353

Quoted 5 lines
include/linux/pm.h +5 -0
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};

There should be kerneldoc for dpm_state. Drivers may very well want to check dev->power.status to see whether a new child registration should be blocked. The documentation should be very clear that dev->power.status doesn't describe the device's actual power state; rather it describes which method calls the PM core has made or is about to make.

Quoted 18 lines
drivers/base/power/main.c +7 -6
@@ -71,22 +71,29 @@ 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 (all_sleeping) {
+		dev_warn(dev, "devices are sleeping, will not add\n");
+		goto Failure;
+	}
+	if (dev->parent) {
+		if (dev->parent->power.status == DPM_OFF) {
+			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");

drivers/base/power/main.c int device_pm_add(struct device *dev) · line 85

Quoted 10 lines
drivers/base/power/main.c +4 -6
-		WARN_ON(true);
-		error = -EBUSY;
-	} else {
-		error = dpm_sysfs_add(dev);
-		if (!error)
-			list_add_tail(&dev->power.entry, &dpm_active);
+			goto Failure;
+		} else if (dev->parent->power.status == DPM_SUSPENDING) {
+			dev->parent->power.status == DPM_ON;
+		}

This part puzzled me until I read the rest of the patch. It's a good way to look specifically for new _child_ registrations as opposed to any new registrations.

Quoted 6 lines
drivers/base/power/main.c +1 -1
@@ -418,22 +629,37 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
-		WARN_ON(dev->parent && dev->parent->power.sleeping);
+		WARN_ON(dev->parent && dev->parent->power.status == DPM_OFF);

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 642

Quoted 10 lines
drivers/base/power/main.c +10 -0
+		dev->power.status = DPM_SUSPENDING;
+		mutex_unlock(&dpm_list_mtx);
+
+		error = prepare_device(dev, state);
+
+		mutex_lock(&dpm_list_mtx);
+		if (error == -EAGAIN)
+			dev->power.status = DPM_ON;
+		else if(error)
+			break;

The last four lines should be:

		if (error) {
			dev->power.status = DPM_ON;
			if (error == -EAGAIN)
				continue;
			break;
		}

Insert: /* Was a child added during prepare_device()? */

Quoted 20 lines
drivers/base/power/main.c +10 -5
+		if (dev->power.status == DPM_ON) {
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
 
-		dev->power.sleeping = true;
+			mutex_lock(&dpm_list_mtx);
+			continue;
+		}
+		dev->power.status = DPM_OFF;
 		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,

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 662

Quoted 10 lines
drivers/base/power/main.c +5 -5
-					(error == -EAGAIN ?
-					" (please convert to suspend_late)" :
-					""));
-			dev->power.sleeping = false;
-			break;
+			dev->power.status = DPM_ON;
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
+			return error;

Early return? I guess it's okay...

drivers/base/power/main.c static int dpm_suspend(pm_message_t stat · line 665

Quoted 3 lines
drivers/base/power/main.c +0 -0
 		}
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_off);

Looks good.

Alan Stern

#7 lore raw
Quoted 10 lines · Alan Stern
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines

Didn't we decide it would be better to call @prepare in a separate pass?

Afterwards I realized that it would be racy. Namely, once a new device is registered, it may require ->prepare() to be called before ->suspend(), so if that happens after we've called ->prepare() for all devices, we'll be in trouble.

Quoted 4 lines

There's nothing wrong with doing it this way. However the race you're worried about will never happen if drivers are written correctly. Once prepare has been called for all devices, no new devices should be registered.

That only applies to the devices with parents. Apart from this, the device can be registered concurrently with its parent's ->prepare() and in that case we should call ->complete() for the parent etc. IOW, almost the same complications as in dpm_suspend() would have to be taken into account.

[--snip--]

Quoted 16 lines
Quoted 7 lines

+ * @prepare: Prepare the device for the upcoming transition, but do NOT change
+ * its hardware state. Prevent new children of the device from being
+ * registered and prevent new calls to the probe method from being made
+ * after @prepare() returns. Also, if a concurrent child registration
+ * or a call to the probe method already in progress is detected by
+ * @prepare(), it may return -EAGAIN, so that the PM core can execute it
+ * once again (e.g. after suspending the newly registered child) to recover

Quoted 8 lines

+ * from the race condition. This method is executed for all kinds of
+ * suspend transitions and is immediately followed by one of the suspend
+ * callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ * [NOTE: The PM core is able to detect a registration of a new child of a
+ * device concurrent with this device's @prepare() callback, in which case
+ * @complete() will be called for the device and the core will try again to
+ * suspend it later. However, the core is NOT able to handle a call to
+ * the probe method concurrent with @prepare().]

Quoted 2 lines

I think you can leave out this NOTE. Direct probe method calls by drivers are pretty rare; almost all probing is done by the driver core.

Done.

Quoted 8 lines
Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * returns -EAGAIN.

The last part is wrong.

Yes, I noticed that too.

Quoted 1 line

If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called.

Hmm, I'd prefer to call it if ->prepare() fails too. The -EAGAIN case is special, because in fact there's no warranty that ->prepare() will be called again (for example, the new child's ->suspend() may fail before that happens), so we need to do the clean up after it. In general, if ->complete() is assumed to be called after a failing ->prepare() too, that simplifies the code a bit.

[--snip--]

Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +5 -0
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};

There should be kerneldoc for dpm_state.

In fact there is one, right above the definition of 'enum dpm_state'. It was placed a bit confusingly under the big comment about the legacy stuff. I changed the ordering of things in pm.h to avoid that confusion.

[--snip--]

Quoted 25 lines
Quoted 25 lines
(quoted diff) +7 -12
@@ -71,22 +71,29 @@ 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 (all_sleeping) {
+		dev_warn(dev, "devices are sleeping, will not add\n");
+		goto Failure;
+	}
+	if (dev->parent) {
+		if (dev->parent->power.status == DPM_OFF) {
+			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);
Quoted 8 lines

Review int device_pm_add(struct device *dev) · line 85

Quoted 4 lines
(quoted diff) +4 -0
+			goto Failure;
+		} else if (dev->parent->power.status == DPM_SUSPENDING) {
+			dev->parent->power.status == DPM_ON;
+		}

This part puzzled me until I read the rest of the patch. It's a good way to look specifically for new _child_ registrations as opposed to any new registrations.

I reworked that a bit.

Quoted 19 lines
Quoted 6 lines
(quoted diff) +1 -1
@@ -418,22 +629,37 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
-		WARN_ON(dev->parent && dev->parent->power.sleeping);
+		WARN_ON(dev->parent && dev->parent->power.status == DPM_OFF);

Review static int dpm_suspend(pm_message_t stat · line 642

Quoted 10 lines
(quoted diff) +10 -0
+		dev->power.status = DPM_SUSPENDING;
+		mutex_unlock(&dpm_list_mtx);
+
+		error = prepare_device(dev, state);
+
+		mutex_lock(&dpm_list_mtx);
+		if (error == -EAGAIN)
+			dev->power.status = DPM_ON;
+		else if(error)
+			break;

The last four lines should be:

Quoted 6 lines
		if (error) {
			dev->power.status = DPM_ON;
			if (error == -EAGAIN)
				continue;
			break;
		}

Well, I reworked this part too, but in a different way.

Quoted 1 line

Insert: /* Was a child added during prepare_device()? */

I added a different comment here.

Quoted 33 lines
Quoted 20 lines
(quoted diff) +10 -5
+		if (dev->power.status == DPM_ON) {
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
 
-		dev->power.sleeping = true;
+			mutex_lock(&dpm_list_mtx);
+			continue;
+		}
+		dev->power.status = DPM_OFF;
 		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,

Review static int dpm_suspend(pm_message_t stat · line 662

Quoted 10 lines
(quoted diff) +5 -5
-					(error == -EAGAIN ?
-					" (please convert to suspend_late)" :
-					""));
-			dev->power.sleeping = false;
-			break;
+			dev->power.status = DPM_ON;
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
+			return error;

Early return? I guess it's okay...

Quoted 5 lines

Review static int dpm_suspend(pm_message_t stat · line 665

Quoted 3 lines
(quoted diff) +0 -0
 		}
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_off);

Looks good.

Well, I reworked it somewhat in the meantime.

First, I moved the pm_dev_dbg() stuff into the inner if () blocks. Second, I made it possible to use ->prepare() and ->complete() in the "noirq" case as well and added a comment about the "noirq" callbacks. Finally, I cut some rough edges here and there (at least I hope so).

The new patch is appended.

Thanks,
Rafael

---

 arch/x86/kernel/apm_32.c  |    4 
 drivers/base/power/main.c |  463 +++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h    |    8 
 include/linux/pm.h        |  238 ++++++++++++++++++++---
 kernel/power/disk.c       |   14 -
 kernel/power/main.c       |    4 
 6 files changed, 579 insertions(+), 152 deletions(-)
include/linux/pm.h +206 -32
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -110,11 +110,9 @@ extern void (*pm_power_off_prepare)(void
 
 struct device;
 
-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 +120,206 @@ 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 and prevent new calls to the probe method from being made
+ *	after @prepare() returns.  Also, if a concurrent child registration
+ *	or a call to the probe method already in progress is detected by
+ *	@prepare(), it may return -EAGAIN, so that the PM core can execute it
+ *	once again (e.g. after suspending the newly registered child) to recover
+ *	from the race condition. This method is executed for all kinds of
+ *	suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, immediately following one of the resume
+ *	callbacks: @resume(), @thaw(), @restore(), or @recover().  Also executed
+ *	if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ *	immediately following a successful @prepare() fails OR if @prepare()
+ *	itself fails.  @complete() cannot fail.
+ *
+ * @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.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ *	Undo the changes made by the preceding @freeze(), so the device can be
+ *	operated in the same was 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.
+ *
+ * @quiesce: Hibernation-specific, executed after loading a hibernation image
+ *	and before restoring the contents of main memory from it.  Quiesce
+ *	operations so that the contents of main memory can be restored from the
+ *	image in a consistent way, but do NOT otherwise put the device into a
+ *	low power state and do NOT emit system wakeup events.  Do NOT save any
+ *	device settings in main memory.
+ *
+ * @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.  Do 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().
+ *
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce(), so the device can be operated in the same way
+ *	as immediately before the failing transition.
+ *
+ * struct pm_ops is also used for defining driver PM operations to be carried
+ * out with interrupts disabled.  In this case, however, there is only one
+ * active CPU while the operations are being performed, so they are executed by
+ * the PM core in a more straightforward way.  In particular, the failure of
+ * @prepare() carried out with interrupts disabled causes the entire PM
+ * transition to fail, regardless of the error code returned by it.
+ */
+
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif
+};
+
+/**
+ * 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 ->quiesce() 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
+ *		->recover() and ->complete() for all devices.
+ */
+
+typedef struct pm_message {
+	int event;
+} pm_message_t;
+
+#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.
+ *
+ * DPM_SUSPENDING	Device is currently being suspended.
+ *
+ * DPM_OFF		Device is regarded as suspended.
+ */
+
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};
+
+struct dev_pm_info {
+	pm_message_t		power_state;
+	unsigned		can_wakeup:1;
+	enum dpm_state		status:2;	/* Owned by the PM core */
+#ifdef	CONFIG_PM_SLEEP
+	unsigned		should_wakeup:1;
+	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 +364,11 @@ 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;
-	bool			sleeping:1;	/* Owned by the PM core */
-#ifdef	CONFIG_PM_SLEEP
-	unsigned		should_wakeup:1;
-	struct list_head	entry;
-#endif
-};
+#ifdef CONFIG_PM_SLEEP
+extern void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
 
 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);
 
drivers/base/power/main.c +354 -109

643 lines not on this page · raw message

include/linux/device.h +8 -0
Index: linux-2.6/include/linux/device.h
===================================================================
@@ -69,6 +69,9 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ops *pm;
+	struct pm_ops *pm_noirq;
+
 	struct bus_type_private *p;
 };
 
@@ -202,6 +205,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 +350,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 */
kernel/power/disk.c +7 -7
Index: linux-2.6/kernel/power/disk.c
===================================================================
@@ -224,7 +224,7 @@ 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();
 	return error;
@@ -280,7 +280,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:
@@ -301,7 +301,7 @@ static int resume_target_kernel(void)
 	int error;
 
 	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,7 +329,7 @@ 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();
 	return error;
@@ -350,7 +350,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 +362,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();
@@ -419,7 +419,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
kernel/power/main.c +2 -2
Index: linux-2.6/kernel/power/main.c
===================================================================
@@ -239,7 +239,7 @@ 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());
@@ -291,7 +291,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:
arch/x86/kernel/apm_32.c +2 -2
Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -1221,9 +1221,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);
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
#8 lore raw
In reply to Alan Stern
Quoted 10 lines · Alan Stern
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines

Didn't we decide it would be better to call @prepare in a separate pass?

Afterwards I realized that it would be racy. Namely, once a new device is registered, it may require ->prepare() to be called before ->suspend(), so if that happens after we've called ->prepare() for all devices, we'll be in trouble.

Quoted 4 lines

There's nothing wrong with doing it this way. However the race you're worried about will never happen if drivers are written correctly. Once prepare has been called for all devices, no new devices should be registered.

That only applies to the devices with parents. Apart from this, the device can be registered concurrently with its parent's ->prepare() and in that case we should call ->complete() for the parent etc. IOW, almost the same complications as in dpm_suspend() would have to be taken into account.

[--snip--]

Quoted 16 lines
Quoted 7 lines

+ * @prepare: Prepare the device for the upcoming transition, but do NOT change
+ * its hardware state. Prevent new children of the device from being
+ * registered and prevent new calls to the probe method from being made
+ * after @prepare() returns. Also, if a concurrent child registration
+ * or a call to the probe method already in progress is detected by
+ * @prepare(), it may return -EAGAIN, so that the PM core can execute it
+ * once again (e.g. after suspending the newly registered child) to recover

Quoted 8 lines

+ * from the race condition. This method is executed for all kinds of
+ * suspend transitions and is immediately followed by one of the suspend
+ * callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ * [NOTE: The PM core is able to detect a registration of a new child of a
+ * device concurrent with this device's @prepare() callback, in which case
+ * @complete() will be called for the device and the core will try again to
+ * suspend it later. However, the core is NOT able to handle a call to
+ * the probe method concurrent with @prepare().]

Quoted 2 lines

I think you can leave out this NOTE. Direct probe method calls by drivers are pretty rare; almost all probing is done by the driver core.

Done.

Quoted 8 lines
Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * returns -EAGAIN.

The last part is wrong.

Yes, I noticed that too.

Quoted 1 line

If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called.

Hmm, I'd prefer to call it if ->prepare() fails too. The -EAGAIN case is special, because in fact there's no warranty that ->prepare() will be called again (for example, the new child's ->suspend() may fail before that happens), so we need to do the clean up after it. In general, if ->complete() is assumed to be called after a failing ->prepare() too, that simplifies the code a bit.

[--snip--]

Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +5 -0
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};

There should be kerneldoc for dpm_state.

In fact there is one, right above the definition of 'enum dpm_state'. It was placed a bit confusingly under the big comment about the legacy stuff. I changed the ordering of things in pm.h to avoid that confusion.

[--snip--]

Quoted 25 lines
Quoted 25 lines
(quoted diff) +7 -12
@@ -71,22 +71,29 @@ 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 (all_sleeping) {
+		dev_warn(dev, "devices are sleeping, will not add\n");
+		goto Failure;
+	}
+	if (dev->parent) {
+		if (dev->parent->power.status == DPM_OFF) {
+			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);
Quoted 8 lines

Review int device_pm_add(struct device *dev) · line 85

Quoted 4 lines
(quoted diff) +4 -0
+			goto Failure;
+		} else if (dev->parent->power.status == DPM_SUSPENDING) {
+			dev->parent->power.status == DPM_ON;
+		}

This part puzzled me until I read the rest of the patch. It's a good way to look specifically for new _child_ registrations as opposed to any new registrations.

I reworked that a bit.

Quoted 19 lines
Quoted 6 lines
(quoted diff) +1 -1
@@ -418,22 +629,37 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
-		WARN_ON(dev->parent && dev->parent->power.sleeping);
+		WARN_ON(dev->parent && dev->parent->power.status == DPM_OFF);

Review static int dpm_suspend(pm_message_t stat · line 642

Quoted 10 lines
(quoted diff) +10 -0
+		dev->power.status = DPM_SUSPENDING;
+		mutex_unlock(&dpm_list_mtx);
+
+		error = prepare_device(dev, state);
+
+		mutex_lock(&dpm_list_mtx);
+		if (error == -EAGAIN)
+			dev->power.status = DPM_ON;
+		else if(error)
+			break;

The last four lines should be:

Quoted 6 lines
		if (error) {
			dev->power.status = DPM_ON;
			if (error == -EAGAIN)
				continue;
			break;
		}

Well, I reworked this part too, but in a different way.

Quoted 1 line

Insert: /* Was a child added during prepare_device()? */

I added a different comment here.

Quoted 33 lines
Quoted 20 lines
(quoted diff) +10 -5
+		if (dev->power.status == DPM_ON) {
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
 
-		dev->power.sleeping = true;
+			mutex_lock(&dpm_list_mtx);
+			continue;
+		}
+		dev->power.status = DPM_OFF;
 		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,

Review static int dpm_suspend(pm_message_t stat · line 662

Quoted 10 lines
(quoted diff) +5 -5
-					(error == -EAGAIN ?
-					" (please convert to suspend_late)" :
-					""));
-			dev->power.sleeping = false;
-			break;
+			dev->power.status = DPM_ON;
+			mutex_unlock(&dpm_list_mtx);
+
+			complete_device(dev, resume_event(state));
+			return error;

Early return? I guess it's okay...

Quoted 5 lines

Review static int dpm_suspend(pm_message_t stat · line 665

Quoted 3 lines
(quoted diff) +0 -0
 		}
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_off);

Looks good.

Well, I reworked it somewhat in the meantime.

First, I moved the pm_dev_dbg() stuff into the inner if () blocks. Second, I made it possible to use ->prepare() and ->complete() in the "noirq" case as well and added a comment about the "noirq" callbacks. Finally, I cut some rough edges here and there (at least I hope so).

The new patch is appended.

Thanks,
Rafael

---

 arch/x86/kernel/apm_32.c  |    4 
 drivers/base/power/main.c |  463 +++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h    |    8 
 include/linux/pm.h        |  238 ++++++++++++++++++++---
 kernel/power/disk.c       |   14 -
 kernel/power/main.c       |    4 
 6 files changed, 579 insertions(+), 152 deletions(-)
include/linux/pm.h +206 -32
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -110,11 +110,9 @@ extern void (*pm_power_off_prepare)(void
 
 struct device;
 
-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 +120,206 @@ 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 and prevent new calls to the probe method from being made
+ *	after @prepare() returns.  Also, if a concurrent child registration
+ *	or a call to the probe method already in progress is detected by
+ *	@prepare(), it may return -EAGAIN, so that the PM core can execute it
+ *	once again (e.g. after suspending the newly registered child) to recover
+ *	from the race condition. This method is executed for all kinds of
+ *	suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, immediately following one of the resume
+ *	callbacks: @resume(), @thaw(), @restore(), or @recover().  Also executed
+ *	if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ *	immediately following a successful @prepare() fails OR if @prepare()
+ *	itself fails.  @complete() cannot fail.
+ *
+ * @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.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ *	Undo the changes made by the preceding @freeze(), so the device can be
+ *	operated in the same was 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.
+ *
+ * @quiesce: Hibernation-specific, executed after loading a hibernation image
+ *	and before restoring the contents of main memory from it.  Quiesce
+ *	operations so that the contents of main memory can be restored from the
+ *	image in a consistent way, but do NOT otherwise put the device into a
+ *	low power state and do NOT emit system wakeup events.  Do NOT save any
+ *	device settings in main memory.
+ *
+ * @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.  Do 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().
+ *
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce(), so the device can be operated in the same way
+ *	as immediately before the failing transition.
+ *
+ * struct pm_ops is also used for defining driver PM operations to be carried
+ * out with interrupts disabled.  In this case, however, there is only one
+ * active CPU while the operations are being performed, so they are executed by
+ * the PM core in a more straightforward way.  In particular, the failure of
+ * @prepare() carried out with interrupts disabled causes the entire PM
+ * transition to fail, regardless of the error code returned by it.
+ */
+
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif
+};
+
+/**
+ * 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 ->quiesce() 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
+ *		->recover() and ->complete() for all devices.
+ */
+
+typedef struct pm_message {
+	int event;
+} pm_message_t;
+
+#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.
+ *
+ * DPM_SUSPENDING	Device is currently being suspended.
+ *
+ * DPM_OFF		Device is regarded as suspended.
+ */
+
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};
+
+struct dev_pm_info {
+	pm_message_t		power_state;
+	unsigned		can_wakeup:1;
+	enum dpm_state		status:2;	/* Owned by the PM core */
+#ifdef	CONFIG_PM_SLEEP
+	unsigned		should_wakeup:1;
+	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 +364,11 @@ 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;
-	bool			sleeping:1;	/* Owned by the PM core */
-#ifdef	CONFIG_PM_SLEEP
-	unsigned		should_wakeup:1;
-	struct list_head	entry;
-#endif
-};
+#ifdef CONFIG_PM_SLEEP
+extern void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
 
 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);
 
drivers/base/power/main.c +354 -109

643 lines not on this page · raw message

include/linux/device.h +8 -0
Index: linux-2.6/include/linux/device.h
===================================================================
@@ -69,6 +69,9 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ops *pm;
+	struct pm_ops *pm_noirq;
+
 	struct bus_type_private *p;
 };
 
@@ -202,6 +205,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 +350,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 */
kernel/power/disk.c +7 -7
Index: linux-2.6/kernel/power/disk.c
===================================================================
@@ -224,7 +224,7 @@ 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();
 	return error;
@@ -280,7 +280,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:
@@ -301,7 +301,7 @@ static int resume_target_kernel(void)
 	int error;
 
 	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,7 +329,7 @@ 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();
 	return error;
@@ -350,7 +350,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 +362,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();
@@ -419,7 +419,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
kernel/power/main.c +2 -2
Index: linux-2.6/kernel/power/main.c
===================================================================
@@ -239,7 +239,7 @@ 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());
@@ -291,7 +291,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:
arch/x86/kernel/apm_32.c +2 -2
Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -1221,9 +1221,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);
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
#9 lore raw
Quoted 7 lines · Rafael J. Wysocki
Quoted 1 line

If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called.

Hmm, I'd prefer to call it if ->prepare() fails too. The -EAGAIN case is special, because in fact there's no warranty that ->prepare() will be called again (for example, the new child's ->suspend() may fail before that happens), so we need to do the clean up after it. In general, if ->complete() is assumed to be called after a failing ->prepare() too, that simplifies the code a bit.

I'm quite certain this is a bad idea.

The kerneldoc says it all: @complete undoes the changes made by @prepare. If @prepare failed then it didn't make any changes (or else it already undid any changes that were made), so @complete shouldn't be called. This is the way all programming interfaces should work.

Don't worry about complicating the core code a little bit. It's just in one spot; much more important is that the calling convention should be clear. My way is very simple: Every successful call to @prepare is balanced by exactly one call to @complete.

Quoted 4 lines

First, I moved the pm_dev_dbg() stuff into the inner if () blocks. Second, I made it possible to use ->prepare() and ->complete() in the "noirq" case as well and added a comment about the "noirq" callbacks. Finally, I cut some rough edges here and there (at least I hope so).

There's almost nothing left to comment on in the patch itself. I'll have to try it out before giving any further feedback...

Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * itself fails. @complete() cannot fail.

Of course this comment should be changed to match the actual behavior. There's no need to mention that @complete cannot fail; this is obvious because it returns void.

Quoted 3 lines

+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ * Undo the changes made by the preceding @freeze(), so the device can be
+ * operated in the same was as immediately before the call to @freeze().

s/was/way/ I spotted this typo in @recover before but missed it here.

Quoted 7 lines
include/linux/pm.h +7 -0
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);

include/linux/pm.h typedef struct pm_message { · line 218

Quoted 10 lines
include/linux/pm.h +10 -0
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif

Mention perhaps that although the "resuming" operations (i.e., resume, thaw, restore, and recover) can return errors, the PM core doesn't do anything about those errors other than print them in the system log. There's nothing it _can_ do...

Quoted 7 lines

+/**
+ * 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.

Add: Set this way initially and when ->resume(), ->thaw(), ->restore(), ->recover(), or ->complete() is about to be called. Also set when ->prepare() fails.

Quoted 2 lines

+ *
+ * DPM_SUSPENDING Device is currently being suspended.

Add: Set when ->prepare() is about to be called.

Quoted 2 lines

+ *
+ * DPM_OFF Device is regarded as suspended.

Add: Set when ->suspend(), ->freeze(), ->poweroff(), or ->quiesce() is about to be called.

Alan Stern

#10 lore raw
In reply to Rafael J. Wysocki
Quoted 7 lines · Rafael J. Wysocki
Quoted 1 line

If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called.

Hmm, I'd prefer to call it if ->prepare() fails too. The -EAGAIN case is special, because in fact there's no warranty that ->prepare() will be called again (for example, the new child's ->suspend() may fail before that happens), so we need to do the clean up after it. In general, if ->complete() is assumed to be called after a failing ->prepare() too, that simplifies the code a bit.

I'm quite certain this is a bad idea.

The kerneldoc says it all: @complete undoes the changes made by @prepare. If @prepare failed then it didn't make any changes (or else it already undid any changes that were made), so @complete shouldn't be called. This is the way all programming interfaces should work.

Don't worry about complicating the core code a little bit. It's just in one spot; much more important is that the calling convention should be clear. My way is very simple: Every successful call to @prepare is balanced by exactly one call to @complete.

Quoted 4 lines

First, I moved the pm_dev_dbg() stuff into the inner if () blocks. Second, I made it possible to use ->prepare() and ->complete() in the "noirq" case as well and added a comment about the "noirq" callbacks. Finally, I cut some rough edges here and there (at least I hope so).

There's almost nothing left to comment on in the patch itself. I'll have to try it out before giving any further feedback...

Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * itself fails. @complete() cannot fail.

Of course this comment should be changed to match the actual behavior. There's no need to mention that @complete cannot fail; this is obvious because it returns void.

Quoted 3 lines

+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ * Undo the changes made by the preceding @freeze(), so the device can be
+ * operated in the same was as immediately before the call to @freeze().

s/was/way/ I spotted this typo in @recover before but missed it here.

Quoted 7 lines
include/linux/pm.h +7 -0
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);

include/linux/pm.h typedef struct pm_message { · line 218

Quoted 10 lines
include/linux/pm.h +10 -0
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif

Mention perhaps that although the "resuming" operations (i.e., resume, thaw, restore, and recover) can return errors, the PM core doesn't do anything about those errors other than print them in the system log. There's nothing it _can_ do...

Quoted 7 lines

+/**
+ * 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.

Add: Set this way initially and when ->resume(), ->thaw(), ->restore(), ->recover(), or ->complete() is about to be called. Also set when ->prepare() fails.

Quoted 2 lines

+ *
+ * DPM_SUSPENDING Device is currently being suspended.

Add: Set when ->prepare() is about to be called.

Quoted 2 lines

+ *
+ * DPM_OFF Device is regarded as suspended.

Add: Set when ->suspend(), ->freeze(), ->poweroff(), or ->quiesce() is about to be called.

Alan Stern

#11 lore raw
Quoted 17 lines · Alan Stern
Quoted 7 lines · Rafael J. Wysocki
Quoted 1 line

If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called.

Hmm, I'd prefer to call it if ->prepare() fails too. The -EAGAIN case is special, because in fact there's no warranty that ->prepare() will be called again (for example, the new child's ->suspend() may fail before that happens), so we need to do the clean up after it. In general, if ->complete() is assumed to be called after a failing ->prepare() too, that simplifies the code a bit.

I'm quite certain this is a bad idea.

The kerneldoc says it all: @complete undoes the changes made by @prepare. If @prepare failed then it didn't make any changes (or else it already undid any changes that were made), so @complete shouldn't be called. This is the way all programming interfaces should work.

Quoted 4 lines

Don't worry about complicating the core code a little bit. It's just in one spot; much more important is that the calling convention should be clear. My way is very simple: Every successful call to @prepare is balanced by exactly one call to @complete.

Okay, I reworked the patch so that ->complete() is not called after a failing ->prepare() (even if that returns -EAGAIN). For this purpose I had to rework dpm_suspend() quite heavily (once again). Hopefully, I didn't break it.

Quoted 7 lines
Quoted 4 lines

First, I moved the pm_dev_dbg() stuff into the inner if () blocks. Second, I made it possible to use ->prepare() and ->complete() in the "noirq" case as well and added a comment about the "noirq" callbacks. Finally, I cut some rough edges here and there (at least I hope so).

There's almost nothing left to comment on in the patch itself. I'll have to try it out before giving any further feedback...

Well, I'm going to develop some test code for checking all of the error paths etc.

Quoted 10 lines
Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * itself fails. @complete() cannot fail.

Of course this comment should be changed to match the actual behavior. There's no need to mention that @complete cannot fail; this is obvious because it returns void.

Corrected.

Quoted 20 lines
Quoted 3 lines

+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ * Undo the changes made by the preceding @freeze(), so the device can be
+ * operated in the same was as immediately before the call to @freeze().

s/was/way/ I spotted this typo in @recover before but missed it here.

Quoted 14 lines
(quoted diff) +14 -0
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
Quoted 8 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif

Mention perhaps that although the "resuming" operations (i.e., resume, thaw, restore, and recover) can return errors, the PM core doesn't do anything about those errors other than print them in the system log. There's nothing it _can_ do...

Added such paragraph to the comment describing the callbacks.

Quoted 15 lines
Quoted 7 lines

+/**
+ * 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.

Add: Set this way initially and when ->resume(), ->thaw(), ->restore(), ->recover(), or ->complete() is about to be called. Also set when ->prepare() fails.

Quoted 2 lines

+ *
+ * DPM_SUSPENDING Device is currently being suspended.

Quoted 7 lines

Add: Set when ->prepare() is about to be called.

Quoted 2 lines

+ *
+ * DPM_OFF Device is regarded as suspended.

Add: Set when ->suspend(), ->freeze(), ->poweroff(), or ->quiesce() is about to be called.

All added.

The updated patch is appended.

Thanks,
Rafael

---

 arch/x86/kernel/apm_32.c  |    4 
 drivers/base/power/main.c |  453 +++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h    |    8 
 include/linux/pm.h        |  249 ++++++++++++++++++++++---
 kernel/power/disk.c       |   14 -
 kernel/power/main.c       |    4 
 6 files changed, 587 insertions(+), 145 deletions(-)
include/linux/pm.h +217 -32
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -110,11 +110,9 @@ extern void (*pm_power_off_prepare)(void
 
 struct device;
 
-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 +120,217 @@ 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 and prevent new calls to the probe method from being made
+ *	after @prepare() returns.  Also, if a concurrent child registration
+ *	or a call to the probe method already in progress is detected by
+ *	@prepare(), it may return -EAGAIN, so that the PM core can execute it
+ *	once again (e.g. after suspending the newly registered child) to recover
+ *	from the race condition. This method is executed for all kinds of
+ *	suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, immediately following one of the resume
+ *	callbacks: @resume(), @thaw(), @restore(), or @recover().  Also executed
+ *	if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ *	immediately following a successful @prepare() fails OR if a new child
+ *	of the device has been registered during @prepare().
+ *
+ * @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.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation 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.
+ *
+ * @quiesce: Hibernation-specific, executed after loading a hibernation image
+ *	and before restoring the contents of main memory from it.  Quiesce
+ *	operations so that the contents of main memory can be restored from the
+ *	image in a consistent way, but do NOT otherwise put the device into a
+ *	low power state and do NOT emit system wakeup events.  Do NOT save any
+ *	device settings in main memory.
+ *
+ * @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.  Do 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().
+ *
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce(), so the device can be operated in the same way
+ *	as immediately before the failing transition.
+ *
+ * struct pm_ops is also used for defining driver PM operations to be carried
+ * out with interrupts disabled.  In this case, however, there is only one
+ * active CPU while the operations are being performed, so they are executed by
+ * the PM core in a more straightforward way.  In particular, the failure of
+ * @prepare() carried out with interrupts disabled causes the entire PM
+ * transition to fail, regardless of the error code returned by it.
+ *
+ * All of the above callbacks, except for @complete(), return error codes.
+ * However, the error codes returned by resume operations, @resume(), @thaw(),
+ * @restore(), and @recover(), are only printed in the system logs, since the
+ * PM core cannot do anything else about them.
+ */
+
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif
+};
+
+/**
+ * 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 ->quiesce() 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
+ *		->recover() and ->complete() for all devices.
+ */
+
+typedef struct pm_message {
+	int event;
+} pm_message_t;
+
+#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 ->resume(), ->thaw(), ->restore(),
+ *			->recover(), or ->complete() is about to be called.
+ *			Also set when ->prepare() fails.
+ *
+ * DPM_SUSPENDING	Device is currently being suspended.  Set when
+ *			->prepare() is about to be called.
+ *
+ * DPM_OFF		Device is regarded as suspended.  Set when ->suspend(),
+ *			->freeze(), ->poweroff(), or ->quiesce() is about to be
+ *			called.
+ */
+
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};
+
+struct dev_pm_info {
+	pm_message_t		power_state;
+	unsigned		can_wakeup:1;
+	enum dpm_state		status:2;	/* Owned by the PM core */
+#ifdef	CONFIG_PM_SLEEP
+	unsigned		should_wakeup:1;
+	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 +375,11 @@ 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;
-	bool			sleeping:1;	/* Owned by the PM core */
-#ifdef	CONFIG_PM_SLEEP
-	unsigned		should_wakeup:1;
-	struct list_head	entry;
-#endif
-};
+#ifdef CONFIG_PM_SLEEP
+extern void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
 
 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);
 
drivers/base/power/main.c +351 -102

635 lines not on this page · raw message

include/linux/device.h +8 -0
Index: linux-2.6/include/linux/device.h
===================================================================
@@ -69,6 +69,9 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ops *pm;
+	struct pm_ops *pm_noirq;
+
 	struct bus_type_private *p;
 };
 
@@ -202,6 +205,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 +350,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 */
kernel/power/disk.c +7 -7
Index: linux-2.6/kernel/power/disk.c
===================================================================
@@ -224,7 +224,7 @@ 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();
 	return error;
@@ -280,7 +280,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:
@@ -301,7 +301,7 @@ static int resume_target_kernel(void)
 	int error;
 
 	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,7 +329,7 @@ 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();
 	return error;
@@ -350,7 +350,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 +362,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();
@@ -419,7 +419,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
kernel/power/main.c +2 -2
Index: linux-2.6/kernel/power/main.c
===================================================================
@@ -239,7 +239,7 @@ 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());
@@ -291,7 +291,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:
arch/x86/kernel/apm_32.c +2 -2
Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -1221,9 +1221,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);
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
#12 lore raw
In reply to Alan Stern
Quoted 17 lines · Alan Stern
Quoted 7 lines · Rafael J. Wysocki
Quoted 1 line

If @prepare returns an error (such as -EAGAIN) then @complete shouldn't be called.

Hmm, I'd prefer to call it if ->prepare() fails too. The -EAGAIN case is special, because in fact there's no warranty that ->prepare() will be called again (for example, the new child's ->suspend() may fail before that happens), so we need to do the clean up after it. In general, if ->complete() is assumed to be called after a failing ->prepare() too, that simplifies the code a bit.

I'm quite certain this is a bad idea.

The kerneldoc says it all: @complete undoes the changes made by @prepare. If @prepare failed then it didn't make any changes (or else it already undid any changes that were made), so @complete shouldn't be called. This is the way all programming interfaces should work.

Quoted 4 lines

Don't worry about complicating the core code a little bit. It's just in one spot; much more important is that the calling convention should be clear. My way is very simple: Every successful call to @prepare is balanced by exactly one call to @complete.

Okay, I reworked the patch so that ->complete() is not called after a failing ->prepare() (even if that returns -EAGAIN). For this purpose I had to rework dpm_suspend() quite heavily (once again). Hopefully, I didn't break it.

Quoted 7 lines
Quoted 4 lines

First, I moved the pm_dev_dbg() stuff into the inner if () blocks. Second, I made it possible to use ->prepare() and ->complete() in the "noirq" case as well and added a comment about the "noirq" callbacks. Finally, I cut some rough edges here and there (at least I hope so).

There's almost nothing left to comment on in the patch itself. I'll have to try it out before giving any further feedback...

Well, I'm going to develop some test code for checking all of the error paths etc.

Quoted 10 lines
Quoted 6 lines

+ * @complete: Undo the changes made by @prepare(). This method is executed for
+ * all kinds of resume transitions, immediately following one of the resume
+ * callbacks: @resume(), @thaw(), @restore(), or @recover(). Also executed
+ * if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ * immediately following a successful @prepare() fails OR if @prepare()
+ * itself fails. @complete() cannot fail.

Of course this comment should be changed to match the actual behavior. There's no need to mention that @complete cannot fail; this is obvious because it returns void.

Corrected.

Quoted 20 lines
Quoted 3 lines

+ * @thaw: Hibernation-specific, executed after creating a hibernation image.
+ * Undo the changes made by the preceding @freeze(), so the device can be
+ * operated in the same was as immediately before the call to @freeze().

s/was/way/ I spotted this typo in @recover before but missed it here.

Quoted 14 lines
(quoted diff) +14 -0
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
Quoted 8 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif

Mention perhaps that although the "resuming" operations (i.e., resume, thaw, restore, and recover) can return errors, the PM core doesn't do anything about those errors other than print them in the system log. There's nothing it _can_ do...

Added such paragraph to the comment describing the callbacks.

Quoted 15 lines
Quoted 7 lines

+/**
+ * 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.

Add: Set this way initially and when ->resume(), ->thaw(), ->restore(), ->recover(), or ->complete() is about to be called. Also set when ->prepare() fails.

Quoted 2 lines

+ *
+ * DPM_SUSPENDING Device is currently being suspended.

Quoted 7 lines

Add: Set when ->prepare() is about to be called.

Quoted 2 lines

+ *
+ * DPM_OFF Device is regarded as suspended.

Add: Set when ->suspend(), ->freeze(), ->poweroff(), or ->quiesce() is about to be called.

All added.

The updated patch is appended.

Thanks,
Rafael

---

 arch/x86/kernel/apm_32.c  |    4 
 drivers/base/power/main.c |  453 +++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h    |    8 
 include/linux/pm.h        |  249 ++++++++++++++++++++++---
 kernel/power/disk.c       |   14 -
 kernel/power/main.c       |    4 
 6 files changed, 587 insertions(+), 145 deletions(-)
include/linux/pm.h +217 -32
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -110,11 +110,9 @@ extern void (*pm_power_off_prepare)(void
 
 struct device;
 
-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 +120,217 @@ 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 and prevent new calls to the probe method from being made
+ *	after @prepare() returns.  Also, if a concurrent child registration
+ *	or a call to the probe method already in progress is detected by
+ *	@prepare(), it may return -EAGAIN, so that the PM core can execute it
+ *	once again (e.g. after suspending the newly registered child) to recover
+ *	from the race condition. This method is executed for all kinds of
+ *	suspend transitions and is immediately followed by one of the suspend
+ *	callbacks: @suspend(), @freeze(), @poweroff(), or @quiesce().
+ *
+ * @complete: Undo the changes made by @prepare().  This method is executed for
+ *	all kinds of resume transitions, immediately following one of the resume
+ *	callbacks: @resume(), @thaw(), @restore(), or @recover().  Also executed
+ *	if a suspend callback (@suspend(), @freeze(), @poweroff(), @quiesce())
+ *	immediately following a successful @prepare() fails OR if a new child
+ *	of the device has been registered during @prepare().
+ *
+ * @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.
+ *
+ * @thaw: Hibernation-specific, executed after creating a hibernation 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.
+ *
+ * @quiesce: Hibernation-specific, executed after loading a hibernation image
+ *	and before restoring the contents of main memory from it.  Quiesce
+ *	operations so that the contents of main memory can be restored from the
+ *	image in a consistent way, but do NOT otherwise put the device into a
+ *	low power state and do NOT emit system wakeup events.  Do NOT save any
+ *	device settings in main memory.
+ *
+ * @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.  Do 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().
+ *
+ * @recover: Hibernation-specific, executed after a failing creation of a
+ *	hibernation image OR after a failing attempt to restore the contents of
+ *	main memory from such an image.  Undo the changes made by the preceding
+ *	@freeze() or @quiesce(), so the device can be operated in the same way
+ *	as immediately before the failing transition.
+ *
+ * struct pm_ops is also used for defining driver PM operations to be carried
+ * out with interrupts disabled.  In this case, however, there is only one
+ * active CPU while the operations are being performed, so they are executed by
+ * the PM core in a more straightforward way.  In particular, the failure of
+ * @prepare() carried out with interrupts disabled causes the entire PM
+ * transition to fail, regardless of the error code returned by it.
+ *
+ * All of the above callbacks, except for @complete(), return error codes.
+ * However, the error codes returned by resume operations, @resume(), @thaw(),
+ * @restore(), and @recover(), are only printed in the system logs, since the
+ * PM core cannot do anything else about them.
+ */
+
+struct pm_ops {
+#ifdef CONFIG_PM_SLEEP
+	int (*prepare)(struct device *dev);
+	void (*complete)(struct device *dev);
+#endif
+#ifdef CONFIG_SUSPEND
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+#endif
+#ifdef CONFIG_HIBERNATION
+	int (*freeze)(struct device *dev);
+	int (*thaw)(struct device *dev);
+	int (*poweroff)(struct device *dev);
+	int (*quiesce)(struct device *dev);
+	int (*restore)(struct device *dev);
+	int (*recover)(struct device *dev);
+#endif
+};
+
+/**
+ * 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 ->quiesce() 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
+ *		->recover() and ->complete() for all devices.
+ */
+
+typedef struct pm_message {
+	int event;
+} pm_message_t;
+
+#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 ->resume(), ->thaw(), ->restore(),
+ *			->recover(), or ->complete() is about to be called.
+ *			Also set when ->prepare() fails.
+ *
+ * DPM_SUSPENDING	Device is currently being suspended.  Set when
+ *			->prepare() is about to be called.
+ *
+ * DPM_OFF		Device is regarded as suspended.  Set when ->suspend(),
+ *			->freeze(), ->poweroff(), or ->quiesce() is about to be
+ *			called.
+ */
+
+enum dpm_state {
+	DPM_ON,
+	DPM_SUSPENDING,
+	DPM_OFF,
+};
+
+struct dev_pm_info {
+	pm_message_t		power_state;
+	unsigned		can_wakeup:1;
+	enum dpm_state		status:2;	/* Owned by the PM core */
+#ifdef	CONFIG_PM_SLEEP
+	unsigned		should_wakeup:1;
+	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 +375,11 @@ 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;
-	bool			sleeping:1;	/* Owned by the PM core */
-#ifdef	CONFIG_PM_SLEEP
-	unsigned		should_wakeup:1;
-	struct list_head	entry;
-#endif
-};
+#ifdef CONFIG_PM_SLEEP
+extern void device_power_up(pm_message_t state);
+extern void device_resume(pm_message_t state);
 
 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);
 
drivers/base/power/main.c +351 -102

635 lines not on this page · raw message

include/linux/device.h +8 -0
Index: linux-2.6/include/linux/device.h
===================================================================
@@ -69,6 +69,9 @@ struct bus_type {
 	int (*resume_early)(struct device *dev);
 	int (*resume)(struct device *dev);
 
+	struct pm_ops *pm;
+	struct pm_ops *pm_noirq;
+
 	struct bus_type_private *p;
 };
 
@@ -202,6 +205,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 +350,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 */
kernel/power/disk.c +7 -7
Index: linux-2.6/kernel/power/disk.c
===================================================================
@@ -224,7 +224,7 @@ 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();
 	return error;
@@ -280,7 +280,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:
@@ -301,7 +301,7 @@ static int resume_target_kernel(void)
 	int error;
 
 	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,7 +329,7 @@ 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();
 	return error;
@@ -350,7 +350,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 +362,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();
@@ -419,7 +419,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
-	device_resume();
+	device_resume(PMSG_RESTORE);
  Resume_console:
 	resume_console();
  Close:
kernel/power/main.c +2 -2
Index: linux-2.6/kernel/power/main.c
===================================================================
@@ -239,7 +239,7 @@ 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());
@@ -291,7 +291,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:
arch/x86/kernel/apm_32.c +2 -2
Index: linux-2.6/arch/x86/kernel/apm_32.c
===================================================================
@@ -1221,9 +1221,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);
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
#13 lore raw
Quoted 5 lines · Rafael J. Wysocki

Okay, I reworked the patch so that ->complete() is not called after a failing ->prepare() (even if that returns -EAGAIN). For this purpose I had to rework dpm_suspend() quite heavily (once again). Hopefully, I didn't break it.

The updated patch is appended.

It all looks good. The next step is testing...

Alan Stern

#14 lore raw
In reply to Rafael J. Wysocki
Quoted 5 lines · Rafael J. Wysocki

Okay, I reworked the patch so that ->complete() is not called after a failing ->prepare() (even if that returns -EAGAIN). For this purpose I had to rework dpm_suspend() quite heavily (once again). Hopefully, I didn't break it.

The updated patch is appended.

It all looks good. The next step is testing...

Alan Stern

#15 lore raw
In reply to Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki

+ * @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.

This seems uncomfortably similar to device_driver.shutdown(). The only obvious difference is wakeup event handling, and even that is already a function of the target system state.

Are both methods needed?

Shouldn't this be more generic, not "hibernation-specific"?

#16 lore raw
In reply to Rafael J. Wysocki
Quoted 4 lines · Rafael J. Wysocki

+ * @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.

This seems uncomfortably similar to device_driver.shutdown(). The only obvious difference is wakeup event handling, and even that is already a function of the target system state.

Are both methods needed?

Shouldn't this be more generic, not "hibernation-specific"?

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#17 lore raw
Quoted 6 lines · David Brownell
Quoted 4 lines · Rafael J. Wysocki

+ * @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.

Quoted 7 lines

This seems uncomfortably similar to device_driver.shutdown(). The only obvious difference is wakeup event handling, and even that is already a function of the target system state.

Are both methods needed?

Shouldn't this be more generic, not "hibernation-specific"?

Well, let's not make restrictions at this point. There's nothing wrong with pointing both at the same function and if everyone turns out to do that, we'll remove one callback.

Thanks,
Rafael

#18 lore raw
In reply to David Brownell
Quoted 6 lines · David Brownell
Quoted 4 lines · Rafael J. Wysocki

+ * @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.

Quoted 7 lines

This seems uncomfortably similar to device_driver.shutdown(). The only obvious difference is wakeup event handling, and even that is already a function of the target system state.

Are both methods needed?

Shouldn't this be more generic, not "hibernation-specific"?

Well, let's not make restrictions at this point. There's nothing wrong with pointing both at the same function and if everyone turns out to do that, we'll remove one callback.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html