| Message ID | 200908122221.20568.rjw@sisk.pl (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
> @@ -659,26 +674,61 @@ static pm_message_t resume_event(pm_mess > } > > /** > - * device_suspend_noirq - Shut down one device (late suspend). > - * @dev: Device. > - * @state: PM transition of the system being carried out. > + * __device_suspend_noirq - Execute a "late" suspend callback for given device. > + * @dev: Device to suspend. > + * @state: PM transition of the system being carried out. > * > - * This is called with interrupts off and only a single CPU running. that still looks like useful comment... why delete it? > + * The driver of the device won't receive interrupts while this function is > + * being executed. > */ > @@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state > suspend_device_irqs(); > mutex_lock(&dpm_list_mtx); > list_for_each_entry_reverse(dev, &dpm_list, power.entry) { > + dev->power.status = DPM_OFF_IRQ; > error = device_suspend_noirq(dev, state); > if (error) { > pm_dev_err(dev, state, " late", error); > + dev->power.status = DPM_OFF; > + break; > + } > + if (async_error) { > + error = async_error; > break; async_error is 'interesting'. How does locking work in noirq case?
On Friday 14 August 2009, Pavel Machek wrote: > > @@ -659,26 +674,61 @@ static pm_message_t resume_event(pm_mess > > } > > > > /** > > - * device_suspend_noirq - Shut down one device (late suspend). > > - * @dev: Device. > > - * @state: PM transition of the system being carried out. > > + * __device_suspend_noirq - Execute a "late" suspend callback for given device. > > + * @dev: Device to suspend. > > + * @state: PM transition of the system being carried out. > > > * > > - * This is called with interrupts off and only a single CPU running. > > that still looks like useful comment... why delete it? Because it's not valid any more. > > + * The driver of the device won't receive interrupts while this function is > > + * being executed. > > */ > > @@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state > > suspend_device_irqs(); > > mutex_lock(&dpm_list_mtx); > > list_for_each_entry_reverse(dev, &dpm_list, power.entry) { > > + dev->power.status = DPM_OFF_IRQ; > > error = device_suspend_noirq(dev, state); > > if (error) { > > pm_dev_err(dev, state, " late", error); > > + dev->power.status = DPM_OFF; > > + break; > > + } > > + if (async_error) { > > + error = async_error; > > break; > > async_error is 'interesting'. How does locking work in noirq case? It's racy, a little bit. :-) If two async drivers return errors exactly at the same time, one of them will win the race, but it doesn't really matter which one wins as long as async_error is different from zero as a result. And it will be, since it's an 'int' and the integrity of these is guaranteed. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > + * The driver of the device won't receive interrupts while this function is > > > + * being executed. > > > */ > > > @@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state > > > suspend_device_irqs(); > > > mutex_lock(&dpm_list_mtx); > > > list_for_each_entry_reverse(dev, &dpm_list, power.entry) { > > > + dev->power.status = DPM_OFF_IRQ; > > > error = device_suspend_noirq(dev, state); > > > if (error) { > > > pm_dev_err(dev, state, " late", error); > > > + dev->power.status = DPM_OFF; > > > + break; > > > + } > > > + if (async_error) { > > > + error = async_error; > > > break; > > > > async_error is 'interesting'. How does locking work in noirq case? > > It's racy, a little bit. :-) > > If two async drivers return errors exactly at the same time, one of them will > win the race, but it doesn't really matter which one wins as long as > async_error is different from zero as a result. And it will be, since it's > an 'int' and the integrity of these is guaranteed. Rather than relying on atomicity of 'int' (where half of kernel hackers says it is and second half says it is not), can we just use atomic_t? It compiles to same code on sane architectures, and serves as documentation/warning... Pavel
On Saturday 22 August 2009, Pavel Machek wrote: > > > > + * The driver of the device won't receive interrupts while this function is > > > > + * being executed. > > > > */ > > > > @@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state > > > > suspend_device_irqs(); > > > > mutex_lock(&dpm_list_mtx); > > > > list_for_each_entry_reverse(dev, &dpm_list, power.entry) { > > > > + dev->power.status = DPM_OFF_IRQ; > > > > error = device_suspend_noirq(dev, state); > > > > if (error) { > > > > pm_dev_err(dev, state, " late", error); > > > > + dev->power.status = DPM_OFF; > > > > + break; > > > > + } > > > > + if (async_error) { > > > > + error = async_error; > > > > break; > > > > > > async_error is 'interesting'. How does locking work in noirq case? > > > > It's racy, a little bit. :-) > > > > If two async drivers return errors exactly at the same time, one of them will > > win the race, but it doesn't really matter which one wins as long as > > async_error is different from zero as a result. And it will be, since it's > > an 'int' and the integrity of these is guaranteed. > > Rather than relying on atomicity of 'int' (where half of kernel > hackers says it is and second half says it is not), can we just use > atomic_t? It compiles to same code on sane architectures, and serves > as documentation/warning... I used atomic_t for that in the updated patches, already sent a few days ago. Please refer to that code. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 @@ -194,6 +194,19 @@ static void device_pm_wait(struct device wait_for_completion(&master->power.comp); } +static int device_pm_wait_wrapper(struct device *dev, void *data) +{ + struct device *parent = (struct device *)data; + + device_pm_wait(parent, dev); + return 0; +} + +static void device_pm_wait_for_children(struct device *parent) +{ + device_for_each_child(parent, parent, device_pm_wait_wrapper); +} + /** * pm_op - execute the PM operation appropiate for given PM event * @dev: Device. @@ -639,6 +652,8 @@ EXPORT_SYMBOL_GPL(dpm_resume_end); /*------------------------- Suspend routines -------------------------*/ +static int async_error; + /** * resume_event - return a PM message representing the resume event * corresponding to given sleep state. @@ -659,26 +674,61 @@ static pm_message_t resume_event(pm_mess } /** - * device_suspend_noirq - Shut down one device (late suspend). - * @dev: Device. - * @state: PM transition of the system being carried out. + * __device_suspend_noirq - Execute a "late" suspend callback for given device. + * @dev: Device to suspend. + * @state: PM transition of the system being carried out. * - * This is called with interrupts off and only a single CPU running. + * The driver of the device won't receive interrupts while this function is + * being executed. */ -static int device_suspend_noirq(struct device *dev, pm_message_t state) +static int __device_suspend_noirq(struct device *dev, pm_message_t state) { int error = 0; - if (!dev->bus) - return 0; + device_pm_wait_for_children(dev); - if (dev->bus->pm) { + if (dev->bus && dev->bus->pm) { pm_dev_dbg(dev, state, "LATE "); error = pm_noirq_op(dev, dev->bus->pm, state); } + + complete_all(&dev->power.comp); + return error; } +static void async_suspend_noirq(void *data, async_cookie_t cookie) +{ + struct device *dev = (struct device *)data; + int error = async_error; + + if (error) + return; + + pm_dev_dbg(dev, dev->power.async_state, "async LATE "); + error = __device_suspend_noirq(dev, dev->power.async_state); + if (error) { + pm_dev_err(dev, dev->power.async_state, " async LATE", error); + dev->power.status = DPM_OFF; + } + put_device(dev); + + if (error && !async_error) + async_error = error; +} + +static int device_suspend_noirq(struct device *dev, pm_message_t state) +{ + if (dev->power.async_suspend) { + get_device(dev); + dev->power.async_state = state; + async_schedule(async_suspend_noirq, dev); + return 0; + } + + return __device_suspend_noirq(dev, state); +} + /** * dpm_suspend_noirq - Power down all regular (non-sysdev) devices. * @state: PM transition of the system being carried out. @@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state suspend_device_irqs(); mutex_lock(&dpm_list_mtx); list_for_each_entry_reverse(dev, &dpm_list, power.entry) { + dev->power.status = DPM_OFF_IRQ; error = device_suspend_noirq(dev, state); if (error) { pm_dev_err(dev, state, " late", error); + dev->power.status = DPM_OFF; + break; + } + if (async_error) { + error = async_error; break; } - dev->power.status = DPM_OFF_IRQ; } + dpm_synchronize_noirq(); mutex_unlock(&dpm_list_mtx); if (error) dpm_resume_noirq(resume_event(state)); @@ -711,14 +767,15 @@ int dpm_suspend_noirq(pm_message_t state EXPORT_SYMBOL_GPL(dpm_suspend_noirq); /** - * device_suspend - Save state of one device. - * @dev: Device. - * @state: PM transition of the system being carried out. + * __device_suspend - Execute suspend callbacks for given device. + * @dev: Device to suspend. + * @state: PM transition of the system being carried out. */ -static int device_suspend(struct device *dev, pm_message_t state) +static int __device_suspend(struct device *dev, pm_message_t state) { int error = 0; + device_pm_wait_for_children(dev); down(&dev->sem); if (dev->class) { @@ -755,10 +812,51 @@ static int device_suspend(struct device } End: up(&dev->sem); + complete_all(&dev->power.comp); return error; } +static void async_suspend(void *data, async_cookie_t cookie) +{ + struct device *dev = (struct device *)data; + int error; + + pm_dev_dbg(dev, dev->power.async_state, "async "); + + mutex_lock(&dpm_list_mtx); + error = async_error; + mutex_unlock(&dpm_list_mtx); + if (error) + goto End; + + error = __device_suspend(dev, dev->power.async_state); + if (error) { + pm_dev_err(dev, dev->power.async_state, " async", error); + + mutex_lock(&dpm_list_mtx); + dev->power.status = DPM_SUSPENDING; + if (!async_error) + async_error = error; + mutex_unlock(&dpm_list_mtx); + } + + End: + put_device(dev); +} + +static int device_suspend(struct device *dev, pm_message_t state) +{ + if (dev->power.async_suspend) { + get_device(dev); + dev->power.async_state = state; + async_schedule(async_suspend, dev); + return 0; + } + + return __device_suspend(dev, state); +} + /** * dpm_suspend - Suspend every device. * @state: PM transition of the system being carried out. @@ -776,6 +874,7 @@ static int dpm_suspend(pm_message_t stat struct device *dev = to_device(dpm_list.prev); get_device(dev); + dev->power.status = DPM_OFF; mutex_unlock(&dpm_list_mtx); error = device_suspend(dev, state); @@ -783,16 +882,21 @@ static int dpm_suspend(pm_message_t stat mutex_lock(&dpm_list_mtx); if (error) { pm_dev_err(dev, state, "", error); + dev->power.status = DPM_SUSPENDING; put_device(dev); break; } - dev->power.status = DPM_OFF; if (!list_empty(&dev->power.entry)) list_move(&dev->power.entry, &list); put_device(dev); + if (async_error) { + error = async_error; + break; + } } list_splice(&list, dpm_list.prev); mutex_unlock(&dpm_list_mtx); + dpm_synchronize(); return error; } @@ -848,6 +952,7 @@ static int dpm_prepare(pm_message_t stat INIT_LIST_HEAD(&list); mutex_lock(&dpm_list_mtx); transition_started = true; + async_error = 0; while (!list_empty(&dpm_list)) { struct device *dev = to_device(dpm_list.next);