On Friday, 21 September 2007 21:37, Alan Stern wrote: > This patch (as994) 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. > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> Acked-by: Rafael J. Wysocki <rjw@xxxxxxx> > --- > > Index: usb-2.6/drivers/base/power/main.c > =================================================================== > --- usb-2.6.orig/drivers/base/power/main.c > +++ usb-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); > > > @@ -59,29 +80,112 @@ 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); > +} > + > +/** > + * 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"); > @@ -98,126 +202,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) { > @@ -228,7 +274,6 @@ static inline char *suspend_verb(u32 eve > } > } > > - > static void > suspend_device_dbg(struct device *dev, pm_message_t state, char *info) > { > @@ -238,16 +283,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); > + > + 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); > > -static int suspend_device(struct device * dev, pm_message_t state) > +/** > + * 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); > @@ -270,123 +368,99 @@ 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); > } > + > + if (error) > + dpm_resume(); > 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; > + int error; > > - dev = to_device(entry); > - error = suspend_device_late(dev, state); > - if (error) > - goto Error; > - list_move(&dev->power.entry, &dpm_off_irq); > + might_sleep(); > + down_write(&pm_sleep_rwsem); > + lock_all_devices(); > + error = dpm_suspend(state); > + if (error) { > + unlock_all_devices(); > + up_write(&pm_sleep_rwsem); > } > - > - error = sysdev_suspend(state); > - Done: > 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: usb-2.6/drivers/base/core.c > =================================================================== > --- usb-2.6.orig/drivers/base/core.c > +++ usb-2.6/drivers/base/core.c > @@ -712,11 +712,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); > > @@ -780,6 +786,7 @@ int device_add(struct device *dev) > } > Done: > put_device(dev); > + pm_sleep_unlock(); > return error; > BusError: > device_pm_remove(dev); > Index: usb-2.6/drivers/base/power/power.h > =================================================================== > --- usb-2.6.orig/drivers/base/power/power.h > +++ usb-2.6/drivers/base/power/power.h > @@ -20,6 +20,8 @@ static inline struct device * to_device( > > extern int device_pm_add(struct device *); > extern void device_pm_remove(struct device *); > +extern int pm_sleep_lock(void); > +extern void pm_sleep_unlock(void); > > /* > * sysfs.c > @@ -37,7 +39,15 @@ static inline int device_pm_add(struct d > } > static inline void device_pm_remove(struct device * dev) > { > +} > + > +static inline int pm_sleep_lock(void) > +{ > + return 0; > +} > > +static inline void pm_sleep_unlock(void) > +{ > } > > #endif > > _______________________________________________ > linux-pm mailing list > linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx > https://lists.linux-foundation.org/mailman/listinfo/linux-pm > > -- "Premature optimization is the root of all evil." - Donald Knuth _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm