[PATCH] PM: acquire device locks prior to suspending

[PATCH] PM: acquire device locks prior to suspending

[Alan Stern]

This patch (as994) reorganizes the way suspend and resume
notifications are sent to drivers.  The major changes are that now the
PM core acquires every device semaphore before calling the methods,
and calls to device_add() during suspends will fail.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/base/power/main.c
===================================================================
--- usb-2.6.orig/drivers/base/power/main.c
+++ usb-2.6/drivers/base/power/main.c
@@ -24,17 +24,38 @@
 #include <linux/mutex.h>
 #include <linux/pm.h>
 #include <linux/resume-trace.h>
+#include <linux/rwsem.h>
 
 #include "../base.h"
 #include "power.h"
 
+/*
+ * The entries in the dpm_active list are in a depth first order, simply
+ * because children are guaranteed to be discovered after parents, and
+ * are inserted at the back of the list on discovery.
+ *
+ * All the other lists are kept in the same order, for consistency.
+ * However the lists aren't always traversed in the same order.
+ * Semaphores must be acquired from the top (i.e., front) down
+ * and released in the opposite order.  Devices must be suspended
+ * from the bottom (i.e., end) up and resumed in the opposite order.
+ * That way no parent will be suspended while it still has an active
+ * child.
+ *
+ * Since device_pm_add() may be called with a device semaphore held,
+ * we must never try to acquire a device semaphore while holding
+ * dpm_list_mutex.
+ */
+
 LIST_HEAD(dpm_active);
+static LIST_HEAD(dpm_locked);
 static LIST_HEAD(dpm_off);
 static LIST_HEAD(dpm_off_irq);
 
-static DEFINE_MUTEX(dpm_mtx);
 static DEFINE_MUTEX(dpm_list_mtx);
 
+static DECLARE_RWSEM(pm_sleep_rwsem);
+
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
 
@@ -59,29 +80,112 @@ void device_pm_remove(struct device *dev
 	pr_debug("PM: Removing info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
+
+	/* Don't remove a device while the PM core has it locked for suspend */
+	down(&dev->sem);
 	mutex_lock(&dpm_list_mtx);
 	dpm_sysfs_remove(dev);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
+	up(&dev->sem);
+}
+
+/**
+ *	pm_sleep_lock - mutual exclusion for registration and suspend
+ *
+ *	Returns 0 if no suspend is underway and device registration
+ *	may proceed, otherwise -EBUSY.
+ */
+int pm_sleep_lock(void)
+{
+	if (down_read_trylock(&pm_sleep_rwsem))
+		return 0;
+	return -EBUSY;
+}
+
+/**
+ *	pm_sleep_unlock - mutual exclusion for registration and suspend
+ *
+ *	This routine undoes the effect of device_pm_add_lock
+ *	when a device's registration is complete.
+ */
+void pm_sleep_unlock(void)
+{
+	up_read(&pm_sleep_rwsem);
 }
 
 
 /*------------------------- Resume routines -------------------------*/
 
 /**
- *	resume_device - Restore state for one device.
+ *	resume_device_early - Power on one device (early resume).
  *	@dev:	Device.
  *
+ *	Must be called with interrupts disabled.
  */
-
-static int resume_device(struct device * dev)
+static int resume_device_early(struct device *dev)
 {
 	int error = 0;
 
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	down(&dev->sem);
+	if (dev->bus && dev->bus->resume_early) {
+		dev_dbg(dev,"EARLY resume\n");
+		error = dev->bus->resume_early(dev);
+	}
+
+	TRACE_RESUME(error);
+	return error;
+}
+
+/**
+ *	dpm_power_up - Power on all regular (non-sysdev) devices.
+ *
+ *	Walk the dpm_off_irq list and power each device up. This
+ *	is used for devices that required they be powered down with
+ *	interrupts disabled. As devices are powered on, they are moved
+ *	to the dpm_off list.
+ *
+ *	Interrupts must be disabled when calling this.
+ */
+static void dpm_power_up(void)
+{
+	while (!list_empty(&dpm_off_irq)) {
+		struct list_head *entry = dpm_off_irq.next;
+		struct device *dev = to_device(entry);
+
+		resume_device_early(dev);
+		list_move_tail(entry, &dpm_off);
+	}
+}
+
+/**
+ *	device_power_up - Turn on all devices that need special attention.
+ *
+ *	Power on system devices, then devices that required we shut them down
+ *	with interrupts disabled.
+ *
+ *	Must be called with interrupts disabled.
+ */
+void device_power_up(void)
+{
+	sysdev_resume();
+	dpm_power_up();
+}
+EXPORT_SYMBOL_GPL(device_power_up);
+
+/**
+ *	resume_device - Restore state for one device.
+ *	@dev:	Device.
+ *
+ */
+static int resume_device(struct device *dev)
+{
+	int error = 0;
+
+	TRACE_DEVICE(dev);
+	TRACE_RESUME(0);
 
 	if (dev->bus && dev->bus->resume) {
 		dev_dbg(dev,"resuming\n");
@@ -98,126 +202,68 @@ static int resume_device(struct device *
 		error = dev->class->resume(dev);
 	}
 
-	up(&dev->sem);
-
 	TRACE_RESUME(error);
 	return error;
 }
 
-
-static int resume_device_early(struct device * dev)
+/**
+ *	dpm_resume - Resume every device.
+ *
+ *	Resume the devices that have either not gone through
+ *	the late suspend, or that did go through it but also
+ *	went through the early resume.
+ *
+ *	Take devices from the dpm_off_list, resume them,
+ *	and put them on the dpm_locked list.
+ */
+static void dpm_resume(void)
 {
-	int error = 0;
+	while(!list_empty(&dpm_off)) {
+		struct list_head *entry = dpm_off.next;
+		struct device *dev = to_device(entry);
 
-	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);
+		resume_device(dev);
+		list_move_tail(entry, &dpm_locked);
 	}
-	TRACE_RESUME(error);
-	return error;
 }
 
-/*
- * Resume the devices that have either not gone through
- * the late suspend, or that did go through it but also
- * went through the early resume
+/**
+ *	unlock_all_devices - Release each device's semaphore
+ *
+ *	Go through the dpm_off list.  Put each device on the dpm_active
+ *	list and unlock it.
  */
-static void dpm_resume(void)
+static void unlock_all_devices(void)
 {
 	mutex_lock(&dpm_list_mtx);
-	while(!list_empty(&dpm_off)) {
-		struct list_head * entry = dpm_off.next;
-		struct device * dev = to_device(entry);
-
-		get_device(dev);
-		list_move_tail(entry, &dpm_active);
-
-		mutex_unlock(&dpm_list_mtx);
-		resume_device(dev);
-		mutex_lock(&dpm_list_mtx);
-		put_device(dev);
-	}
+ 	while (!list_empty(&dpm_locked)) {
+ 		struct list_head *entry = dpm_locked.prev;
+ 		struct device *dev = to_device(entry);
+
+ 		list_move(entry, &dpm_active);
+ 		up(&dev->sem);
+ 	}
 	mutex_unlock(&dpm_list_mtx);
 }
 
-
 /**
  *	device_resume - Restore state of each device in system.
  *
- *	Walk the dpm_off list, remove each entry, resume the device,
- *	then add it to the dpm_active list.
+ *	Resume all the devices, unlock them all, and allow new
+ *	devices to be registered once again.
  */
-
 void device_resume(void)
 {
 	might_sleep();
-	mutex_lock(&dpm_mtx);
 	dpm_resume();
-	mutex_unlock(&dpm_mtx);
+	unlock_all_devices();
+	up_write(&pm_sleep_rwsem);
 }
-
 EXPORT_SYMBOL_GPL(device_resume);
 
 
-/**
- *	dpm_power_up - Power on some devices.
- *
- *	Walk the dpm_off_irq list and power each device up. This
- *	is used for devices that required they be powered down with
- *	interrupts disabled. As devices are powered on, they are moved
- *	to the dpm_active list.
- *
- *	Interrupts must be disabled when calling this.
- */
-
-static void dpm_power_up(void)
-{
-	while(!list_empty(&dpm_off_irq)) {
-		struct list_head * entry = dpm_off_irq.next;
-		struct device * dev = to_device(entry);
-
-		list_move_tail(entry, &dpm_off);
-		resume_device_early(dev);
-	}
-}
-
-
-/**
- *	device_power_up - Turn on all devices that need special attention.
- *
- *	Power on system devices then devices that required we shut them down
- *	with interrupts disabled.
- *	Called with interrupts disabled.
- */
-
-void device_power_up(void)
-{
-	sysdev_resume();
-	dpm_power_up();
-}
-
-EXPORT_SYMBOL_GPL(device_power_up);
-
-
 /*------------------------- Suspend routines -------------------------*/
 
-/*
- * The entries in the dpm_active list are in a depth first order, simply
- * because children are guaranteed to be discovered after parents, and
- * are inserted at the back of the list on discovery.
- *
- * All list on the suspend path are done in reverse order, so we operate
- * on the leaves of the device tree (or forests, depending on how you want
- * to look at it ;) first. As nodes are removed from the back of the list,
- * they are inserted into the front of their destintation lists.
- *
- * Things are the reverse on the resume path - iterations are done in
- * forward order, and nodes are inserted at the back of their destination
- * lists. This way, the ancestors will be accessed before their descendents.
- */
-
 static inline char *suspend_verb(u32 event)
 {
 	switch (event) {
@@ -228,7 +274,6 @@ static inline char *suspend_verb(u32 eve
 	}
 }
 
-
 static void
 suspend_device_dbg(struct device *dev, pm_message_t state, char *info)
 {
@@ -238,16 +283,69 @@ suspend_device_dbg(struct device *dev, p
 }
 
 /**
- *	suspend_device - Save state of one device.
+ *	suspend_device_late - Shut down one device (late suspend).
  *	@dev:	Device.
  *	@state:	Power state device is entering.
+ *
+ *	This is called with interrupts off and only a single CPU running.
+ */
+static int suspend_device_late(struct device *dev, pm_message_t state)
+{
+	int error = 0;
+
+	if (dev->bus && dev->bus->suspend_late) {
+		suspend_device_dbg(dev, state, "LATE ");
+		error = dev->bus->suspend_late(dev, state);
+		suspend_report_result(dev->bus->suspend_late, error);
+	}
+	return error;
+}
+
+/**
+ *	device_power_down - Shut down special devices.
+ *	@state:		Power state to enter.
+ *
+ *	Power down devices that require interrupts to be disabled
+ *	and move them from the dpm_off list to the dpm_off_irq list.
+ *	Then power down system devices.
+ *
+ *	Must be called with interrupts disabled and only one CPU running.
  */
+int device_power_down(pm_message_t state)
+{
+	int error = 0;
+
+	while (!list_empty(&dpm_off)) {
+		struct list_head *entry = dpm_off.prev;
+		struct device *dev = to_device(entry);
+
+		error = suspend_device_late(dev, state);
+		if (error) {
+			printk(KERN_ERR "Could not power down device %s: "
+					"error %d\n",
+					kobject_name(&dev->kobj), error);
+			break;
+		}
+		list_move(&dev->power.entry, &dpm_off_irq);
+	}
+
+	if (!error)
+		error = sysdev_suspend(state);
+	if (error)
+		dpm_power_up();
+	return error;
+}
+EXPORT_SYMBOL_GPL(device_power_down);
 
-static int suspend_device(struct device * dev, pm_message_t state)
+/**
+ *	suspend_device - Save state of one device.
+ *	@dev:	Device.
+ *	@state:	Power state device is entering.
+ */
+int suspend_device(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
-	down(&dev->sem);
 	if (dev->power.power_state.event) {
 		dev_dbg(dev, "PM: suspend %d-->%d\n",
 			dev->power.power_state.event, state.event);
@@ -270,123 +368,99 @@ static int suspend_device(struct device 
 		error = dev->bus->suspend(dev, state);
 		suspend_report_result(dev->bus->suspend, error);
 	}
-	up(&dev->sem);
 	return error;
 }
 
-
-/*
- * This is called with interrupts off, only a single CPU
- * running. We can't acquire a mutex or semaphore (and we don't
- * need the protection)
+/**
+ *	dpm_suspend - Suspend every device.
+ *	@state:	Power state to put each device in.
+ *
+ *	Walk the dpm_locked list.  Suspend each device and move it
+ *	to the dpm_off list.
+ *
+ *	(For historical reasons, if it returns -EAGAIN, that used to mean
+ *	that the device would be called again with interrupts disabled.
+ *	These days, we use the "suspend_late()" callback for that, so we
+ *	print a warning and consider it an error).
  */
-static int suspend_device_late(struct device *dev, pm_message_t state)
+static int dpm_suspend(pm_message_t state)
 {
 	int error = 0;
 
-	if (dev->bus && dev->bus->suspend_late) {
-		suspend_device_dbg(dev, state, "LATE ");
-		error = dev->bus->suspend_late(dev, state);
-		suspend_report_result(dev->bus->suspend_late, error);
+	while (!list_empty(&dpm_locked)) {
+		struct list_head *entry = dpm_locked.prev;
+		struct device *dev = to_device(entry);
+
+		error = suspend_device(dev, state);
+		if (error) {
+			printk(KERN_ERR "Could not suspend device %s: "
+					"error %d%s\n",
+					kobject_name(&dev->kobj),
+					error,
+					(error == -EAGAIN ?
+					" (please convert to suspend_late)" :
+					""));
+			break;
+		}
+ 		list_move(&dev->power.entry, &dpm_off);
 	}
+
+	if (error)
+		dpm_resume();
 	return error;
 }
 
 /**
- *	device_suspend - Save state and stop all devices in system.
- *	@state:		Power state to put each device in.
- *
- *	Walk the dpm_active list, call ->suspend() for each device, and move
- *	it to the dpm_off list.
- *
- *	(For historical reasons, if it returns -EAGAIN, that used to mean
- *	that the device would be called again with interrupts disabled.
- *	These days, we use the "suspend_late()" callback for that, so we
- *	print a warning and consider it an error).
- *
- *	If we get a different error, try and back out.
- *
- *	If we hit a failure with any of the devices, call device_resume()
- *	above to bring the suspended devices back to life.
+ *	lock_all_devices - Acquire every device's semaphore
  *
+ *	Go through the dpm_active list. Carefully lock each device's
+ *	semaphore and put it in on the dpm_locked list.
  */
-
-int device_suspend(pm_message_t state)
+static void lock_all_devices(void)
 {
-	int error = 0;
-
-	might_sleep();
-	mutex_lock(&dpm_mtx);
 	mutex_lock(&dpm_list_mtx);
-	while (!list_empty(&dpm_active) && error == 0) {
-		struct list_head * entry = dpm_active.prev;
-		struct device * dev = to_device(entry);
-
+	while (!list_empty(&dpm_active)) {
+		struct list_head *entry = dpm_active.next;
+		struct device *dev = to_device(entry);
+
+		/* Required locking order is dev->sem first,
+		 * then dpm_list_mutex.  Hence this awkward code.
+		 */
 		get_device(dev);
 		mutex_unlock(&dpm_list_mtx);
-
-		error = suspend_device(dev, state);
-
+		down(&dev->sem);
 		mutex_lock(&dpm_list_mtx);
 
-		/* Check if the device got removed */
-		if (!list_empty(&dev->power.entry)) {
-			/* Move it to the dpm_off list */
-			if (!error)
-				list_move(&dev->power.entry, &dpm_off);
-		}
-		if (error)
-			printk(KERN_ERR "Could not suspend device %s: "
-				"error %d%s\n",
-				kobject_name(&dev->kobj), error,
-				error == -EAGAIN ? " (please convert to suspend_late)" : "");
+		if (list_empty(entry))
+			up(&dev->sem);		/* Device was removed */
+		else
+			list_move_tail(entry, &dpm_locked);
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
-	if (error)
-		dpm_resume();
-
-	mutex_unlock(&dpm_mtx);
-	return error;
 }
 
-EXPORT_SYMBOL_GPL(device_suspend);
-
 /**
- *	device_power_down - Shut down special devices.
- *	@state:		Power state to enter.
+ *	device_suspend - Save state and stop all devices in system.
  *
- *	Walk the dpm_off_irq list, calling ->power_down() for each device that
- *	couldn't power down the device with interrupts enabled. When we're
- *	done, power down system devices.
+ *	Prevent new devices from being registered, then lock all devices
+ *	and suspend them.
  */
-
-int device_power_down(pm_message_t state)
+int device_suspend(pm_message_t state)
 {
-	int error = 0;
-	struct device * dev;
-
-	while (!list_empty(&dpm_off)) {
-		struct list_head * entry = dpm_off.prev;
+	int error;
 
-		dev = to_device(entry);
-		error = suspend_device_late(dev, state);
-		if (error)
-			goto Error;
-		list_move(&dev->power.entry, &dpm_off_irq);
+	might_sleep();
+	down_write(&pm_sleep_rwsem);
+	lock_all_devices();
+	error = dpm_suspend(state);
+	if (error) {
+		unlock_all_devices();
+		up_write(&pm_sleep_rwsem);
 	}
-
-	error = sysdev_suspend(state);
- Done:
 	return error;
- Error:
-	printk(KERN_ERR "Could not power down device %s: "
-		"error %d\n", kobject_name(&dev->kobj), error);
-	dpm_power_up();
-	goto Done;
 }
-
-EXPORT_SYMBOL_GPL(device_power_down);
+EXPORT_SYMBOL_GPL(device_suspend);
 
 void __suspend_report_result(const char *function, void *fn, int ret)
 {
Index: usb-2.6/drivers/base/core.c
===================================================================
--- usb-2.6.orig/drivers/base/core.c
+++ usb-2.6/drivers/base/core.c
@@ -712,11 +712,17 @@ int device_add(struct device *dev)
 {
 	struct device *parent = NULL;
 	struct class_interface *class_intf;
-	int error = -EINVAL;
+	int error;
+
+	error = pm_sleep_lock();
+	if (error)
+		return error;
 
 	dev = get_device(dev);
-	if (!dev || !strlen(dev->bus_id))
-		goto Error;
+	if (!dev || !strlen(dev->bus_id)) {
+		error = -EINVAL;
+		goto Done;
+	}
 
 	pr_debug("DEV: registering device: ID = '%s'\n", dev->bus_id);
 
@@ -780,6 +786,7 @@ int device_add(struct device *dev)
 	}
  Done:
 	put_device(dev);
+	pm_sleep_unlock();
 	return error;
  BusError:
 	device_pm_remove(dev);
Index: usb-2.6/drivers/base/power/power.h
===================================================================
--- usb-2.6.orig/drivers/base/power/power.h
+++ usb-2.6/drivers/base/power/power.h
@@ -20,6 +20,8 @@ static inline struct device * to_device(
 
 extern int device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern int pm_sleep_lock(void);
+extern void pm_sleep_unlock(void);
 
 /*
  * sysfs.c
@@ -37,7 +39,15 @@ static inline int device_pm_add(struct d
 }
 static inline void device_pm_remove(struct device * dev)
 {
+}
+
+static inline int pm_sleep_lock(void)
+{
+	return 0;
+}
 
+static inline void pm_sleep_unlock(void)
+{
 }
 
 #endif

Re: [PATCH] PM: acquire device locks prior to suspending

[Rafael J. Wysocki]

On Friday, 21 September 2007 21:37, Alan Stern wrote:
> This patch (as994) reorganizes the way suspend and resume
> notifications are sent to drivers.  The major changes are that now the
> PM core acquires every device semaphore before calling the methods,
> and calls to device_add() during suspends will fail.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

> ---
> 
> Index: usb-2.6/drivers/base/power/main.c
> ===================================================================
> --- usb-2.6.orig/drivers/base/power/main.c
> +++ usb-2.6/drivers/base/power/main.c
> @@ -24,17 +24,38 @@
>  #include <linux/mutex.h>
>  #include <linux/pm.h>
>  #include <linux/resume-trace.h>
> +#include <linux/rwsem.h>
>  
>  #include "../base.h"
>  #include "power.h"
>  
> +/*
> + * The entries in the dpm_active list are in a depth first order, simply
> + * because children are guaranteed to be discovered after parents, and
> + * are inserted at the back of the list on discovery.
> + *
> + * All the other lists are kept in the same order, for consistency.
> + * However the lists aren't always traversed in the same order.
> + * Semaphores must be acquired from the top (i.e., front) down
> + * and released in the opposite order.  Devices must be suspended
> + * from the bottom (i.e., end) up and resumed in the opposite order.
> + * That way no parent will be suspended while it still has an active
> + * child.
> + *
> + * Since device_pm_add() may be called with a device semaphore held,
> + * we must never try to acquire a device semaphore while holding
> + * dpm_list_mutex.
> + */
> +
>  LIST_HEAD(dpm_active);
> +static LIST_HEAD(dpm_locked);
>  static LIST_HEAD(dpm_off);
>  static LIST_HEAD(dpm_off_irq);
>  
> -static DEFINE_MUTEX(dpm_mtx);
>  static DEFINE_MUTEX(dpm_list_mtx);
>  
> +static DECLARE_RWSEM(pm_sleep_rwsem);
> +
>  int (*platform_enable_wakeup)(struct device *dev, int is_on);
>  
>  
> @@ -59,29 +80,112 @@ void device_pm_remove(struct device *dev
>  	pr_debug("PM: Removing info for %s:%s\n",
>  		 dev->bus ? dev->bus->name : "No Bus",
>  		 kobject_name(&dev->kobj));
> +
> +	/* Don't remove a device while the PM core has it locked for suspend */
> +	down(&dev->sem);
>  	mutex_lock(&dpm_list_mtx);
>  	dpm_sysfs_remove(dev);
>  	list_del_init(&dev->power.entry);
>  	mutex_unlock(&dpm_list_mtx);
> +	up(&dev->sem);
> +}
> +
> +/**
> + *	pm_sleep_lock - mutual exclusion for registration and suspend
> + *
> + *	Returns 0 if no suspend is underway and device registration
> + *	may proceed, otherwise -EBUSY.
> + */
> +int pm_sleep_lock(void)
> +{
> +	if (down_read_trylock(&pm_sleep_rwsem))
> +		return 0;
> +	return -EBUSY;
> +}
> +
> +/**
> + *	pm_sleep_unlock - mutual exclusion for registration and suspend
> + *
> + *	This routine undoes the effect of device_pm_add_lock
> + *	when a device's registration is complete.
> + */
> +void pm_sleep_unlock(void)
> +{
> +	up_read(&pm_sleep_rwsem);
>  }
>  
>  
>  /*------------------------- Resume routines -------------------------*/
>  
>  /**
> - *	resume_device - Restore state for one device.
> + *	resume_device_early - Power on one device (early resume).
>   *	@dev:	Device.
>   *
> + *	Must be called with interrupts disabled.
>   */
> -
> -static int resume_device(struct device * dev)
> +static int resume_device_early(struct device *dev)
>  {
>  	int error = 0;
>  
>  	TRACE_DEVICE(dev);
>  	TRACE_RESUME(0);
>  
> -	down(&dev->sem);
> +	if (dev->bus && dev->bus->resume_early) {
> +		dev_dbg(dev,"EARLY resume\n");
> +		error = dev->bus->resume_early(dev);
> +	}
> +
> +	TRACE_RESUME(error);
> +	return error;
> +}
> +
> +/**
> + *	dpm_power_up - Power on all regular (non-sysdev) devices.
> + *
> + *	Walk the dpm_off_irq list and power each device up. This
> + *	is used for devices that required they be powered down with
> + *	interrupts disabled. As devices are powered on, they are moved
> + *	to the dpm_off list.
> + *
> + *	Interrupts must be disabled when calling this.
> + */
> +static void dpm_power_up(void)
> +{
> +	while (!list_empty(&dpm_off_irq)) {
> +		struct list_head *entry = dpm_off_irq.next;
> +		struct device *dev = to_device(entry);
> +
> +		resume_device_early(dev);
> +		list_move_tail(entry, &dpm_off);
> +	}
> +}
> +
> +/**
> + *	device_power_up - Turn on all devices that need special attention.
> + *
> + *	Power on system devices, then devices that required we shut them down
> + *	with interrupts disabled.
> + *
> + *	Must be called with interrupts disabled.
> + */
> +void device_power_up(void)
> +{
> +	sysdev_resume();
> +	dpm_power_up();
> +}
> +EXPORT_SYMBOL_GPL(device_power_up);
> +
> +/**
> + *	resume_device - Restore state for one device.
> + *	@dev:	Device.
> + *
> + */
> +static int resume_device(struct device *dev)
> +{
> +	int error = 0;
> +
> +	TRACE_DEVICE(dev);
> +	TRACE_RESUME(0);
>  
>  	if (dev->bus && dev->bus->resume) {
>  		dev_dbg(dev,"resuming\n");
> @@ -98,126 +202,68 @@ static int resume_device(struct device *
>  		error = dev->class->resume(dev);
>  	}
>  
> -	up(&dev->sem);
> -
>  	TRACE_RESUME(error);
>  	return error;
>  }
>  
> -
> -static int resume_device_early(struct device * dev)
> +/**
> + *	dpm_resume - Resume every device.
> + *
> + *	Resume the devices that have either not gone through
> + *	the late suspend, or that did go through it but also
> + *	went through the early resume.
> + *
> + *	Take devices from the dpm_off_list, resume them,
> + *	and put them on the dpm_locked list.
> + */
> +static void dpm_resume(void)
>  {
> -	int error = 0;
> +	while(!list_empty(&dpm_off)) {
> +		struct list_head *entry = dpm_off.next;
> +		struct device *dev = to_device(entry);
>  
> -	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);
> +		resume_device(dev);
> +		list_move_tail(entry, &dpm_locked);
>  	}
> -	TRACE_RESUME(error);
> -	return error;
>  }
>  
> -/*
> - * Resume the devices that have either not gone through
> - * the late suspend, or that did go through it but also
> - * went through the early resume
> +/**
> + *	unlock_all_devices - Release each device's semaphore
> + *
> + *	Go through the dpm_off list.  Put each device on the dpm_active
> + *	list and unlock it.
>   */
> -static void dpm_resume(void)
> +static void unlock_all_devices(void)
>  {
>  	mutex_lock(&dpm_list_mtx);
> -	while(!list_empty(&dpm_off)) {
> -		struct list_head * entry = dpm_off.next;
> -		struct device * dev = to_device(entry);
> -
> -		get_device(dev);
> -		list_move_tail(entry, &dpm_active);
> -
> -		mutex_unlock(&dpm_list_mtx);
> -		resume_device(dev);
> -		mutex_lock(&dpm_list_mtx);
> -		put_device(dev);
> -	}
> + 	while (!list_empty(&dpm_locked)) {
> + 		struct list_head *entry = dpm_locked.prev;
> + 		struct device *dev = to_device(entry);
> +
> + 		list_move(entry, &dpm_active);
> + 		up(&dev->sem);
> + 	}
>  	mutex_unlock(&dpm_list_mtx);
>  }
>  
> -
>  /**
>   *	device_resume - Restore state of each device in system.
>   *
> - *	Walk the dpm_off list, remove each entry, resume the device,
> - *	then add it to the dpm_active list.
> + *	Resume all the devices, unlock them all, and allow new
> + *	devices to be registered once again.
>   */
> -
>  void device_resume(void)
>  {
>  	might_sleep();
> -	mutex_lock(&dpm_mtx);
>  	dpm_resume();
> -	mutex_unlock(&dpm_mtx);
> +	unlock_all_devices();
> +	up_write(&pm_sleep_rwsem);
>  }
> -
>  EXPORT_SYMBOL_GPL(device_resume);
>  
>  
> -/**
> - *	dpm_power_up - Power on some devices.
> - *
> - *	Walk the dpm_off_irq list and power each device up. This
> - *	is used for devices that required they be powered down with
> - *	interrupts disabled. As devices are powered on, they are moved
> - *	to the dpm_active list.
> - *
> - *	Interrupts must be disabled when calling this.
> - */
> -
> -static void dpm_power_up(void)
> -{
> -	while(!list_empty(&dpm_off_irq)) {
> -		struct list_head * entry = dpm_off_irq.next;
> -		struct device * dev = to_device(entry);
> -
> -		list_move_tail(entry, &dpm_off);
> -		resume_device_early(dev);
> -	}
> -}
> -
> -
> -/**
> - *	device_power_up - Turn on all devices that need special attention.
> - *
> - *	Power on system devices then devices that required we shut them down
> - *	with interrupts disabled.
> - *	Called with interrupts disabled.
> - */
> -
> -void device_power_up(void)
> -{
> -	sysdev_resume();
> -	dpm_power_up();
> -}
> -
> -EXPORT_SYMBOL_GPL(device_power_up);
> -
> -
>  /*------------------------- Suspend routines -------------------------*/
>  
> -/*
> - * The entries in the dpm_active list are in a depth first order, simply
> - * because children are guaranteed to be discovered after parents, and
> - * are inserted at the back of the list on discovery.
> - *
> - * All list on the suspend path are done in reverse order, so we operate
> - * on the leaves of the device tree (or forests, depending on how you want
> - * to look at it ;) first. As nodes are removed from the back of the list,
> - * they are inserted into the front of their destintation lists.
> - *
> - * Things are the reverse on the resume path - iterations are done in
> - * forward order, and nodes are inserted at the back of their destination
> - * lists. This way, the ancestors will be accessed before their descendents.
> - */
> -
>  static inline char *suspend_verb(u32 event)
>  {
>  	switch (event) {
> @@ -228,7 +274,6 @@ static inline char *suspend_verb(u32 eve
>  	}
>  }
>  
> -
>  static void
>  suspend_device_dbg(struct device *dev, pm_message_t state, char *info)
>  {
> @@ -238,16 +283,69 @@ suspend_device_dbg(struct device *dev, p
>  }
>  
>  /**
> - *	suspend_device - Save state of one device.
> + *	suspend_device_late - Shut down one device (late suspend).
>   *	@dev:	Device.
>   *	@state:	Power state device is entering.
> + *
> + *	This is called with interrupts off and only a single CPU running.
> + */
> +static int suspend_device_late(struct device *dev, pm_message_t state)
> +{
> +	int error = 0;
> +
> +	if (dev->bus && dev->bus->suspend_late) {
> +		suspend_device_dbg(dev, state, "LATE ");
> +		error = dev->bus->suspend_late(dev, state);
> +		suspend_report_result(dev->bus->suspend_late, error);
> +	}
> +	return error;
> +}
> +
> +/**
> + *	device_power_down - Shut down special devices.
> + *	@state:		Power state to enter.
> + *
> + *	Power down devices that require interrupts to be disabled
> + *	and move them from the dpm_off list to the dpm_off_irq list.
> + *	Then power down system devices.
> + *
> + *	Must be called with interrupts disabled and only one CPU running.
>   */
> +int device_power_down(pm_message_t state)
> +{
> +	int error = 0;
> +
> +	while (!list_empty(&dpm_off)) {
> +		struct list_head *entry = dpm_off.prev;
> +		struct device *dev = to_device(entry);
> +
> +		error = suspend_device_late(dev, state);
> +		if (error) {
> +			printk(KERN_ERR "Could not power down device %s: "
> +					"error %d\n",
> +					kobject_name(&dev->kobj), error);
> +			break;
> +		}
> +		list_move(&dev->power.entry, &dpm_off_irq);
> +	}
> +
> +	if (!error)
> +		error = sysdev_suspend(state);
> +	if (error)
> +		dpm_power_up();
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(device_power_down);
>  
> -static int suspend_device(struct device * dev, pm_message_t state)
> +/**
> + *	suspend_device - Save state of one device.
> + *	@dev:	Device.
> + *	@state:	Power state device is entering.
> + */
> +int suspend_device(struct device *dev, pm_message_t state)
>  {
>  	int error = 0;
>  
> -	down(&dev->sem);
>  	if (dev->power.power_state.event) {
>  		dev_dbg(dev, "PM: suspend %d-->%d\n",
>  			dev->power.power_state.event, state.event);
> @@ -270,123 +368,99 @@ static int suspend_device(struct device 
>  		error = dev->bus->suspend(dev, state);
>  		suspend_report_result(dev->bus->suspend, error);
>  	}
> -	up(&dev->sem);
>  	return error;
>  }
>  
> -
> -/*
> - * This is called with interrupts off, only a single CPU
> - * running. We can't acquire a mutex or semaphore (and we don't
> - * need the protection)
> +/**
> + *	dpm_suspend - Suspend every device.
> + *	@state:	Power state to put each device in.
> + *
> + *	Walk the dpm_locked list.  Suspend each device and move it
> + *	to the dpm_off list.
> + *
> + *	(For historical reasons, if it returns -EAGAIN, that used to mean
> + *	that the device would be called again with interrupts disabled.
> + *	These days, we use the "suspend_late()" callback for that, so we
> + *	print a warning and consider it an error).
>   */
> -static int suspend_device_late(struct device *dev, pm_message_t state)
> +static int dpm_suspend(pm_message_t state)
>  {
>  	int error = 0;
>  
> -	if (dev->bus && dev->bus->suspend_late) {
> -		suspend_device_dbg(dev, state, "LATE ");
> -		error = dev->bus->suspend_late(dev, state);
> -		suspend_report_result(dev->bus->suspend_late, error);
> +	while (!list_empty(&dpm_locked)) {
> +		struct list_head *entry = dpm_locked.prev;
> +		struct device *dev = to_device(entry);
> +
> +		error = suspend_device(dev, state);
> +		if (error) {
> +			printk(KERN_ERR "Could not suspend device %s: "
> +					"error %d%s\n",
> +					kobject_name(&dev->kobj),
> +					error,
> +					(error == -EAGAIN ?
> +					" (please convert to suspend_late)" :
> +					""));
> +			break;
> +		}
> + 		list_move(&dev->power.entry, &dpm_off);
>  	}
> +
> +	if (error)
> +		dpm_resume();
>  	return error;
>  }
>  
>  /**
> - *	device_suspend - Save state and stop all devices in system.
> - *	@state:		Power state to put each device in.
> - *
> - *	Walk the dpm_active list, call ->suspend() for each device, and move
> - *	it to the dpm_off list.
> - *
> - *	(For historical reasons, if it returns -EAGAIN, that used to mean
> - *	that the device would be called again with interrupts disabled.
> - *	These days, we use the "suspend_late()" callback for that, so we
> - *	print a warning and consider it an error).
> - *
> - *	If we get a different error, try and back out.
> - *
> - *	If we hit a failure with any of the devices, call device_resume()
> - *	above to bring the suspended devices back to life.
> + *	lock_all_devices - Acquire every device's semaphore
>   *
> + *	Go through the dpm_active list. Carefully lock each device's
> + *	semaphore and put it in on the dpm_locked list.
>   */
> -
> -int device_suspend(pm_message_t state)
> +static void lock_all_devices(void)
>  {
> -	int error = 0;
> -
> -	might_sleep();
> -	mutex_lock(&dpm_mtx);
>  	mutex_lock(&dpm_list_mtx);
> -	while (!list_empty(&dpm_active) && error == 0) {
> -		struct list_head * entry = dpm_active.prev;
> -		struct device * dev = to_device(entry);
> -
> +	while (!list_empty(&dpm_active)) {
> +		struct list_head *entry = dpm_active.next;
> +		struct device *dev = to_device(entry);
> +
> +		/* Required locking order is dev->sem first,
> +		 * then dpm_list_mutex.  Hence this awkward code.
> +		 */
>  		get_device(dev);
>  		mutex_unlock(&dpm_list_mtx);
> -
> -		error = suspend_device(dev, state);
> -
> +		down(&dev->sem);
>  		mutex_lock(&dpm_list_mtx);
>  
> -		/* Check if the device got removed */
> -		if (!list_empty(&dev->power.entry)) {
> -			/* Move it to the dpm_off list */
> -			if (!error)
> -				list_move(&dev->power.entry, &dpm_off);
> -		}
> -		if (error)
> -			printk(KERN_ERR "Could not suspend device %s: "
> -				"error %d%s\n",
> -				kobject_name(&dev->kobj), error,
> -				error == -EAGAIN ? " (please convert to suspend_late)" : "");
> +		if (list_empty(entry))
> +			up(&dev->sem);		/* Device was removed */
> +		else
> +			list_move_tail(entry, &dpm_locked);
>  		put_device(dev);
>  	}
>  	mutex_unlock(&dpm_list_mtx);
> -	if (error)
> -		dpm_resume();
> -
> -	mutex_unlock(&dpm_mtx);
> -	return error;
>  }
>  
> -EXPORT_SYMBOL_GPL(device_suspend);
> -
>  /**
> - *	device_power_down - Shut down special devices.
> - *	@state:		Power state to enter.
> + *	device_suspend - Save state and stop all devices in system.
>   *
> - *	Walk the dpm_off_irq list, calling ->power_down() for each device that
> - *	couldn't power down the device with interrupts enabled. When we're
> - *	done, power down system devices.
> + *	Prevent new devices from being registered, then lock all devices
> + *	and suspend them.
>   */
> -
> -int device_power_down(pm_message_t state)
> +int device_suspend(pm_message_t state)
>  {
> -	int error = 0;
> -	struct device * dev;
> -
> -	while (!list_empty(&dpm_off)) {
> -		struct list_head * entry = dpm_off.prev;
> +	int error;
>  
> -		dev = to_device(entry);
> -		error = suspend_device_late(dev, state);
> -		if (error)
> -			goto Error;
> -		list_move(&dev->power.entry, &dpm_off_irq);
> +	might_sleep();
> +	down_write(&pm_sleep_rwsem);
> +	lock_all_devices();
> +	error = dpm_suspend(state);
> +	if (error) {
> +		unlock_all_devices();
> +		up_write(&pm_sleep_rwsem);
>  	}
> -
> -	error = sysdev_suspend(state);
> - Done:
>  	return error;
> - Error:
> -	printk(KERN_ERR "Could not power down device %s: "
> -		"error %d\n", kobject_name(&dev->kobj), error);
> -	dpm_power_up();
> -	goto Done;
>  }
> -
> -EXPORT_SYMBOL_GPL(device_power_down);
> +EXPORT_SYMBOL_GPL(device_suspend);
>  
>  void __suspend_report_result(const char *function, void *fn, int ret)
>  {
> Index: usb-2.6/drivers/base/core.c
> ===================================================================
> --- usb-2.6.orig/drivers/base/core.c
> +++ usb-2.6/drivers/base/core.c
> @@ -712,11 +712,17 @@ int device_add(struct device *dev)
>  {
>  	struct device *parent = NULL;
>  	struct class_interface *class_intf;
> -	int error = -EINVAL;
> +	int error;
> +
> +	error = pm_sleep_lock();
> +	if (error)
> +		return error;
>  
>  	dev = get_device(dev);
> -	if (!dev || !strlen(dev->bus_id))
> -		goto Error;
> +	if (!dev || !strlen(dev->bus_id)) {
> +		error = -EINVAL;
> +		goto Done;
> +	}
>  
>  	pr_debug("DEV: registering device: ID = '%s'\n", dev->bus_id);
>  
> @@ -780,6 +786,7 @@ int device_add(struct device *dev)
>  	}
>   Done:
>  	put_device(dev);
> +	pm_sleep_unlock();
>  	return error;
>   BusError:
>  	device_pm_remove(dev);
> Index: usb-2.6/drivers/base/power/power.h
> ===================================================================
> --- usb-2.6.orig/drivers/base/power/power.h
> +++ usb-2.6/drivers/base/power/power.h
> @@ -20,6 +20,8 @@ static inline struct device * to_device(
>  
>  extern int device_pm_add(struct device *);
>  extern void device_pm_remove(struct device *);
> +extern int pm_sleep_lock(void);
> +extern void pm_sleep_unlock(void);
>  
>  /*
>   * sysfs.c
> @@ -37,7 +39,15 @@ static inline int device_pm_add(struct d
>  }
>  static inline void device_pm_remove(struct device * dev)
>  {
> +}
> +
> +static inline int pm_sleep_lock(void)
> +{
> +	return 0;
> +}
>  
> +static inline void pm_sleep_unlock(void)
> +{
>  }
>  
>  #endif
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
> 
> 

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

patch pm-acquire-device-locks-prior-to-suspending.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: PM: acquire device locks prior to suspending

to my gregkh-2.6 tree.  Its filename is

     pm-acquire-device-locks-prior-to-suspending.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


>From stern@rowland.harvard.edu Fri Sep 21 12:37:48 2007
From: Alan Stern <stern@rowland.harvard.edu>
Date: Fri, 21 Sep 2007 15:37:40 -0400 (EDT)
Subject: PM: acquire device locks prior to suspending
To: Greg KH <greg@kroah.com>
Cc: Linux-pm mailing list <linux-pm@lists.linux-foundation.org>
Message-ID: <Pine.LNX.4.44L0.0709211536570.5816-100000@iolanthe.rowland.org>


This patch (as994) reorganizes the way suspend and resume
notifications are sent to drivers.  The major changes are that now the
PM core acquires every device semaphore before calling the methods,
and calls to device_add() during suspends will fail.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/base/core.c        |   13 +
 drivers/base/power/main.c  |  442 ++++++++++++++++++++++++++-------------------
 drivers/base/power/power.h |   10 +
 3 files changed, 278 insertions(+), 187 deletions(-)

--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -733,11 +733,17 @@ int device_add(struct device *dev)
 {
 	struct device *parent = NULL;
 	struct class_interface *class_intf;
-	int error = -EINVAL;
+	int error;
+
+	error = pm_sleep_lock();
+	if (error)
+		return error;
 
 	dev = get_device(dev);
-	if (!dev || !strlen(dev->bus_id))
-		goto Error;
+	if (!dev || !strlen(dev->bus_id)) {
+		error = -EINVAL;
+		goto Done;
+	}
 
 	pr_debug("DEV: registering device: ID = '%s'\n", dev->bus_id);
 
@@ -801,6 +807,7 @@ int device_add(struct device *dev)
 	}
  Done:
 	put_device(dev);
+	pm_sleep_unlock();
 	return error;
  BusError:
 	device_pm_remove(dev);
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -24,17 +24,38 @@
 #include <linux/mutex.h>
 #include <linux/pm.h>
 #include <linux/resume-trace.h>
+#include <linux/rwsem.h>
 
 #include "../base.h"
 #include "power.h"
 
+/*
+ * The entries in the dpm_active list are in a depth first order, simply
+ * because children are guaranteed to be discovered after parents, and
+ * are inserted at the back of the list on discovery.
+ *
+ * All the other lists are kept in the same order, for consistency.
+ * However the lists aren't always traversed in the same order.
+ * Semaphores must be acquired from the top (i.e., front) down
+ * and released in the opposite order.  Devices must be suspended
+ * from the bottom (i.e., end) up and resumed in the opposite order.
+ * That way no parent will be suspended while it still has an active
+ * child.
+ *
+ * Since device_pm_add() may be called with a device semaphore held,
+ * we must never try to acquire a device semaphore while holding
+ * dpm_list_mutex.
+ */
+
 LIST_HEAD(dpm_active);
+static LIST_HEAD(dpm_locked);
 static LIST_HEAD(dpm_off);
 static LIST_HEAD(dpm_off_irq);
 
-static DEFINE_MUTEX(dpm_mtx);
 static DEFINE_MUTEX(dpm_list_mtx);
 
+static DECLARE_RWSEM(pm_sleep_rwsem);
+
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
 
@@ -59,29 +80,112 @@ void device_pm_remove(struct device *dev
 	pr_debug("PM: Removing info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
+
+	/* Don't remove a device while the PM core has it locked for suspend */
+	down(&dev->sem);
 	mutex_lock(&dpm_list_mtx);
 	dpm_sysfs_remove(dev);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
+	up(&dev->sem);
+}
+
+/**
+ *	pm_sleep_lock - mutual exclusion for registration and suspend
+ *
+ *	Returns 0 if no suspend is underway and device registration
+ *	may proceed, otherwise -EBUSY.
+ */
+int pm_sleep_lock(void)
+{
+	if (down_read_trylock(&pm_sleep_rwsem))
+		return 0;
+	return -EBUSY;
+}
+
+/**
+ *	pm_sleep_unlock - mutual exclusion for registration and suspend
+ *
+ *	This routine undoes the effect of device_pm_add_lock
+ *	when a device's registration is complete.
+ */
+void pm_sleep_unlock(void)
+{
+	up_read(&pm_sleep_rwsem);
 }
 
 
 /*------------------------- Resume routines -------------------------*/
 
 /**
- *	resume_device - Restore state for one device.
+ *	resume_device_early - Power on one device (early resume).
  *	@dev:	Device.
  *
+ *	Must be called with interrupts disabled.
  */
-
-static int resume_device(struct device * dev)
+static int resume_device_early(struct device *dev)
 {
 	int error = 0;
 
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	down(&dev->sem);
+	if (dev->bus && dev->bus->resume_early) {
+		dev_dbg(dev,"EARLY resume\n");
+		error = dev->bus->resume_early(dev);
+	}
+
+	TRACE_RESUME(error);
+	return error;
+}
+
+/**
+ *	dpm_power_up - Power on all regular (non-sysdev) devices.
+ *
+ *	Walk the dpm_off_irq list and power each device up. This
+ *	is used for devices that required they be powered down with
+ *	interrupts disabled. As devices are powered on, they are moved
+ *	to the dpm_off list.
+ *
+ *	Interrupts must be disabled when calling this.
+ */
+static void dpm_power_up(void)
+{
+	while (!list_empty(&dpm_off_irq)) {
+		struct list_head *entry = dpm_off_irq.next;
+		struct device *dev = to_device(entry);
+
+		resume_device_early(dev);
+		list_move_tail(entry, &dpm_off);
+	}
+}
+
+/**
+ *	device_power_up - Turn on all devices that need special attention.
+ *
+ *	Power on system devices, then devices that required we shut them down
+ *	with interrupts disabled.
+ *
+ *	Must be called with interrupts disabled.
+ */
+void device_power_up(void)
+{
+	sysdev_resume();
+	dpm_power_up();
+}
+EXPORT_SYMBOL_GPL(device_power_up);
+
+/**
+ *	resume_device - Restore state for one device.
+ *	@dev:	Device.
+ *
+ */
+static int resume_device(struct device *dev)
+{
+	int error = 0;
+
+	TRACE_DEVICE(dev);
+	TRACE_RESUME(0);
 
 	if (dev->bus && dev->bus->resume) {
 		dev_dbg(dev,"resuming\n");
@@ -98,126 +202,68 @@ static int resume_device(struct device *
 		error = dev->class->resume(dev);
 	}
 
-	up(&dev->sem);
-
 	TRACE_RESUME(error);
 	return error;
 }
 
-
-static int resume_device_early(struct device * dev)
+/**
+ *	dpm_resume - Resume every device.
+ *
+ *	Resume the devices that have either not gone through
+ *	the late suspend, or that did go through it but also
+ *	went through the early resume.
+ *
+ *	Take devices from the dpm_off_list, resume them,
+ *	and put them on the dpm_locked list.
+ */
+static void dpm_resume(void)
 {
-	int error = 0;
+	while(!list_empty(&dpm_off)) {
+		struct list_head *entry = dpm_off.next;
+		struct device *dev = to_device(entry);
 
-	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);
+		resume_device(dev);
+		list_move_tail(entry, &dpm_locked);
 	}
-	TRACE_RESUME(error);
-	return error;
 }
 
-/*
- * Resume the devices that have either not gone through
- * the late suspend, or that did go through it but also
- * went through the early resume
+/**
+ *	unlock_all_devices - Release each device's semaphore
+ *
+ *	Go through the dpm_off list.  Put each device on the dpm_active
+ *	list and unlock it.
  */
-static void dpm_resume(void)
+static void unlock_all_devices(void)
 {
 	mutex_lock(&dpm_list_mtx);
-	while(!list_empty(&dpm_off)) {
-		struct list_head * entry = dpm_off.next;
-		struct device * dev = to_device(entry);
-
-		get_device(dev);
-		list_move_tail(entry, &dpm_active);
-
-		mutex_unlock(&dpm_list_mtx);
-		resume_device(dev);
-		mutex_lock(&dpm_list_mtx);
-		put_device(dev);
-	}
+ 	while (!list_empty(&dpm_locked)) {
+ 		struct list_head *entry = dpm_locked.prev;
+ 		struct device *dev = to_device(entry);
+
+ 		list_move(entry, &dpm_active);
+ 		up(&dev->sem);
+ 	}
 	mutex_unlock(&dpm_list_mtx);
 }
 
-
 /**
  *	device_resume - Restore state of each device in system.
  *
- *	Walk the dpm_off list, remove each entry, resume the device,
- *	then add it to the dpm_active list.
+ *	Resume all the devices, unlock them all, and allow new
+ *	devices to be registered once again.
  */
-
 void device_resume(void)
 {
 	might_sleep();
-	mutex_lock(&dpm_mtx);
 	dpm_resume();
-	mutex_unlock(&dpm_mtx);
+	unlock_all_devices();
+	up_write(&pm_sleep_rwsem);
 }
-
 EXPORT_SYMBOL_GPL(device_resume);
 
 
-/**
- *	dpm_power_up - Power on some devices.
- *
- *	Walk the dpm_off_irq list and power each device up. This
- *	is used for devices that required they be powered down with
- *	interrupts disabled. As devices are powered on, they are moved
- *	to the dpm_active list.
- *
- *	Interrupts must be disabled when calling this.
- */
-
-static void dpm_power_up(void)
-{
-	while(!list_empty(&dpm_off_irq)) {
-		struct list_head * entry = dpm_off_irq.next;
-		struct device * dev = to_device(entry);
-
-		list_move_tail(entry, &dpm_off);
-		resume_device_early(dev);
-	}
-}
-
-
-/**
- *	device_power_up - Turn on all devices that need special attention.
- *
- *	Power on system devices then devices that required we shut them down
- *	with interrupts disabled.
- *	Called with interrupts disabled.
- */
-
-void device_power_up(void)
-{
-	sysdev_resume();
-	dpm_power_up();
-}
-
-EXPORT_SYMBOL_GPL(device_power_up);
-
-
 /*------------------------- Suspend routines -------------------------*/
 
-/*
- * The entries in the dpm_active list are in a depth first order, simply
- * because children are guaranteed to be discovered after parents, and
- * are inserted at the back of the list on discovery.
- *
- * All list on the suspend path are done in reverse order, so we operate
- * on the leaves of the device tree (or forests, depending on how you want
- * to look at it ;) first. As nodes are removed from the back of the list,
- * they are inserted into the front of their destintation lists.
- *
- * Things are the reverse on the resume path - iterations are done in
- * forward order, and nodes are inserted at the back of their destination
- * lists. This way, the ancestors will be accessed before their descendents.
- */
-
 static inline char *suspend_verb(u32 event)
 {
 	switch (event) {
@@ -228,7 +274,6 @@ static inline char *suspend_verb(u32 eve
 	}
 }
 
-
 static void
 suspend_device_dbg(struct device *dev, pm_message_t state, char *info)
 {
@@ -238,16 +283,69 @@ suspend_device_dbg(struct device *dev, p
 }
 
 /**
- *	suspend_device - Save state of one device.
+ *	suspend_device_late - Shut down one device (late suspend).
  *	@dev:	Device.
  *	@state:	Power state device is entering.
+ *
+ *	This is called with interrupts off and only a single CPU running.
+ */
+static int suspend_device_late(struct device *dev, pm_message_t state)
+{
+	int error = 0;
+
+	if (dev->bus && dev->bus->suspend_late) {
+		suspend_device_dbg(dev, state, "LATE ");
+		error = dev->bus->suspend_late(dev, state);
+		suspend_report_result(dev->bus->suspend_late, error);
+	}
+	return error;
+}
+
+/**
+ *	device_power_down - Shut down special devices.
+ *	@state:		Power state to enter.
+ *
+ *	Power down devices that require interrupts to be disabled
+ *	and move them from the dpm_off list to the dpm_off_irq list.
+ *	Then power down system devices.
+ *
+ *	Must be called with interrupts disabled and only one CPU running.
  */
+int device_power_down(pm_message_t state)
+{
+	int error = 0;
+
+	while (!list_empty(&dpm_off)) {
+		struct list_head *entry = dpm_off.prev;
+		struct device *dev = to_device(entry);
+
+		error = suspend_device_late(dev, state);
+		if (error) {
+			printk(KERN_ERR "Could not power down device %s: "
+					"error %d\n",
+					kobject_name(&dev->kobj), error);
+			break;
+		}
+		list_move(&dev->power.entry, &dpm_off_irq);
+	}
+
+	if (!error)
+		error = sysdev_suspend(state);
+	if (error)
+		dpm_power_up();
+	return error;
+}
+EXPORT_SYMBOL_GPL(device_power_down);
 
-static int suspend_device(struct device * dev, pm_message_t state)
+/**
+ *	suspend_device - Save state of one device.
+ *	@dev:	Device.
+ *	@state:	Power state device is entering.
+ */
+int suspend_device(struct device *dev, pm_message_t state)
 {
 	int error = 0;
 
-	down(&dev->sem);
 	if (dev->power.power_state.event) {
 		dev_dbg(dev, "PM: suspend %d-->%d\n",
 			dev->power.power_state.event, state.event);
@@ -270,123 +368,99 @@ static int suspend_device(struct device 
 		error = dev->bus->suspend(dev, state);
 		suspend_report_result(dev->bus->suspend, error);
 	}
-	up(&dev->sem);
 	return error;
 }
 
-
-/*
- * This is called with interrupts off, only a single CPU
- * running. We can't acquire a mutex or semaphore (and we don't
- * need the protection)
+/**
+ *	dpm_suspend - Suspend every device.
+ *	@state:	Power state to put each device in.
+ *
+ *	Walk the dpm_locked list.  Suspend each device and move it
+ *	to the dpm_off list.
+ *
+ *	(For historical reasons, if it returns -EAGAIN, that used to mean
+ *	that the device would be called again with interrupts disabled.
+ *	These days, we use the "suspend_late()" callback for that, so we
+ *	print a warning and consider it an error).
  */
-static int suspend_device_late(struct device *dev, pm_message_t state)
+static int dpm_suspend(pm_message_t state)
 {
 	int error = 0;
 
-	if (dev->bus && dev->bus->suspend_late) {
-		suspend_device_dbg(dev, state, "LATE ");
-		error = dev->bus->suspend_late(dev, state);
-		suspend_report_result(dev->bus->suspend_late, error);
+	while (!list_empty(&dpm_locked)) {
+		struct list_head *entry = dpm_locked.prev;
+		struct device *dev = to_device(entry);
+
+		error = suspend_device(dev, state);
+		if (error) {
+			printk(KERN_ERR "Could not suspend device %s: "
+					"error %d%s\n",
+					kobject_name(&dev->kobj),
+					error,
+					(error == -EAGAIN ?
+					" (please convert to suspend_late)" :
+					""));
+			break;
+		}
+ 		list_move(&dev->power.entry, &dpm_off);
 	}
+
+	if (error)
+		dpm_resume();
 	return error;
 }
 
 /**
- *	device_suspend - Save state and stop all devices in system.
- *	@state:		Power state to put each device in.
- *
- *	Walk the dpm_active list, call ->suspend() for each device, and move
- *	it to the dpm_off list.
- *
- *	(For historical reasons, if it returns -EAGAIN, that used to mean
- *	that the device would be called again with interrupts disabled.
- *	These days, we use the "suspend_late()" callback for that, so we
- *	print a warning and consider it an error).
- *
- *	If we get a different error, try and back out.
- *
- *	If we hit a failure with any of the devices, call device_resume()
- *	above to bring the suspended devices back to life.
+ *	lock_all_devices - Acquire every device's semaphore
  *
+ *	Go through the dpm_active list. Carefully lock each device's
+ *	semaphore and put it in on the dpm_locked list.
  */
-
-int device_suspend(pm_message_t state)
+static void lock_all_devices(void)
 {
-	int error = 0;
-
-	might_sleep();
-	mutex_lock(&dpm_mtx);
 	mutex_lock(&dpm_list_mtx);
-	while (!list_empty(&dpm_active) && error == 0) {
-		struct list_head * entry = dpm_active.prev;
-		struct device * dev = to_device(entry);
-
+	while (!list_empty(&dpm_active)) {
+		struct list_head *entry = dpm_active.next;
+		struct device *dev = to_device(entry);
+
+		/* Required locking order is dev->sem first,
+		 * then dpm_list_mutex.  Hence this awkward code.
+		 */
 		get_device(dev);
 		mutex_unlock(&dpm_list_mtx);
-
-		error = suspend_device(dev, state);
-
+		down(&dev->sem);
 		mutex_lock(&dpm_list_mtx);
 
-		/* Check if the device got removed */
-		if (!list_empty(&dev->power.entry)) {
-			/* Move it to the dpm_off list */
-			if (!error)
-				list_move(&dev->power.entry, &dpm_off);
-		}
-		if (error)
-			printk(KERN_ERR "Could not suspend device %s: "
-				"error %d%s\n",
-				kobject_name(&dev->kobj), error,
-				error == -EAGAIN ? " (please convert to suspend_late)" : "");
+		if (list_empty(entry))
+			up(&dev->sem);		/* Device was removed */
+		else
+			list_move_tail(entry, &dpm_locked);
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
-	if (error)
-		dpm_resume();
-
-	mutex_unlock(&dpm_mtx);
-	return error;
 }
 
-EXPORT_SYMBOL_GPL(device_suspend);
-
 /**
- *	device_power_down - Shut down special devices.
- *	@state:		Power state to enter.
+ *	device_suspend - Save state and stop all devices in system.
  *
- *	Walk the dpm_off_irq list, calling ->power_down() for each device that
- *	couldn't power down the device with interrupts enabled. When we're
- *	done, power down system devices.
+ *	Prevent new devices from being registered, then lock all devices
+ *	and suspend them.
  */
-
-int device_power_down(pm_message_t state)
+int device_suspend(pm_message_t state)
 {
-	int error = 0;
-	struct device * dev;
-
-	while (!list_empty(&dpm_off)) {
-		struct list_head * entry = dpm_off.prev;
+	int error;
 
-		dev = to_device(entry);
-		error = suspend_device_late(dev, state);
-		if (error)
-			goto Error;
-		list_move(&dev->power.entry, &dpm_off_irq);
+	might_sleep();
+	down_write(&pm_sleep_rwsem);
+	lock_all_devices();
+	error = dpm_suspend(state);
+	if (error) {
+		unlock_all_devices();
+		up_write(&pm_sleep_rwsem);
 	}
-
-	error = sysdev_suspend(state);
- Done:
 	return error;
- Error:
-	printk(KERN_ERR "Could not power down device %s: "
-		"error %d\n", kobject_name(&dev->kobj), error);
-	dpm_power_up();
-	goto Done;
 }
-
-EXPORT_SYMBOL_GPL(device_power_down);
+EXPORT_SYMBOL_GPL(device_suspend);
 
 void __suspend_report_result(const char *function, void *fn, int ret)
 {
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -20,6 +20,8 @@ static inline struct device * to_device(
 
 extern int device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern int pm_sleep_lock(void);
+extern void pm_sleep_unlock(void);
 
 /*
  * sysfs.c
@@ -37,7 +39,15 @@ static inline int device_pm_add(struct d
 }
 static inline void device_pm_remove(struct device * dev)
 {
+}
+
+static inline int pm_sleep_lock(void)
+{
+	return 0;
+}
 
+static inline void pm_sleep_unlock(void)
+{
 }
 
 #endif


Patches currently in gregkh-2.6 which might be from stern@rowland.harvard.edu are

driver/sysfs-remove-first-pass-at-shadow-directory-support.patch
driver/sysfs-fix-i_mutex-locking-in-sysfs_get_dentry.patch
driver/sysfs-in-sysfs_lookup-don-t-open-code-sysfs_find_dirent.patch
driver/sysfs-cosmetic-changes-in-sysfs_lookup.patch
driver/sysfs-make-sysfs_add-remove_one-call-link-unlink_sibling-implictly.patch
driver/sysfs-make-sysfs_add_one-automatically-check-for-duplicate-entry.patch
driver/sysfs-make-sysfs_addrm_finish-return-void.patch
driver/sysfs-simplify-sysfs_rename_dir.patch
driver/sysfs-introduce-sysfs_rename_mutex.patch
driver/sysfs-clean-up-header-files.patch
driver/sysfs-make-sysfs_mount-static.patch
driver/sysfs-remove-s_dentry.patch
driver/sysfs-remove-sysfs_instantiate.patch
driver/sysfs-rewrite-rename-in-terms-of-sysfs-dirents.patch
driver/sysfs-move-all-of-inode-initialization-into-sysfs_init_inode.patch
driver/sysfs-rewrite-sysfs_drop_dentry.patch
driver/sysfs-rewrite-sysfs_move_dir-in-terms-of-sysfs-dirents.patch
driver/sysfs-simplify-readdir.patch
driver/sysfs-simply-sysfs_get_dentry.patch
driver/sysfs-use-kill_anon_super.patch
driver/sysfs-fix-comments-of-sysfs_add-remove_one.patch
driver/sysfs-fix-sysfs_chmod_file-such-that-it-updates-sd-s_mode-too.patch
driver/sysfs-implement-sysfs_open_dirent.patch
driver/sysfs-kill-sysfs_update_file.patch
driver/sysfs-kill-unnecessary-null-pointer-check-in-sysfs_release.patch
driver/sysfs-kill-unnecessary-sysfs_get-in-open-paths.patch
driver/sysfs-make-bin-attr-open-get-active-reference-of-parent-too.patch
driver/sysfs-make-s_elem-an-anonymous-union.patch
driver/sysfs-make-sysfs_root-a-regular-directory-dirent.patch
driver/sysfs-move-sysfs-file-poll-implementation-to-sysfs_open_dirent.patch
driver/sysfs-move-sysfs_dirent-s_children-into-sysfs_dirent-s_dir.patch
driver/sysfs-open-code-sysfs_attach_dentry.patch
driver/sysfs-reposition-sysfs_dirent-s_mode.patch
driver/pm-acquire-device-locks-prior-to-suspending.patch
driver/pm-merge-device-power-management-source-files.patch
driver/sysfs-add-copyrights.patch
usb/usb-add-ep-enable.patch
usb/usb-avoid-the-donelist-after-an-error-in-ohci-hcd.patch
usb/usb-add-direction-bit-to-urb-transfer_flags.patch
usb/usb-add-urb-ep.patch
usb/usb-address-0-handling-during-device-initialization.patch
usb/usb-avoid-urb-pipe-in-usbfs.patch
usb/usb-avoid-urb-pipe-in-usbmon.patch
usb/usb-avoid-using-urb-pipe-in-usbcore.patch
usb/usb-cleanup-for-previous-patches.patch
usb/usb-gadget-file-storage-gadget-cleanups.patch
usb/usb-separate-out-endpoint-queue-management-and-dma-mapping-routines.patch
usb/usb-update-spinlock-usage-for-root-hub-urbs.patch
usb/usb-fix-mistake-in-usb_hcd_giveback_urb.patch
usb/usb-less-restrictive-command-checking-in-g-file-storage.patch
usb/usb-reorganize-urb-status-use-in-dummy-hcd.patch
usb/usb-reorganize-urb-status-use-in-ehci-hcd.patch
usb/usb-reorganize-urb-status-use-in-ohci-hcd.patch
usb/usb-reorganize-urb-status-use-in-r8a66597-hcd.patch
usb/usb-reorganize-urb-status-use-in-sl811-hcd.patch
usb/usb-reorganize-urb-status-use-in-usbmon.patch
usb/usb-eliminate-urb-status-usage.patch
usb/usb-cleanups-for-g_file_storage.patch
usb/usb-don-t-touch-sysfs-stuff-when-altsetting-is-unchanged.patch
usb/usb-make-hcds-responsible-for-managing-endpoint-queues.patch
usb/usb-get-rid-of-urb-lock.patch
usb/usb-remove-traces-of-urb-status-from-usbcore.patch
usb/usb-usbmon-doc-update-mention-new-wildcard-bus.patch
usb/usbmon-update-pipe-removal-to-suit-my-taste.patch
usb/usb-break-apart-flush_endpoint-and-disable_endpoint.patch
usb/usb-fix-location-of-statement-label-in-dummy-hcd.patch
usb/usb-flush-outstanding-urbs-when-suspending.patch
usb/usb-get-rid-of-annoying-endpoint-release-message.patch
usb/usb-move-decision-to-ignore-freeze-events.patch
usb/usb-remove-unnecessary-tests-in-isp116x-and-sl811.patch
usb/usb-add-urb-unlinked-field.patch
usb/usb-centralize-eremoteio-handling.patch
usb/usb-driver-for-ch341-usb-serial-adaptor.patch
usb/usb-minor-fixes-for-r8a66597-driver.patch
usb/usb-remove-iso-status-value-in-uhci-hcd.patch
usb/usb-fix-double-frees-in-error-code-paths-of-ipaq-driver.patch
usb/usb-fix-limited_power-setting-mistake-in-hub.c.patch
usb/usb-don-t-propagate-freeze-or-prethaw-suspends.patch
usb/usb-remove-usb_quirk_no_autosuspend.patch
usb/usb-unusual_devs-modification-for-nikon-d200.patch
usb/usb-unusual_devs-update-for-nokia-6131.patch
usb/usb-unusual_devs-entry-for-nikon-dsc-d2xs.patch

Re: [PATCH] PM: acquire device locks prior to suspending

[Andrew Morton]

On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:

> This patch (as994) reorganizes the way suspend and resume
> notifications are sent to drivers.  The major changes are that now the
> PM core acquires every device semaphore before calling the methods,
> and calls to device_add() during suspends will fail.

Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
moon symbol has started to flash but the LCD is still powered and the
cursor still blinks.  Only a poweroff restores control.

Re: [PATCH] PM: acquire device locks prior to suspending

[Alan Stern]

On Wed, 12 Dec 2007, Andrew Morton wrote:

> On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > This patch (as994) reorganizes the way suspend and resume
> > notifications are sent to drivers.  The major changes are that now the
> > PM core acquires every device semaphore before calling the methods,
> > and calls to device_add() during suspends will fail.
> 
> Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> moon symbol has started to flash but the LCD is still powered and the
> cursor still blinks.  Only a poweroff restores control.

Is there any chance of debugging this?  You have removed the patch from 
the current -mm source.  Does anybody else report a similar problem?

Proper debugging would require, to begin with, enabling 
CONFIG_PM_VERBOSE and booting with no_console_suspend.

Alan Stern

Re: [PATCH] PM: acquire device locks prior to suspending

[Rafael J. Wysocki]

On Thursday, 13 of December 2007, Andrew Morton wrote:
> On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > This patch (as994) reorganizes the way suspend and resume
> > notifications are sent to drivers.  The major changes are that now the
> > PM core acquires every device semaphore before calling the methods,
> > and calls to device_add() during suspends will fail.
> 
> Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> moon symbol has started to flash but the LCD is still powered and the
> cursor still blinks.  Only a poweroff restores control.

Most probably, one of the drivers or a CPU hotplug notifier unregisters a
device during suspend (wrong).

Please boot with no_console_suspend and check if the box survives (with this
patch applied):

# echo 8 > /proc/sys/kernel/printk
# echo processors > /sys/power/pm_test
# echo mem > /sys/power/state

If it doesn't, you can try 

# echo platform > /sys/power/pm_test
# echo mem > /sys/power/state

and

# echo devices > /sys/power/pm_test
# echo mem > /sys/power/state

Re: [PATCH] PM: acquire device locks prior to suspending

[Andrew Morton]

On Thu, 13 Dec 2007 11:02:36 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 12 Dec 2007, Andrew Morton wrote:
> 
> > On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > This patch (as994) reorganizes the way suspend and resume
> > > notifications are sent to drivers.  The major changes are that now the
> > > PM core acquires every device semaphore before calling the methods,
> > > and calls to device_add() during suspends will fail.
> > 
> > Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> > moon symbol has started to flash but the LCD is still powered and the
> > cursor still blinks.  Only a poweroff restores control.
> 
> Is there any chance of debugging this?

Spose so, although debugging 2.6.24-rc5 regressions seems a more useful way
to spend one's time.

>  You have removed the patch from 
> the current -mm source.  Does anybody else report a similar problem?
> 
> Proper debugging would require, to begin with, enabling 
> CONFIG_PM_VERBOSE and booting with no_console_suspend.

Will try to remember to take a look this evening.

Speaking of efficient use of resources: how many other people have tested
suspend-to-disk with this patch applied?  And on how many machines?

Re: [PATCH] PM: acquire device locks prior to suspending

[Rafael J. Wysocki]

On Thursday, 13 of December 2007, Rafael J. Wysocki wrote:
> On Thursday, 13 of December 2007, Andrew Morton wrote:
> > On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > This patch (as994) reorganizes the way suspend and resume
> > > notifications are sent to drivers.  The major changes are that now the
> > > PM core acquires every device semaphore before calling the methods,
> > > and calls to device_add() during suspends will fail.
> > 
> > Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> > moon symbol has started to flash but the LCD is still powered and the
> > cursor still blinks.  Only a poweroff restores control.
> 
> Most probably, one of the drivers or a CPU hotplug notifier unregisters a
> device during suspend (wrong).
> 
> Please boot with no_console_suspend and check if the box survives (with this
> patch applied):
> 
> # echo 8 > /proc/sys/kernel/printk
> # echo processors > /sys/power/pm_test
> # echo mem > /sys/power/state
> 
> If it doesn't, you can try 
> 
> # echo platform > /sys/power/pm_test
> # echo mem > /sys/power/state
> 
> and
> 
> # echo devices > /sys/power/pm_test
> # echo mem > /sys/power/state

Ah, that won't work, because git-acpi-build-fix.patch killed
/sys/power/pm_test altogether.  Sorry.

Re: [PATCH] PM: acquire device locks prior to suspending

[Andrew Morton]

On Fri, 14 Dec 2007 01:08:37 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Thursday, 13 of December 2007, Rafael J. Wysocki wrote:
> > On Thursday, 13 of December 2007, Andrew Morton wrote:
> > > On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> > > 
> > > > This patch (as994) reorganizes the way suspend and resume
> > > > notifications are sent to drivers.  The major changes are that now the
> > > > PM core acquires every device semaphore before calling the methods,
> > > > and calls to device_add() during suspends will fail.
> > > 
> > > Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> > > moon symbol has started to flash but the LCD is still powered and the
> > > cursor still blinks.  Only a poweroff restores control.
> > 
> > Most probably, one of the drivers or a CPU hotplug notifier unregisters a
> > device during suspend (wrong).
> > 
> > Please boot with no_console_suspend and check if the box survives (with this
> > patch applied):
> > 
> > # echo 8 > /proc/sys/kernel/printk
> > # echo processors > /sys/power/pm_test
> > # echo mem > /sys/power/state
> > 
> > If it doesn't, you can try 
> > 
> > # echo platform > /sys/power/pm_test
> > # echo mem > /sys/power/state
> > 
> > and
> > 
> > # echo devices > /sys/power/pm_test
> > # echo mem > /sys/power/state
> 
> Ah, that won't work, because git-acpi-build-fix.patch killed
> /sys/power/pm_test altogether.  Sorry.

I was wondering..

What's going on with git-acpi-build-fix.patch anyway?  Is there
something different we should do?

Re: [PATCH] PM: acquire device locks prior to suspending

[Rafael J. Wysocki]

On Friday, 14 of December 2007, Andrew Morton wrote:
> On Fri, 14 Dec 2007 01:08:37 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Thursday, 13 of December 2007, Rafael J. Wysocki wrote:
> > > On Thursday, 13 of December 2007, Andrew Morton wrote:
> > > > On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > 
> > > > > This patch (as994) reorganizes the way suspend and resume
> > > > > notifications are sent to drivers.  The major changes are that now the
> > > > > PM core acquires every device semaphore before calling the methods,
> > > > > and calls to device_add() during suspends will fail.
> > > > 
> > > > Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> > > > moon symbol has started to flash but the LCD is still powered and the
> > > > cursor still blinks.  Only a poweroff restores control.
> > > 
> > > Most probably, one of the drivers or a CPU hotplug notifier unregisters a
> > > device during suspend (wrong).
> > > 
> > > Please boot with no_console_suspend and check if the box survives (with this
> > > patch applied):
> > > 
> > > # echo 8 > /proc/sys/kernel/printk
> > > # echo processors > /sys/power/pm_test
> > > # echo mem > /sys/power/state
> > > 
> > > If it doesn't, you can try 
> > > 
> > > # echo platform > /sys/power/pm_test
> > > # echo mem > /sys/power/state
> > > 
> > > and
> > > 
> > > # echo devices > /sys/power/pm_test
> > > # echo mem > /sys/power/state
> > 
> > Ah, that won't work, because git-acpi-build-fix.patch killed
> > /sys/power/pm_test altogether.  Sorry.
> 
> I was wondering..
> 
> What's going on with git-acpi-build-fix.patch anyway?  Is there
> something different we should do?

Yes.  I've sent a patch to you and Len, here:
http://marc.info/?l=linux-acpi&m=119758974523027&w=2

Re: [PATCH] PM: acquire device locks prior to suspending

[Andrew Morton]

On Thu, 13 Dec 2007 17:58:37 +0100 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Thursday, 13 of December 2007, Andrew Morton wrote:
> > On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > This patch (as994) reorganizes the way suspend and resume
> > > notifications are sent to drivers.  The major changes are that now the
> > > PM core acquires every device semaphore before calling the methods,
> > > and calls to device_add() during suspends will fail.
> > 
> > Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> > moon symbol has started to flash but the LCD is still powered and the
> > cursor still blinks.  Only a poweroff restores control.
> 
> Most probably, one of the drivers or a CPU hotplug notifier unregisters a
> device during suspend (wrong).

Not much is coming out over netconsole.

Disabling netconsole doesn't work around this bug.

> Please boot with no_console_suspend and check if the box survives (with this
> patch applied):
> 
> # echo 8 > /proc/sys/kernel/printk
> # echo processors > /sys/power/pm_test
> # echo mem > /sys/power/state

http://userweb.kernel.org/~akpm/pc131696.jpg

It hangs in the same way.

> If it doesn't, you can try 
> 
> # echo platform > /sys/power/pm_test
> # echo mem > /sys/power/state

It gets here: http://userweb.kernel.org/~akpm/pc131697.jpg then locks up.

> and
> 
> # echo devices > /sys/power/pm_test
> # echo mem > /sys/power/state

It gets here: http://userweb.kernel.org/~akpm/pc131698.jpg then locks up.

I spent a bit of time ding crude debugging hacks but ran out of patience..

Re: [PATCH] PM: acquire device locks prior to suspending

[Andrew Morton]

On Thu, 13 Dec 2007 17:58:37 +0100 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Thursday, 13 of December 2007, Andrew Morton wrote:
> > On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > This patch (as994) reorganizes the way suspend and resume
> > > notifications are sent to drivers.  The major changes are that now the
> > > PM core acquires every device semaphore before calling the methods,
> > > and calls to device_add() during suspends will fail.
> > 
> > Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> > moon symbol has started to flash but the LCD is still powered and the
> > cursor still blinks.  Only a poweroff restores control.
> 
> Most probably, one of the drivers or a CPU hotplug notifier unregisters a
> device during suspend (wrong).
> 
> Please boot with no_console_suspend and check if the box survives (with this
> patch applied):
> 
> # echo 8 > /proc/sys/kernel/printk
> # echo processors > /sys/power/pm_test
> # echo mem > /sys/power/state
> 
> If it doesn't, you can try 
> 
> # echo platform > /sys/power/pm_test
> # echo mem > /sys/power/state
> 
> and
> 
> # echo devices > /sys/power/pm_test
> # echo mem > /sys/power/state

hm, that was all fairly helpful.  <looks at the document> <erk, long>

This:

--- a/drivers/base/power/main.c~a
+++ a/drivers/base/power/main.c
@@ -58,13 +58,36 @@ static DECLARE_RWSEM(pm_sleep_rwsem);
 
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
+static void __do_down(struct semaphore *s, const char *file, int line)
+{
+	printk("%s:%d\n", file, line);
+	dump_stack();
+	down(s);
+}
+#define do_down(s) __do_down(s, __FILE__, __LINE__)
+
+static void __do_mutex_lock(struct mutex *m, const char *file, int line)
+{
+	printk("%s:%d\n", file, line);
+	dump_stack();
+	mutex_lock(m);
+}
+#define do_mutex_lock(m) __do_mutex_lock(m, __FILE__, __LINE__)
+
+static void __do_down_write(struct rw_semaphore *s, const char *file, int line)
+{
+	printk("%s:%d\n", file, line);
+	dump_stack();
+	down_write(s);
+}
+#define do_down_write(s) __do_down_write(s, __FILE__, __LINE__)
 
 void device_pm_add(struct device *dev)
 {
 	pr_debug("PM: Adding info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
-	mutex_lock(&dpm_list_mtx);
+	do_mutex_lock(&dpm_list_mtx);
 	list_add_tail(&dev->power.entry, &dpm_active);
 	mutex_unlock(&dpm_list_mtx);
 }
@@ -76,8 +99,8 @@ void device_pm_remove(struct device *dev
 		 kobject_name(&dev->kobj));
 
 	/* Don't remove a device while the PM core has it locked for suspend */
-	down(&dev->sem);
-	mutex_lock(&dpm_list_mtx);
+	do_down(&dev->sem);
+	do_mutex_lock(&dpm_list_mtx);
 	dpm_sysfs_remove(dev);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
@@ -229,7 +252,7 @@ static void dpm_resume(void)
  */
 static void unlock_all_devices(void)
 {
-	mutex_lock(&dpm_list_mtx);
+	do_mutex_lock(&dpm_list_mtx);
  	while (!list_empty(&dpm_locked)) {
  		struct list_head *entry = dpm_locked.prev;
  		struct device *dev = to_device(entry);
@@ -412,7 +435,7 @@ static int dpm_suspend(pm_message_t stat
  */
 static void lock_all_devices(void)
 {
-	mutex_lock(&dpm_list_mtx);
+	do_mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_active)) {
 		struct list_head *entry = dpm_active.next;
 		struct device *dev = to_device(entry);
@@ -422,8 +445,8 @@ static void lock_all_devices(void)
 		 */
 		get_device(dev);
 		mutex_unlock(&dpm_list_mtx);
-		down(&dev->sem);
-		mutex_lock(&dpm_list_mtx);
+		do_down(&dev->sem);
+		do_mutex_lock(&dpm_list_mtx);
 
 		if (list_empty(entry))
 			up(&dev->sem);		/* Device was removed */
@@ -445,7 +468,7 @@ int device_suspend(pm_message_t state)
 	int error;
 
 	might_sleep();
-	down_write(&pm_sleep_rwsem);
+	do_down_write(&pm_sleep_rwsem);
 	lock_all_devices();
 	error = dpm_suspend(state);
 	if (error) {
_

gives me this:

http://userweb.kernel.org/~akpm/pc131699.jpg

which identifies your culprit: msr.

I'd suggest that the above debug patch be turned into some
boot-option-enabled thing and that it be rolled out with this locking
change for a while at least.

Re: [PATCH] PM: acquire device locks prior to suspending

[Alan Stern]

On Thu, 13 Dec 2007, Andrew Morton wrote:

> Speaking of efficient use of resources: how many other people have tested
> suspend-to-disk with this patch applied?  And on how many machines?

I have no idea -- they don't notify me when they do their testing.  :-)

Anybody who has been testing Greg's development tree for the last month
or two certainly has had that patch installed.  Yours is the second 
complaint I have heard; the first was the CPU hardware monitor driver 
(which Rafael fixed by making it not unregister anything during 
hibernation).

Alan Stern

Re: [PATCH] PM: acquire device locks prior to suspending

[Alan Stern]

On Thu, 13 Dec 2007, Andrew Morton wrote:

> hm, that was all fairly helpful.  <looks at the document> <erk, long>
> 
> This:

> gives me this:
> 
> http://userweb.kernel.org/~akpm/pc131699.jpg
> 
> which identifies your culprit: msr.
> 
> I'd suggest that the above debug patch be turned into some
> boot-option-enabled thing and that it be rolled out with this locking
> change for a while at least.

Rafael, that looks like exactly the same sort of problem as we saw 
before.  Global searching for CPU_DEAD_FROZEN shows several other 
places that perhaps also could be simplified; the actions they take may 
or may not be needed for hibernation.

The following three seem definitely dangerous:

	arch/x86/kernel/cpu/mcheck/mce_64.c
	arch/x86/kernel/cpuid.c
	arch/x86/kernel/msr.c

And of course the third is the one that Andrew spotted.  Would you like 
to address them?  I can package up Andrew's debugging changes into a 
real patch.

Alan Stern

Re: [PATCH] PM: acquire device locks prior to suspending

[Rafael J. Wysocki]

On Friday, 14 of December 2007, Alan Stern wrote:
> On Thu, 13 Dec 2007, Andrew Morton wrote:
> 
> > hm, that was all fairly helpful.  <looks at the document> <erk, long>
> > 
> > This:
> 
> > gives me this:
> > 
> > http://userweb.kernel.org/~akpm/pc131699.jpg
> > 
> > which identifies your culprit: msr.
> > 
> > I'd suggest that the above debug patch be turned into some
> > boot-option-enabled thing and that it be rolled out with this locking
> > change for a while at least.
> 
> Rafael, that looks like exactly the same sort of problem as we saw 
> before.  Global searching for CPU_DEAD_FROZEN shows several other 
> places that perhaps also could be simplified; the actions they take may 
> or may not be needed for hibernation.
> 
> The following three seem definitely dangerous:
> 
> 	arch/x86/kernel/cpu/mcheck/mce_64.c
> 	arch/x86/kernel/cpuid.c
> 	arch/x86/kernel/msr.c
> 
> And of course the third is the one that Andrew spotted.  Would you like 
> to address them?

Yes, I think I can take care of them, but not earlier than on Sunday.

> I can package up Andrew's debugging changes into a real patch.

Yes, that certainly is worth doing. :-)

Thanks,
Rafael

Re: [PATCH] PM: acquire device locks prior to suspending

[Alan Stern]

On Thu, 13 Dec 2007 Andrew Morton wrote:

> On Thu, 13 Dec 2007 17:58:37 +0100 "Rafael J. Wysocki" <rjw at sisk.pl <https://lists.linux-foundation.org/mailman/listinfo/linux-pm>> wrote:
> 
> > On Thursday, 13 of December 2007, Andrew Morton wrote:
> > > On Fri, 21 Sep 2007 15:37:40 -0400 (EDT) Alan Stern <stern at rowland.harvard.edu <https://lists.linux-foundation.org/mailman/listinfo/linux-pm>> wrote:
> > > 
> > > > This patch (as994) reorganizes the way suspend and resume
> > > > notifications are sent to drivers.  The major changes are that now the
> > > > PM core acquires every device semaphore before calling the methods,
> > > > and calls to device_add() during suspends will fail.
> > > 
> > > Causes my t61p to deadlock during suspend-to-RAM.  Really late - the little
> > > moon symbol has started to flash but the LCD is still powered and the
> > > cursor still blinks.  Only a poweroff restores control.
> > 
> > Most probably, one of the drivers or a CPU hotplug notifier unregisters a
> > device during suspend (wrong).
> > 
> > Please boot with no_console_suspend and check if the box survives (with this
> > patch applied):
> > 
> > # echo 8 > /proc/sys/kernel/printk
> > # echo processors > /sys/power/pm_test
> > # echo mem > /sys/power/state
> > 
> > If it doesn't, you can try 
> > 
> > # echo platform > /sys/power/pm_test
> > # echo mem > /sys/power/state
> > 
> > and
> > 
> > # echo devices > /sys/power/pm_test
> > # echo mem > /sys/power/state
> 
> hm, that was all fairly helpful.  <looks at the document> <erk, long>
> 
> This:
...
> gives me this:
> 
> http://userweb.kernel.org/~akpm/pc131699.jpg
> 
> which identifies your culprit: msr.
> 
> I'd suggest that the above debug patch be turned into some
> boot-option-enabled thing and that it be rolled out with this locking
> change for a while at least.

Going through the debugging changes you made led to some
process-of-elimination reasoning:

	There's no need to trace the locking of dpm_list_mutex,
	since the things done while that mutex are held are quite
	safe and should never deadlock (other than trying to
	acquire other locks!).

	There's no need to trace the locking of pm_sleep_rwsem.
	It gets locked for writing just once, at the very start
	of the device-suspend sequence.  If that failed, it would
	be easy to diagnose.  The only other place it is locked
	uses down_read_trylock, which can't block.

	This leaves only dev->sem, which is acquired in
	lock_all_devices and device_pm_remove.  But the call in
	device_pm_remove isn't a good place to monitor.  It worked
	in this case, but if the device has a driver then the deadlock
	will occur earlier, in device_release_driver.

	On further thought, the call in lock_all_devices should be
	safe enough.  It doesn't do anything until all the locks
	have been acquired, and so far it hasn't caused any problems.

Put together, this suggests a better strategy.  Instead of monitoring
the various locking calls, instead we should warn about dangerous
actions on the part of drivers.  In both cases where problems
occurred, it was because a driver tried to unregister a device during
suspend.  It's easy to detect such actions and print a big fat
warning.  We might as well warn about attempts to register too.

Andrew, can you confirm that the patch below, without any of your
changes, would indeed have caught the problem with the msr driver?
For proper testing you have to:

	boot with "no_console_suspend";

	"echo 8 >/proc/sys/kernel/printk" before suspending.
	Perhaps also "echo processors >/sys/power/pm_test".

If it does, I'll submit this properly with a changelog and
Signed-Off-By line.  It's a lot simpler than your checks, doesn't need
a command-line parameter, and can safely be used all the time.

Alan Stern


Index: usb-2.6/drivers/base/core.c
===================================================================
--- usb-2.6.orig/drivers/base/core.c
+++ usb-2.6/drivers/base/core.c
@@ -789,8 +789,11 @@ int device_add(struct device *dev)
 	int error;
 
 	error = pm_sleep_lock();
-	if (error)
+	if (error) {
+		dev_warn(dev, "Illegal %s during suspend\n", __FUNCTION__);
+		dump_stack();
 		return error;
+	}
 
 	dev = get_device(dev);
 	if (!dev || !strlen(dev->bus_id)) {
@@ -953,6 +956,13 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (pm_sleep_lock()) {
+		dev_warn(dev, "Illegal %s during suspend\n", __FUNCTION__);
+		dump_stack();
+	} else {
+		pm_sleep_unlock();
+	}
+
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt))