Skip to content

Commit

Permalink
PM: Do not acquire device semaphores upfront during suspend
Browse files Browse the repository at this point in the history
Remove the code that acquires all device semaphores from the suspend
code path as it causes multiple problems to appear (most notably,
http://bugzilla.kernel.org/show_bug.cgi?id=10030) and revert the
change introduced by commit 4145ed6
depending on the code being removed.

Remove pm_sleep_lock()/pm_sleep_unlock() from device_add() to avoid
the issue reported at http://bugzilla.kernel.org/show_bug.cgi?id=9874.

It should fix the regreesions reported at:
	http://bugzilla.kernel.org/show_bug.cgi?id=9874
	http://bugzilla.kernel.org/show_bug.cgi?id=10030

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
  • Loading branch information
rjwysocki authored and gregkh committed Mar 4, 2008
1 parent a4573c4 commit 7a8d37a
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 99 deletions.
8 changes: 0 additions & 8 deletions drivers/base/core.c
Original file line number Diff line number Diff line change
Expand Up @@ -770,13 +770,6 @@ int device_add(struct device *dev)
struct class_interface *class_intf;
int error;

error = pm_sleep_lock();
if (error) {
dev_warn(dev, "Suspicious %s during suspend\n", __FUNCTION__);
dump_stack();
return error;
}

dev = get_device(dev);
if (!dev || !strlen(dev->bus_id)) {
error = -EINVAL;
Expand Down Expand Up @@ -843,7 +836,6 @@ int device_add(struct device *dev)
}
Done:
put_device(dev);
pm_sleep_unlock();
return error;
BusError:
device_pm_remove(dev);
Expand Down
106 changes: 16 additions & 90 deletions drivers/base/power/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
*/

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);
Expand Down Expand Up @@ -81,36 +80,13 @@ void device_pm_add(struct device *dev)
*/
void device_pm_remove(struct device *dev)
{
/*
* If this function is called during a suspend, it will be blocked,
* because we're holding the device's semaphore at that time, which may
* lead to a deadlock. In that case we want to print a warning.
* However, it may also be called by unregister_dropped_devices() with
* the device's semaphore released, in which case the warning should
* not be printed.
*/
if (down_trylock(&dev->sem)) {
if (down_read_trylock(&pm_sleep_rwsem)) {
/* No suspend in progress, wait on dev->sem */
down(&dev->sem);
up_read(&pm_sleep_rwsem);
} else {
/* Suspend in progress, we may deadlock */
dev_warn(dev, "Suspicious %s during suspend\n",
__FUNCTION__);
dump_stack();
/* The user has been warned ... */
down(&dev->sem);
}
}
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_del_init(&dev->power.entry);
mutex_unlock(&dpm_list_mtx);
up(&dev->sem);
}

/**
Expand Down Expand Up @@ -230,6 +206,8 @@ static int resume_device(struct device *dev)
TRACE_DEVICE(dev);
TRACE_RESUME(0);

down(&dev->sem);

if (dev->bus && dev->bus->resume) {
dev_dbg(dev,"resuming\n");
error = dev->bus->resume(dev);
Expand All @@ -245,6 +223,8 @@ static int resume_device(struct device *dev)
error = dev->class->resume(dev);
}

up(&dev->sem);

TRACE_RESUME(error);
return error;
}
Expand All @@ -266,33 +246,14 @@ static void dpm_resume(void)
struct list_head *entry = dpm_off.next;
struct device *dev = to_device(entry);

list_move_tail(entry, &dpm_locked);
list_move_tail(entry, &dpm_active);
mutex_unlock(&dpm_list_mtx);
resume_device(dev);
mutex_lock(&dpm_list_mtx);
}
mutex_unlock(&dpm_list_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.
*/
static void unlock_all_devices(void)
{
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);
}

/**
* unregister_dropped_devices - Unregister devices scheduled for removal
*
Expand All @@ -305,7 +266,6 @@ static void unregister_dropped_devices(void)
struct list_head *entry = dpm_destroy.next;
struct device *dev = to_device(entry);

up(&dev->sem);
mutex_unlock(&dpm_list_mtx);
/* This also removes the device from the list */
device_unregister(dev);
Expand All @@ -324,7 +284,6 @@ void device_resume(void)
{
might_sleep();
dpm_resume();
unlock_all_devices();
unregister_dropped_devices();
up_write(&pm_sleep_rwsem);
}
Expand Down Expand Up @@ -388,18 +347,15 @@ int device_power_down(pm_message_t state)
struct list_head *entry = dpm_off.prev;
struct device *dev = to_device(entry);

list_del_init(&dev->power.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);
if (list_empty(&dev->power.entry))
list_add(&dev->power.entry, &dpm_off);
break;
}
if (list_empty(&dev->power.entry))
list_add(&dev->power.entry, &dpm_off_irq);
if (!list_empty(&dev->power.entry))
list_move(&dev->power.entry, &dpm_off_irq);
}

if (!error)
Expand All @@ -419,6 +375,8 @@ static 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);
Expand All @@ -441,6 +399,9 @@ static int suspend_device(struct device *dev, pm_message_t state)
error = dev->bus->suspend(dev, state);
suspend_report_result(dev->bus->suspend, error);
}

up(&dev->sem);

return error;
}

Expand All @@ -461,11 +422,10 @@ static int dpm_suspend(pm_message_t state)
int error = 0;

mutex_lock(&dpm_list_mtx);
while (!list_empty(&dpm_locked)) {
struct list_head *entry = dpm_locked.prev;
while (!list_empty(&dpm_active)) {
struct list_head *entry = dpm_active.prev;
struct device *dev = to_device(entry);

list_del_init(&dev->power.entry);
mutex_unlock(&dpm_list_mtx);
error = suspend_device(dev, state);
if (error) {
Expand All @@ -476,50 +436,17 @@ static int dpm_suspend(pm_message_t state)
(error == -EAGAIN ?
" (please convert to suspend_late)" :
""));
mutex_lock(&dpm_list_mtx);
if (list_empty(&dev->power.entry))
list_add(&dev->power.entry, &dpm_locked);
break;
}
mutex_lock(&dpm_list_mtx);
if (list_empty(&dev->power.entry))
list_add(&dev->power.entry, &dpm_off);
if (!list_empty(&dev->power.entry))
list_move(&dev->power.entry, &dpm_off);
}
mutex_unlock(&dpm_list_mtx);

return error;
}

/**
* 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.
*/
static void lock_all_devices(void)
{
mutex_lock(&dpm_list_mtx);
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);
down(&dev->sem);
mutex_lock(&dpm_list_mtx);

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);
}

/**
* device_suspend - Save state and stop all devices in system.
* @state: new power management state
Expand All @@ -533,7 +460,6 @@ int device_suspend(pm_message_t state)

might_sleep();
down_write(&pm_sleep_rwsem);
lock_all_devices();
error = dpm_suspend(state);
if (error)
device_resume();
Expand Down
2 changes: 1 addition & 1 deletion drivers/usb/core/usb.c
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ static int ksuspend_usb_init(void)
* singlethreaded. Its job doesn't justify running on more
* than one CPU.
*/
ksuspend_usb_wq = create_singlethread_workqueue("ksuspend_usbd");
ksuspend_usb_wq = create_freezeable_workqueue("ksuspend_usbd");
if (!ksuspend_usb_wq)
return -ENOMEM;
return 0;
Expand Down

0 comments on commit 7a8d37a

Please sign in to comment.