Hi Alan,
Some time ago Linus suggested that we might use a separate list of devices
at each stage of device suspend/resume and you seemed to like the idea.
The following patches implement that and remove some code that's not necessary
any more in the wake of this change.
Please let me know what you think.
Thanks,
Rafael
From: Rafael J. Wysocki <[email protected]>
The device power.status field is too complicated for its purpose
(storing the information about whether or not the device is in the
"active" state from the PM core's point of view), so replace it with
a bit field and modify all of its users accordingly.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/main.c | 17 ++++-------------
drivers/usb/core/driver.c | 7 +++----
include/linux/device.h | 4 ++--
include/linux/pm.h | 43 ++-----------------------------------------
4 files changed, 11 insertions(+), 60 deletions(-)
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ linux-2.6/include/linux/device.h
@@ -508,13 +508,13 @@ static inline int device_is_registered(s
static inline void device_enable_async_suspend(struct device *dev)
{
- if (dev->power.status == DPM_ON)
+ if (!dev->power.in_suspend)
dev->power.async_suspend = true;
}
static inline void device_disable_async_suspend(struct device *dev)
{
- if (dev->power.status == DPM_ON)
+ if (!dev->power.in_suspend)
dev->power.async_suspend = false;
}
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -367,45 +367,6 @@ extern struct dev_pm_ops generic_subsys_
{ .event = PM_EVENT_AUTO_RESUME, })
/**
- * Device power management states
- *
- * These state labels are used internally by the PM core to indicate the current
- * status of a device with respect to the PM core operations.
- *
- * DPM_ON Device is regarded as operational. Set this way
- * initially and when ->complete() is about to be called.
- * Also set when ->prepare() fails.
- *
- * DPM_PREPARING Device is going to be prepared for a PM transition. Set
- * when ->prepare() is about to be called.
- *
- * DPM_RESUMING Device is going to be resumed. Set when ->resume(),
- * ->thaw(), or ->restore() is about to be called.
- *
- * DPM_SUSPENDING Device has been prepared for a power transition. Set
- * when ->prepare() has just succeeded.
- *
- * DPM_OFF Device is regarded as inactive. Set immediately after
- * ->suspend(), ->freeze(), or ->poweroff() has succeeded.
- * Also set when ->resume()_noirq, ->thaw_noirq(), or
- * ->restore_noirq() is about to be called.
- *
- * DPM_OFF_IRQ Device is in a "deep sleep". Set immediately after
- * ->suspend_noirq(), ->freeze_noirq(), or
- * ->poweroff_noirq() has just succeeded.
- */
-
-enum dpm_state {
- DPM_INVALID,
- DPM_ON,
- DPM_PREPARING,
- DPM_RESUMING,
- DPM_SUSPENDING,
- DPM_OFF,
- DPM_OFF_IRQ,
-};
-
-/**
* Device run-time power management status.
*
* These status labels are used internally by the PM core to indicate the
@@ -463,8 +424,8 @@ struct wakeup_source;
struct dev_pm_info {
pm_message_t power_state;
unsigned int can_wakeup:1;
- unsigned async_suspend:1;
- enum dpm_state status; /* Owned by the PM core */
+ unsigned int async_suspend:1;
+ unsigned int in_suspend:1; /* Owned by the PM core */
spinlock_t lock;
#ifdef CONFIG_PM_SLEEP
struct list_head entry;
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
@@ -63,7 +63,7 @@ static int async_error;
*/
void device_pm_init(struct device *dev)
{
- dev->power.status = DPM_ON;
+ dev->power.in_suspend = false;
init_completion(&dev->power.completion);
complete_all(&dev->power.completion);
dev->power.wakeup = NULL;
@@ -98,7 +98,7 @@ void device_pm_add(struct device *dev)
kobject_name(&dev->kobj));
mutex_lock(&dpm_list_mtx);
if (dev->parent) {
- if (dev->parent->power.status >= DPM_SUSPENDING)
+ if (dev->parent->power.in_suspend)
dev_warn(dev, "parent %s should not be sleeping\n",
dev_name(dev->parent));
} else if (transition_started) {
@@ -488,7 +488,6 @@ void dpm_resume_noirq(pm_message_t state
int error;
get_device(dev);
- dev->power.status = DPM_OFF;
mutex_unlock(&dpm_list_mtx);
error = device_resume_noirq(dev, state);
@@ -542,8 +541,6 @@ static int device_resume(struct device *
dpm_wait(dev->parent, async);
device_lock(dev);
- dev->power.status = DPM_RESUMING;
-
if (dev->bus) {
if (dev->bus->pm) {
pm_dev_dbg(dev, state, "");
@@ -691,7 +688,7 @@ static void dpm_complete(pm_message_t st
struct device *dev = to_device(dpm_prepared_list.prev);
get_device(dev);
- dev->power.status = DPM_ON;
+ dev->power.in_suspend = false;
mutex_unlock(&dpm_list_mtx);
device_complete(dev, state);
@@ -808,7 +805,6 @@ int dpm_suspend_noirq(pm_message_t state
put_device(dev);
break;
}
- dev->power.status = DPM_OFF_IRQ;
if (!list_empty(&dev->power.entry))
list_move(&dev->power.entry, &dpm_noirq_list);
put_device(dev);
@@ -896,9 +892,6 @@ static int __device_suspend(struct devic
}
}
- if (!error)
- dev->power.status = DPM_OFF;
-
End:
device_unlock(dev);
complete_all(&dev->power.completion);
@@ -1032,7 +1025,6 @@ static int dpm_prepare(pm_message_t stat
struct device *dev = to_device(dpm_list.next);
get_device(dev);
- dev->power.status = DPM_PREPARING;
mutex_unlock(&dpm_list_mtx);
pm_runtime_get_noresume(dev);
@@ -1048,7 +1040,6 @@ static int dpm_prepare(pm_message_t stat
mutex_lock(&dpm_list_mtx);
if (error) {
- dev->power.status = DPM_ON;
if (error == -EAGAIN) {
put_device(dev);
error = 0;
@@ -1060,7 +1051,7 @@ static int dpm_prepare(pm_message_t stat
put_device(dev);
break;
}
- dev->power.status = DPM_SUSPENDING;
+ dev->power.in_suspend = true;
if (!list_empty(&dev->power.entry))
list_move_tail(&dev->power.entry, &dpm_prepared_list);
put_device(dev);
Index: linux-2.6/drivers/usb/core/driver.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/driver.c
+++ linux-2.6/drivers/usb/core/driver.c
@@ -376,7 +376,7 @@ static int usb_unbind_interface(struct d
* Just re-enable it without affecting the endpoint toggles.
*/
usb_enable_interface(udev, intf, false);
- } else if (!error && intf->dev.power.status == DPM_ON) {
+ } else if (!error && !intf->dev.power.in_suspend) {
r = usb_set_interface(udev, intf->altsetting[0].
desc.bInterfaceNumber, 0);
if (r < 0)
@@ -961,7 +961,7 @@ void usb_rebind_intf(struct usb_interfac
}
/* Try to rebind the interface */
- if (intf->dev.power.status == DPM_ON) {
+ if (!intf->dev.power.in_suspend) {
intf->needs_binding = 0;
rc = device_attach(&intf->dev);
if (rc < 0)
@@ -1108,8 +1108,7 @@ static int usb_resume_interface(struct u
if (intf->condition == USB_INTERFACE_UNBOUND) {
/* Carry out a deferred switch to altsetting 0 */
- if (intf->needs_altsetting0 &&
- intf->dev.power.status == DPM_ON) {
+ if (intf->needs_altsetting0 && !intf->dev.power.in_suspend) {
usb_set_interface(udev, intf->altsetting[0].
desc.bInterfaceNumber, 0);
intf->needs_altsetting0 = 0;
From: Rafael J. Wysocki <[email protected]>
Instead of keeping all devices in the same list during system suspend
and resume, regardless of what suspend-resume callbacks have been
executed for them already, use separate lists of devices that have
had their ->prepare(), ->suspend() and ->suspend_noirq() callbacks
executed. This will allow us to simplify the core device suspend and
resume routines.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/main.c | 53 ++++++++++++++++------------------------------
1 file changed, 19 insertions(+), 34 deletions(-)
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
@@ -42,6 +42,9 @@
*/
LIST_HEAD(dpm_list);
+LIST_HEAD(dpm_prepared_list);
+LIST_HEAD(dpm_suspended_list);
+LIST_HEAD(dpm_noirq_list);
static DEFINE_MUTEX(dpm_list_mtx);
static pm_message_t pm_transition;
@@ -476,14 +479,12 @@ End:
*/
void dpm_resume_noirq(pm_message_t state)
{
- struct list_head list;
ktime_t starttime = ktime_get();
- INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
transition_started = false;
- while (!list_empty(&dpm_list)) {
- struct device *dev = to_device(dpm_list.next);
+ while (!list_empty(&dpm_noirq_list)) {
+ struct device *dev = to_device(dpm_noirq_list.next);
get_device(dev);
if (dev->power.status > DPM_OFF) {
@@ -499,10 +500,9 @@ void dpm_resume_noirq(pm_message_t state
pm_dev_err(dev, state, " early", error);
}
if (!list_empty(&dev->power.entry))
- list_move_tail(&dev->power.entry, &list);
+ list_move_tail(&dev->power.entry, &dpm_suspended_list);
put_device(dev);
}
- list_splice(&list, &dpm_list);
mutex_unlock(&dpm_list_mtx);
dpm_show_time(starttime, state, "early");
resume_device_irqs();
@@ -611,16 +611,14 @@ static bool is_async(struct device *dev)
*/
static void dpm_resume(pm_message_t state)
{
- struct list_head list;
struct device *dev;
ktime_t starttime = ktime_get();
- INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
pm_transition = state;
async_error = 0;
- list_for_each_entry(dev, &dpm_list, power.entry) {
+ list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
if (dev->power.status < DPM_OFF)
continue;
@@ -631,8 +629,8 @@ static void dpm_resume(pm_message_t stat
}
}
- while (!list_empty(&dpm_list)) {
- dev = to_device(dpm_list.next);
+ while (!list_empty(&dpm_suspended_list)) {
+ dev = to_device(dpm_suspended_list.next);
get_device(dev);
if (dev->power.status >= DPM_OFF && !is_async(dev)) {
int error;
@@ -644,15 +642,11 @@ static void dpm_resume(pm_message_t stat
mutex_lock(&dpm_list_mtx);
if (error)
pm_dev_err(dev, state, "", error);
- } else if (dev->power.status == DPM_SUSPENDING) {
- /* Allow new children of the device to be registered */
- dev->power.status = DPM_RESUMING;
}
if (!list_empty(&dev->power.entry))
- list_move_tail(&dev->power.entry, &list);
+ list_move_tail(&dev->power.entry, &dpm_prepared_list);
put_device(dev);
}
- list_splice(&list, &dpm_list);
mutex_unlock(&dpm_list_mtx);
async_synchronize_full();
dpm_show_time(starttime, state, NULL);
@@ -699,8 +693,8 @@ static void dpm_complete(pm_message_t st
INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
transition_started = false;
- while (!list_empty(&dpm_list)) {
- struct device *dev = to_device(dpm_list.prev);
+ while (!list_empty(&dpm_prepared_list)) {
+ struct device *dev = to_device(dpm_prepared_list.prev);
get_device(dev);
if (dev->power.status > DPM_ON) {
@@ -803,15 +797,13 @@ End:
*/
int dpm_suspend_noirq(pm_message_t state)
{
- struct list_head list;
ktime_t starttime = ktime_get();
int error = 0;
- INIT_LIST_HEAD(&list);
suspend_device_irqs();
mutex_lock(&dpm_list_mtx);
- while (!list_empty(&dpm_list)) {
- struct device *dev = to_device(dpm_list.prev);
+ while (!list_empty(&dpm_suspended_list)) {
+ struct device *dev = to_device(dpm_suspended_list.prev);
get_device(dev);
mutex_unlock(&dpm_list_mtx);
@@ -826,10 +818,9 @@ int dpm_suspend_noirq(pm_message_t state
}
dev->power.status = DPM_OFF_IRQ;
if (!list_empty(&dev->power.entry))
- list_move(&dev->power.entry, &list);
+ list_move(&dev->power.entry, &dpm_noirq_list);
put_device(dev);
}
- list_splice_tail(&list, &dpm_list);
mutex_unlock(&dpm_list_mtx);
if (error)
dpm_resume_noirq(resume_event(state));
@@ -957,16 +948,14 @@ static int device_suspend(struct device
*/
static int dpm_suspend(pm_message_t state)
{
- struct list_head list;
ktime_t starttime = ktime_get();
int error = 0;
- INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
pm_transition = state;
async_error = 0;
- while (!list_empty(&dpm_list)) {
- struct device *dev = to_device(dpm_list.prev);
+ while (!list_empty(&dpm_prepared_list)) {
+ struct device *dev = to_device(dpm_prepared_list.prev);
get_device(dev);
mutex_unlock(&dpm_list_mtx);
@@ -980,12 +969,11 @@ static int dpm_suspend(pm_message_t stat
break;
}
if (!list_empty(&dev->power.entry))
- list_move(&dev->power.entry, &list);
+ list_move(&dev->power.entry, &dpm_suspended_list);
put_device(dev);
if (async_error)
break;
}
- list_splice(&list, dpm_list.prev);
mutex_unlock(&dpm_list_mtx);
async_synchronize_full();
if (!error)
@@ -1044,10 +1032,8 @@ static int device_prepare(struct device
*/
static int dpm_prepare(pm_message_t state)
{
- struct list_head list;
int error = 0;
- INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
transition_started = true;
while (!list_empty(&dpm_list)) {
@@ -1084,10 +1070,9 @@ static int dpm_prepare(pm_message_t stat
}
dev->power.status = DPM_SUSPENDING;
if (!list_empty(&dev->power.entry))
- list_move_tail(&dev->power.entry, &list);
+ list_move_tail(&dev->power.entry, &dpm_prepared_list);
put_device(dev);
}
- list_splice(&list, &dpm_list);
mutex_unlock(&dpm_list_mtx);
return error;
}
From: Rafael J. Wysocki <[email protected]>
Since a separate list of devices is used to link devices that have
completed each stage of suspend (or resume), it is not necessary to
check dev->power.status in the core device resume routines any more.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/main.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
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
@@ -485,20 +485,17 @@ void dpm_resume_noirq(pm_message_t state
transition_started = false;
while (!list_empty(&dpm_noirq_list)) {
struct device *dev = to_device(dpm_noirq_list.next);
+ int error;
get_device(dev);
- if (dev->power.status > DPM_OFF) {
- int error;
-
- dev->power.status = DPM_OFF;
- mutex_unlock(&dpm_list_mtx);
+ dev->power.status = DPM_OFF;
+ mutex_unlock(&dpm_list_mtx);
- error = device_resume_noirq(dev, state);
+ error = device_resume_noirq(dev, state);
- mutex_lock(&dpm_list_mtx);
- if (error)
- pm_dev_err(dev, state, " early", error);
- }
+ mutex_lock(&dpm_list_mtx);
+ if (error)
+ pm_dev_err(dev, state, " early", error);
if (!list_empty(&dev->power.entry))
list_move_tail(&dev->power.entry, &dpm_suspended_list);
put_device(dev);
@@ -619,9 +616,6 @@ static void dpm_resume(pm_message_t stat
async_error = 0;
list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
- if (dev->power.status < DPM_OFF)
- continue;
-
INIT_COMPLETION(dev->power.completion);
if (is_async(dev)) {
get_device(dev);
@@ -632,7 +626,7 @@ static void dpm_resume(pm_message_t stat
while (!list_empty(&dpm_suspended_list)) {
dev = to_device(dpm_suspended_list.next);
get_device(dev);
- if (dev->power.status >= DPM_OFF && !is_async(dev)) {
+ if (!is_async(dev)) {
int error;
mutex_unlock(&dpm_list_mtx);
@@ -697,15 +691,13 @@ static void dpm_complete(pm_message_t st
struct device *dev = to_device(dpm_prepared_list.prev);
get_device(dev);
- if (dev->power.status > DPM_ON) {
- dev->power.status = DPM_ON;
- mutex_unlock(&dpm_list_mtx);
+ dev->power.status = DPM_ON;
+ mutex_unlock(&dpm_list_mtx);
- device_complete(dev, state);
- pm_runtime_put_sync(dev);
+ device_complete(dev, state);
+ pm_runtime_put_sync(dev);
- mutex_lock(&dpm_list_mtx);
- }
+ mutex_lock(&dpm_list_mtx);
if (!list_empty(&dev->power.entry))
list_move(&dev->power.entry, &list);
put_device(dev);
From: Rafael J. Wysocki <[email protected]>
The registration of a new parentless device during system suspend
will not lead to any complications affecting the PM core (the device
will be effectively seen after the subsequent resume has completed),
so remove the code used for detection of such events.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/main.c | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
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
@@ -49,12 +49,6 @@ LIST_HEAD(dpm_noirq_list);
static DEFINE_MUTEX(dpm_list_mtx);
static pm_message_t pm_transition;
-/*
- * Set once the preparation of devices for a PM transition has started, reset
- * before starting to resume devices. Protected by dpm_list_mtx.
- */
-static bool transition_started;
-
static int async_error;
/**
@@ -97,19 +91,9 @@ void device_pm_add(struct device *dev)
dev->bus ? dev->bus->name : "No Bus",
kobject_name(&dev->kobj));
mutex_lock(&dpm_list_mtx);
- if (dev->parent) {
- if (dev->parent->power.in_suspend)
- dev_warn(dev, "parent %s should not be sleeping\n",
- dev_name(dev->parent));
- } else if (transition_started) {
- /*
- * We refuse to register parentless devices while a PM
- * transition is in progress in order to avoid leaving them
- * unhandled down the road
- */
- dev_WARN(dev, "Parentless device registered during a PM transaction\n");
- }
-
+ if (dev->parent && dev->parent->power.in_suspend)
+ dev_warn(dev, "parent %s should not be sleeping\n",
+ dev_name(dev->parent));
list_add_tail(&dev->power.entry, &dpm_list);
mutex_unlock(&dpm_list_mtx);
}
@@ -482,7 +466,6 @@ void dpm_resume_noirq(pm_message_t state
ktime_t starttime = ktime_get();
mutex_lock(&dpm_list_mtx);
- transition_started = false;
while (!list_empty(&dpm_noirq_list)) {
struct device *dev = to_device(dpm_noirq_list.next);
int error;
@@ -683,7 +666,6 @@ static void dpm_complete(pm_message_t st
INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
- transition_started = false;
while (!list_empty(&dpm_prepared_list)) {
struct device *dev = to_device(dpm_prepared_list.prev);
@@ -1020,7 +1002,6 @@ static int dpm_prepare(pm_message_t stat
int error = 0;
mutex_lock(&dpm_list_mtx);
- transition_started = true;
while (!list_empty(&dpm_list)) {
struct device *dev = to_device(dpm_list.next);
So I really like this series not only because it implements what I
suggested, but also because each patch seems to remove more lines than
it adds. That's always nice, and much too unusual.
But in this one, I really think you should simplify/clarify things further:
On Sun, Dec 12, 2010 at 4:31 PM, Rafael J. Wysocki <[email protected]> wrote:
>
> +++ linux-2.6/drivers/base/power/main.c
> @@ -485,20 +485,17 @@ void dpm_resume_noirq(pm_message_t state
> ? ? ? ?transition_started = false;
> ? ? ? ?while (!list_empty(&dpm_noirq_list)) {
> ? ? ? ? ? ? ? ?struct device *dev = to_device(dpm_noirq_list.next);
> + ? ? ? ? ? ? ? int error;
>
> ? ? ? ? ? ? ? ?get_device(dev);
> - ? ? ? ? ? ? ? if (dev->power.status > DPM_OFF) {
> - ? ? ? ? ? ? ? ? ? ? ? int error;
> -
> - ? ? ? ? ? ? ? ? ? ? ? dev->power.status = DPM_OFF;
> - ? ? ? ? ? ? ? ? ? ? ? mutex_unlock(&dpm_list_mtx);
> + ? ? ? ? ? ? ? dev->power.status = DPM_OFF;
> + ? ? ? ? ? ? ? mutex_unlock(&dpm_list_mtx);
I think you should move the device to the dpm_suspended list _here_,
before dropping the mutex. That way the power.status thing matches the
list.
So then you'd just remove the crazy conditional "if it's still on a
list, move it to the right list" thing, and these two lines:
> ? ? ? ? ? ? ? ?if (!list_empty(&dev->power.entry))
> ? ? ? ? ? ? ? ? ? ? ? ?list_move_tail(&dev->power.entry, &dpm_suspended_list);
Would just be that plain
list_move_tail(&dev->power.entry, &dpm_suspended_list);
before you even drop the lock. That look much simpler, and the list
movement seems a lot more obvious, no?
If an unregister event (or whatever) happens while you had the mutex
unlocked, it will just remove it from the new list (the one that
matches the power state). So no need for that whole complexity with
"what happens with the list if somebody removed the device while we
were busy suspending/resuming it".
Or am I missing something?
(And same comment for that other identical case in dpm_complete())
Linus
On Sun, Dec 12, 2010 at 4:28 PM, Rafael J. Wysocki <[email protected]> wrote:
>
> Please let me know what you think.
All looks A-ok to me, with the small simplification suggestion posted
separately.
Linus
On Mon, 13 Dec 2010, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Instead of keeping all devices in the same list during system suspend
> and resume, regardless of what suspend-resume callbacks have been
> executed for them already, use separate lists of devices that have
> had their ->prepare(), ->suspend() and ->suspend_noirq() callbacks
> executed. This will allow us to simplify the core device suspend and
> resume routines.
Okay in principle. But there's one mistake...
> @@ -699,8 +693,8 @@ static void dpm_complete(pm_message_t st
> INIT_LIST_HEAD(&list);
> mutex_lock(&dpm_list_mtx);
> transition_started = false;
> - while (!list_empty(&dpm_list)) {
> - struct device *dev = to_device(dpm_list.prev);
> + while (!list_empty(&dpm_prepared_list)) {
> + struct device *dev = to_device(dpm_prepared_list.prev);
>
> get_device(dev);
> if (dev->power.status > DPM_ON) {
The parts about getting rid of "list" and putting dev back onto
dpm_list got left out.
Alan Stern
On Sun, 12 Dec 2010, Alan Stern wrote:
> On Mon, 13 Dec 2010, Rafael J. Wysocki wrote:
>
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Instead of keeping all devices in the same list during system suspend
> > and resume, regardless of what suspend-resume callbacks have been
> > executed for them already, use separate lists of devices that have
> > had their ->prepare(), ->suspend() and ->suspend_noirq() callbacks
> > executed. This will allow us to simplify the core device suspend and
> > resume routines.
>
> Okay in principle. But there's one mistake...
>
> > @@ -699,8 +693,8 @@ static void dpm_complete(pm_message_t st
> > INIT_LIST_HEAD(&list);
> > mutex_lock(&dpm_list_mtx);
> > transition_started = false;
> > - while (!list_empty(&dpm_list)) {
> > - struct device *dev = to_device(dpm_list.prev);
> > + while (!list_empty(&dpm_prepared_list)) {
> > + struct device *dev = to_device(dpm_prepared_list.prev);
> >
> > get_device(dev);
> > if (dev->power.status > DPM_ON) {
>
> The parts about getting rid of "list" and putting dev back onto
> dpm_list got left out.
Never mind. I forgot that we need to keep using the intermediate list
in order to handle new devices being registered while the resumes are
taking place.
Alan Stern
On Mon, 13 Dec 2010, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> The registration of a new parentless device during system suspend
> will not lead to any complications affecting the PM core (the device
> will be effectively seen after the subsequent resume has completed),
> so remove the code used for detection of such events.
Actually the tests you're changing were never as strong as they should
have been. Drivers are supposed to avoid registering new children
beneath a device as soon as the device has gone through the "prepare"
stage, not just after the device is suspended. Should there be a
"prepared" bitflag to help implement this stronger test?
In principle the same idea applies to parentless devices, since they
can be considered children of the "system device" (a fictitious node at
the root of the device tree). The "system" goes into the prepared
state before all the real devices; that's what the transition_started
variable was all about. It's nothing more than the "prepared" bitflag
for the "system device".
I guess it's okay to be lenient and not check for this. But should we
then change the documentation to match? (Note that the warning won't
be triggered if a new child is registered _as_ the parent is
suspending. Not to mention the possibilities for mischief when devices
are suspended asynchronously. But as you say, these complications
don't affect the PM core.)
Alan Stern
On Monday, December 13, 2010, Linus Torvalds wrote:
> So I really like this series not only because it implements what I
> suggested, but also because each patch seems to remove more lines than
> it adds. That's always nice, and much too unusual.
>
> But in this one, I really think you should simplify/clarify things further:
>
> On Sun, Dec 12, 2010 at 4:31 PM, Rafael J. Wysocki <[email protected]> wrote:
> >
> > +++ linux-2.6/drivers/base/power/main.c
> > @@ -485,20 +485,17 @@ void dpm_resume_noirq(pm_message_t state
> > transition_started = false;
> > while (!list_empty(&dpm_noirq_list)) {
> > struct device *dev = to_device(dpm_noirq_list.next);
> > + int error;
> >
> > get_device(dev);
> > - if (dev->power.status > DPM_OFF) {
> > - int error;
> > -
> > - dev->power.status = DPM_OFF;
> > - mutex_unlock(&dpm_list_mtx);
> > + dev->power.status = DPM_OFF;
> > + mutex_unlock(&dpm_list_mtx);
>
> I think you should move the device to the dpm_suspended list _here_,
> before dropping the mutex. That way the power.status thing matches the
> list.
>
> So then you'd just remove the crazy conditional "if it's still on a
> list, move it to the right list" thing, and these two lines:
>
> > if (!list_empty(&dev->power.entry))
> > list_move_tail(&dev->power.entry, &dpm_suspended_list);
>
> Would just be that plain
>
> list_move_tail(&dev->power.entry, &dpm_suspended_list);
>
> before you even drop the lock. That look much simpler, and the list
> movement seems a lot more obvious, no?
>
> If an unregister event (or whatever) happens while you had the mutex
> unlocked, it will just remove it from the new list (the one that
> matches the power state). So no need for that whole complexity with
> "what happens with the list if somebody removed the device while we
> were busy suspending/resuming it".
>
> Or am I missing something?
No, you're right. Somehow I didn't notice this possible simplification.
> (And same comment for that other identical case in dpm_complete())
Yeah.
In addition to that error messages need not be printed under the mutex.
Updated patch is appended.
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM: Remove redundant checks from core device resume routines
Since a separate list of devices is used to link devices that have
completed each stage of suspend (or resume), it is not necessary to
check dev->power.status in the core device resume routines any more.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/main.c | 44 +++++++++++++++++---------------------------
1 file changed, 17 insertions(+), 27 deletions(-)
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
@@ -485,22 +485,18 @@ void dpm_resume_noirq(pm_message_t state
transition_started = false;
while (!list_empty(&dpm_noirq_list)) {
struct device *dev = to_device(dpm_noirq_list.next);
+ int error;
get_device(dev);
- if (dev->power.status > DPM_OFF) {
- int error;
-
- dev->power.status = DPM_OFF;
- mutex_unlock(&dpm_list_mtx);
+ dev->power.status = DPM_OFF;
+ list_move_tail(&dev->power.entry, &dpm_suspended_list);
+ mutex_unlock(&dpm_list_mtx);
- error = device_resume_noirq(dev, state);
+ error = device_resume_noirq(dev, state);
+ if (error)
+ pm_dev_err(dev, state, " early", error);
- mutex_lock(&dpm_list_mtx);
- if (error)
- pm_dev_err(dev, state, " early", error);
- }
- if (!list_empty(&dev->power.entry))
- list_move_tail(&dev->power.entry, &dpm_suspended_list);
+ mutex_lock(&dpm_list_mtx);
put_device(dev);
}
mutex_unlock(&dpm_list_mtx);
@@ -619,9 +615,6 @@ static void dpm_resume(pm_message_t stat
async_error = 0;
list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
- if (dev->power.status < DPM_OFF)
- continue;
-
INIT_COMPLETION(dev->power.completion);
if (is_async(dev)) {
get_device(dev);
@@ -632,16 +625,16 @@ static void dpm_resume(pm_message_t stat
while (!list_empty(&dpm_suspended_list)) {
dev = to_device(dpm_suspended_list.next);
get_device(dev);
- if (dev->power.status >= DPM_OFF && !is_async(dev)) {
+ if (!is_async(dev)) {
int error;
mutex_unlock(&dpm_list_mtx);
error = device_resume(dev, state, false);
-
- mutex_lock(&dpm_list_mtx);
if (error)
pm_dev_err(dev, state, "", error);
+
+ mutex_lock(&dpm_list_mtx);
}
if (!list_empty(&dev->power.entry))
list_move_tail(&dev->power.entry, &dpm_prepared_list);
@@ -697,17 +690,14 @@ static void dpm_complete(pm_message_t st
struct device *dev = to_device(dpm_prepared_list.prev);
get_device(dev);
- if (dev->power.status > DPM_ON) {
- dev->power.status = DPM_ON;
- mutex_unlock(&dpm_list_mtx);
+ dev->power.status = DPM_ON;
+ list_move(&dev->power.entry, &list);
+ mutex_unlock(&dpm_list_mtx);
- device_complete(dev, state);
- pm_runtime_put_sync(dev);
+ device_complete(dev, state);
+ pm_runtime_put_sync(dev);
- mutex_lock(&dpm_list_mtx);
- }
- if (!list_empty(&dev->power.entry))
- list_move(&dev->power.entry, &list);
+ mutex_lock(&dpm_list_mtx);
put_device(dev);
}
list_splice(&list, &dpm_list);
On Monday, December 13, 2010, Alan Stern wrote:
> On Mon, 13 Dec 2010, Rafael J. Wysocki wrote:
>
> > From: Rafael J. Wysocki <[email protected]>
> >
> > The registration of a new parentless device during system suspend
> > will not lead to any complications affecting the PM core (the device
> > will be effectively seen after the subsequent resume has completed),
> > so remove the code used for detection of such events.
>
> Actually the tests you're changing were never as strong as they should
> have been. Drivers are supposed to avoid registering new children
> beneath a device as soon as the device has gone through the "prepare"
> stage, not just after the device is suspended. Should there be a
> "prepared" bitflag to help implement this stronger test?
The in_suspend flag introduced by [3/4] works like this, actually.
> In principle the same idea applies to parentless devices, since they
> can be considered children of the "system device" (a fictitious node at
> the root of the device tree). The "system" goes into the prepared
> state before all the real devices; that's what the transition_started
> variable was all about. It's nothing more than the "prepared" bitflag
> for the "system device".
It has never worked like this, because it was cleared as early as at the
_noirq() stage.
Hmm. It looks like I should modify [3/4] to clear the in_suspend flag earlier
to follow the current behavior (if a device is DPM_RESUMING, registration of
new children doesn't trigger the warning).
> I guess it's okay to be lenient and not check for this. But should we
> then change the documentation to match? (Note that the warning won't
> be triggered if a new child is registered _as_ the parent is
> suspending. Not to mention the possibilities for mischief when devices
> are suspended asynchronously. But as you say, these complications
> don't affect the PM core.)
The documentation is fine, I think, as it says what people are not supposed to
do and that doesn't really change. :-)
Thanks,
Rafael
On Tue, 14 Dec 2010, Rafael J. Wysocki wrote:
> On Monday, December 13, 2010, Alan Stern wrote:
> > On Mon, 13 Dec 2010, Rafael J. Wysocki wrote:
> >
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > The registration of a new parentless device during system suspend
> > > will not lead to any complications affecting the PM core (the device
> > > will be effectively seen after the subsequent resume has completed),
> > > so remove the code used for detection of such events.
> >
> > Actually the tests you're changing were never as strong as they should
> > have been. Drivers are supposed to avoid registering new children
> > beneath a device as soon as the device has gone through the "prepare"
> > stage, not just after the device is suspended. Should there be a
> > "prepared" bitflag to help implement this stronger test?
>
> The in_suspend flag introduced by [3/4] works like this, actually.
Not entirely, because it doesn't get set until the device has gone
through the "suspend" stage.
> > In principle the same idea applies to parentless devices, since they
> > can be considered children of the "system device" (a fictitious node at
> > the root of the device tree). The "system" goes into the prepared
> > state before all the real devices; that's what the transition_started
> > variable was all about. It's nothing more than the "prepared" bitflag
> > for the "system device".
>
> It has never worked like this, because it was cleared as early as at the
> _noirq() stage.
That was part of our lenient approach, allowing devices to be
registered during system resume earlier than the documentation says
they should be.
> Hmm. It looks like I should modify [3/4] to clear the in_suspend flag earlier
> to follow the current behavior (if a device is DPM_RESUMING, registration of
> new children doesn't trigger the warning).
You could clear in_suspend at the start of device_resume.
In the end, it's a question of what are we trying to accomplish. The
warnings catch the most egregious violations of the documented
requirements. Is the purpose to let people know about the violations,
or is it to warn about actions that appear genuinely dangerous?
Alan Stern
2010/12/13 Linus Torvalds <[email protected]>:
> So I really like this series not only because it implements what I
> suggested, but also because each patch seems to remove more lines than
> it adds. That's always nice, and much too unusual.
>
> But in this one, I really think you should simplify/clarify things further:
>
> On Sun, Dec 12, 2010 at 4:31 PM, Rafael J. Wysocki <[email protected]> wrote:
>>
>> +++ linux-2.6/drivers/base/power/main.c
>> @@ -485,20 +485,17 @@ void dpm_resume_noirq(pm_message_t state
>> ? ? ? ?transition_started = false;
>> ? ? ? ?while (!list_empty(&dpm_noirq_list)) {
>> ? ? ? ? ? ? ? ?struct device *dev = to_device(dpm_noirq_list.next);
>> + ? ? ? ? ? ? ? int error;
>>
>> ? ? ? ? ? ? ? ?get_device(dev);
>> - ? ? ? ? ? ? ? if (dev->power.status > DPM_OFF) {
>> - ? ? ? ? ? ? ? ? ? ? ? int error;
>> -
>> - ? ? ? ? ? ? ? ? ? ? ? dev->power.status = DPM_OFF;
>> - ? ? ? ? ? ? ? ? ? ? ? mutex_unlock(&dpm_list_mtx);
>> + ? ? ? ? ? ? ? dev->power.status = DPM_OFF;
>> + ? ? ? ? ? ? ? mutex_unlock(&dpm_list_mtx);
>
> I think you should move the device to the dpm_suspended list _here_,
> before dropping the mutex. That way the power.status thing matches the
> list.
>
> So then you'd just remove the crazy conditional "if it's still on a
> list, move it to the right list" thing, and these two lines:
>
>> ? ? ? ? ? ? ? ?if (!list_empty(&dev->power.entry))
>> ? ? ? ? ? ? ? ? ? ? ? ?list_move_tail(&dev->power.entry, &dpm_suspended_list);
>
> Would just be that plain
>
> ? ? ? ?list_move_tail(&dev->power.entry, &dpm_suspended_list);
>
> before you even drop the lock. That look much simpler, and the list
> movement seems a lot more obvious, no?
>
> If an unregister event (or whatever) happens while you had the mutex
> unlocked, it will just remove it from the new list (the one that
> matches the power state). So no need for that whole complexity with
> "what happens with the list if somebody removed the device while we
> were busy suspending/resuming it".
>
> Or am I missing something?
>
> (And same comment for that other identical case in dpm_complete())
Seems it may apply in other cases(dpm_prepare/dpm_suspend
/dpm_suspend_noirq) too?
thanks,
--
Lei Ming
On Tuesday, December 14, 2010, Ming Lei wrote:
> 2010/12/13 Linus Torvalds <[email protected]>:
> > So I really like this series not only because it implements what I
> > suggested, but also because each patch seems to remove more lines than
> > it adds. That's always nice, and much too unusual.
> >
> > But in this one, I really think you should simplify/clarify things further:
> >
> > On Sun, Dec 12, 2010 at 4:31 PM, Rafael J. Wysocki <[email protected]> wrote:
> >>
> >> +++ linux-2.6/drivers/base/power/main.c
> >> @@ -485,20 +485,17 @@ void dpm_resume_noirq(pm_message_t state
> >> transition_started = false;
> >> while (!list_empty(&dpm_noirq_list)) {
> >> struct device *dev = to_device(dpm_noirq_list.next);
> >> + int error;
> >>
> >> get_device(dev);
> >> - if (dev->power.status > DPM_OFF) {
> >> - int error;
> >> -
> >> - dev->power.status = DPM_OFF;
> >> - mutex_unlock(&dpm_list_mtx);
> >> + dev->power.status = DPM_OFF;
> >> + mutex_unlock(&dpm_list_mtx);
> >
> > I think you should move the device to the dpm_suspended list _here_,
> > before dropping the mutex. That way the power.status thing matches the
> > list.
> >
> > So then you'd just remove the crazy conditional "if it's still on a
> > list, move it to the right list" thing, and these two lines:
> >
> >> if (!list_empty(&dev->power.entry))
> >> list_move_tail(&dev->power.entry, &dpm_suspended_list);
> >
> > Would just be that plain
> >
> > list_move_tail(&dev->power.entry, &dpm_suspended_list);
> >
> > before you even drop the lock. That look much simpler, and the list
> > movement seems a lot more obvious, no?
> >
> > If an unregister event (or whatever) happens while you had the mutex
> > unlocked, it will just remove it from the new list (the one that
> > matches the power state). So no need for that whole complexity with
> > "what happens with the list if somebody removed the device while we
> > were busy suspending/resuming it".
> >
> > Or am I missing something?
> >
> > (And same comment for that other identical case in dpm_complete())
>
> Seems it may apply in other cases(dpm_prepare/dpm_suspend
> /dpm_suspend_noirq) too?
I thought about that, but in these cases the status is changed after the
callback has returned and only if it's succeeded. Moreover, if the callback
hasn't been successful, the device is not moved to the new list, so I think
it's better to leave that as is.
Thanks,
Rafael
On Tuesday, December 14, 2010, Alan Stern wrote:
> On Tue, 14 Dec 2010, Rafael J. Wysocki wrote:
>
> > On Monday, December 13, 2010, Alan Stern wrote:
> > > On Mon, 13 Dec 2010, Rafael J. Wysocki wrote:
> > >
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > The registration of a new parentless device during system suspend
> > > > will not lead to any complications affecting the PM core (the device
> > > > will be effectively seen after the subsequent resume has completed),
> > > > so remove the code used for detection of such events.
> > >
> > > Actually the tests you're changing were never as strong as they should
> > > have been. Drivers are supposed to avoid registering new children
> > > beneath a device as soon as the device has gone through the "prepare"
> > > stage, not just after the device is suspended. Should there be a
> > > "prepared" bitflag to help implement this stronger test?
> >
> > The in_suspend flag introduced by [3/4] works like this, actually.
>
> Not entirely, because it doesn't get set until the device has gone
> through the "suspend" stage.
>
> > > In principle the same idea applies to parentless devices, since they
> > > can be considered children of the "system device" (a fictitious node at
> > > the root of the device tree). The "system" goes into the prepared
> > > state before all the real devices; that's what the transition_started
> > > variable was all about. It's nothing more than the "prepared" bitflag
> > > for the "system device".
> >
> > It has never worked like this, because it was cleared as early as at the
> > _noirq() stage.
>
> That was part of our lenient approach, allowing devices to be
> registered during system resume earlier than the documentation says
> they should be.
>
> > Hmm. It looks like I should modify [3/4] to clear the in_suspend flag earlier
> > to follow the current behavior (if a device is DPM_RESUMING, registration of
> > new children doesn't trigger the warning).
>
> You could clear in_suspend at the start of device_resume.
>
> In the end, it's a question of what are we trying to accomplish. The
> warnings catch the most egregious violations of the documented
> requirements. Is the purpose to let people know about the violations,
> or is it to warn about actions that appear genuinely dangerous?
I'd say the latter, like trying to register a device (child) under a suspended
controller (parent). However, I think the new code shouldn't trigger the
warning when the old code didin't or people will report that as an apparent
issue.
Thanks,
Rafael
On Tuesday, December 14, 2010, Alan Stern wrote:
> On Tue, 14 Dec 2010, Rafael J. Wysocki wrote:
>
> > On Monday, December 13, 2010, Alan Stern wrote:
> > > On Mon, 13 Dec 2010, Rafael J. Wysocki wrote:
> > >
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > The registration of a new parentless device during system suspend
> > > > will not lead to any complications affecting the PM core (the device
> > > > will be effectively seen after the subsequent resume has completed),
> > > > so remove the code used for detection of such events.
> > >
> > > Actually the tests you're changing were never as strong as they should
> > > have been. Drivers are supposed to avoid registering new children
> > > beneath a device as soon as the device has gone through the "prepare"
> > > stage, not just after the device is suspended. Should there be a
> > > "prepared" bitflag to help implement this stronger test?
> >
> > The in_suspend flag introduced by [3/4] works like this, actually.
>
> Not entirely, because it doesn't get set until the device has gone
> through the "suspend" stage.
>
> > > In principle the same idea applies to parentless devices, since they
> > > can be considered children of the "system device" (a fictitious node at
> > > the root of the device tree). The "system" goes into the prepared
> > > state before all the real devices; that's what the transition_started
> > > variable was all about. It's nothing more than the "prepared" bitflag
> > > for the "system device".
> >
> > It has never worked like this, because it was cleared as early as at the
> > _noirq() stage.
>
> That was part of our lenient approach, allowing devices to be
> registered during system resume earlier than the documentation says
> they should be.
>
> > Hmm. It looks like I should modify [3/4] to clear the in_suspend flag earlier
> > to follow the current behavior (if a device is DPM_RESUMING, registration of
> > new children doesn't trigger the warning).
>
> You could clear in_suspend at the start of device_resume.
The patch below (replacing [3/4]) does that (the second clearing is for
devices whose ->suspend() methods have not been called, which can happen if
there's a suspend error).
I hope the USB part is still valid?
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM: Replace the device power.status field with a bit field
The device power.status field is too complicated for its purpose
(storing the information about whether or not the device is in the
"active" state from the PM core's point of view), so replace it with
a bit field and modify all of its users accordingly.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/main.c | 17 +++++------------
drivers/usb/core/driver.c | 7 +++----
include/linux/device.h | 4 ++--
include/linux/pm.h | 43 ++-----------------------------------------
4 files changed, 12 insertions(+), 59 deletions(-)
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ linux-2.6/include/linux/device.h
@@ -508,13 +508,13 @@ static inline int device_is_registered(s
static inline void device_enable_async_suspend(struct device *dev)
{
- if (dev->power.status == DPM_ON)
+ if (!dev->power.in_suspend)
dev->power.async_suspend = true;
}
static inline void device_disable_async_suspend(struct device *dev)
{
- if (dev->power.status == DPM_ON)
+ if (!dev->power.in_suspend)
dev->power.async_suspend = false;
}
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -367,45 +367,6 @@ extern struct dev_pm_ops generic_subsys_
{ .event = PM_EVENT_AUTO_RESUME, })
/**
- * Device power management states
- *
- * These state labels are used internally by the PM core to indicate the current
- * status of a device with respect to the PM core operations.
- *
- * DPM_ON Device is regarded as operational. Set this way
- * initially and when ->complete() is about to be called.
- * Also set when ->prepare() fails.
- *
- * DPM_PREPARING Device is going to be prepared for a PM transition. Set
- * when ->prepare() is about to be called.
- *
- * DPM_RESUMING Device is going to be resumed. Set when ->resume(),
- * ->thaw(), or ->restore() is about to be called.
- *
- * DPM_SUSPENDING Device has been prepared for a power transition. Set
- * when ->prepare() has just succeeded.
- *
- * DPM_OFF Device is regarded as inactive. Set immediately after
- * ->suspend(), ->freeze(), or ->poweroff() has succeeded.
- * Also set when ->resume()_noirq, ->thaw_noirq(), or
- * ->restore_noirq() is about to be called.
- *
- * DPM_OFF_IRQ Device is in a "deep sleep". Set immediately after
- * ->suspend_noirq(), ->freeze_noirq(), or
- * ->poweroff_noirq() has just succeeded.
- */
-
-enum dpm_state {
- DPM_INVALID,
- DPM_ON,
- DPM_PREPARING,
- DPM_RESUMING,
- DPM_SUSPENDING,
- DPM_OFF,
- DPM_OFF_IRQ,
-};
-
-/**
* Device run-time power management status.
*
* These status labels are used internally by the PM core to indicate the
@@ -463,8 +424,8 @@ struct wakeup_source;
struct dev_pm_info {
pm_message_t power_state;
unsigned int can_wakeup:1;
- unsigned async_suspend:1;
- enum dpm_state status; /* Owned by the PM core */
+ unsigned int async_suspend:1;
+ unsigned int in_suspend:1; /* Owned by the PM core */
spinlock_t lock;
#ifdef CONFIG_PM_SLEEP
struct list_head entry;
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
@@ -63,7 +63,7 @@ static int async_error;
*/
void device_pm_init(struct device *dev)
{
- dev->power.status = DPM_ON;
+ dev->power.in_suspend = false;
init_completion(&dev->power.completion);
complete_all(&dev->power.completion);
dev->power.wakeup = NULL;
@@ -98,7 +98,7 @@ void device_pm_add(struct device *dev)
kobject_name(&dev->kobj));
mutex_lock(&dpm_list_mtx);
if (dev->parent) {
- if (dev->parent->power.status >= DPM_SUSPENDING)
+ if (dev->parent->power.in_suspend)
dev_warn(dev, "parent %s should not be sleeping\n",
dev_name(dev->parent));
} else if (transition_started) {
@@ -488,7 +488,6 @@ void dpm_resume_noirq(pm_message_t state
int error;
get_device(dev);
- dev->power.status = DPM_OFF;
list_move_tail(&dev->power.entry, &dpm_suspended_list);
mutex_unlock(&dpm_list_mtx);
@@ -541,7 +540,7 @@ static int device_resume(struct device *
dpm_wait(dev->parent, async);
device_lock(dev);
- dev->power.status = DPM_RESUMING;
+ dev->power.in_suspend = false;
if (dev->bus) {
if (dev->bus->pm) {
@@ -690,7 +689,7 @@ static void dpm_complete(pm_message_t st
struct device *dev = to_device(dpm_prepared_list.prev);
get_device(dev);
- dev->power.status = DPM_ON;
+ dev->power.in_suspend = false;
list_move(&dev->power.entry, &list);
mutex_unlock(&dpm_list_mtx);
@@ -806,7 +805,6 @@ int dpm_suspend_noirq(pm_message_t state
put_device(dev);
break;
}
- dev->power.status = DPM_OFF_IRQ;
if (!list_empty(&dev->power.entry))
list_move(&dev->power.entry, &dpm_noirq_list);
put_device(dev);
@@ -894,9 +892,6 @@ static int __device_suspend(struct devic
}
}
- if (!error)
- dev->power.status = DPM_OFF;
-
End:
device_unlock(dev);
complete_all(&dev->power.completion);
@@ -1030,7 +1025,6 @@ static int dpm_prepare(pm_message_t stat
struct device *dev = to_device(dpm_list.next);
get_device(dev);
- dev->power.status = DPM_PREPARING;
mutex_unlock(&dpm_list_mtx);
pm_runtime_get_noresume(dev);
@@ -1046,7 +1040,6 @@ static int dpm_prepare(pm_message_t stat
mutex_lock(&dpm_list_mtx);
if (error) {
- dev->power.status = DPM_ON;
if (error == -EAGAIN) {
put_device(dev);
error = 0;
@@ -1058,7 +1051,7 @@ static int dpm_prepare(pm_message_t stat
put_device(dev);
break;
}
- dev->power.status = DPM_SUSPENDING;
+ dev->power.in_suspend = true;
if (!list_empty(&dev->power.entry))
list_move_tail(&dev->power.entry, &dpm_prepared_list);
put_device(dev);
Index: linux-2.6/drivers/usb/core/driver.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/driver.c
+++ linux-2.6/drivers/usb/core/driver.c
@@ -376,7 +376,7 @@ static int usb_unbind_interface(struct d
* Just re-enable it without affecting the endpoint toggles.
*/
usb_enable_interface(udev, intf, false);
- } else if (!error && intf->dev.power.status == DPM_ON) {
+ } else if (!error && !intf->dev.power.in_suspend) {
r = usb_set_interface(udev, intf->altsetting[0].
desc.bInterfaceNumber, 0);
if (r < 0)
@@ -961,7 +961,7 @@ void usb_rebind_intf(struct usb_interfac
}
/* Try to rebind the interface */
- if (intf->dev.power.status == DPM_ON) {
+ if (!intf->dev.power.in_suspend) {
intf->needs_binding = 0;
rc = device_attach(&intf->dev);
if (rc < 0)
@@ -1108,8 +1108,7 @@ static int usb_resume_interface(struct u
if (intf->condition == USB_INTERFACE_UNBOUND) {
/* Carry out a deferred switch to altsetting 0 */
- if (intf->needs_altsetting0 &&
- intf->dev.power.status == DPM_ON) {
+ if (intf->needs_altsetting0 && !intf->dev.power.in_suspend) {
usb_set_interface(udev, intf->altsetting[0].
desc.bInterfaceNumber, 0);
intf->needs_altsetting0 = 0;
2010/12/15 Rafael J. Wysocki <[email protected]>:
> On Tuesday, December 14, 2010, Ming Lei wrote:
>> Seems it may apply in other cases(dpm_prepare/dpm_suspend
>> /dpm_suspend_noirq) too?
>
> I thought about that, but in these cases the status is changed after the
> callback has returned and only if it's succeeded. ?Moreover, if the callback
> hasn't been successful, the device is not moved to the new list, so I think
> it's better to leave that as is.
Yes, you are right, sorry for my noise, :-(
thanks,
--
Lei Ming
On Wed, 15 Dec 2010, Rafael J. Wysocki wrote:
> > You could clear in_suspend at the start of device_resume.
>
> The patch below (replacing [3/4]) does that (the second clearing is for
> devices whose ->suspend() methods have not been called, which can happen if
> there's a suspend error).
>
> I hope the USB part is still valid?
It is. Although now that I look at it, the third USB test is (and has
always been) totally wrong. It should be removed completely. I'll do
that separately; for now your update won't affect the way it works.
Alan Stern