PM: Do not destroy/create devices while suspended (rev. 2)

PATCH In review Rafael J. Wysocki 10 participants 70 messages Jan 1, 2008 – Jan 12, 2008

Subsystems (7)

x86 ACPI Driver core Suspend to RAM +3 more subsystems

lore

Share & tools

lore

Rafael J. Wysocki

author

2008-01-01 23:40 UTC #0 lore raw

cover PM: Do not destroy/create devices while suspended (rev. 2)

Hi,

Some device drivers register CPU hotplug notifiers and use them to destroy device objects when removing the corresponding CPUs and to create these objects when adding the CPUs back.

Unfortunately, this is not the right thing to do during suspend/hibernation, since in that cases the CPU hotplug notifiers are called after suspending devices and before resuming them, so the operations in question are carried out on the objects representing suspended devices which shouldn't be unregistered behing the PM core's back. Although right now it usually doesn't lead to any practical complications, it will predictably deadlock if gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.

The solution is to prevent drivers from removing/adding devices from within CPU hotplug notifiers during suspend/hibernation using the FROZEN bit in the notifier's action argument. However, this has to be done with care, since the devices objects related to the nonboot CPUs that failed to go online during resume should not be present in the system. For this reason, it seems reasonable to introduce a mechanism allowing drivers to ask the PM core to remove device objects corresponding to suspended devices on their behalf.

The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.

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

Rafael J. Wysocki

author

2008-01-01 23:34 UTC #1 lore raw

PATCH 1/4 PM: Introduce destroy_suspended_device()

From: Rafael J. Wysocki <[email protected]>

It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.

Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c        |   44 +++++++++++++++++++++++++++++++++++++++-----
 drivers/base/power/main.c  |   36 +++++++++++++++++++++++++++---------
 drivers/base/power/power.h |    1 +
 include/linux/device.h     |    8 ++++++++
 4 files changed, 75 insertions(+), 14 deletions(-)

drivers/base/core.c +39 -5

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -1156,14 +1156,11 @@ error:
 EXPORT_SYMBOL_GPL(device_create);
 
 /**
- * device_destroy - removes a device that was created with device_create()
+ * find_device - finds a device that was created with device_create()
  * @class: pointer to the struct class that this device was registered with
  * @devt: the dev_t of the device that was previously registered
- *
- * This call unregisters and cleans up a device that was created with a
- * call to device_create().
  */
-void device_destroy(struct class *class, dev_t devt)
+static struct device *find_device(struct class *class, dev_t devt)
 {
 	struct device *dev = NULL;
 	struct device *dev_tmp;

@@ -1176,12 +1173,49 @@ void device_destroy(struct class *class,
 		}
 	}
 	up(&class->sem);
+	return dev;
+}
 
+/**
+ * device_destroy - removes a device that was created with device_create()
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call unregisters and cleans up a device that was created with a
+ * call to device_create().
+ */
+void device_destroy(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
 	if (dev)
 		device_unregister(dev);
 }
 EXPORT_SYMBOL_GPL(device_destroy);
 
+#ifdef CONFIG_PM_SLEEP
+/**
+ * destroy_suspended_device - schedules the removal of a suspended device
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call notifies the PM core of the necessity to unregister a suspended
+ * device created with a call to device_create() (devices cannot be
+ * unregistered directly while suspended, since the PM core controls them in
+ * that cases).
+ */
+void destroy_suspended_device(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
+	if (dev)
+		device_pm_schedule_removal(dev);
+}
+EXPORT_SYMBOL_GPL(destroy_suspended_device);
+#endif /* CONFIG_PM_SLEEP */
+
 /**
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed

include/linux/device.h +8 -0

Index: linux-2.6/include/linux/device.h
===================================================================
@@ -521,6 +521,14 @@ extern struct device *device_create(stru
 				    dev_t devt, const char *fmt, ...)
 				    __attribute__((format(printf,4,5)));
 extern void device_destroy(struct class *cls, dev_t devt);
+#ifdef CONFIG_PM_SLEEP
+extern void destroy_suspended_device(struct class *cls, dev_t devt);
+#else /* !CONFIG_PM_SLEEP */
+static inline void destroy_suspended_device(struct class *cls, dev_t devt)
+{
+	device_destroy(cls, devt);
+}
+#endif /* !CONFIG_PM_SLEEP */
 
 /*
  * Platform "fixup" functions - allow the platform to have their say

drivers/base/power/power.h +1 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -20,6 +20,7 @@ static inline struct device *to_device(s
 
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern void device_pm_schedule_removal(struct device *);
 
 #else /* CONFIG_PM_SLEEP */

drivers/base/power/main.c +27 -9

Index: linux-2.6/drivers/base/power/main.c
===================================================================
@@ -31,6 +31,7 @@
 LIST_HEAD(dpm_active);
 static LIST_HEAD(dpm_off);
 static LIST_HEAD(dpm_off_irq);
+static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_mtx);
 static DEFINE_MUTEX(dpm_list_mtx);

@@ -59,6 +60,17 @@ void device_pm_remove(struct device *dev
 	mutex_unlock(&dpm_list_mtx);
 }
 
+void device_pm_schedule_removal(struct device *dev)
+{
+	pr_debug("PM: Removing info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	mutex_lock(&dpm_list_mtx);
+	dpm_sysfs_remove(dev);
+	list_move_tail(&dev->power.entry, &dpm_destroy);
+	mutex_unlock(&dpm_list_mtx);
+}
+
 
 /*------------------------- Resume routines -------------------------*/

@@ -113,11 +125,14 @@ static int resume_device_early(struct de
 	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
+/**
+ *	dpm_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.  Unregister devices scheduled for
+ *	removal.
  */
+
 static void dpm_resume(void)
 {
 	mutex_lock(&dpm_list_mtx);

@@ -134,14 +149,17 @@ static void dpm_resume(void)
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
-}
+	/* Unregister devices scheduled for removal */
+	while (!list_empty(&dpm_destroy)) {
+		struct list_head *entry = dpm_destroy.next;
+		struct device *dev = to_device(entry);
 
+		device_unregister(dev);
+	}
+}
 
 /**
- *	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.
+ *	device_resume - Invoke dpm_resume() under dpm_mtx.
  */
 
 void device_resume(void)

Rafael J. Wysocki

author

2008-01-01 23:40 UTC #2 lore raw

PATCH 1/4 PM: Introduce destroy_suspended_device()

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c        |   44 +++++++++++++++++++++++++++++++++++++++-----
 drivers/base/power/main.c  |   36 +++++++++++++++++++++++++++---------
 drivers/base/power/power.h |    1 +
 include/linux/device.h     |    8 ++++++++
 4 files changed, 75 insertions(+), 14 deletions(-)

drivers/base/core.c +39 -5

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -1156,14 +1156,11 @@ error:
 EXPORT_SYMBOL_GPL(device_create);
 
 /**
- * device_destroy - removes a device that was created with device_create()
+ * find_device - finds a device that was created with device_create()
  * @class: pointer to the struct class that this device was registered with
  * @devt: the dev_t of the device that was previously registered
- *
- * This call unregisters and cleans up a device that was created with a
- * call to device_create().
  */
-void device_destroy(struct class *class, dev_t devt)
+static struct device *find_device(struct class *class, dev_t devt)
 {
 	struct device *dev = NULL;
 	struct device *dev_tmp;

@@ -1176,12 +1173,49 @@ void device_destroy(struct class *class,
 		}
 	}
 	up(&class->sem);
+	return dev;
+}
 
+/**
+ * device_destroy - removes a device that was created with device_create()
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call unregisters and cleans up a device that was created with a
+ * call to device_create().
+ */
+void device_destroy(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
 	if (dev)
 		device_unregister(dev);
 }
 EXPORT_SYMBOL_GPL(device_destroy);
 
+#ifdef CONFIG_PM_SLEEP
+/**
+ * destroy_suspended_device - schedules the removal of a suspended device
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call notifies the PM core of the necessity to unregister a suspended
+ * device created with a call to device_create() (devices cannot be
+ * unregistered directly while suspended, since the PM core controls them in
+ * that cases).
+ */
+void destroy_suspended_device(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
+	if (dev)
+		device_pm_schedule_removal(dev);
+}
+EXPORT_SYMBOL_GPL(destroy_suspended_device);
+#endif /* CONFIG_PM_SLEEP */
+
 /**
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed

include/linux/device.h +8 -0

Index: linux-2.6/include/linux/device.h
===================================================================
@@ -521,6 +521,14 @@ extern struct device *device_create(stru
 				    dev_t devt, const char *fmt, ...)
 				    __attribute__((format(printf,4,5)));
 extern void device_destroy(struct class *cls, dev_t devt);
+#ifdef CONFIG_PM_SLEEP
+extern void destroy_suspended_device(struct class *cls, dev_t devt);
+#else /* !CONFIG_PM_SLEEP */
+static inline void destroy_suspended_device(struct class *cls, dev_t devt)
+{
+	device_destroy(cls, devt);
+}
+#endif /* !CONFIG_PM_SLEEP */
 
 /*
  * Platform "fixup" functions - allow the platform to have their say

drivers/base/power/power.h +1 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -20,6 +20,7 @@ static inline struct device *to_device(s
 
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern void device_pm_schedule_removal(struct device *);
 
 #else /* CONFIG_PM_SLEEP */

drivers/base/power/main.c +27 -9

Index: linux-2.6/drivers/base/power/main.c
===================================================================
@@ -31,6 +31,7 @@
 LIST_HEAD(dpm_active);
 static LIST_HEAD(dpm_off);
 static LIST_HEAD(dpm_off_irq);
+static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_mtx);
 static DEFINE_MUTEX(dpm_list_mtx);

@@ -59,6 +60,17 @@ void device_pm_remove(struct device *dev
 	mutex_unlock(&dpm_list_mtx);
 }
 
+void device_pm_schedule_removal(struct device *dev)
+{
+	pr_debug("PM: Removing info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	mutex_lock(&dpm_list_mtx);
+	dpm_sysfs_remove(dev);
+	list_move_tail(&dev->power.entry, &dpm_destroy);
+	mutex_unlock(&dpm_list_mtx);
+}
+
 
 /*------------------------- Resume routines -------------------------*/

@@ -113,11 +125,14 @@ static int resume_device_early(struct de
 	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
+/**
+ *	dpm_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.  Unregister devices scheduled for
+ *	removal.
  */
+
 static void dpm_resume(void)
 {
 	mutex_lock(&dpm_list_mtx);

@@ -134,14 +149,17 @@ static void dpm_resume(void)
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
-}
+	/* Unregister devices scheduled for removal */
+	while (!list_empty(&dpm_destroy)) {
+		struct list_head *entry = dpm_destroy.next;
+		struct device *dev = to_device(entry);
 
+		device_unregister(dev);
+	}
+}
 
 /**
- *	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.
+ *	device_resume - Invoke dpm_resume() under dpm_mtx.
  */
 
 void device_resume(void)

Rafael J. Wysocki

author

2008-01-02 13:31 UTC #3 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

Quoted 8 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.

Quoted 5 lines

Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Signed-off-by Rafael J. Wysocki <[email protected]>

Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).

Corrected patch follows.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[email protected]>

Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c        |   44 +++++++++++++++++++++++++++++++++++++++-----
 drivers/base/power/main.c  |   35 ++++++++++++++++++++++++++---------
 drivers/base/power/power.h |    1 +
 include/linux/device.h     |    8 ++++++++
 4 files changed, 74 insertions(+), 14 deletions(-)

drivers/base/core.c +39 -5

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -1156,14 +1156,11 @@ error:
 EXPORT_SYMBOL_GPL(device_create);
 
 /**
- * device_destroy - removes a device that was created with device_create()
+ * find_device - finds a device that was created with device_create()
  * @class: pointer to the struct class that this device was registered with
  * @devt: the dev_t of the device that was previously registered
- *
- * This call unregisters and cleans up a device that was created with a
- * call to device_create().
  */
-void device_destroy(struct class *class, dev_t devt)
+static struct device *find_device(struct class *class, dev_t devt)
 {
 	struct device *dev = NULL;
 	struct device *dev_tmp;

@@ -1176,12 +1173,49 @@ void device_destroy(struct class *class,
 		}
 	}
 	up(&class->sem);
+	return dev;
+}
 
+/**
+ * device_destroy - removes a device that was created with device_create()
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call unregisters and cleans up a device that was created with a
+ * call to device_create().
+ */
+void device_destroy(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
 	if (dev)
 		device_unregister(dev);
 }
 EXPORT_SYMBOL_GPL(device_destroy);
 
+#ifdef CONFIG_PM_SLEEP
+/**
+ * destroy_suspended_device - schedules the removal of a suspended device
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call notifies the PM core of the necessity to unregister a suspended
+ * device created with a call to device_create() (devices cannot be
+ * unregistered directly while suspended, since the PM core controls them in
+ * that cases).
+ */
+void destroy_suspended_device(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
+	if (dev)
+		device_pm_schedule_removal(dev);
+}
+EXPORT_SYMBOL_GPL(destroy_suspended_device);
+#endif /* CONFIG_PM_SLEEP */
+
 /**
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed

include/linux/device.h +8 -0

Index: linux-2.6/include/linux/device.h
===================================================================
@@ -521,6 +521,14 @@ extern struct device *device_create(stru
 				    dev_t devt, const char *fmt, ...)
 				    __attribute__((format(printf,4,5)));
 extern void device_destroy(struct class *cls, dev_t devt);
+#ifdef CONFIG_PM_SLEEP
+extern void destroy_suspended_device(struct class *cls, dev_t devt);
+#else /* !CONFIG_PM_SLEEP */
+static inline void destroy_suspended_device(struct class *cls, dev_t devt)
+{
+	device_destroy(cls, devt);
+}
+#endif /* !CONFIG_PM_SLEEP */
 
 /*
  * Platform "fixup" functions - allow the platform to have their say

drivers/base/power/power.h +1 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -20,6 +20,7 @@ static inline struct device *to_device(s
 
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern void device_pm_schedule_removal(struct device *);
 
 #else /* CONFIG_PM_SLEEP */

drivers/base/power/main.c +26 -9

Index: linux-2.6/drivers/base/power/main.c
===================================================================
@@ -31,6 +31,7 @@
 LIST_HEAD(dpm_active);
 static LIST_HEAD(dpm_off);
 static LIST_HEAD(dpm_off_irq);
+static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_mtx);
 static DEFINE_MUTEX(dpm_list_mtx);

@@ -59,6 +60,16 @@ void device_pm_remove(struct device *dev
 	mutex_unlock(&dpm_list_mtx);
 }
 
+void device_pm_schedule_removal(struct device *dev)
+{
+	pr_debug("PM: Removing info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	mutex_lock(&dpm_list_mtx);
+	list_move_tail(&dev->power.entry, &dpm_destroy);
+	mutex_unlock(&dpm_list_mtx);
+}
+
 
 /*------------------------- Resume routines -------------------------*/

@@ -113,11 +124,14 @@ static int resume_device_early(struct de
 	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
+/**
+ *	dpm_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.  Unregister devices scheduled for
+ *	removal.
  */
+
 static void dpm_resume(void)
 {
 	mutex_lock(&dpm_list_mtx);

@@ -134,14 +148,17 @@ static void dpm_resume(void)
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
-}
+	/* Unregister devices scheduled for removal */
+	while (!list_empty(&dpm_destroy)) {
+		struct list_head *entry = dpm_destroy.next;
+		struct device *dev = to_device(entry);
 
+		device_unregister(dev);
+	}
+}
 
 /**
- *	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.
+ *	device_resume - Invoke dpm_resume() under dpm_mtx.
  */
 
 void device_resume(void)

Alan Stern

2008-01-02 16:41 UTC #4 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

Quoted 13 lines · Rafael J. Wysocki

Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.

Quoted 6 lines

Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).

The situation is a little confusing, because the source files under drivers/base/power are maintained in Greg's tree and he already has gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch installed. That patch conflicts with this one.

One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?

Alan Stern

2008-01-02 16:41 UTC #5 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

In reply to Rafael J. Wysocki

Quoted 13 lines · Rafael J. Wysocki

Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.

Quoted 6 lines

Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).

One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?

Alan Stern

Rafael J. Wysocki

author

2008-01-02 16:48 UTC #6 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

Quoted 22 lines · Alan Stern

Quoted 13 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Quoted 6 lines
Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).

Quoted 7 lines

The situation is a little confusing, because the source files under drivers/base/power are maintained in Greg's tree and he already has gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch installed. That patch conflicts with this one.
One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?

Well, from the bisectability point of view, it would be better to adjust gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and let the $subject patch series go first, if you don't mind.

Rafael

Rafael J. Wysocki

author

2008-01-04 22:03 UTC #7 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

Quoted 28 lines · Rafael J. Wysocki

Quoted 27 lines · Alan Stern
Quoted 13 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Quoted 6 lines
Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).
The situation is a little confusing, because the source files under drivers/base/power are maintained in Greg's tree and he already has gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch installed. That patch conflicts with this one.

Quoted 6 lines

Quoted 2 lines
One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?
Well, from the bisectability point of view, it would be better to adjust gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and let the $subject patch series go first, if you don't mind.

I have rebased gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch on top of the $subject series, the result is appended. It has only been compilation tested for now, but I'll be testing it for the next couple of days.

Please review.

Thanks,
Rafael

---
From: Alan Stern <[email protected]>, Rafael J. Wysocki <[email protected]>

This patch 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.

---
 drivers/base/core.c        |   13 -
 drivers/base/power/main.c  |  452 ++++++++++++++++++++++++++-------------------
 drivers/base/power/power.h |   11 +
 3 files changed, 290 insertions(+), 186 deletions(-)

drivers/base/core.c +10 -3

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -726,11 +726,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);

@@ -795,6 +801,7 @@ int device_add(struct device *dev)
 	}
  Done:
 	put_device(dev);
+	pm_sleep_unlock();
 	return error;
  BusError:
 	device_pm_remove(dev);

drivers/base/power/main.c +269 ~~-183~~

600 lines not on this page · raw message

drivers/base/power/power.h +11 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -21,6 +21,8 @@ static inline struct device *to_device(s
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
 extern void device_pm_schedule_removal(struct device *);
+extern int pm_sleep_lock(void);
+extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */

@@ -33,6 +35,15 @@ static inline void device_pm_remove(stru
 {
 }
 
+static inline int pm_sleep_lock(void)
+{
+	return 0;
+}
+
+static inline void pm_sleep_unlock(void)
+{
+}
+
 #endif
 
 #ifdef CONFIG_PM

Rafael J. Wysocki

author

2008-01-04 23:28 UTC #8 lore raw

RFC PATCH 1/1 PM: Acquire device locks on suspend (was: Re: [PATCH 1/4] PM: Introduce destroy_suspended_device())

Quoted 36 lines · Rafael J. Wysocki

Quoted 28 lines · Rafael J. Wysocki
Quoted 27 lines · Alan Stern
Quoted 13 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Quoted 6 lines
Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).
The situation is a little confusing, because the source files under drivers/base/power are maintained in Greg's tree and he already has gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch installed. That patch conflicts with this one.
Quoted 6 lines
Quoted 2 lines
One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?
Well, from the bisectability point of view, it would be better to adjust gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and let the $subject patch series go first, if you don't mind.

Quoted 5 lines

I have rebased gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch on top of the $subject series, the result is appended. It has only been compilation tested for now, but I'll be testing it for the next couple of days.
Please review.

Actually, please scratch that. We can do better.

The appended patch (compilation tested) combines patch [1/4] with the gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch in such a way that it is now possible to remove a suspended device at once via the PM core using the destroy_suspended_device() callback.

Thus, the drivers that need to remove device objects while suspended (eg. from the CPU hotplug notifiers) can use destroy_suspended_device() for this purpose without deadlocking, but using device_destroy() to remove suspended devices is prohibited.

Please review.

Thanks,
Rafael

---
From: Alan Stern <[email protected]>, Rafael J. Wysocki <[email protected]>

It also provides a way to safely remove a suspended device with the help of the PM core, by using the destroy_suspended_device() callback introduced specifically for this purpose.

Not-yet-signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c        |   57 ++++-
 drivers/base/power/main.c  |  452 ++++++++++++++++++++++++++-------------------
 drivers/base/power/power.h |   12 +
 include/linux/device.h     |    8 
 4 files changed, 336 insertions(+), 193 deletions(-)

drivers/base/core.c +49 -8

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -726,11 +726,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);

@@ -795,6 +801,7 @@ int device_add(struct device *dev)
 	}
  Done:
 	put_device(dev);
+	pm_sleep_unlock();
 	return error;
  BusError:
 	device_pm_remove(dev);

@@ -1156,14 +1163,11 @@ error:
 EXPORT_SYMBOL_GPL(device_create);
 
 /**
- * device_destroy - removes a device that was created with device_create()
+ * find_device - finds a device that was created with device_create()
  * @class: pointer to the struct class that this device was registered with
  * @devt: the dev_t of the device that was previously registered
- *
- * This call unregisters and cleans up a device that was created with a
- * call to device_create().
  */
-void device_destroy(struct class *class, dev_t devt)
+static struct device *find_device(struct class *class, dev_t devt)
 {
 	struct device *dev = NULL;
 	struct device *dev_tmp;

@@ -1176,12 +1180,49 @@ void device_destroy(struct class *class,
 		}
 	}
 	up(&class->sem);
+	return dev;
+}
 
+/**
+ * device_destroy - removes a device that was created with device_create()
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call unregisters and cleans up a device that was created with a
+ * call to device_create().
+ */
+void device_destroy(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
 	if (dev)
 		device_unregister(dev);
 }
 EXPORT_SYMBOL_GPL(device_destroy);
 
+#ifdef CONFIG_PM_SLEEP
+/**
+ * destroy_suspended_device - asks the PM core to remove a suspended device
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call causes the PM core to release and unregister a suspended device
+ * created with a call to device_create() (devices cannot be unregistered
+ * directly while suspended, since the PM core holds their semaphores at that
+ * time).
+ */
+void destroy_suspended_device(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
+	if (dev)
+		device_pm_destroy_suspended(dev);
+}
+EXPORT_SYMBOL_GPL(destroy_suspended_device);
+#endif /* CONFIG_PM_SLEEP */
+
 /**
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed

include/linux/device.h +8 -0

Index: linux-2.6/include/linux/device.h
===================================================================
@@ -521,6 +521,14 @@ extern struct device *device_create(stru
 				    dev_t devt, const char *fmt, ...)
 				    __attribute__((format(printf,4,5)));
 extern void device_destroy(struct class *cls, dev_t devt);
+#ifdef CONFIG_PM_SLEEP
+extern void destroy_suspended_device(struct class *cls, dev_t devt);
+#else /* !CONFIG_PM_SLEEP */
+static inline void destroy_suspended_device(struct class *cls, dev_t devt)
+{
+	device_destroy(cls, devt);
+}
+#endif /* !CONFIG_PM_SLEEP */
 
 /*
  * Platform "fixup" functions - allow the platform to have their say

drivers/base/power/power.h +12 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -20,6 +20,9 @@ static inline struct device *to_device(s
 
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern void device_pm_destroy_suspended(struct device *);
+extern int pm_sleep_lock(void);
+extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */

@@ -32,6 +35,15 @@ static inline void device_pm_remove(stru
 {
 }
 
+static inline int pm_sleep_lock(void)
+{
+	return 0;
+}
+
+static inline void pm_sleep_unlock(void)
+{
+}
+
 #endif
 
 #ifdef CONFIG_PM

drivers/base/power/main.c +267 ~~-185~~

585 lines not on this page · raw message

Rafael J. Wysocki

author

2008-01-04 23:29 UTC #9 lore raw

RFC PATCH 1/1 PM: Acquire device locks on suspend (was: Re: [PATCH 1/4] PM: Introduce destroy_suspended_device())

In reply to Rafael J. Wysocki

Quoted 36 lines · Rafael J. Wysocki

Quoted 28 lines · Rafael J. Wysocki
Quoted 27 lines · Alan Stern
Quoted 13 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Quoted 6 lines
Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).
The situation is a little confusing, because the source files under drivers/base/power are maintained in Greg's tree and he already has gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch installed. That patch conflicts with this one.
Quoted 6 lines
Quoted 2 lines
One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?
Well, from the bisectability point of view, it would be better to adjust gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and let the $subject patch series go first, if you don't mind.

Quoted 5 lines

I have rebased gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch on top of the $subject series, the result is appended. It has only been compilation tested for now, but I'll be testing it for the next couple of days.
Please review.

Actually, please scratch that. We can do better.

Please review.

Thanks,
Rafael

---
From: Alan Stern <[email protected]>, Rafael J. Wysocki <[email protected]>

It also provides a way to safely remove a suspended device with the help of the PM core, by using the destroy_suspended_device() callback introduced specifically for this purpose.

Not-yet-signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c        |   57 ++++-
 drivers/base/power/main.c  |  452 ++++++++++++++++++++++++++-------------------
 drivers/base/power/power.h |   12 +
 include/linux/device.h     |    8 
 4 files changed, 336 insertions(+), 193 deletions(-)

drivers/base/core.c +49 -8

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -726,11 +726,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);

@@ -795,6 +801,7 @@ int device_add(struct device *dev)
 	}
  Done:
 	put_device(dev);
+	pm_sleep_unlock();
 	return error;
  BusError:
 	device_pm_remove(dev);

@@ -1156,14 +1163,11 @@ error:
 EXPORT_SYMBOL_GPL(device_create);
 
 /**
- * device_destroy - removes a device that was created with device_create()
+ * find_device - finds a device that was created with device_create()
  * @class: pointer to the struct class that this device was registered with
  * @devt: the dev_t of the device that was previously registered
- *
- * This call unregisters and cleans up a device that was created with a
- * call to device_create().
  */
-void device_destroy(struct class *class, dev_t devt)
+static struct device *find_device(struct class *class, dev_t devt)
 {
 	struct device *dev = NULL;
 	struct device *dev_tmp;

@@ -1176,12 +1180,49 @@ void device_destroy(struct class *class,
 		}
 	}
 	up(&class->sem);
+	return dev;
+}
 
+/**
+ * device_destroy - removes a device that was created with device_create()
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call unregisters and cleans up a device that was created with a
+ * call to device_create().
+ */
+void device_destroy(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
 	if (dev)
 		device_unregister(dev);
 }
 EXPORT_SYMBOL_GPL(device_destroy);
 
+#ifdef CONFIG_PM_SLEEP
+/**
+ * destroy_suspended_device - asks the PM core to remove a suspended device
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call causes the PM core to release and unregister a suspended device
+ * created with a call to device_create() (devices cannot be unregistered
+ * directly while suspended, since the PM core holds their semaphores at that
+ * time).
+ */
+void destroy_suspended_device(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
+	if (dev)
+		device_pm_destroy_suspended(dev);
+}
+EXPORT_SYMBOL_GPL(destroy_suspended_device);
+#endif /* CONFIG_PM_SLEEP */
+
 /**
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed

include/linux/device.h +8 -0

Index: linux-2.6/include/linux/device.h
===================================================================
@@ -521,6 +521,14 @@ extern struct device *device_create(stru
 				    dev_t devt, const char *fmt, ...)
 				    __attribute__((format(printf,4,5)));
 extern void device_destroy(struct class *cls, dev_t devt);
+#ifdef CONFIG_PM_SLEEP
+extern void destroy_suspended_device(struct class *cls, dev_t devt);
+#else /* !CONFIG_PM_SLEEP */
+static inline void destroy_suspended_device(struct class *cls, dev_t devt)
+{
+	device_destroy(cls, devt);
+}
+#endif /* !CONFIG_PM_SLEEP */
 
 /*
  * Platform "fixup" functions - allow the platform to have their say

drivers/base/power/power.h +12 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -20,6 +20,9 @@ static inline struct device *to_device(s
 
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern void device_pm_destroy_suspended(struct device *);
+extern int pm_sleep_lock(void);
+extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */

@@ -32,6 +35,15 @@ static inline void device_pm_remove(stru
 {
 }
 
+static inline int pm_sleep_lock(void)
+{
+	return 0;
+}
+
+static inline void pm_sleep_unlock(void)
+{
+}
+
 #endif
 
 #ifdef CONFIG_PM

drivers/base/power/main.c +267 ~~-185~~

585 lines not on this page · raw message

Alan Stern

2008-01-05 03:11 UTC #10 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

In reply to Rafael J. Wysocki

Quoted 5 lines · Rafael J. Wysocki

I have rebased gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch on top of the $subject series, the result is appended. It has only been compilation tested for now, but I'll be testing it for the next couple of days.
Please review.

I would prefer it if you could also merge in this patch at the same time:

https://lists.linux-foundation.org/pipermail/linux-pm/2007-December/015921.html

drivers/base/power/main.c static int resume_device(struct device * · line 288

Quoted 10 lines

drivers/base/power/main.c +6 -2

+void device_resume(void)
 {
-	sysdev_resume();
-	dpm_power_up();
+	might_sleep();
+	dpm_resume();
+	unlock_all_devices();
+	unregister_dropped_devices();
+	up_write(&pm_sleep_rwsem);
 }

With the aforementioned patch merged in, this will generate a warning for each dropped device. The call to unregister_dropped_devices() should come after the up_write().

You might also consider adding a call to unregister_dropped_devices() in the error path of device_suspend() -- in theory even an aborted suspend might cause a device to malfunction.

Otherwise this looks okay.

Alan Stern

2008-01-05 03:11 UTC #11 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

In reply to Rafael J. Wysocki

Quoted 5 lines · Rafael J. Wysocki

I have rebased gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch on top of the $subject series, the result is appended. It has only been compilation tested for now, but I'll be testing it for the next couple of days.
Please review.

I would prefer it if you could also merge in this patch at the same time:

https://lists.linux-foundation.org/pipermail/linux-pm/2007-December/015921.html

drivers/base/power/main.c static int resume_device(struct device * · line 288

Quoted 10 lines

drivers/base/power/main.c +6 -2

+void device_resume(void)
 {
-	sysdev_resume();
-	dpm_power_up();
+	might_sleep();
+	dpm_resume();
+	unlock_all_devices();
+	unregister_dropped_devices();
+	up_write(&pm_sleep_rwsem);
 }

With the aforementioned patch merged in, this will generate a warning for each dropped device. The call to unregister_dropped_devices() should come after the up_write().

You might also consider adding a call to unregister_dropped_devices() in the error path of device_suspend() -- in theory even an aborted suspend might cause a device to malfunction.

Otherwise this looks okay.

Alan Stern

Rafael J. Wysocki

author

2008-01-05 11:53 UTC #12 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

Quoted 6 lines · Alan Stern

Quoted 4 lines · Rafael J. Wysocki
I have rebased gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch on top of the $subject series, the result is appended. It has only been compilation tested for now, but I'll be testing it for the next couple of days.

Quoted 6 lines

Quoted 1 line
Please review.
I would prefer it if you could also merge in this patch at the same time:
https://lists.linux-foundation.org/pipermail/linux-pm/2007-December/015921.html

Makes sense, I will.

Quoted 15 lines

drivers/base/power/main.c static int resume_device(struct device * · line 288
Quoted 10 lines
drivers/base/power/main.c +6 -2
+void device_resume(void)
 {
-	sysdev_resume();
-	dpm_power_up();
+	might_sleep();
+	dpm_resume();
+	unlock_all_devices();
+	unregister_dropped_devices();
+	up_write(&pm_sleep_rwsem);
 }
With the aforementioned patch merged in, this will generate a warning for each dropped device. The call to unregister_dropped_devices() should come after the up_write().

Quoted 3 lines

You might also consider adding a call to unregister_dropped_devices() in the error path of device_suspend() -- in theory even an aborted suspend might cause a device to malfunction.

In fact it already works like this, since device_suspend() now calls the entire device_resume() on error.

Quoted 1 line

Otherwise this looks okay.

However, I think we don't need to wait with unregistering suspended devices until after the other ones are resumed. We only need a special function for unregistering suspended devices that will make the PM core release the device's semaphore before unregistering it.

I have already sent a replacemet for gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch that includes some code from the $subject patch and implements the above idea:

http://lkml.org/lkml/2008/1/4/278

I'm going to merge it with your patch at:
https://lists.linux-foundation.org/pipermail/linux-pm/2007-December/015921.html
and with patches [2/4] and [4/4] from the $subject series. I'll post the result for a review later today.

Thanks,
Rafael

Rafael J. Wysocki

author

2008-01-05 11:55 UTC #13 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

In reply to Alan Stern

Quoted 6 lines · Alan Stern

Quoted 4 lines · Rafael J. Wysocki
I have rebased gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch on top of the $subject series, the result is appended. It has only been compilation tested for now, but I'll be testing it for the next couple of days.

Quoted 6 lines

Quoted 1 line
Please review.
I would prefer it if you could also merge in this patch at the same time:
https://lists.linux-foundation.org/pipermail/linux-pm/2007-December/015921.html

Makes sense, I will.

Quoted 15 lines

drivers/base/power/main.c static int resume_device(struct device * · line 288
Quoted 10 lines
drivers/base/power/main.c +6 -2
+void device_resume(void)
 {
-	sysdev_resume();
-	dpm_power_up();
+	might_sleep();
+	dpm_resume();
+	unlock_all_devices();
+	unregister_dropped_devices();
+	up_write(&pm_sleep_rwsem);
 }
With the aforementioned patch merged in, this will generate a warning for each dropped device. The call to unregister_dropped_devices() should come after the up_write().

Quoted 3 lines

You might also consider adding a call to unregister_dropped_devices() in the error path of device_suspend() -- in theory even an aborted suspend might cause a device to malfunction.

In fact it already works like this, since device_suspend() now calls the entire device_resume() on error.

Quoted 1 line

Otherwise this looks okay.

I have already sent a replacemet for gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch that includes some code from the $subject patch and implements the above idea:

http://lkml.org/lkml/2008/1/4/278

Thanks,
Rafael

Rafael J. Wysocki

author

2008-01-04 22:05 UTC #14 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

In reply to Rafael J. Wysocki

Quoted 28 lines · Rafael J. Wysocki

Quoted 27 lines · Alan Stern
Quoted 13 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Quoted 6 lines
Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).
The situation is a little confusing, because the source files under drivers/base/power are maintained in Greg's tree and he already has gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch installed. That patch conflicts with this one.

Quoted 6 lines

Quoted 2 lines
One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?
Well, from the bisectability point of view, it would be better to adjust gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and let the $subject patch series go first, if you don't mind.

Please review.

Thanks,
Rafael

---
From: Alan Stern <[email protected]>, Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c        |   13 -
 drivers/base/power/main.c  |  452 ++++++++++++++++++++++++++-------------------
 drivers/base/power/power.h |   11 +
 3 files changed, 290 insertions(+), 186 deletions(-)

drivers/base/core.c +10 -3

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -726,11 +726,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);

@@ -795,6 +801,7 @@ int device_add(struct device *dev)
 	}
  Done:
 	put_device(dev);
+	pm_sleep_unlock();
 	return error;
  BusError:
 	device_pm_remove(dev);

drivers/base/power/main.c +269 ~~-183~~

600 lines not on this page · raw message

drivers/base/power/power.h +11 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -21,6 +21,8 @@ static inline struct device *to_device(s
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
 extern void device_pm_schedule_removal(struct device *);
+extern int pm_sleep_lock(void);
+extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */

@@ -33,6 +35,15 @@ static inline void device_pm_remove(stru
 {
 }
 
+static inline int pm_sleep_lock(void)
+{
+	return 0;
+}
+
+static inline void pm_sleep_unlock(void)
+{
+}
+
 #endif
 
 #ifdef CONFIG_PM

Rafael J. Wysocki

author

2008-01-02 16:50 UTC #15 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

In reply to Alan Stern

Quoted 22 lines · Alan Stern

Quoted 13 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.
Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Quoted 6 lines
Quoted 1 line
Signed-off-by Rafael J. Wysocki <[email protected]>
Sorry, a small fix is needed for this patch. Namely, dpm_sysfs_remove(dev) should not be called by device_pm_schedule_removal(), because it will be called anyway from device_pm_remove() when the device object is finally unregistered (we're talking here about unlikely error paths only, but still).

Quoted 7 lines

The situation is a little confusing, because the source files under drivers/base/power are maintained in Greg's tree and he already has gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch installed. That patch conflicts with this one.
One of the these two patches will have to be rewritten to apply on top of the other. Which do you think should be changed?

Well, from the bisectability point of view, it would be better to adjust gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and let the $subject patch series go first, if you don't mind.

Rafael

Rafael J. Wysocki

author

2008-01-02 13:33 UTC #16 lore raw

Re: [PATCH 1/4] PM: Introduce destroy_suspended_device()

In reply to Rafael J. Wysocki

Quoted 8 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
It sometimes is necessary to destroy a device object during a suspend or hibernation, but the PM core is supposed to control all device objects in that cases. For this reason, it is necessary to introduce a mechanism allowing one to ask the PM core to remove a device object corresponding to a suspended device on one's behalf.

Quoted 5 lines

Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.
Signed-off-by Rafael J. Wysocki <[email protected]>

Corrected patch follows.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[email protected]>

Define function destroy_suspended_device() that will schedule the removal of a device object corresponding to a suspended device by the PM core during the subsequent resume.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c        |   44 +++++++++++++++++++++++++++++++++++++++-----
 drivers/base/power/main.c  |   35 ++++++++++++++++++++++++++---------
 drivers/base/power/power.h |    1 +
 include/linux/device.h     |    8 ++++++++
 4 files changed, 74 insertions(+), 14 deletions(-)

drivers/base/core.c +39 -5

Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -1156,14 +1156,11 @@ error:
 EXPORT_SYMBOL_GPL(device_create);
 
 /**
- * device_destroy - removes a device that was created with device_create()
+ * find_device - finds a device that was created with device_create()
  * @class: pointer to the struct class that this device was registered with
  * @devt: the dev_t of the device that was previously registered
- *
- * This call unregisters and cleans up a device that was created with a
- * call to device_create().
  */
-void device_destroy(struct class *class, dev_t devt)
+static struct device *find_device(struct class *class, dev_t devt)
 {
 	struct device *dev = NULL;
 	struct device *dev_tmp;

@@ -1176,12 +1173,49 @@ void device_destroy(struct class *class,
 		}
 	}
 	up(&class->sem);
+	return dev;
+}
 
+/**
+ * device_destroy - removes a device that was created with device_create()
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call unregisters and cleans up a device that was created with a
+ * call to device_create().
+ */
+void device_destroy(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
 	if (dev)
 		device_unregister(dev);
 }
 EXPORT_SYMBOL_GPL(device_destroy);
 
+#ifdef CONFIG_PM_SLEEP
+/**
+ * destroy_suspended_device - schedules the removal of a suspended device
+ * @class: pointer to the struct class that this device was registered with
+ * @devt: the dev_t of the device that was previously registered
+ *
+ * This call notifies the PM core of the necessity to unregister a suspended
+ * device created with a call to device_create() (devices cannot be
+ * unregistered directly while suspended, since the PM core controls them in
+ * that cases).
+ */
+void destroy_suspended_device(struct class *class, dev_t devt)
+{
+	struct device *dev;
+
+	dev = find_device(class, devt);
+	if (dev)
+		device_pm_schedule_removal(dev);
+}
+EXPORT_SYMBOL_GPL(destroy_suspended_device);
+#endif /* CONFIG_PM_SLEEP */
+
 /**
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed

include/linux/device.h +8 -0

Index: linux-2.6/include/linux/device.h
===================================================================
@@ -521,6 +521,14 @@ extern struct device *device_create(stru
 				    dev_t devt, const char *fmt, ...)
 				    __attribute__((format(printf,4,5)));
 extern void device_destroy(struct class *cls, dev_t devt);
+#ifdef CONFIG_PM_SLEEP
+extern void destroy_suspended_device(struct class *cls, dev_t devt);
+#else /* !CONFIG_PM_SLEEP */
+static inline void destroy_suspended_device(struct class *cls, dev_t devt)
+{
+	device_destroy(cls, devt);
+}
+#endif /* !CONFIG_PM_SLEEP */
 
 /*
  * Platform "fixup" functions - allow the platform to have their say

drivers/base/power/power.h +1 -0

Index: linux-2.6/drivers/base/power/power.h
===================================================================
@@ -20,6 +20,7 @@ static inline struct device *to_device(s
 
 extern void device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
+extern void device_pm_schedule_removal(struct device *);
 
 #else /* CONFIG_PM_SLEEP */

drivers/base/power/main.c +26 -9

Index: linux-2.6/drivers/base/power/main.c
===================================================================
@@ -31,6 +31,7 @@
 LIST_HEAD(dpm_active);
 static LIST_HEAD(dpm_off);
 static LIST_HEAD(dpm_off_irq);
+static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_mtx);
 static DEFINE_MUTEX(dpm_list_mtx);

@@ -59,6 +60,16 @@ void device_pm_remove(struct device *dev
 	mutex_unlock(&dpm_list_mtx);
 }
 
+void device_pm_schedule_removal(struct device *dev)
+{
+	pr_debug("PM: Removing info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	mutex_lock(&dpm_list_mtx);
+	list_move_tail(&dev->power.entry, &dpm_destroy);
+	mutex_unlock(&dpm_list_mtx);
+}
+
 
 /*------------------------- Resume routines -------------------------*/

@@ -113,11 +124,14 @@ static int resume_device_early(struct de
 	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
+/**
+ *	dpm_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.  Unregister devices scheduled for
+ *	removal.
  */
+
 static void dpm_resume(void)
 {
 	mutex_lock(&dpm_list_mtx);

@@ -134,14 +148,17 @@ static void dpm_resume(void)
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
-}
+	/* Unregister devices scheduled for removal */
+	while (!list_empty(&dpm_destroy)) {
+		struct list_head *entry = dpm_destroy.next;
+		struct device *dev = to_device(entry);
 
+		device_unregister(dev);
+	}
+}
 
 /**
- *	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.
+ *	device_resume - Invoke dpm_resume() under dpm_mtx.
  */
 
 void device_resume(void)

Rafael J. Wysocki

author

2008-01-01 23:40 UTC #17 lore raw

PATCH 2/4 PM: Do not destroy/create devices while suspended in msr.c (rev. 2)

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The MSR driver should not attempt to destroy/create a device while suspended, unless this device corresponds to a nonboot CPU that failed to go online during a resume, in which case the PM core should be asked to remove it.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/msr.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/x86/kernel/msr.c +3 -3

Index: linux-2.6/arch/x86/kernel/msr.c
===================================================================
@@ -155,15 +155,15 @@ static int __cpuinit msr_class_cpu_callb
 
 	switch (action) {
 	case CPU_UP_PREPARE:
-	case CPU_UP_PREPARE_FROZEN:
 		err = msr_device_create(cpu);
 		break;
 	case CPU_UP_CANCELED:
-	case CPU_UP_CANCELED_FROZEN:
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
 		msr_device_destroy(cpu);
 		break;
+	case CPU_UP_CANCELED_FROZEN:
+		destroy_suspended_device(msr_class, MKDEV(MSR_MAJOR, cpu));
+		break;
 	}
 	return err ? NOTIFY_BAD : NOTIFY_OK;
 }

Rafael J. Wysocki

author

2008-01-01 23:40 UTC #18 lore raw

PATCH 2/4 PM: Do not destroy/create devices while suspended in msr.c (rev. 2)

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/msr.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/x86/kernel/msr.c +3 -3

Index: linux-2.6/arch/x86/kernel/msr.c
===================================================================
@@ -155,15 +155,15 @@ static int __cpuinit msr_class_cpu_callb
 
 	switch (action) {
 	case CPU_UP_PREPARE:
-	case CPU_UP_PREPARE_FROZEN:
 		err = msr_device_create(cpu);
 		break;
 	case CPU_UP_CANCELED:
-	case CPU_UP_CANCELED_FROZEN:
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
 		msr_device_destroy(cpu);
 		break;
+	case CPU_UP_CANCELED_FROZEN:
+		destroy_suspended_device(msr_class, MKDEV(MSR_MAJOR, cpu));
+		break;
 	}
 	return err ? NOTIFY_BAD : NOTIFY_OK;
 }

Rafael J. Wysocki

author

2008-01-01 23:40 UTC #19 lore raw

PATCH 3/4 PM: Do not destroy/create devices while suspended in mce_64.c

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The x86-64 MCE driver should not attempt to destroy/create a suspended device, unless it corresponds to a nonboot CPU that failed to go online during a resume.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_64.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_64.c +1 -2

Index: linux-2.6/arch/x86/kernel/cpu/mcheck/mce_64.c
===================================================================
@@ -862,11 +862,10 @@ mce_cpu_callback(struct notifier_block *
 
 	switch (action) {
 	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
 		mce_create_device(cpu);
 		break;
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
+	case CPU_UP_CANCELED_FROZEN:
 		mce_remove_device(cpu);
 		break;
 	}

Rafael J. Wysocki

author

2008-01-01 23:40 UTC #20 lore raw

PATCH 3/4 PM: Do not destroy/create devices while suspended in mce_64.c

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The x86-64 MCE driver should not attempt to destroy/create a suspended device, unless it corresponds to a nonboot CPU that failed to go online during a resume.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_64.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_64.c +1 -2

Index: linux-2.6/arch/x86/kernel/cpu/mcheck/mce_64.c
===================================================================
@@ -862,11 +862,10 @@ mce_cpu_callback(struct notifier_block *
 
 	switch (action) {
 	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
 		mce_create_device(cpu);
 		break;
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
+	case CPU_UP_CANCELED_FROZEN:
 		mce_remove_device(cpu);
 		break;
 	}

Rafael J. Wysocki

author

2008-01-01 23:40 UTC #21 lore raw

PATCH 4/4 PM: Do not destroy/create devices while suspended in cpuid.c

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The cpuid driver should not attempt to destroy/create a device while suspended, unless this device corresponds to a nonboot CPU that failed to go online during a resume, in which case the PM core should be asked to remove it.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/cpuid.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/x86/kernel/cpuid.c +3 -3

Index: linux-2.6/arch/x86/kernel/cpuid.c
===================================================================
@@ -157,15 +157,15 @@ static int __cpuinit cpuid_class_cpu_cal
 
 	switch (action) {
 	case CPU_UP_PREPARE:
-	case CPU_UP_PREPARE_FROZEN:
 		err = cpuid_device_create(cpu);
 		break;
 	case CPU_UP_CANCELED:
-	case CPU_UP_CANCELED_FROZEN:
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
 		cpuid_device_destroy(cpu);
 		break;
+	case CPU_UP_CANCELED_FROZEN:
+		destroy_suspended_device(cpuid_class, MKDEV(CPUID_MAJOR, cpu));
+		break;
 	}
 	return err ? NOTIFY_BAD : NOTIFY_OK;
 }

Rafael J. Wysocki

author

2008-01-01 23:42 UTC #22 lore raw

PATCH 4/4 PM: Do not destroy/create devices while suspended in cpuid.c

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/cpuid.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/x86/kernel/cpuid.c +3 -3

Index: linux-2.6/arch/x86/kernel/cpuid.c
===================================================================
@@ -157,15 +157,15 @@ static int __cpuinit cpuid_class_cpu_cal
 
 	switch (action) {
 	case CPU_UP_PREPARE:
-	case CPU_UP_PREPARE_FROZEN:
 		err = cpuid_device_create(cpu);
 		break;
 	case CPU_UP_CANCELED:
-	case CPU_UP_CANCELED_FROZEN:
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
 		cpuid_device_destroy(cpu);
 		break;
+	case CPU_UP_CANCELED_FROZEN:
+		destroy_suspended_device(cpuid_class, MKDEV(CPUID_MAJOR, cpu));
+		break;
 	}
 	return err ? NOTIFY_BAD : NOTIFY_OK;
 }

Ingo Molnar

2008-01-02 10:52 UTC #23 lore raw

In reply to Rafael J. Wysocki

Quoted 25 lines · Rafael J. Wysocki

Hi,
Some device drivers register CPU hotplug notifiers and use them to destroy device objects when removing the corresponding CPUs and to create these objects when adding the CPUs back.
Unfortunately, this is not the right thing to do during suspend/hibernation, since in that cases the CPU hotplug notifiers are called after suspending devices and before resuming them, so the operations in question are carried out on the objects representing suspended devices which shouldn't be unregistered behing the PM core's back. Although right now it usually doesn't lead to any practical complications, it will predictably deadlock if gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.
The solution is to prevent drivers from removing/adding devices from within CPU hotplug notifiers during suspend/hibernation using the FROZEN bit in the notifier's action argument. However, this has to be done with care, since the devices objects related to the nonboot CPUs that failed to go online during resume should not be present in the system. For this reason, it seems reasonable to introduce a mechanism allowing drivers to ask the PM core to remove device objects corresponding to suspended devices on their behalf.

Quoted 3 lines

The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.

btw., it would be really, really cool if there was a scriptable way i could test suspend/resume functionality. Pavel has this /dev/rtc thing to set up an alarm (not sure how functional it is) - would it be possible to have it as a "suspend for 10 seconds then resume" debug functionality? That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.

Ingo

Ingo Molnar

2008-01-02 10:53 UTC #24 lore raw

In reply to Rafael J. Wysocki

Quoted 25 lines · Rafael J. Wysocki

Hi,
Some device drivers register CPU hotplug notifiers and use them to destroy device objects when removing the corresponding CPUs and to create these objects when adding the CPUs back.
Unfortunately, this is not the right thing to do during suspend/hibernation, since in that cases the CPU hotplug notifiers are called after suspending devices and before resuming them, so the operations in question are carried out on the objects representing suspended devices which shouldn't be unregistered behing the PM core's back. Although right now it usually doesn't lead to any practical complications, it will predictably deadlock if gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.
The solution is to prevent drivers from removing/adding devices from within CPU hotplug notifiers during suspend/hibernation using the FROZEN bit in the notifier's action argument. However, this has to be done with care, since the devices objects related to the nonboot CPUs that failed to go online during resume should not be present in the system. For this reason, it seems reasonable to introduce a mechanism allowing drivers to ask the PM core to remove device objects corresponding to suspended devices on their behalf.

Quoted 3 lines

The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.

Ingo
-
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

Rafael J. Wysocki

author

2008-01-02 12:55 UTC #25 lore raw

Quoted 28 lines · Ingo Molnar

Quoted 25 lines · Rafael J. Wysocki
Hi,
Some device drivers register CPU hotplug notifiers and use them to destroy device objects when removing the corresponding CPUs and to create these objects when adding the CPUs back.
Unfortunately, this is not the right thing to do during suspend/hibernation, since in that cases the CPU hotplug notifiers are called after suspending devices and before resuming them, so the operations in question are carried out on the objects representing suspended devices which shouldn't be unregistered behing the PM core's back. Although right now it usually doesn't lead to any practical complications, it will predictably deadlock if gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.
The solution is to prevent drivers from removing/adding devices from within CPU hotplug notifiers during suspend/hibernation using the FROZEN bit in the notifier's action argument. However, this has to be done with care, since the devices objects related to the nonboot CPUs that failed to go online during resume should not be present in the system. For this reason, it seems reasonable to introduce a mechanism allowing drivers to ask the PM core to remove device objects corresponding to suspended devices on their behalf.

Quoted 6 lines

Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
btw., it would be really, really cool if there was a scriptable way i could test suspend/resume functionality.

First, there are patches queued for 2.6.25 that allow you to test various phases of suspend (specifically, patches 09-11 in the series at
http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.24-rc6/patches/).

With these patches applied you can do something like:

# echo core > /sys/power/pm_test
# echo mem > /sys/power/state
and it will run the suspend code up to, but not including, entering the sleep
state (it will busy wait for 5 sec. instead).  Then, it will run the resume
code.

There are 6 testing levels available, documented in patch 11 and in the changelogs.

Second, there's the rtc wakealarm thing that can be used to test the real suspend.

Quoted 3 lines

Pavel has this /dev/rtc thing to set up an alarm (not sure how functional it is) - would it be possible to have it as a "suspend for 10 seconds then resume" debug functionality?

Well, we have the following test script in the userland suspend package that is supposed to work right now:

#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date

provided that the new rtc driver code is compiled (and the old one is not).

Quoted 3 lines

That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.

Yes, but please note that some systems require user space manipulations of the graphics adapter for suspend to work and to detect a breakage of such a system you need to boot it into X and use s2ram to suspend.

Greetings,
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

Ingo Molnar

2008-01-02 13:15 UTC #26 lore raw

(David Brownell Cc:-ed too)

Quoted 11 lines · Rafael J. Wysocki

Well, we have the following test script in the userland suspend package that is supposed to work right now:
#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date
provided that the new rtc driver code is compiled (and the old one is not).

ok, will try that. A stupid question. The old RTC driver is in drivers/char/rtc.c, and maps to:

crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc

the new driver is in drivers/rtc/*, and maps to:

crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0

but all the x86 distro boxes i have access to make use of /dev/rtc. There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it appears to me that the new RTC driver isnt actually utilized on most distributions.

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right? That way distros could start migrating to it as well, without depending on any udev hackery.

Quoted 8 lines

Quoted 3 lines
That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.
Yes, but please note that some systems require user space manipulations of the graphics adapter for suspend to work and to detect a breakage of such a system you need to boot it into X and use s2ram to suspend.

yeah, i wouldnt expect graphics mode to come back without quirks. But it should still work fine over the network, right? (which is my main mode of testing anyway)

Ingo

Ingo Molnar

2008-01-02 13:16 UTC #27 lore raw

In reply to Rafael J. Wysocki

(David Brownell Cc:-ed too)

Quoted 11 lines · Rafael J. Wysocki

Well, we have the following test script in the userland suspend package that is supposed to work right now:
#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date
provided that the new rtc driver code is compiled (and the old one is not).

ok, will try that. A stupid question. The old RTC driver is in drivers/char/rtc.c, and maps to:

crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc

the new driver is in drivers/rtc/*, and maps to:

crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0

Quoted 8 lines

Quoted 3 lines
That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.
Yes, but please note that some systems require user space manipulations of the graphics adapter for suspend to work and to detect a breakage of such a system you need to boot it into X and use s2ram to suspend.

yeah, i wouldnt expect graphics mode to come back without quirks. But it should still work fine over the network, right? (which is my main mode of testing anyway)

Ingo

Rafael J. Wysocki

author

2008-01-02 13:26 UTC #28 lore raw

Quoted 22 lines · Ingo Molnar

(David Brownell Cc:-ed too)
Quoted 11 lines · Rafael J. Wysocki
Well, we have the following test script in the userland suspend package that is supposed to work right now:
#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date
provided that the new rtc driver code is compiled (and the old one is not).
ok, will try that. A stupid question. The old RTC driver is in drivers/char/rtc.c, and maps to:
crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc

Quoted 8 lines

the new driver is in drivers/rtc/*, and maps to:
crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0
but all the x86 distro boxes i have access to make use of /dev/rtc. There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it appears to me that the new RTC driver isnt actually utilized on most distributions.

I think so.

Quoted 5 lines

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right? That way distros could start migrating to it as well, without depending on any udev hackery.

Question for Dave. ;-)

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.

Quoted 8 lines

Quoted 4 lines
Yes, but please note that some systems require user space manipulations of the graphics adapter for suspend to work and to detect a breakage of such a system you need to boot it into X and use s2ram to suspend.
yeah, i wouldnt expect graphics mode to come back without quirks. But it should still work fine over the network, right? (which is my main mode of testing anyway)

Well, if the graphics is sufficiently broken, it won't resume at all.

The "echo core > /sys/power/pm_test && echo mem > /sys/power/state" thing should always work, though (IOW, if this doesn't work, we cartainly have a bug).

Rafael

Pavel Machek

2008-01-03 10:56 UTC #29 lore raw

Hi!

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.
Yes, but please note that some systems require user space manipulations of the graphics adapter for suspend to work and to detect a breakage of such a system you need to boot it into X and use s2ram to suspend.

Quoted 6 lines

Quoted 3 lines
yeah, i wouldnt expect graphics mode to come back without quirks. But it should still work fine over the network, right? (which is my main mode of testing anyway)
Well, if the graphics is sufficiently broken, it won't resume at all.

Actually, no. Unless you try to boot the bios, it should come up without graphics.

Hmm... first framebuffer access may kill the machine at that point... so disable framebuffer...? ;-).

vga=1 and no acpi_sleep options usually does the trick for me. That should work everywhere, independend of graphics options, AFAICT.
Pavel

Signature · 2 lines

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

Pavel Machek

2008-01-03 10:56 UTC #30 lore raw

In reply to Rafael J. Wysocki

Hi!

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.
Yes, but please note that some systems require user space manipulations of the graphics adapter for suspend to work and to detect a breakage of such a system you need to boot it into X and use s2ram to suspend.

Quoted 6 lines

Quoted 3 lines
yeah, i wouldnt expect graphics mode to come back without quirks. But it should still work fine over the network, right? (which is my main mode of testing anyway)
Well, if the graphics is sufficiently broken, it won't resume at all.

Actually, no. Unless you try to boot the bios, it should come up without graphics.

Hmm... first framebuffer access may kill the machine at that point... so disable framebuffer...? ;-).

vga=1 and no acpi_sleep options usually does the trick for me. That should work everywhere, independend of graphics options, AFAICT.
Pavel

Signature · 2 lines

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

Rafael J. Wysocki

author

2008-01-02 13:28 UTC #31 lore raw

In reply to Ingo Molnar

Quoted 22 lines · Ingo Molnar

(David Brownell Cc:-ed too)
Quoted 11 lines · Rafael J. Wysocki
Well, we have the following test script in the userland suspend package that is supposed to work right now:
#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date
provided that the new rtc driver code is compiled (and the old one is not).
ok, will try that. A stupid question. The old RTC driver is in drivers/char/rtc.c, and maps to:
crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc

Quoted 8 lines

the new driver is in drivers/rtc/*, and maps to:
crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0
but all the x86 distro boxes i have access to make use of /dev/rtc. There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it appears to me that the new RTC driver isnt actually utilized on most distributions.

I think so.

Quoted 5 lines

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right? That way distros could start migrating to it as well, without depending on any udev hackery.

Question for Dave. ;-)

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.

Quoted 8 lines

Quoted 4 lines
Yes, but please note that some systems require user space manipulations of the graphics adapter for suspend to work and to detect a breakage of such a system you need to boot it into X and use s2ram to suspend.
yeah, i wouldnt expect graphics mode to come back without quirks. But it should still work fine over the network, right? (which is my main mode of testing anyway)

Well, if the graphics is sufficiently broken, it won't resume at all.

The "echo core > /sys/power/pm_test && echo mem > /sys/power/state" thing should always work, though (IOW, if this doesn't work, we cartainly have a bug).

Rafael

Kay Sievers

2008-01-02 14:55 UTC #32 lore raw

In reply to Ingo Molnar

Quoted 14 lines · Ingo Molnar

A stupid question. The old RTC driver is in
drivers/char/rtc.c, and maps to:
crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc
the new driver is in drivers/rtc/*, and maps to:
crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0
but all the x86 distro boxes i have access to make use of /dev/rtc. There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it appears to me that the new RTC driver isnt actually utilized on most distributions.

Quoted 4 lines

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right?

It's not compatible enough to "fake" only the old major/minor. Userspace must be fixed not to depend on stuff like: /proc/sys/dev/rtc/max-user-freq:
http://lkml.org/lkml/2007/8/4/183

Quoted 2 lines

That way distros could start migrating to it
as well, without depending on any udev hackery.

It's in the default udev setup:
KERNEL=="rtc|rtc0", MODE="0644"
KERNEL=="rtc0", SYMLINK+="rtc"

Kay

Kay Sievers

2008-01-02 14:55 UTC #33 lore raw

In reply to Ingo Molnar

Quoted 14 lines · Ingo Molnar

A stupid question. The old RTC driver is in
drivers/char/rtc.c, and maps to:
crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc
the new driver is in drivers/rtc/*, and maps to:
crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0
but all the x86 distro boxes i have access to make use of /dev/rtc. There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it appears to me that the new RTC driver isnt actually utilized on most distributions.

Quoted 4 lines

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right?

It's not compatible enough to "fake" only the old major/minor. Userspace must be fixed not to depend on stuff like: /proc/sys/dev/rtc/max-user-freq:
http://lkml.org/lkml/2007/8/4/183

Quoted 2 lines

That way distros could start migrating to it
as well, without depending on any udev hackery.

It's in the default udev setup:
KERNEL=="rtc|rtc0", MODE="0644"
KERNEL=="rtc0", SYMLINK+="rtc"

Kay

Ingo Molnar

2008-01-02 16:01 UTC #34 lore raw

Quoted 8 lines · Kay Sievers

Quoted 4 lines
shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right?
It's not compatible enough to "fake" only the old major/minor. Userspace must be fixed not to depend on stuff like: /proc/sys/dev/rtc/max-user-freq:
http://lkml.org/lkml/2007/8/4/183

i think that should be fixed by providing a compatible /proc/sys/dev/rtc/max-user-freq implementation in the new driver too.

Quoted 6 lines

Quoted 2 lines
That way distros could start migrating to it
as well, without depending on any udev hackery.
It's in the default udev setup:
KERNEL=="rtc|rtc0", MODE="0644"
KERNEL=="rtc0", SYMLINK+="rtc"

but if it breaks max-user-freq (which is needed by qemu for example) then distros would likely disable it, right?

or this rule might be broken in some way. For example my Fedora 8 box has this in /etc/udev/rules.d/50-udev-default.rules:

 # miscellaneous
 KERNEL=="fuse",                 MODE="0666"
 KERNEL=="rtc|rtc0",             MODE="0644"
 KERNEL=="rtc0",                 SYMLINK+="rtc"

still i've got:

crw------- 1 root root 10, 135 Dec 28 08:13 /dev/rtc
crw-r--r-- 1 root root 254, 0 Dec 28 08:13 /dev/rtc0

_and_ my distro kernel doesnt even have CONFIG_RTC enabled - i run the Fedora 9 devel/rawhide kernel on this box:

# CONFIG_RTC is not set
# CONFIG_GEN_RTC is not set
# CONFIG_HPET_RTC_IRQ is not set
CONFIG_RTC_LIB=y
CONFIG_RTC_CLASS=y
# CONFIG_RTC_HCTOSYS is not set
# CONFIG_RTC_DEBUG is not set
# RTC interfaces
CONFIG_RTC_INTF_SYSFS=y
CONFIG_RTC_INTF_PROC=y
CONFIG_RTC_INTF_DEV=y
# CONFIG_RTC_INTF_DEV_UIE_EMUL is not set
# CONFIG_RTC_DRV_TEST is not set

udev-116-3.fc8. Maybe i just misunderstood what the grand plan was here
- i assumed it was to smoothly convert from old driver to new driver, without forcing any changes on user-space.

Ingo

Ingo Molnar

2008-01-02 16:02 UTC #35 lore raw

In reply to Kay Sievers

Quoted 8 lines · Kay Sievers

Quoted 4 lines
shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right?
It's not compatible enough to "fake" only the old major/minor. Userspace must be fixed not to depend on stuff like: /proc/sys/dev/rtc/max-user-freq:
http://lkml.org/lkml/2007/8/4/183

i think that should be fixed by providing a compatible /proc/sys/dev/rtc/max-user-freq implementation in the new driver too.

Quoted 6 lines

Quoted 2 lines
That way distros could start migrating to it
as well, without depending on any udev hackery.
It's in the default udev setup:
KERNEL=="rtc|rtc0", MODE="0644"
KERNEL=="rtc0", SYMLINK+="rtc"

but if it breaks max-user-freq (which is needed by qemu for example) then distros would likely disable it, right?

or this rule might be broken in some way. For example my Fedora 8 box has this in /etc/udev/rules.d/50-udev-default.rules:

 # miscellaneous
 KERNEL=="fuse",                 MODE="0666"
 KERNEL=="rtc|rtc0",             MODE="0644"
 KERNEL=="rtc0",                 SYMLINK+="rtc"

still i've got:

crw------- 1 root root 10, 135 Dec 28 08:13 /dev/rtc
crw-r--r-- 1 root root 254, 0 Dec 28 08:13 /dev/rtc0

_and_ my distro kernel doesnt even have CONFIG_RTC enabled - i run the Fedora 9 devel/rawhide kernel on this box:

udev-116-3.fc8. Maybe i just misunderstood what the grand plan was here
- i assumed it was to smoothly convert from old driver to new driver, without forcing any changes on user-space.

Ingo

David Brownell

2008-01-02 17:54 UTC #36 lore raw

Quoted 11 lines

Quoted 8 lines
Quoted 4 lines
shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right?
It's not compatible enough to "fake" only the old major/minor. Userspace must be fixed not to depend on stuff like: /proc/sys/dev/rtc/max-user-freq:
http://lkml.org/lkml/2007/8/4/183
i think that should be fixed by providing a compatible /proc/sys/dev/rtc/max-user-freq implementation in the new driver too.

I see /sys/class/rtc/rtc0/max_user_freq, fwiw ...

Quoted 18 lines

Quoted 6 lines
Quoted 2 lines
That way distros could start migrating to it
as well, without depending on any udev hackery.
It's in the default udev setup:
KERNEL=="rtc|rtc0", MODE="0644"
KERNEL=="rtc0", SYMLINK+="rtc"
but if it breaks max-user-freq (which is needed by qemu for example) then distros would likely disable it, right?
or this rule might be broken in some way. For example my Fedora 8 box has this in /etc/udev/rules.d/50-udev-default.rules:
 # miscellaneous
 KERNEL=="fuse",                 MODE="0666"
 KERNEL=="rtc|rtc0",             MODE="0644"
 KERNEL=="rtc0",                 SYMLINK+="rtc"

Quoted 4 lines

still i've got:
crw------- 1 root root 10, 135 Dec 28 08:13 /dev/rtc
crw-r--r-- 1 root root 254, 0 Dec 28 08:13 /dev/rtc0

I suspect the /dev/rtc node was provided by the distro, so those udev rules won't trigger.

Quoted 17 lines

_and_ my distro kernel doesnt even have CONFIG_RTC enabled - i run the Fedora 9 devel/rawhide kernel on this box:
# CONFIG_RTC is not set
# CONFIG_GEN_RTC is not set
# CONFIG_HPET_RTC_IRQ is not set
CONFIG_RTC_LIB=y
CONFIG_RTC_CLASS=y
# CONFIG_RTC_HCTOSYS is not set
# CONFIG_RTC_DEBUG is not set
# RTC interfaces
CONFIG_RTC_INTF_SYSFS=y
CONFIG_RTC_INTF_PROC=y
CONFIG_RTC_INTF_DEV=y
# CONFIG_RTC_INTF_DEV_UIE_EMUL is not set
# CONFIG_RTC_DRV_TEST is not set

Quoted 1 line

udev-116-3.fc8.

I run my x86 systems like that (also with RTC_DRV_CMOS), but the ARM systems usually have RTC_HCTOSYS set too (plus some RTC driver other than rtc-cmos). The x86 system init code pokes directly at the legacy RTC hardware in several cases.

Quoted 3 lines

Maybe i just misunderstood what the grand plan was here
- i assumed it was to smoothly convert from old driver to new driver, without forcing any changes on user-space.

If there was a Grand Plan, I never heard of it. :)

It'd need to have some NTP sync solution for RTC_LIB devices, but ISTR the gentime stuff still assumes an update_persistent_clock() that doesn't sleep ... and hence can't be used with I2C based RTCs.

I've been trying to make sure the x86 world could realistically switch to the RTC framework used by other Linux platforms, hence e.g. the util-unix-ng updates, but never assumed there would be no userspace changes. After all, userspace was using a mishmash of tools that were far from platform-agnostic, and moving away from x86-dependency implies such things will change.

However I *do* think that symlinking rtc to /dev/rtc0 should make it practical to use most older binaries.

- Dave

David Brownell

2008-01-02 17:54 UTC #37 lore raw

In reply to Ingo Molnar

Quoted 11 lines

Quoted 8 lines
Quoted 4 lines
shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.) It's all supposed to be 100% ioctl ABI compatible with the old driver, right?
It's not compatible enough to "fake" only the old major/minor. Userspace must be fixed not to depend on stuff like: /proc/sys/dev/rtc/max-user-freq:
http://lkml.org/lkml/2007/8/4/183
i think that should be fixed by providing a compatible /proc/sys/dev/rtc/max-user-freq implementation in the new driver too.

I see /sys/class/rtc/rtc0/max_user_freq, fwiw ...

Quoted 18 lines

Quoted 6 lines
Quoted 2 lines
That way distros could start migrating to it
as well, without depending on any udev hackery.
It's in the default udev setup:
KERNEL=="rtc|rtc0", MODE="0644"
KERNEL=="rtc0", SYMLINK+="rtc"
but if it breaks max-user-freq (which is needed by qemu for example) then distros would likely disable it, right?
or this rule might be broken in some way. For example my Fedora 8 box has this in /etc/udev/rules.d/50-udev-default.rules:
 # miscellaneous
 KERNEL=="fuse",                 MODE="0666"
 KERNEL=="rtc|rtc0",             MODE="0644"
 KERNEL=="rtc0",                 SYMLINK+="rtc"

Quoted 4 lines

still i've got:
crw------- 1 root root 10, 135 Dec 28 08:13 /dev/rtc
crw-r--r-- 1 root root 254, 0 Dec 28 08:13 /dev/rtc0

I suspect the /dev/rtc node was provided by the distro, so those udev rules won't trigger.

Quoted 17 lines

_and_ my distro kernel doesnt even have CONFIG_RTC enabled - i run the Fedora 9 devel/rawhide kernel on this box:
# CONFIG_RTC is not set
# CONFIG_GEN_RTC is not set
# CONFIG_HPET_RTC_IRQ is not set
CONFIG_RTC_LIB=y
CONFIG_RTC_CLASS=y
# CONFIG_RTC_HCTOSYS is not set
# CONFIG_RTC_DEBUG is not set
# RTC interfaces
CONFIG_RTC_INTF_SYSFS=y
CONFIG_RTC_INTF_PROC=y
CONFIG_RTC_INTF_DEV=y
# CONFIG_RTC_INTF_DEV_UIE_EMUL is not set
# CONFIG_RTC_DRV_TEST is not set

Quoted 1 line

udev-116-3.fc8.

Quoted 3 lines

Maybe i just misunderstood what the grand plan was here
- i assumed it was to smoothly convert from old driver to new driver, without forcing any changes on user-space.

If there was a Grand Plan, I never heard of it. :)

However I *do* think that symlinking rtc to /dev/rtc0 should make it practical to use most older binaries.

- Dave

Alessandro Zummo

2008-01-02 18:05 UTC #38 lore raw

On Wed, 02 Jan 2008 09:54:00 -0800
David Bro

Quoted 3 lines

It'd need to have some NTP sync solution for RTC_LIB devices, but ISTR the gentime stuff still assumes an update_persistent_clock() that doesn't sleep ... and hence can't be used with I2C based RTCs.

I still believe NTP sync stuff should be done outside of the kernel. given the mean accuracy of RTC chips and other sync factors, imho you haven't so much to gain with an in-kernel sync code.

Signature · 6 lines

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

David Brownell

2008-01-02 18:12 UTC #39 lore raw

Quoted 7 lines

Quoted 3 lines
It'd need to have some NTP sync solution for RTC_LIB devices, but ISTR the gentime stuff still assumes an update_persistent_clock() that doesn't sleep ... and hence can't be used with I2C based RTCs.
I still believe NTP sync stuff should be done outside of the kernel. given the mean accuracy of RTC chips and other sync factors, imho you haven't so much to gain with an in-kernel sync code.

Then the kernel/time/ntp.c stuff should be removed on all systems?

David Brownell

2008-01-02 18:12 UTC #40 lore raw

In reply to Alessandro Zummo

Quoted 7 lines

Quoted 3 lines
It'd need to have some NTP sync solution for RTC_LIB devices, but ISTR the gentime stuff still assumes an update_persistent_clock() that doesn't sleep ... and hence can't be used with I2C based RTCs.
I still believe NTP sync stuff should be done outside of the kernel. given the mean accuracy of RTC chips and other sync factors, imho you haven't so much to gain with an in-kernel sync code.

Then the kernel/time/ntp.c stuff should be removed on all systems?

Alessandro Zummo

2008-01-02 18:34 UTC #41 lore raw

Quoted 9 lines · On Wed, 02 Jan 2008 10:12:54 -0800 David Brownell <[email protected]> wrote:

Quoted 7 lines
Quoted 3 lines
It'd need to have some NTP sync solution for RTC_LIB devices, but ISTR the gentime stuff still assumes an update_persistent_clock() that doesn't sleep ... and hence can't be used with I2C based RTCs.
I still believe NTP sync stuff should be done outside of the kernel. given the mean accuracy of RTC chips and other sync factors, imho you haven't so much to gain with an in-kernel sync code.
Then the kernel/time/ntp.c stuff should be removed on all systems?

I'd say yes, but I think that would be dangerous to my own life :)

Signature · 6 lines

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Alessandro Zummo

2008-01-02 18:34 UTC #42 lore raw

In reply to David Brownell

Quoted 9 lines · On Wed, 02 Jan 2008 10:12:54 -0800 David Brownell <[email protected]> wrote:

Quoted 7 lines
Quoted 3 lines
It'd need to have some NTP sync solution for RTC_LIB devices, but ISTR the gentime stuff still assumes an update_persistent_clock() that doesn't sleep ... and hence can't be used with I2C based RTCs.
I still believe NTP sync stuff should be done outside of the kernel. given the mean accuracy of RTC chips and other sync factors, imho you haven't so much to gain with an in-kernel sync code.
Then the kernel/time/ntp.c stuff should be removed on all systems?

I'd say yes, but I think that would be dangerous to my own life :)

Signature · 6 lines

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Alessandro Zummo

2008-01-02 18:05 UTC #43 lore raw

In reply to David Brownell

On Wed, 02 Jan 2008 09:54:00 -0800
David Bro

Quoted 3 lines

It'd need to have some NTP sync solution for RTC_LIB devices, but ISTR the gentime stuff still assumes an update_persistent_clock() that doesn't sleep ... and hence can't be used with I2C based RTCs.

I still believe NTP sync stuff should be done outside of the kernel. given the mean accuracy of RTC chips and other sync factors, imho you haven't so much to gain with an in-kernel sync code.

Signature · 6 lines

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Ingo Molnar

2008-01-02 20:14 UTC #44 lore raw

In reply to David Brownell

Quoted 9 lines · David Brownell

I've been trying to make sure the x86 world could realistically switch to the RTC framework used by other Linux platforms, hence e.g. the util-unix-ng updates, but never assumed there would be no userspace changes. After all, userspace was using a mishmash of tools that were far from platform-agnostic, and moving away from x86-dependency implies such things will change.
However I *do* think that symlinking rtc to /dev/rtc0 should make it practical to use most older binaries.

then please provide a kernel config option for the new driver to take over 10:135 too. There's nothing worse to the adoption of new kernel features necessiating user-space attention. I've got several images of old distros that i dont want to reconfigure in any way, and it would be nice to have a drop-in /dev/rtc replacement for them. Really. This is how we do it for just about everything else too.

Ingo

Ingo Molnar

2008-01-02 20:15 UTC #45 lore raw

In reply to David Brownell

Quoted 9 lines · David Brownell

I've been trying to make sure the x86 world could realistically switch to the RTC framework used by other Linux platforms, hence e.g. the util-unix-ng updates, but never assumed there would be no userspace changes. After all, userspace was using a mishmash of tools that were far from platform-agnostic, and moving away from x86-dependency implies such things will change.
However I *do* think that symlinking rtc to /dev/rtc0 should make it practical to use most older binaries.

Ingo

David Brownell

2008-01-02 20:29 UTC #46 lore raw

Quoted 7 lines · Ingo Molnar

then please provide a kernel config option for the new driver to take over 10:135 too. There's nothing worse to the adoption of new kernel features necessiating user-space attention. I've got several images of old distros that i dont want to reconfigure in any way, and it would be nice to have a drop-in /dev/rtc replacement for them. Really. This is how we do it for just about everything else too.

I'll let someone else chase that particular issue. :)

David Brownell

2008-01-02 20:29 UTC #47 lore raw

In reply to Ingo Molnar

Quoted 7 lines · Ingo Molnar

then please provide a kernel config option for the new driver to take over 10:135 too. There's nothing worse to the adoption of new kernel features necessiating user-space attention. I've got several images of old distros that i dont want to reconfigure in any way, and it would be nice to have a drop-in /dev/rtc replacement for them. Really. This is how we do it for just about everything else too.

I'll let someone else chase that particular issue. :)

David Brownell

2008-01-02 17:26 UTC #48 lore raw

In reply to Ingo Molnar

(Alessandro Zummo Cc:-ed too -- RTC subsystem maintainer)

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines
Well, we have the following test script in the userland suspend package that is supposed to work right now:

Quoted 8 lines

Quoted 8 lines
#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date
provided that the new rtc driver code is compiled (and the old one is not).

Eventually, swapping driver modules ought to work too. The legacy /proc/acpi/wakeup files would ISTR cause problems in current code.

Of course, one reason to want to use the RTC framework code is to stop depending on x86-isms like ACPI or "s2ram", and thus to work on more Linux platforms. ;)

Quoted 11 lines

ok, will try that. A stupid question. The old RTC driver is in drivers/char/rtc.c, and maps to:
crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc
the new driver is in drivers/rtc/*, and maps to:
crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0
but all the x86 distro boxes i have access to make use of /dev/rtc. There's no symlink set up from /dev/rtc to /dev/rtc0 either.

Current util-linux-ng code uses either RTC device file; and udev sets up /dev/rtc0 as needed. (But not /dev/rtc, as I recall...) Have distros switched away from the old unmaintained util-linux?

Quoted 3 lines

So it
appears to me that the new RTC driver isnt actually utilized on most distributions.

That might be so. There are some HPET issues, but those show up with both drivers.

The main other issue I know about which would seem to argue for using the legacy driver, instead of the RTC framework, is that some BIOSes don't seem to provide PNPACPI entries for their RTCs. I got one report of a newish HP Opteron system that doesn't. I have no idea how common that is. The drivers/acpi/glue.c code could detect that, but maybe a better place to address that would be in PNP code; in that case, ISTR that PNP0c01 claimed the RTC ioports, and so would be the natural place to make provide a real driver model node for that hardware.

Quoted 3 lines

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.)

The major number 254 is not statically allocated, ISTR; that should be managed only by udev.

Quoted 3 lines

It's all supposed to be 100% ioctl ABI compatible
with the old driver, right? That way distros could start migrating to it as well, without depending on any udev hackery.

I don't know of any ioctl differences userspace would care about.

- Dave

Ingo Molnar

2008-01-02 20:17 UTC #49 lore raw

Quoted 6 lines · David Brownell

Quoted 3 lines
shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.)
The major number 254 is not statically allocated, ISTR; that should be managed only by udev.

sorry, i meant a .config option that would cause 10:135 to be managed by the new RTC code too. (Config option can be default-off.)

Ingo

Ingo Molnar

2008-01-02 20:18 UTC #50 lore raw

In reply to David Brownell

Quoted 6 lines · David Brownell

Quoted 3 lines
shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.)
The major number 254 is not statically allocated, ISTR; that should be managed only by udev.

sorry, i meant a .config option that would cause 10:135 to be managed by the new RTC code too. (Config option can be default-off.)

Ingo

David Brownell

2008-01-02 17:26 UTC #51 lore raw

In reply to Ingo Molnar

(Alessandro Zummo Cc:-ed too -- RTC subsystem maintainer)

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines
Well, we have the following test script in the userland suspend package that is supposed to work right now:

Quoted 8 lines

Quoted 8 lines
#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date
provided that the new rtc driver code is compiled (and the old one is not).

Eventually, swapping driver modules ought to work too. The legacy /proc/acpi/wakeup files would ISTR cause problems in current code.

Of course, one reason to want to use the RTC framework code is to stop depending on x86-isms like ACPI or "s2ram", and thus to work on more Linux platforms. ;)

Quoted 11 lines

ok, will try that. A stupid question. The old RTC driver is in drivers/char/rtc.c, and maps to:
crw-r--r-- 1 root root 10, 135 Oct 25 18:02 /dev/rtc
the new driver is in drivers/rtc/*, and maps to:
crw-r--r-- 1 root root 254, 0 Dec 12 02:30 /dev/rtc0
but all the x86 distro boxes i have access to make use of /dev/rtc. There's no symlink set up from /dev/rtc to /dev/rtc0 either.

Current util-linux-ng code uses either RTC device file; and udev sets up /dev/rtc0 as needed. (But not /dev/rtc, as I recall...) Have distros switched away from the old unmaintained util-linux?

Quoted 3 lines

So it
appears to me that the new RTC driver isnt actually utilized on most distributions.

That might be so. There are some HPET issues, but those show up with both drivers.

Quoted 3 lines

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new driver's 254:0 device? (while keeping all the current modes of operation as well, of course.)

The major number 254 is not statically allocated, ISTR; that should be managed only by udev.

Quoted 3 lines

It's all supposed to be 100% ioctl ABI compatible
with the old driver, right? That way distros could start migrating to it as well, without depending on any udev hackery.

I don't know of any ioctl differences userspace would care about.

- Dave

Rafael J. Wysocki

author

2008-01-02 12:56 UTC #52 lore raw

In reply to Ingo Molnar

Quoted 28 lines · Ingo Molnar

Quoted 25 lines · Rafael J. Wysocki
Hi,
Some device drivers register CPU hotplug notifiers and use them to destroy device objects when removing the corresponding CPUs and to create these objects when adding the CPUs back.
Unfortunately, this is not the right thing to do during suspend/hibernation, since in that cases the CPU hotplug notifiers are called after suspending devices and before resuming them, so the operations in question are carried out on the objects representing suspended devices which shouldn't be unregistered behing the PM core's back. Although right now it usually doesn't lead to any practical complications, it will predictably deadlock if gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.
The solution is to prevent drivers from removing/adding devices from within CPU hotplug notifiers during suspend/hibernation using the FROZEN bit in the notifier's action argument. However, this has to be done with care, since the devices objects related to the nonboot CPUs that failed to go online during resume should not be present in the system. For this reason, it seems reasonable to introduce a mechanism allowing drivers to ask the PM core to remove device objects corresponding to suspended devices on their behalf.

Quoted 6 lines

Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
btw., it would be really, really cool if there was a scriptable way i could test suspend/resume functionality.

With these patches applied you can do something like:

# echo core > /sys/power/pm_test
# echo mem > /sys/power/state
and it will run the suspend code up to, but not including, entering the sleep
state (it will busy wait for 5 sec. instead).  Then, it will run the resume
code.

There are 6 testing levels available, documented in patch 11 and in the changelogs.

Second, there's the rtc wakealarm thing that can be used to test the real suspend.

Quoted 3 lines

Pavel has this /dev/rtc thing to set up an alarm (not sure how functional it is) - would it be possible to have it as a "suspend for 10 seconds then resume" debug functionality?

Well, we have the following test script in the userland suspend package that is supposed to work right now:

#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date

provided that the new rtc driver code is compiled (and the old one is not).

Quoted 3 lines

That way any suspend breakage would be detectable (and bisectable) in automated testing - if the resume does not come back after 10-20 seconds then the test failed.

Greetings,
Rafael

Andrew Morton

2008-01-12 00:46 UTC #53 lore raw

In reply to Rafael J. Wysocki

Quoted 20 lines · On Wed, 2 Jan 2008 00:32:44 +0100 "Rafael J. Wysocki" <[email protected]> wrote:

Some device drivers register CPU hotplug notifiers and use them to destroy device objects when removing the corresponding CPUs and to create these objects when adding the CPUs back.
Unfortunately, this is not the right thing to do during suspend/hibernation, since in that cases the CPU hotplug notifiers are called after suspending devices and before resuming them, so the operations in question are carried out on the objects representing suspended devices which shouldn't be unregistered behing the PM core's back. __Although right now it usually doesn't lead to any practical complications, it will predictably deadlock if gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.
The solution is to prevent drivers from removing/adding devices from within CPU hotplug notifiers during suspend/hibernation using the FROZEN bit in the notifier's action argument. However, this has to be done with care, since the devices objects related to the nonboot CPUs that failed to go online during resume should not be present in the system. For this reason, it seems reasonable to introduce a mechanism allowing drivers to ask the PM core to remove device objects corresponding to suspended devices on their behalf.

Quoted 3 lines

The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.

These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.

So what I have now is

revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch pm-introduce-destroy_suspended_device.patch pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch

So what needs to happen in Greg's tree is

a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch

b) apply these four patches

c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)

Andrew Morton

2008-01-12 00:47 UTC #54 lore raw

In reply to Rafael J. Wysocki

Quoted 20 lines · On Wed, 2 Jan 2008 00:32:44 +0100 "Rafael J. Wysocki" <[email protected]> wrote:

Some device drivers register CPU hotplug notifiers and use them to destroy device objects when removing the corresponding CPUs and to create these objects when adding the CPUs back.
Unfortunately, this is not the right thing to do during suspend/hibernation, since in that cases the CPU hotplug notifiers are called after suspending devices and before resuming them, so the operations in question are carried out on the objects representing suspended devices which shouldn't be unregistered behing the PM core's back. __Although right now it usually doesn't lead to any practical complications, it will predictably deadlock if gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.
The solution is to prevent drivers from removing/adding devices from within CPU hotplug notifiers during suspend/hibernation using the FROZEN bit in the notifier's action argument. However, this has to be done with care, since the devices objects related to the nonboot CPUs that failed to go online during resume should not be present in the system. For this reason, it seems reasonable to introduce a mechanism allowing drivers to ask the PM core to remove device objects corresponding to suspended devices on their behalf.

Quoted 3 lines

The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.

These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.

So what I have now is

So what needs to happen in Greg's tree is

a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch

b) apply these four patches

c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)

Andrew Morton

2008-01-12 00:49 UTC #55 lore raw

Quoted 19 lines · On Fri, 11 Jan 2008 16:46:13 -0800 Andrew Morton <[email protected]>…

Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.
So what I have now is
revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch pm-introduce-destroy_suspended_device.patch pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch

Quoted 7 lines

So what needs to happen in Greg's tree is
a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
b) apply these four patches
c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)

err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch

Confused, giving up.

Andrew Morton

2008-01-12 00:49 UTC #56 lore raw

In reply to Andrew Morton

Quoted 19 lines · On Fri, 11 Jan 2008 16:46:13 -0800 Andrew Morton <[email protected]>…

Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.
So what I have now is
revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch pm-introduce-destroy_suspended_device.patch pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch

Quoted 7 lines

So what needs to happen in Greg's tree is
a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
b) apply these four patches
c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)

err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch

Confused, giving up.

Greg KH

2008-01-12 00:56 UTC #57 lore raw

Quoted 26 lines · Andrew Morton

Quoted 23 lines · On Fri, 11 Jan 2008 16:46:13 -0800 Andrew Morton <[email protected]>…
Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.
So what I have now is
revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch pm-introduce-destroy_suspended_device.patch pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch
So what needs to happen in Greg's tree is
a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch

Quoted 8 lines

Quoted 3 lines
b) apply these four patches
c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.

I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?

thanks,

greg k-h

Alan Stern

2008-01-12 03:11 UTC #58 lore raw

Quoted 9 lines · Greg KH

Quoted 4 lines · Andrew Morton
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.
I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?

Sorry for the confusion. The correct patch to apply is pm-acquire-device-locks-on-suspend-rev-3 (plus the attending style-fixups). It encompasses those earlier patches.

The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?

Alan Stern

Andi Kleen

2008-01-12 03:15 UTC #59 lore raw

Quoted 6 lines

The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?

The versioning approach pioneered by Christoph Lameter seems to work reasonably well.

If you post a new version increase a version number and add it with "vXXX" to the Subject.

Also add a short change log between versions at the bottom; e.g. v1->v2: .... etc.

Then it is always clear what is the latest'n'greatest.

-Andi

Andi Kleen

2008-01-12 03:15 UTC #60 lore raw

In reply to Alan Stern

Quoted 6 lines

The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?

The versioning approach pioneered by Christoph Lameter seems to work reasonably well.

If you post a new version increase a version number and add it with "vXXX" to the Subject.

Also add a short change log between versions at the bottom; e.g. v1->v2: .... etc.

Then it is always clear what is the latest'n'greatest.

-Andi

Andrew Morton

2008-01-12 03:21 UTC #61 lore raw

In reply to Alan Stern

Quoted 16 lines · On Fri, 11 Jan 2008 22:11:52 -0500 (EST) Alan Stern…

Quoted 9 lines · Greg KH
Quoted 4 lines · Andrew Morton
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.
I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?
Sorry for the confusion. The correct patch to apply is pm-acquire-device-locks-on-suspend-rev-3 (plus the attending style-fixups). It encompasses those earlier patches.

Quoted 6 lines

The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?

Don't formally send it until it's ready. Seriously. You can always resend it if it didn't get applied anywhere.

Once a patch reaches a sufficient level of maturity for it to be ready to be merged into a subsystem tree, any subsequent changes should be sufficiently small that incremental patches are the way to apply touchups.

The core problem here is that (lots of) people are flinging patches at tree-owners before they are sufficiently baked.

Greg KH

2008-01-12 04:28 UTC #62 lore raw

In reply to Alan Stern

Quoted 7 lines · Alan Stern

Quoted 5 lines · Greg KH
Quoted 3 lines · Andrew Morton
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Confused, giving up.
I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?
Sorry for the confusion. The correct patch to apply is pm-acquire-device-locks-on-suspend-rev-3 (plus the attending style-fixups). It encompasses those earlier patches.

Can someone resend this to me? Do I need to drop the patch I currently have in my tree as well? Or put it before/after that one?

Quoted 5 lines

The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?

Yeah, just tell me what you want me to do with it (drop an old one, replace it, add it, etc.) We usually can handle this pretty well :)

thanks,

greg k-h

Rafael J. Wysocki

author

2008-01-12 11:22 UTC #63 lore raw

Quoted 20 lines · Greg KH

Quoted 7 lines · Alan Stern
Quoted 5 lines · Greg KH
Quoted 3 lines · Andrew Morton
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Confused, giving up.
I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?
Sorry for the confusion. The correct patch to apply is pm-acquire-device-locks-on-suspend-rev-3 (plus the attending style-fixups). It encompasses those earlier patches.
Can someone resend this to me? Do I need to drop the patch I currently have in my tree as well? Or put it before/after that one?

Quoted 8 lines

Quoted 5 lines
The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?
Yeah, just tell me what you want me to do with it (drop an old one, replace it, add it, etc.) We usually can handle this pretty well :)

I'll repost the new patch along with instructions what to do with it.

Greetings,
Rafael

Rafael J. Wysocki

author

2008-01-12 11:25 UTC #64 lore raw

In reply to Greg KH

Quoted 20 lines · Greg KH

Quoted 7 lines · Alan Stern
Quoted 5 lines · Greg KH
Quoted 3 lines · Andrew Morton
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Confused, giving up.
I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?
Sorry for the confusion. The correct patch to apply is pm-acquire-device-locks-on-suspend-rev-3 (plus the attending style-fixups). It encompasses those earlier patches.
Can someone resend this to me? Do I need to drop the patch I currently have in my tree as well? Or put it before/after that one?

Quoted 8 lines

Quoted 5 lines
The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?
Yeah, just tell me what you want me to do with it (drop an old one, replace it, add it, etc.) We usually can handle this pretty well :)

I'll repost the new patch along with instructions what to do with it.

Greetings,
Rafael

Greg KH

2008-01-12 04:29 UTC #65 lore raw

In reply to Alan Stern

Quoted 7 lines · Alan Stern

Quoted 5 lines · Greg KH
Quoted 3 lines · Andrew Morton
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Confused, giving up.
I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?
Sorry for the confusion. The correct patch to apply is pm-acquire-device-locks-on-suspend-rev-3 (plus the attending style-fixups). It encompasses those earlier patches.

Can someone resend this to me? Do I need to drop the patch I currently have in my tree as well? Or put it before/after that one?

Quoted 5 lines

The real problem is that our current email workflow patterns don't provide a standardized way for maintainers to tell when a new patch submission is meant to override or replace an earlier submission (or even a set of earlier submissions). Does anybody have some suggestions for a good way to do this?

Yeah, just tell me what you want me to do with it (drop an old one, replace it, add it, etc.) We usually can handle this pretty well :)

thanks,

greg k-h

Alan Stern

2008-01-12 03:11 UTC #66 lore raw

In reply to Greg KH

Quoted 9 lines · Greg KH

Quoted 4 lines · Andrew Morton
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.
I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?

Sorry for the confusion. The correct patch to apply is pm-acquire-device-locks-on-suspend-rev-3 (plus the attending style-fixups). It encompasses those earlier patches.

Alan Stern

Greg KH

2008-01-12 00:56 UTC #67 lore raw

In reply to Andrew Morton

Quoted 26 lines · Andrew Morton

Quoted 23 lines · On Fri, 11 Jan 2008 16:46:13 -0800 Andrew Morton <[email protected]>…
Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.
So what I have now is
revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch pm-introduce-destroy_suspended_device.patch pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch
So what needs to happen in Greg's tree is
a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch

Quoted 8 lines

Quoted 3 lines
b) apply these four patches
c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.

I'm confused too, I have no idea what the proper order of things should be either. Anyone want to give me a hint?

thanks,

greg k-h

Rafael J. Wysocki

author

2008-01-12 11:17 UTC #68 lore raw

In reply to Andrew Morton

Quoted 26 lines · Andrew Morton

Quoted 23 lines · On Fri, 11 Jan 2008 16:46:13 -0800 Andrew Morton <[email protected]>…
Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.
So what I have now is
revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch pm-introduce-destroy_suspended_device.patch pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch
So what needs to happen in Greg's tree is
a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch

Quoted 8 lines

Quoted 3 lines
b) apply these four patches
c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.

Sorry, I didn't know you still had pm-introduce-destroy_suspended_device.patch.

Please drop:

pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch

and I'll resend a new checkpatch.pl-compliant version of the pm-acquire-device-locks-on-suspend patch with appropriate sign-offs to you and Greg.

Rafael J. Wysocki

author

2008-01-12 11:20 UTC #69 lore raw

In reply to Andrew Morton

Quoted 26 lines · Andrew Morton

Quoted 23 lines · On Fri, 11 Jan 2008 16:46:13 -0800 Andrew Morton <[email protected]>…
Quoted 3 lines
The first patch in the series introduces such a mechanism. The remaining three patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the above approach.
These patches are a preresuisite to gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its later fixed-up versions.
So what I have now is
revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch pm-introduce-destroy_suspended_device.patch pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch pm-acquire-device-locks-on-suspend-rev-3.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch
So what needs to happen in Greg's tree is
a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch

Quoted 8 lines

Quoted 3 lines
b) apply these four patches
c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)
err, no. pm-introduce-destroy_suspended_device.patch demolishes pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.

Sorry, I didn't know you still had pm-introduce-destroy_suspended_device.patch.

Please drop:

and I'll resend a new checkpatch.pl-compliant version of the pm-acquire-device-locks-on-suspend patch with appropriate sign-offs to you and Greg.