PM: Do not acquire device semaphores upfront during suspend
Subsystems (6)
PATCH 1/1 PM: Do not acquire device semaphores upfront during suspend
Hi Greg,
The appended patch should fix the regreesions reported at:
http://bugzilla.kernel.org/show_bug.cgi?id=9874
http://bugzilla.kernel.org/show_bug.cgi?id=10030
Please accept it for 2.6.25.
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
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 4145ed6dc597a9bea5f6ae8c574653b2de10620f 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.
--- drivers/base/core.c | 8 --- drivers/base/power/main.c | 106 ++++++---------------------------------------- drivers/usb/core/usb.c | 2 3 files changed, 17 insertions(+), 99 deletions(-)
drivers/base/power/main.c +16 -90
@@ -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);
@@ -81,28 +80,6 @@ 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));
@@ -110,7 +87,6 @@ void device_pm_remove(struct device *devdpm_sysfs_remove (dev);list_del_init (&dev->power.entry);mutex_unlock (&dpm_list_mtx); -up (&dev->sem); }/**
@@ -230,6 +206,8 @@ static int resume_device(struct device *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);
@@ -245,6 +223,8 @@ static int resume_device(struct device * error = dev->class->resume (dev); } +up (&dev->sem); +TRACE_RESUME (error);return error; }
@@ -266,7 +246,7 @@ 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);
@@ -275,25 +255,6 @@ static void dpm_resume(void) }/** -* 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 * * Unregister all devices on the dpm_destroy list.
@@ -305,7 +266,6 @@ static void unregister_dropped_devices(vstruct 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);
@@ -324,7 +284,6 @@ void device_resume(void) {might_sleep ();dpm_resume (); -unlock_all_devices ();unregister_dropped_devices ();up_write (&pm_sleep_rwsem); }
@@ -388,18 +347,15 @@ int device_power_down(pm_message_t statestruct 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)
@@ -419,6 +375,8 @@ static int suspend_device(struct device {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);
@@ -441,6 +399,9 @@ static int suspend_device(struct device error = dev->bus->suspend (dev, state);suspend_report_result (dev->bus->suspend, error); } + +up (&dev->sem); +return error; }
@@ -461,11 +422,10 @@ static int dpm_suspend(pm_message_t statint 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) {
@@ -476,14 +436,11 @@ static int dpm_suspend(pm_message_t stat (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);
@@ -491,36 +448,6 @@ static int dpm_suspend(pm_message_t stat }/** -* 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 *
@@ -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 ();
drivers/usb/core/usb.c +1 -1
@@ -234,7 +234,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 ;
drivers/base/core.c +0 -8
@@ -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 ;
@@ -843,7 +836,6 @@ int device_add(struct device *dev) } Done:put_device (dev); -pm_sleep_unlock ();return error; BusError:device_pm_remove (dev);