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
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(-)
Index: linux-2.6/arch/x86/kernel/cpuid.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpuid.c
+++ 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;
}
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(-)
Index: linux-2.6/arch/x86/kernel/cpu/mcheck/mce_64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/mcheck/mce_64.c
+++ 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;
}
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(-)
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ 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
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ 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
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ 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 */
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ 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)
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(-)
Index: linux-2.6/arch/x86/kernel/msr.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/msr.c
+++ 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 <[email protected]> wrote:
> 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.
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
On Wednesday, 2 of January 2008, Ingo Molnar wrote:
>
> * Rafael J. Wysocki <[email protected]> wrote:
>
> > 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.
>
> 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.
> 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).
> 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
(David Brownell Cc:-ed too)
* Rafael J. Wysocki <[email protected]> wrote:
> 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.
> > 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
On Wednesday, 2 of January 2008, Ingo Molnar wrote:
>
> (David Brownell Cc:-ed too)
>
> * Rafael J. Wysocki <[email protected]> wrote:
>
> > 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.
I think so.
> 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. ;-)
> > > 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)
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
On Wednesday, 2 of January 2008, Rafael J. Wysocki wrote:
> 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]>
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]>
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 | 35 ++++++++++++++++++++++++++---------
drivers/base/power/power.h | 1 +
include/linux/device.h | 8 ++++++++
4 files changed, 74 insertions(+), 14 deletions(-)
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ 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
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ 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
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ 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 */
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ 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)
On Jan 2, 2008 2:15 PM, Ingo Molnar <[email protected]> wrote:
> 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?
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
> 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 <[email protected]> wrote:
> > 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.
> > 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
On Wed, 2 Jan 2008, Rafael J. Wysocki wrote:
> On Wednesday, 2 of January 2008, Rafael J. Wysocki wrote:
> > 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]>
>
> 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
On Wednesday, 2 of January 2008, Alan Stern wrote:
> On Wed, 2 Jan 2008, Rafael J. Wysocki wrote:
>
> > On Wednesday, 2 of January 2008, Rafael J. Wysocki wrote:
> > > 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]>
> >
> > 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?
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
(Alessandro Zummo Cc:-ed too -- RTC subsystem maintainer)
> * Rafael J. Wysocki <[email protected]> wrote:
>
> > 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).
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. ;)
> 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?
> 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.
> 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.
> 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
> > > 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 ...
> > > 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
I suspect the /dev/rtc node was provided by the distro, so those
udev rules won't trigger.
> _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.
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.
> 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
On Wed, 02 Jan 2008 09:54:00 -0800
David Bro
> 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.
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.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 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?
On Wed, 02 Jan 2008 10:12:54 -0800
David Brownell <[email protected]> wrote:
> > > 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 :)
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
* David Brownell <[email protected]> wrote:
> 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
* David Brownell <[email protected]> wrote:
> > 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
On Wednesday 02 January 2008, Ingo Molnar wrote:
>
> 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. :)
Hi!
> > > > 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)
>
> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wednesday, 2 of January 2008, Rafael J. Wysocki wrote:
> On Wednesday, 2 of January 2008, Alan Stern wrote:
> > On Wed, 2 Jan 2008, Rafael J. Wysocki wrote:
> >
> > > On Wednesday, 2 of January 2008, Rafael J. Wysocki wrote:
> > > > 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]>
> > >
> > > 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?
>
> 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(-)
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ 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);
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -24,18 +24,39 @@
#include <linux/mutex.h>
#include <linux/pm.h>
#include <linux/resume-trace.h>
+#include <linux/rwsem.h>
#include "../base.h"
#include "power.h"
+/*
+ * The entries in the dpm_active list are in a depth first order, simply
+ * because children are guaranteed to be discovered after parents, and
+ * are inserted at the back of the list on discovery.
+ *
+ * All the other lists are kept in the same order, for consistency.
+ * However the lists aren't always traversed in the same order.
+ * Semaphores must be acquired from the top (i.e., front) down
+ * and released in the opposite order. Devices must be suspended
+ * from the bottom (i.e., end) up and resumed in the opposite order.
+ * That way no parent will be suspended while it still has an active
+ * child.
+ *
+ * Since device_pm_add() may be called with a device semaphore held,
+ * we must never try to acquire a device semaphore while holding
+ * dpm_list_mutex.
+ */
+
LIST_HEAD(dpm_active);
+static LIST_HEAD(dpm_locked);
static LIST_HEAD(dpm_off);
static LIST_HEAD(dpm_off_irq);
static LIST_HEAD(dpm_destroy);
-static DEFINE_MUTEX(dpm_mtx);
static DEFINE_MUTEX(dpm_list_mtx);
+static DECLARE_RWSEM(pm_sleep_rwsem);
+
int (*platform_enable_wakeup)(struct device *dev, int is_on);
@@ -54,12 +75,23 @@ void device_pm_remove(struct device *dev
pr_debug("PM: Removing info for %s:%s\n",
dev->bus ? dev->bus->name : "No Bus",
kobject_name(&dev->kobj));
+
+ /* Don't remove a device while the PM core has it locked for suspend */
+ down(&dev->sem);
mutex_lock(&dpm_list_mtx);
dpm_sysfs_remove(dev);
list_del_init(&dev->power.entry);
mutex_unlock(&dpm_list_mtx);
+ up(&dev->sem);
}
+/**
+ * device_pm_schedule_removal - Schedule device for removal
+ *
+ * Move the device to the list of devices to be removed during the
+ * subsequent resume. To be called when pm_sleep_rwsem is held for
+ * writing and all devices' semaphores are locked.
+ */
void device_pm_schedule_removal(struct device *dev)
{
pr_debug("PM: Removing info for %s:%s\n",
@@ -70,23 +102,102 @@ void device_pm_schedule_removal(struct d
mutex_unlock(&dpm_list_mtx);
}
+/**
+ * pm_sleep_lock - mutual exclusion for registration and suspend
+ *
+ * Returns 0 if no suspend is underway and device registration
+ * may proceed, otherwise -EBUSY.
+ */
+int pm_sleep_lock(void)
+{
+ if (down_read_trylock(&pm_sleep_rwsem))
+ return 0;
+ return -EBUSY;
+}
+
+/**
+ * pm_sleep_unlock - mutual exclusion for registration and suspend
+ *
+ * This routine undoes the effect of device_pm_add_lock
+ * when a device's registration is complete.
+ */
+void pm_sleep_unlock(void)
+{
+ up_read(&pm_sleep_rwsem);
+}
+
/*------------------------- Resume routines -------------------------*/
/**
- * resume_device - Restore state for one device.
+ * resume_device_early - Power on one device (early resume).
* @dev: Device.
*
+ * Must be called with interrupts disabled.
*/
-
-static int resume_device(struct device * dev)
+static int resume_device_early(struct device *dev)
{
int error = 0;
TRACE_DEVICE(dev);
TRACE_RESUME(0);
- down(&dev->sem);
+ if (dev->bus && dev->bus->resume_early) {
+ dev_dbg(dev,"EARLY resume\n");
+ error = dev->bus->resume_early(dev);
+ }
+
+ TRACE_RESUME(error);
+ return error;
+}
+
+/**
+ * dpm_power_up - Power on all regular (non-sysdev) devices.
+ *
+ * Walk the dpm_off_irq list and power each device up. This
+ * is used for devices that required they be powered down with
+ * interrupts disabled. As devices are powered on, they are moved
+ * to the dpm_off list.
+ *
+ * Interrupts must be disabled when calling this.
+ */
+static void dpm_power_up(void)
+{
+ while (!list_empty(&dpm_off_irq)) {
+ struct list_head *entry = dpm_off_irq.next;
+ struct device *dev = to_device(entry);
+
+ resume_device_early(dev);
+ list_move_tail(entry, &dpm_off);
+ }
+}
+
+/**
+ * device_power_up - Turn on all devices that need special attention.
+ *
+ * Power on system devices, then devices that required we shut them down
+ * with interrupts disabled.
+ *
+ * Must be called with interrupts disabled.
+ */
+void device_power_up(void)
+{
+ sysdev_resume();
+ dpm_power_up();
+}
+EXPORT_SYMBOL_GPL(device_power_up);
+
+/**
+ * resume_device - Restore state for one device.
+ * @dev: Device.
+ *
+ */
+static int resume_device(struct device *dev)
+{
+ int error = 0;
+
+ TRACE_DEVICE(dev);
+ TRACE_RESUME(0);
if (dev->bus && dev->bus->resume) {
dev_dbg(dev,"resuming\n");
@@ -103,132 +214,83 @@ static int resume_device(struct device *
error = dev->class->resume(dev);
}
- up(&dev->sem);
-
- TRACE_RESUME(error);
- return error;
-}
-
-
-static int resume_device_early(struct device * dev)
-{
- int error = 0;
-
- TRACE_DEVICE(dev);
- TRACE_RESUME(0);
- if (dev->bus && dev->bus->resume_early) {
- dev_dbg(dev,"EARLY resume\n");
- error = dev->bus->resume_early(dev);
- }
TRACE_RESUME(error);
return error;
}
/**
- * dpm_resume - Restore state of each device in system.
+ * dpm_resume - Resume every device.
+ *
+ * Resume the devices that have either not gone through
+ * the late suspend, or that did go through it but also
+ * went through the early resume.
*
- * Walk the dpm_off list, remove each entry, resume the device,
- * then add it to the dpm_active list. Unregister devices scheduled for
- * removal.
+ * Take devices from the dpm_off_list, resume them,
+ * and put them on the dpm_locked list.
*/
-
static void dpm_resume(void)
{
- mutex_lock(&dpm_list_mtx);
while(!list_empty(&dpm_off)) {
- struct list_head * entry = dpm_off.next;
- struct device * dev = to_device(entry);
-
- get_device(dev);
- list_move_tail(entry, &dpm_active);
-
- mutex_unlock(&dpm_list_mtx);
- resume_device(dev);
- mutex_lock(&dpm_list_mtx);
- put_device(dev);
- }
- mutex_unlock(&dpm_list_mtx);
- /* Unregister devices scheduled for removal */
- while (!list_empty(&dpm_destroy)) {
- struct list_head *entry = dpm_destroy.next;
+ struct list_head *entry = dpm_off.next;
struct device *dev = to_device(entry);
- device_unregister(dev);
+ resume_device(dev);
+ list_move_tail(entry, &dpm_locked);
}
}
/**
- * device_resume - Invoke dpm_resume() under dpm_mtx.
+ * unlock_all_devices - Release each device's semaphore
+ *
+ * Go through the dpm_off list. Put each device on the dpm_active
+ * list and unlock it.
*/
-
-void device_resume(void)
+static void unlock_all_devices(void)
{
- might_sleep();
- mutex_lock(&dpm_mtx);
- dpm_resume();
- mutex_unlock(&dpm_mtx);
+ mutex_lock(&dpm_list_mtx);
+ while (!list_empty(&dpm_locked)) {
+ struct list_head *entry = dpm_locked.prev;
+ struct device *dev = to_device(entry);
+
+ list_move(entry, &dpm_active);
+ up(&dev->sem);
+ }
+ mutex_unlock(&dpm_list_mtx);
}
-EXPORT_SYMBOL_GPL(device_resume);
-
-
/**
- * dpm_power_up - Power on some devices.
- *
- * Walk the dpm_off_irq list and power each device up. This
- * is used for devices that required they be powered down with
- * interrupts disabled. As devices are powered on, they are moved
- * to the dpm_active list.
- *
- * Interrupts must be disabled when calling this.
+ * unregister_dropped_devices - Unregister devices scheduled for removal
*/
-
-static void dpm_power_up(void)
+static void unregister_dropped_devices(void)
{
- while(!list_empty(&dpm_off_irq)) {
- struct list_head * entry = dpm_off_irq.next;
- struct device * dev = to_device(entry);
+ while (!list_empty(&dpm_destroy)) {
+ struct list_head *entry = dpm_destroy.next;
+ struct device *dev = to_device(entry);
- list_move_tail(entry, &dpm_off);
- resume_device_early(dev);
+ up(&dev->sem);
+ device_unregister(dev);
}
}
-
/**
- * device_power_up - Turn on all devices that need special attention.
+ * device_resume - Restore state of each device in system.
*
- * Power on system devices then devices that required we shut them down
- * with interrupts disabled.
- * Called with interrupts disabled.
+ * Resume all the devices, unlock them all, and allow new
+ * devices to be registered once again.
*/
-
-void device_power_up(void)
+void device_resume(void)
{
- sysdev_resume();
- dpm_power_up();
+ might_sleep();
+ dpm_resume();
+ unlock_all_devices();
+ unregister_dropped_devices();
+ up_write(&pm_sleep_rwsem);
}
-
-EXPORT_SYMBOL_GPL(device_power_up);
+EXPORT_SYMBOL_GPL(device_resume);
/*------------------------- Suspend routines -------------------------*/
-/*
- * The entries in the dpm_active list are in a depth first order, simply
- * because children are guaranteed to be discovered after parents, and
- * are inserted at the back of the list on discovery.
- *
- * All list on the suspend path are done in reverse order, so we operate
- * on the leaves of the device tree (or forests, depending on how you want
- * to look at it ;) first. As nodes are removed from the back of the list,
- * they are inserted into the front of their destintation lists.
- *
- * Things are the reverse on the resume path - iterations are done in
- * forward order, and nodes are inserted at the back of their destination
- * lists. This way, the ancestors will be accessed before their descendents.
- */
-
static inline char *suspend_verb(u32 event)
{
switch (event) {
@@ -239,7 +301,6 @@ static inline char *suspend_verb(u32 eve
}
}
-
static void
suspend_device_dbg(struct device *dev, pm_message_t state, char *info)
{
@@ -249,16 +310,69 @@ suspend_device_dbg(struct device *dev, p
}
/**
- * suspend_device - Save state of one device.
+ * suspend_device_late - Shut down one device (late suspend).
* @dev: Device.
* @state: Power state device is entering.
+ *
+ * This is called with interrupts off and only a single CPU running.
+ */
+static int suspend_device_late(struct device *dev, pm_message_t state)
+{
+ int error = 0;
+
+ if (dev->bus && dev->bus->suspend_late) {
+ suspend_device_dbg(dev, state, "LATE ");
+ error = dev->bus->suspend_late(dev, state);
+ suspend_report_result(dev->bus->suspend_late, error);
+ }
+ return error;
+}
+
+/**
+ * device_power_down - Shut down special devices.
+ * @state: Power state to enter.
+ *
+ * Power down devices that require interrupts to be disabled
+ * and move them from the dpm_off list to the dpm_off_irq list.
+ * Then power down system devices.
+ *
+ * Must be called with interrupts disabled and only one CPU running.
*/
+int device_power_down(pm_message_t state)
+{
+ int error = 0;
+
+ while (!list_empty(&dpm_off)) {
+ struct list_head *entry = dpm_off.prev;
+ struct device *dev = to_device(entry);
-static int suspend_device(struct device * dev, pm_message_t state)
+ error = suspend_device_late(dev, state);
+ if (error) {
+ printk(KERN_ERR "Could not power down device %s: "
+ "error %d\n",
+ kobject_name(&dev->kobj), error);
+ break;
+ }
+ list_move(&dev->power.entry, &dpm_off_irq);
+ }
+
+ if (!error)
+ error = sysdev_suspend(state);
+ if (error)
+ dpm_power_up();
+ return error;
+}
+EXPORT_SYMBOL_GPL(device_power_down);
+
+/**
+ * suspend_device - Save state of one device.
+ * @dev: Device.
+ * @state: Power state device is entering.
+ */
+int suspend_device(struct device *dev, pm_message_t state)
{
int error = 0;
- down(&dev->sem);
if (dev->power.power_state.event) {
dev_dbg(dev, "PM: suspend %d-->%d\n",
dev->power.power_state.event, state.event);
@@ -281,123 +395,95 @@ static int suspend_device(struct device
error = dev->bus->suspend(dev, state);
suspend_report_result(dev->bus->suspend, error);
}
- up(&dev->sem);
return error;
}
-
-/*
- * This is called with interrupts off, only a single CPU
- * running. We can't acquire a mutex or semaphore (and we don't
- * need the protection)
+/**
+ * dpm_suspend - Suspend every device.
+ * @state: Power state to put each device in.
+ *
+ * Walk the dpm_locked list. Suspend each device and move it
+ * to the dpm_off list.
+ *
+ * (For historical reasons, if it returns -EAGAIN, that used to mean
+ * that the device would be called again with interrupts disabled.
+ * These days, we use the "suspend_late()" callback for that, so we
+ * print a warning and consider it an error).
*/
-static int suspend_device_late(struct device *dev, pm_message_t state)
+static int dpm_suspend(pm_message_t state)
{
int error = 0;
- if (dev->bus && dev->bus->suspend_late) {
- suspend_device_dbg(dev, state, "LATE ");
- error = dev->bus->suspend_late(dev, state);
- suspend_report_result(dev->bus->suspend_late, error);
+ while (!list_empty(&dpm_locked)) {
+ struct list_head *entry = dpm_locked.prev;
+ struct device *dev = to_device(entry);
+
+ error = suspend_device(dev, state);
+ if (error) {
+ printk(KERN_ERR "Could not suspend device %s: "
+ "error %d%s\n",
+ kobject_name(&dev->kobj),
+ error,
+ (error == -EAGAIN ?
+ " (please convert to suspend_late)" :
+ ""));
+ break;
+ }
+ list_move(&dev->power.entry, &dpm_off);
}
+
return error;
}
/**
- * device_suspend - Save state and stop all devices in system.
- * @state: Power state to put each device in.
- *
- * Walk the dpm_active list, call ->suspend() for each device, and move
- * it to the dpm_off list.
- *
- * (For historical reasons, if it returns -EAGAIN, that used to mean
- * that the device would be called again with interrupts disabled.
- * These days, we use the "suspend_late()" callback for that, so we
- * print a warning and consider it an error).
- *
- * If we get a different error, try and back out.
- *
- * If we hit a failure with any of the devices, call device_resume()
- * above to bring the suspended devices back to life.
+ * lock_all_devices - Acquire every device's semaphore
*
+ * Go through the dpm_active list. Carefully lock each device's
+ * semaphore and put it in on the dpm_locked list.
*/
-
-int device_suspend(pm_message_t state)
+static void lock_all_devices(void)
{
- int error = 0;
-
- might_sleep();
- mutex_lock(&dpm_mtx);
mutex_lock(&dpm_list_mtx);
- while (!list_empty(&dpm_active) && error == 0) {
- struct list_head * entry = dpm_active.prev;
- struct device * dev = to_device(entry);
+ while (!list_empty(&dpm_active)) {
+ struct list_head *entry = dpm_active.next;
+ struct device *dev = to_device(entry);
+ /* Required locking order is dev->sem first,
+ * then dpm_list_mutex. Hence this awkward code.
+ */
get_device(dev);
mutex_unlock(&dpm_list_mtx);
-
- error = suspend_device(dev, state);
-
+ down(&dev->sem);
mutex_lock(&dpm_list_mtx);
- /* Check if the device got removed */
- if (!list_empty(&dev->power.entry)) {
- /* Move it to the dpm_off list */
- if (!error)
- list_move(&dev->power.entry, &dpm_off);
- }
- if (error)
- printk(KERN_ERR "Could not suspend device %s: "
- "error %d%s\n",
- kobject_name(&dev->kobj), error,
- error == -EAGAIN ? " (please convert to suspend_late)" : "");
+ if (list_empty(entry))
+ up(&dev->sem); /* Device was removed */
+ else
+ list_move_tail(entry, &dpm_locked);
put_device(dev);
}
mutex_unlock(&dpm_list_mtx);
- if (error)
- dpm_resume();
-
- mutex_unlock(&dpm_mtx);
- return error;
}
-EXPORT_SYMBOL_GPL(device_suspend);
-
/**
- * device_power_down - Shut down special devices.
- * @state: Power state to enter.
+ * device_suspend - Save state and stop all devices in system.
*
- * Walk the dpm_off_irq list, calling ->power_down() for each device that
- * couldn't power down the device with interrupts enabled. When we're
- * done, power down system devices.
+ * Prevent new devices from being registered, then lock all devices
+ * and suspend them.
*/
-
-int device_power_down(pm_message_t state)
+int device_suspend(pm_message_t state)
{
- int error = 0;
- struct device * dev;
-
- while (!list_empty(&dpm_off)) {
- struct list_head * entry = dpm_off.prev;
-
- dev = to_device(entry);
- error = suspend_device_late(dev, state);
- if (error)
- goto Error;
- list_move(&dev->power.entry, &dpm_off_irq);
- }
+ int error;
- error = sysdev_suspend(state);
- Done:
+ might_sleep();
+ down_write(&pm_sleep_rwsem);
+ lock_all_devices();
+ error = dpm_suspend(state);
+ if (error)
+ device_resume();
return error;
- Error:
- printk(KERN_ERR "Could not power down device %s: "
- "error %d\n", kobject_name(&dev->kobj), error);
- dpm_power_up();
- goto Done;
}
-
-EXPORT_SYMBOL_GPL(device_power_down);
+EXPORT_SYMBOL_GPL(device_suspend);
void __suspend_report_result(const char *function, void *fn, int ret)
{
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ 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
On Friday, 4 of January 2008, Rafael J. Wysocki wrote:
> On Wednesday, 2 of January 2008, Rafael J. Wysocki wrote:
> > On Wednesday, 2 of January 2008, Alan Stern wrote:
> > > On Wed, 2 Jan 2008, Rafael J. Wysocki wrote:
> > >
> > > > On Wednesday, 2 of January 2008, Rafael J. Wysocki wrote:
> > > > > 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]>
> > > >
> > > > 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?
> >
> > 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.
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]>
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.
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(-)
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ 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
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ 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
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ 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
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -24,17 +24,38 @@
#include <linux/mutex.h>
#include <linux/pm.h>
#include <linux/resume-trace.h>
+#include <linux/rwsem.h>
#include "../base.h"
#include "power.h"
+/*
+ * The entries in the dpm_active list are in a depth first order, simply
+ * because children are guaranteed to be discovered after parents, and
+ * are inserted at the back of the list on discovery.
+ *
+ * All the other lists are kept in the same order, for consistency.
+ * However the lists aren't always traversed in the same order.
+ * Semaphores must be acquired from the top (i.e., front) down
+ * and released in the opposite order. Devices must be suspended
+ * from the bottom (i.e., end) up and resumed in the opposite order.
+ * That way no parent will be suspended while it still has an active
+ * child.
+ *
+ * Since device_pm_add() may be called with a device semaphore held,
+ * we must never try to acquire a device semaphore while holding
+ * dpm_list_mutex.
+ */
+
LIST_HEAD(dpm_active);
+static LIST_HEAD(dpm_locked);
static LIST_HEAD(dpm_off);
static LIST_HEAD(dpm_off_irq);
-static DEFINE_MUTEX(dpm_mtx);
static DEFINE_MUTEX(dpm_list_mtx);
+static DECLARE_RWSEM(pm_sleep_rwsem);
+
int (*platform_enable_wakeup)(struct device *dev, int is_on);
@@ -53,29 +74,124 @@ void device_pm_remove(struct device *dev
pr_debug("PM: Removing info for %s:%s\n",
dev->bus ? dev->bus->name : "No Bus",
kobject_name(&dev->kobj));
+
+ /* Don't remove a device while the PM core has it locked for suspend */
+ down(&dev->sem);
mutex_lock(&dpm_list_mtx);
dpm_sysfs_remove(dev);
list_del_init(&dev->power.entry);
mutex_unlock(&dpm_list_mtx);
+ up(&dev->sem);
+}
+
+void device_pm_destroy_suspended(struct device *dev)
+{
+ pr_debug("PM: Removing suspended device %s:%s\n",
+ dev->bus ? dev->bus->name : "No Bus",
+ kobject_name(&dev->kobj));
+ mutex_lock(&dpm_list_mtx);
+ list_del_init(&dev->power.entry);
+ mutex_unlock(&dpm_list_mtx);
+ up(&dev->sem);
+ device_unregister(dev);
+}
+
+/**
+ * pm_sleep_lock - mutual exclusion for registration and suspend
+ *
+ * Returns 0 if no suspend is underway and device registration
+ * may proceed, otherwise -EBUSY.
+ */
+int pm_sleep_lock(void)
+{
+ if (down_read_trylock(&pm_sleep_rwsem))
+ return 0;
+ return -EBUSY;
+}
+
+/**
+ * pm_sleep_unlock - mutual exclusion for registration and suspend
+ *
+ * This routine undoes the effect of device_pm_add_lock
+ * when a device's registration is complete.
+ */
+void pm_sleep_unlock(void)
+{
+ up_read(&pm_sleep_rwsem);
}
/*------------------------- Resume routines -------------------------*/
/**
- * resume_device - Restore state for one device.
+ * resume_device_early - Power on one device (early resume).
* @dev: Device.
*
+ * Must be called with interrupts disabled.
*/
-
-static int resume_device(struct device * dev)
+static int resume_device_early(struct device *dev)
{
int error = 0;
TRACE_DEVICE(dev);
TRACE_RESUME(0);
- down(&dev->sem);
+ if (dev->bus && dev->bus->resume_early) {
+ dev_dbg(dev,"EARLY resume\n");
+ error = dev->bus->resume_early(dev);
+ }
+
+ TRACE_RESUME(error);
+ return error;
+}
+
+/**
+ * dpm_power_up - Power on all regular (non-sysdev) devices.
+ *
+ * Walk the dpm_off_irq list and power each device up. This
+ * is used for devices that required they be powered down with
+ * interrupts disabled. As devices are powered on, they are moved
+ * to the dpm_off list.
+ *
+ * Interrupts must be disabled when calling this.
+ */
+static void dpm_power_up(void)
+{
+ while (!list_empty(&dpm_off_irq)) {
+ struct list_head *entry = dpm_off_irq.next;
+ struct device *dev = to_device(entry);
+
+ resume_device_early(dev);
+ list_move_tail(entry, &dpm_off);
+ }
+}
+
+/**
+ * device_power_up - Turn on all devices that need special attention.
+ *
+ * Power on system devices, then devices that required we shut them down
+ * with interrupts disabled.
+ *
+ * Must be called with interrupts disabled.
+ */
+void device_power_up(void)
+{
+ sysdev_resume();
+ dpm_power_up();
+}
+EXPORT_SYMBOL_GPL(device_power_up);
+
+/**
+ * resume_device - Restore state for one device.
+ * @dev: Device.
+ *
+ */
+static int resume_device(struct device *dev)
+{
+ int error = 0;
+
+ TRACE_DEVICE(dev);
+ TRACE_RESUME(0);
if (dev->bus && dev->bus->resume) {
dev_dbg(dev,"resuming\n");
@@ -92,126 +208,68 @@ static int resume_device(struct device *
error = dev->class->resume(dev);
}
- up(&dev->sem);
-
TRACE_RESUME(error);
return error;
}
-
-static int resume_device_early(struct device * dev)
+/**
+ * dpm_resume - Resume every device.
+ *
+ * Resume the devices that have either not gone through
+ * the late suspend, or that did go through it but also
+ * went through the early resume.
+ *
+ * Take devices from the dpm_off_list, resume them,
+ * and put them on the dpm_locked list.
+ */
+static void dpm_resume(void)
{
- int error = 0;
+ while(!list_empty(&dpm_off)) {
+ struct list_head *entry = dpm_off.next;
+ struct device *dev = to_device(entry);
- TRACE_DEVICE(dev);
- TRACE_RESUME(0);
- if (dev->bus && dev->bus->resume_early) {
- dev_dbg(dev,"EARLY resume\n");
- error = dev->bus->resume_early(dev);
+ resume_device(dev);
+ list_move_tail(entry, &dpm_locked);
}
- TRACE_RESUME(error);
- return error;
}
-/*
- * Resume the devices that have either not gone through
- * the late suspend, or that did go through it but also
- * went through the early resume
+/**
+ * unlock_all_devices - Release each device's semaphore
+ *
+ * Go through the dpm_off list. Put each device on the dpm_active
+ * list and unlock it.
*/
-static void dpm_resume(void)
+static void unlock_all_devices(void)
{
mutex_lock(&dpm_list_mtx);
- while(!list_empty(&dpm_off)) {
- struct list_head * entry = dpm_off.next;
- struct device * dev = to_device(entry);
-
- get_device(dev);
- list_move_tail(entry, &dpm_active);
-
- mutex_unlock(&dpm_list_mtx);
- resume_device(dev);
- mutex_lock(&dpm_list_mtx);
- put_device(dev);
- }
+ while (!list_empty(&dpm_locked)) {
+ struct list_head *entry = dpm_locked.prev;
+ struct device *dev = to_device(entry);
+
+ list_move(entry, &dpm_active);
+ up(&dev->sem);
+ }
mutex_unlock(&dpm_list_mtx);
}
-
/**
* device_resume - Restore state of each device in system.
*
- * Walk the dpm_off list, remove each entry, resume the device,
- * then add it to the dpm_active list.
+ * Resume all the devices, unlock them all, and allow new
+ * devices to be registered once again.
*/
-
void device_resume(void)
{
might_sleep();
- mutex_lock(&dpm_mtx);
dpm_resume();
- mutex_unlock(&dpm_mtx);
+ unlock_all_devices();
+ up_write(&pm_sleep_rwsem);
}
-
EXPORT_SYMBOL_GPL(device_resume);
-/**
- * dpm_power_up - Power on some devices.
- *
- * Walk the dpm_off_irq list and power each device up. This
- * is used for devices that required they be powered down with
- * interrupts disabled. As devices are powered on, they are moved
- * to the dpm_active list.
- *
- * Interrupts must be disabled when calling this.
- */
-
-static void dpm_power_up(void)
-{
- while(!list_empty(&dpm_off_irq)) {
- struct list_head * entry = dpm_off_irq.next;
- struct device * dev = to_device(entry);
-
- list_move_tail(entry, &dpm_off);
- resume_device_early(dev);
- }
-}
-
-
-/**
- * device_power_up - Turn on all devices that need special attention.
- *
- * Power on system devices then devices that required we shut them down
- * with interrupts disabled.
- * Called with interrupts disabled.
- */
-
-void device_power_up(void)
-{
- sysdev_resume();
- dpm_power_up();
-}
-
-EXPORT_SYMBOL_GPL(device_power_up);
-
-
/*------------------------- Suspend routines -------------------------*/
-/*
- * The entries in the dpm_active list are in a depth first order, simply
- * because children are guaranteed to be discovered after parents, and
- * are inserted at the back of the list on discovery.
- *
- * All list on the suspend path are done in reverse order, so we operate
- * on the leaves of the device tree (or forests, depending on how you want
- * to look at it ;) first. As nodes are removed from the back of the list,
- * they are inserted into the front of their destintation lists.
- *
- * Things are the reverse on the resume path - iterations are done in
- * forward order, and nodes are inserted at the back of their destination
- * lists. This way, the ancestors will be accessed before their descendents.
- */
-
static inline char *suspend_verb(u32 event)
{
switch (event) {
@@ -222,7 +280,6 @@ static inline char *suspend_verb(u32 eve
}
}
-
static void
suspend_device_dbg(struct device *dev, pm_message_t state, char *info)
{
@@ -232,16 +289,69 @@ suspend_device_dbg(struct device *dev, p
}
/**
- * suspend_device - Save state of one device.
+ * suspend_device_late - Shut down one device (late suspend).
* @dev: Device.
* @state: Power state device is entering.
+ *
+ * This is called with interrupts off and only a single CPU running.
*/
+static int suspend_device_late(struct device *dev, pm_message_t state)
+{
+ int error = 0;
+
+ if (dev->bus && dev->bus->suspend_late) {
+ suspend_device_dbg(dev, state, "LATE ");
+ error = dev->bus->suspend_late(dev, state);
+ suspend_report_result(dev->bus->suspend_late, error);
+ }
+ return error;
+}
-static int suspend_device(struct device * dev, pm_message_t state)
+/**
+ * device_power_down - Shut down special devices.
+ * @state: Power state to enter.
+ *
+ * Power down devices that require interrupts to be disabled
+ * and move them from the dpm_off list to the dpm_off_irq list.
+ * Then power down system devices.
+ *
+ * Must be called with interrupts disabled and only one CPU running.
+ */
+int device_power_down(pm_message_t state)
+{
+ int error = 0;
+
+ while (!list_empty(&dpm_off)) {
+ struct list_head *entry = dpm_off.prev;
+ struct device *dev = to_device(entry);
+
+ error = suspend_device_late(dev, state);
+ if (error) {
+ printk(KERN_ERR "Could not power down device %s: "
+ "error %d\n",
+ kobject_name(&dev->kobj), error);
+ break;
+ }
+ list_move(&dev->power.entry, &dpm_off_irq);
+ }
+
+ if (!error)
+ error = sysdev_suspend(state);
+ if (error)
+ dpm_power_up();
+ return error;
+}
+EXPORT_SYMBOL_GPL(device_power_down);
+
+/**
+ * suspend_device - Save state of one device.
+ * @dev: Device.
+ * @state: Power state device is entering.
+ */
+int suspend_device(struct device *dev, pm_message_t state)
{
int error = 0;
- down(&dev->sem);
if (dev->power.power_state.event) {
dev_dbg(dev, "PM: suspend %d-->%d\n",
dev->power.power_state.event, state.event);
@@ -264,123 +374,95 @@ static int suspend_device(struct device
error = dev->bus->suspend(dev, state);
suspend_report_result(dev->bus->suspend, error);
}
- up(&dev->sem);
return error;
}
-
-/*
- * This is called with interrupts off, only a single CPU
- * running. We can't acquire a mutex or semaphore (and we don't
- * need the protection)
+/**
+ * dpm_suspend - Suspend every device.
+ * @state: Power state to put each device in.
+ *
+ * Walk the dpm_locked list. Suspend each device and move it
+ * to the dpm_off list.
+ *
+ * (For historical reasons, if it returns -EAGAIN, that used to mean
+ * that the device would be called again with interrupts disabled.
+ * These days, we use the "suspend_late()" callback for that, so we
+ * print a warning and consider it an error).
*/
-static int suspend_device_late(struct device *dev, pm_message_t state)
+static int dpm_suspend(pm_message_t state)
{
int error = 0;
- if (dev->bus && dev->bus->suspend_late) {
- suspend_device_dbg(dev, state, "LATE ");
- error = dev->bus->suspend_late(dev, state);
- suspend_report_result(dev->bus->suspend_late, error);
+ while (!list_empty(&dpm_locked)) {
+ struct list_head *entry = dpm_locked.prev;
+ struct device *dev = to_device(entry);
+
+ error = suspend_device(dev, state);
+ if (error) {
+ printk(KERN_ERR "Could not suspend device %s: "
+ "error %d%s\n",
+ kobject_name(&dev->kobj),
+ error,
+ (error == -EAGAIN ?
+ " (please convert to suspend_late)" :
+ ""));
+ break;
+ }
+ list_move(&dev->power.entry, &dpm_off);
}
+
return error;
}
/**
- * device_suspend - Save state and stop all devices in system.
- * @state: Power state to put each device in.
- *
- * Walk the dpm_active list, call ->suspend() for each device, and move
- * it to the dpm_off list.
- *
- * (For historical reasons, if it returns -EAGAIN, that used to mean
- * that the device would be called again with interrupts disabled.
- * These days, we use the "suspend_late()" callback for that, so we
- * print a warning and consider it an error).
- *
- * If we get a different error, try and back out.
- *
- * If we hit a failure with any of the devices, call device_resume()
- * above to bring the suspended devices back to life.
+ * lock_all_devices - Acquire every device's semaphore
*
+ * Go through the dpm_active list. Carefully lock each device's
+ * semaphore and put it in on the dpm_locked list.
*/
-
-int device_suspend(pm_message_t state)
+static void lock_all_devices(void)
{
- int error = 0;
-
- might_sleep();
- mutex_lock(&dpm_mtx);
mutex_lock(&dpm_list_mtx);
- while (!list_empty(&dpm_active) && error == 0) {
- struct list_head * entry = dpm_active.prev;
- struct device * dev = to_device(entry);
-
+ while (!list_empty(&dpm_active)) {
+ struct list_head *entry = dpm_active.next;
+ struct device *dev = to_device(entry);
+
+ /* Required locking order is dev->sem first,
+ * then dpm_list_mutex. Hence this awkward code.
+ */
get_device(dev);
mutex_unlock(&dpm_list_mtx);
-
- error = suspend_device(dev, state);
-
+ down(&dev->sem);
mutex_lock(&dpm_list_mtx);
- /* Check if the device got removed */
- if (!list_empty(&dev->power.entry)) {
- /* Move it to the dpm_off list */
- if (!error)
- list_move(&dev->power.entry, &dpm_off);
- }
- if (error)
- printk(KERN_ERR "Could not suspend device %s: "
- "error %d%s\n",
- kobject_name(&dev->kobj), error,
- error == -EAGAIN ? " (please convert to suspend_late)" : "");
+ if (list_empty(entry))
+ up(&dev->sem); /* Device was removed */
+ else
+ list_move_tail(entry, &dpm_locked);
put_device(dev);
}
mutex_unlock(&dpm_list_mtx);
- if (error)
- dpm_resume();
-
- mutex_unlock(&dpm_mtx);
- return error;
}
-EXPORT_SYMBOL_GPL(device_suspend);
-
/**
- * device_power_down - Shut down special devices.
- * @state: Power state to enter.
+ * device_suspend - Save state and stop all devices in system.
*
- * Walk the dpm_off_irq list, calling ->power_down() for each device that
- * couldn't power down the device with interrupts enabled. When we're
- * done, power down system devices.
+ * Prevent new devices from being registered, then lock all devices
+ * and suspend them.
*/
-
-int device_power_down(pm_message_t state)
+int device_suspend(pm_message_t state)
{
- int error = 0;
- struct device * dev;
-
- while (!list_empty(&dpm_off)) {
- struct list_head * entry = dpm_off.prev;
-
- dev = to_device(entry);
- error = suspend_device_late(dev, state);
- if (error)
- goto Error;
- list_move(&dev->power.entry, &dpm_off_irq);
- }
+ int error;
- error = sysdev_suspend(state);
- Done:
+ might_sleep();
+ down_write(&pm_sleep_rwsem);
+ lock_all_devices();
+ error = dpm_suspend(state);
+ if (error)
+ device_resume();
return error;
- Error:
- printk(KERN_ERR "Could not power down device %s: "
- "error %d\n", kobject_name(&dev->kobj), error);
- dpm_power_up();
- goto Done;
}
-
-EXPORT_SYMBOL_GPL(device_power_down);
+EXPORT_SYMBOL_GPL(device_suspend);
void __suspend_report_result(const char *function, void *fn, int ret)
{
On Fri, 4 Jan 2008, Rafael J. Wysocki wrote:
> 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
> +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
On Saturday, 5 of January 2008, Alan Stern wrote:
> On Fri, 4 Jan 2008, Rafael J. Wysocki wrote:
>
> > 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
Makes sense, I will.
> > +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.
In fact it already works like this, since device_suspend() now calls the entire
device_resume() on error.
> 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
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.
>
> 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)
On Fri, 11 Jan 2008 16:46:13 -0800
Andrew Morton <[email protected]> wrote:
> > 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)
err, no. pm-introduce-destroy_suspended_device.patch demolishes
pm-acquire-device-locks-on-suspend-rev-3.patch
Confused, giving up.
On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
> On Fri, 11 Jan 2008 16:46:13 -0800
> Andrew Morton <[email protected]> wrote:
>
> > > 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)
>
> 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
On Fri, 11 Jan 2008, Greg KH wrote:
> On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
> > 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
> 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
On Fri, 11 Jan 2008 22:11:52 -0500 (EST) Alan Stern <[email protected]> wrote:
> On Fri, 11 Jan 2008, Greg KH wrote:
>
> > On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
>
> > > 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?
>
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.
On Fri, Jan 11, 2008 at 10:11:52PM -0500, Alan Stern wrote:
> On Fri, 11 Jan 2008, Greg KH wrote:
>
> > On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
>
> > > 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.
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?
> 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
On Saturday, 12 of January 2008, Andrew Morton wrote:
> On Fri, 11 Jan 2008 16:46:13 -0800
> Andrew Morton <[email protected]> wrote:
>
> > > 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)
>
> 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.
On Saturday, 12 of January 2008, Greg KH wrote:
> On Fri, Jan 11, 2008 at 10:11:52PM -0500, Alan Stern wrote:
> > On Fri, 11 Jan 2008, Greg KH wrote:
> >
> > > On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
> >
> > > > 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.
>
> 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?
>
> > 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