PM: Add suspend block api.
in [Kernel]
|
Prev: A share can be mounted multiple times on the same mountpoint when using the option 'noac'
Next: parport_pc.c and parport_serial.c need improvements
From: Rafael J. Wysocki on 28 Apr 2010 17:00 On Wednesday 28 April 2010, Arve Hjønnevåg wrote: > Adds /sys/power/policy that selects the behaviour of /sys/power/state. > After setting the policy to opportunistic, writes to /sys/power/state > become non-blocking requests that specify which suspend state to enter > when no suspend blockers are active. A special state, "on", stops the > process by activating the "main" suspend blocker. > > Signed-off-by: Arve Hjønnevåg <arve(a)android.com> > --- .... > @@ -20,6 +21,27 @@ DEFINE_MUTEX(pm_mutex); > unsigned int pm_flags; > EXPORT_SYMBOL(pm_flags); > > +struct policy { > + const char *name; > + bool (*valid_state)(suspend_state_t state); > + int (*set_state)(suspend_state_t state); > +}; > +static struct policy policies[] = { > + { > + .name = "forced", > + .valid_state = valid_state, > + .set_state = enter_state, > + }, > +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND > + { > + .name = "opportunistic", > + .valid_state = request_suspend_valid_state, > + .set_state = request_suspend_state, > + }, > +#endif > +}; > +static int policy; > + > #ifdef CONFIG_PM_SLEEP > > /* Routines for PM-transition notifications */ > @@ -146,6 +168,12 @@ struct kobject *power_kobj; > * > * store() accepts one of those strings, translates it into the > * proper enumerated value, and initiates a suspend transition. > + * > + * If policy is set to opportunistic, store() does not block until the > + * system resumes, and it will try to re-enter the state until another > + * state is requested. Suspend blockers are respected and the requested > + * state will only be entered when no suspend blockers are active. > + * Write "on" to cancel. > */ > static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > @@ -155,12 +183,13 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr, > int i; > > for (i = 0; i < PM_SUSPEND_MAX; i++) { > - if (pm_states[i] && valid_state(i)) > + if (pm_states[i] && policies[policy].valid_state(i)) > s += sprintf(s,"%s ", pm_states[i]); > } > #endif > #ifdef CONFIG_HIBERNATION > - s += sprintf(s, "%s\n", "disk"); > + if (!policy) > + s += sprintf(s, "%s\n", "disk"); > #else > if (s != buf) > /* convert the last space to a newline */ > @@ -173,7 +202,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, > const char *buf, size_t n) > { > #ifdef CONFIG_SUSPEND > - suspend_state_t state = PM_SUSPEND_STANDBY; > + suspend_state_t state = PM_SUSPEND_ON; > const char * const *s; > #endif > char *p; > @@ -184,7 +213,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, > len = p ? p - buf : n; > > /* First, check if we are requested to hibernate */ > - if (len == 4 && !strncmp(buf, "disk", len)) { > + if (len == 4 && !strncmp(buf, "disk", len) && !policy) { > error = hibernate(); > goto Exit; > } > @@ -195,7 +224,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, > break; > } > if (state < PM_SUSPEND_MAX && *s) > - error = enter_state(state); > + error = policies[policy].set_state(state); > #endif > > Exit: > @@ -204,6 +233,55 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, > > power_attr(state); > > +/** > + * policy - set policy for state > + */ > + > +static ssize_t policy_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + char *s = buf; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(policies); i++) { > + if (i == policy) > + s += sprintf(s, "[%s] ", policies[i].name); > + else > + s += sprintf(s, "%s ", policies[i].name); > + } > + if (s != buf) > + /* convert the last space to a newline */ > + *(s-1) = '\n'; > + return (s - buf); > +} > + > +static ssize_t policy_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t n) > +{ > + const char *s; > + char *p; > + int len; > + int i; > + > + p = memchr(buf, '\n', n); > + len = p ? p - buf : n; > + > + for (i = 0; i < ARRAY_SIZE(policies); i++) { > + s = policies[i].name; > + if (s && len == strlen(s) && !strncmp(buf, s, len)) { > + mutex_lock(&pm_mutex); > + policies[policy].set_state(PM_SUSPEND_ON); > + policy = i; > + mutex_unlock(&pm_mutex); > + return n; > + } > + } > + return -EINVAL; > +} > + > +power_attr(policy); > + > #ifdef CONFIG_PM_TRACE > int pm_trace_enabled; > Would you mind if I changed the above so that "policy" doesn't even show up if CONFIG_OPPORTUNISTIC_SUSPEND is unset? .... > +static void suspend_worker(struct work_struct *work) > +{ > + int ret; > + int entry_event_num; > + > + enable_suspend_blockers = true; > + while (!suspend_is_blocked()) { > + entry_event_num = current_event_num; > + > + if (debug_mask & DEBUG_SUSPEND) > + pr_info("suspend: enter suspend\n"); > + > + ret = pm_suspend(requested_suspend_state); > + > + if (debug_mask & DEBUG_EXIT_SUSPEND) > + pr_info_time("suspend: exit suspend, ret = %d ", ret); > + > + if (current_event_num == entry_event_num) > + pr_info("suspend: pm_suspend returned with no event\n"); Hmm, what exactly is this for? It looks like a debug thing to me. I'd use pr_debug() here and in both debug printk()s above. Would you agree? > + } > + enable_suspend_blockers = false; > +} > +static DECLARE_WORK(suspend_work, suspend_worker); > + > +/** > + * suspend_blocker_init() - Initialize a suspend blocker > + * @blocker: The suspend blocker to initialize. > + * @name: The name of the suspend blocker to show in debug messages. > + * > + * The suspend blocker struct and name must not be freed before calling > + * suspend_blocker_destroy. > + */ > +void suspend_blocker_init(struct suspend_blocker *blocker, const char *name) > +{ > + unsigned long irqflags = 0; > + > + WARN_ON(!name); > + > + if (debug_mask & DEBUG_SUSPEND_BLOCKER) > + pr_info("suspend_blocker_init name=%s\n", name); > + > + blocker->name = name; > + blocker->flags = SB_INITIALIZED; > + INIT_LIST_HEAD(&blocker->link); > + > + spin_lock_irqsave(&list_lock, irqflags); > + list_add(&blocker->link, &inactive_blockers); > + spin_unlock_irqrestore(&list_lock, irqflags); > +} > +EXPORT_SYMBOL(suspend_blocker_init); Is there a strong objection to changing that (and the other instances below) to EXPORT_SYMBOL_GPL? > + > +/** > + * suspend_blocker_destroy() - Destroy a suspend blocker > + * @blocker: The suspend blocker to destroy. > + */ > +void suspend_blocker_destroy(struct suspend_blocker *blocker) > +{ > + unsigned long irqflags; > + if (WARN_ON(!(blocker->flags & SB_INITIALIZED))) > + return; > + > + if (debug_mask & DEBUG_SUSPEND_BLOCKER) > + pr_info("suspend_blocker_destroy name=%s\n", blocker->name); > + > + spin_lock_irqsave(&list_lock, irqflags); > + blocker->flags &= ~SB_INITIALIZED; > + list_del(&blocker->link); > + if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers)) > + queue_work(suspend_work_queue, &suspend_work); > + spin_unlock_irqrestore(&list_lock, irqflags); > +} > +EXPORT_SYMBOL(suspend_blocker_destroy); > + > +/** > + * suspend_block() - Block suspend > + * @blocker: The suspend blocker to use > + * > + * It is safe to call this function from interrupt context. > + */ > +void suspend_block(struct suspend_blocker *blocker) > +{ > + unsigned long irqflags; > + > + if (WARN_ON(!(blocker->flags & SB_INITIALIZED))) > + return; > + > + spin_lock_irqsave(&list_lock, irqflags); > + blocker->flags |= SB_ACTIVE; > + list_del(&blocker->link); > + > + if (debug_mask & DEBUG_SUSPEND_BLOCKER) > + pr_info("suspend_block: %s\n", blocker->name); > + > + list_add(&blocker->link, &active_blockers); > + > + current_event_num++; > + spin_unlock_irqrestore(&list_lock, irqflags); > +} > +EXPORT_SYMBOL(suspend_block); > + > +/** > + * suspend_unblock() - Unblock suspend > + * @blocker: The suspend blocker to unblock. > + * > + * If no other suspend blockers block suspend, the system will suspend. > + * > + * It is safe to call this function from interrupt context. > + */ > +void suspend_unblock(struct suspend_blocker *blocker) > +{ > + unsigned long irqflags; > + > + if (WARN_ON(!(blocker->flags & SB_INITIALIZED))) > + return; > + > + spin_lock_irqsave(&list_lock, irqflags); > + > + if (debug_mask & DEBUG_SUSPEND_BLOCKER) > + pr_info("suspend_unblock: %s\n", blocker->name); > + > + list_del(&blocker->link); > + list_add(&blocker->link, &inactive_blockers); > + > + if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers)) > + queue_work(suspend_work_queue, &suspend_work); > + blocker->flags &= ~(SB_ACTIVE); > + if (blocker == &main_suspend_blocker) { > + if (debug_mask & DEBUG_SUSPEND) > + print_active_blockers_locked(); > + } > + spin_unlock_irqrestore(&list_lock, irqflags); > +} > +EXPORT_SYMBOL(suspend_unblock); > + > +/** > + * suspend_blocker_is_active() - Test if a suspend blocker is blocking suspend > + * @blocker: The suspend blocker to check. > + * > + * Returns true if the suspend_blocker is currently active. > + */ > +bool suspend_blocker_is_active(struct suspend_blocker *blocker) > +{ > + WARN_ON(!(blocker->flags & SB_INITIALIZED)); > + > + return !!(blocker->flags & SB_ACTIVE); > +} > +EXPORT_SYMBOL(suspend_blocker_is_active); > + > +bool request_suspend_valid_state(suspend_state_t state) > +{ > + return (state == PM_SUSPEND_ON) || valid_state(state); > +} > + > +int request_suspend_state(suspend_state_t state) > +{ > + unsigned long irqflags; > + > + if (!request_suspend_valid_state(state)) > + return -ENODEV; > + > + spin_lock_irqsave(&state_lock, irqflags); > + > + if (debug_mask & DEBUG_USER_STATE) > + pr_info_time("request_suspend_state: %s (%d->%d) at %lld ", > + state != PM_SUSPEND_ON ? "sleep" : "wakeup", > + requested_suspend_state, state, > + ktime_to_ns(ktime_get())); > + > + requested_suspend_state = state; > + if (state == PM_SUSPEND_ON) > + suspend_block(&main_suspend_blocker); > + else > + suspend_unblock(&main_suspend_blocker); > + spin_unlock_irqrestore(&state_lock, irqflags); > + return 0; > +} I think the two functions above should be static, shouldn't they? > +static int __init suspend_block_init(void) > +{ > + suspend_work_queue = create_singlethread_workqueue("suspend"); > + if (!suspend_work_queue) > + return -ENOMEM; > + > + suspend_blocker_init(&main_suspend_blocker, "main"); > + suspend_block(&main_suspend_blocker); > + return 0; > +} > + > +core_initcall(suspend_block_init); Hmm. Why don't you want to put that initialization into pm_init() (in kernel/power/main.c)? Also, we already have one PM workqueue. It is used for runtime PM, but I guess it may be used just as well for the opportunistic suspend. It is freezable, but would it hurt? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rafael J. Wysocki on 28 Apr 2010 17:20 On Wednesday 28 April 2010, Alan Stern wrote: > On Tue, 27 Apr 2010, [UTF-8] Arve Hjønnevåg wrote: > > > +For example, in cell phones or other embedded systems, where powering the screen > > +is a significant drain on the battery, suspend blockers can be used to allow > > +user-space to decide whether a keystroke received while the system is suspended > > +should cause the screen to be turned back on or allow the system to go back into > > +suspend. Use set_irq_wake or a platform specific api to make sure the keypad > > +interrupt wakes up the cpu. Once the keypad driver has resumed, the sequence of > > +events can look like this: > > + > > +- The Keypad driver gets an interrupt. It then calls suspend_block on the > > + keypad-scan suspend_blocker and starts scanning the keypad matrix. > > +- The keypad-scan code detects a key change and reports it to the input-event > > + driver. > > +- The input-event driver sees the key change, enqueues an event, and calls > > + suspend_block on the input-event-queue suspend_blocker. > > +- The keypad-scan code detects that no keys are held and calls suspend_unblock > > + on the keypad-scan suspend_blocker. > > +- The user-space input-event thread returns from select/poll, calls > > + suspend_block on the process-input-events suspend_blocker and then calls read > > + on the input-event device. > > +- The input-event driver dequeues the key-event and, since the queue is now > > + empty, it calls suspend_unblock on the input-event-queue suspend_blocker. > > +- The user-space input-event thread returns from read. If it determines that > > + the key should leave the screen off, it calls suspend_unblock on the > > + process_input_events suspend_blocker and then calls select or poll. The > > + system will automatically suspend again, since now no suspend blockers are > > + active. > > + > > + Key pressed Key released > > + | | > > +keypad-scan ++++++++++++++++++ > > +input-event-queue +++ +++ > > +process-input-events +++ +++ > > This is better than before, but it still isn't ideal. Here's what I > mean: > > > suspend blockers can be used to allow > > +user-space to decide whether a keystroke received while the system is suspended > > +should cause the screen to be turned back on or allow the system to go back into > > +suspend. > > That's not right. Handling the screen doesn't need suspend blockers: > The program decides what to do and then either turns on the screen or > else writes "mem" to /sys/power/state. What suspend blockers add is > the ability to resolve races and satisfy multiple constraints when > going into suspend -- which has nothing to do with operating the > screen. > > I _think_ what you're trying to get at can be expressed this way: > > Here's an example showing how a cell phone or other embedded > system can handle keystrokes (or other input events) in the > presence of suspend blockers. Use set_irq_wake... > > ... > > - The user-space input-event thread returns from read. It > carries out whatever activities are appropriate (for example, > powering up the display screen, running other programs, and so > on). When it is finished, it calls suspend_unblock on the > process_input_events suspend_blocker and then calls select or > poll. The system will automatically suspend again when it is > idle and no suspend blockers remain active. Yeah, that sounds better. Arve, what do you think? > > +/** > > + * suspend_block() - Block suspend > > + * @blocker: The suspend blocker to use > > + * > > + * It is safe to call this function from interrupt context. > > + */ > > +void suspend_block(struct suspend_blocker *blocker) > > +{ > > + unsigned long irqflags; > > + > > + if (WARN_ON(!(blocker->flags & SB_INITIALIZED))) > > + return; > > + > > + spin_lock_irqsave(&list_lock, irqflags); > > + blocker->flags |= SB_ACTIVE; > > + list_del(&blocker->link); > > + > > + if (debug_mask & DEBUG_SUSPEND_BLOCKER) > > + pr_info("suspend_block: %s\n", blocker->name); > > + > > + list_add(&blocker->link, &active_blockers); > > Here and in suspend_unblock(), you can use list_move() in place of > list_del() followed by list_add(). Indeed. And the debug statement might be moved out of the critical section IMHO. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arve Hjønnevåg on 28 Apr 2010 19:40 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: > On Wednesday 28 April 2010, Alan Stern wrote: >> On Tue, 27 Apr 2010, [UTF-8] Arve Hjønnevåg wrote: >> >> > +For example, in cell phones or other embedded systems, where powering the screen >> > +is a significant drain on the battery, suspend blockers can be used to allow >> > +user-space to decide whether a keystroke received while the system is suspended >> > +should cause the screen to be turned back on or allow the system to go back into >> > +suspend. Use set_irq_wake or a platform specific api to make sure the keypad >> > +interrupt wakes up the cpu. Once the keypad driver has resumed, the sequence of >> > +events can look like this: >> > + >> > +- The Keypad driver gets an interrupt. It then calls suspend_block on the >> > + �keypad-scan suspend_blocker and starts scanning the keypad matrix. >> > +- The keypad-scan code detects a key change and reports it to the input-event >> > + �driver. >> > +- The input-event driver sees the key change, enqueues an event, and calls >> > + �suspend_block on the input-event-queue suspend_blocker. >> > +- The keypad-scan code detects that no keys are held and calls suspend_unblock >> > + �on the keypad-scan suspend_blocker. >> > +- The user-space input-event thread returns from select/poll, calls >> > + �suspend_block on the process-input-events suspend_blocker and then calls read >> > + �on the input-event device. >> > +- The input-event driver dequeues the key-event and, since the queue is now >> > + �empty, it calls suspend_unblock on the input-event-queue suspend_blocker. >> > +- The user-space input-event thread returns from read. If it determines that >> > + �the key should leave the screen off, it calls suspend_unblock on the >> > + �process_input_events suspend_blocker and then calls select or poll. The >> > + �system will automatically suspend again, since now no suspend blockers are >> > + �active. >> > + >> > + � � � � � � � � Key pressed � Key released >> > + � � � � � � � � � � | � � � � � � | >> > +keypad-scan � � � � �++++++++++++++++++ >> > +input-event-queue � � � �+++ � � � � �+++ >> > +process-input-events � � � +++ � � � � �+++ >> >> This is better than before, but it still isn't ideal. �Here's what I >> mean: >> >> > �suspend blockers can be used to allow >> > +user-space to decide whether a keystroke received while the system is suspended >> > +should cause the screen to be turned back on or allow the system to go back into >> > +suspend. >> >> That's not right. �Handling the screen doesn't need suspend blockers: >> The program decides what to do and then either turns on the screen or >> else writes "mem" to /sys/power/state. That does not work though. Unless every key turns the screen on you will have a race every time the user presses a key you want to ignore. >> �What suspend blockers add is >> the ability to resolve races and satisfy multiple constraints when >> going into suspend -- which has nothing to do with operating the >> screen. I'm not sure I agree with this. You cannot reliably turn the screen on from user space when the user presses a wakeup-key without suspend blockers. >> >> I _think_ what you're trying to get at can be expressed this way: >> >> � � � Here's an example showing how a cell phone or other embedded >> � � � system can handle keystrokes (or other input events) in the >> � � � presence of suspend blockers. �Use set_irq_wake... OK, but the last version was what you (Alan) suggested last year. >> >> � � � ... >> >> � � � - The user-space input-event thread returns from read. �It >> � � � carries out whatever activities are appropriate (for example, >> � � � powering up the display screen, running other programs, and so >> � � � on). �When it is finished, it calls suspend_unblock on the >> � � � process_input_events suspend_blocker and then calls select or >> � � � poll. �The system will automatically suspend again when it is >> � � � idle and no suspend blockers remain active. > > Yeah, that sounds better. �Arve, what do you think? > Idle is irrelevant and needs to be removed. This new last step is also no longer a concrete example, but if you really think is it better I can change it. >> > +/** >> > + * suspend_block() - Block suspend >> > + * @blocker: � � � The suspend blocker to use >> > + * >> > + * It is safe to call this function from interrupt context. >> > + */ >> > +void suspend_block(struct suspend_blocker *blocker) >> > +{ >> > + � unsigned long irqflags; >> > + >> > + � if (WARN_ON(!(blocker->flags & SB_INITIALIZED))) >> > + � � � � � return; >> > + >> > + � spin_lock_irqsave(&list_lock, irqflags); >> > + � blocker->flags |= SB_ACTIVE; >> > + � list_del(&blocker->link); >> > + >> > + � if (debug_mask & DEBUG_SUSPEND_BLOCKER) >> > + � � � � � pr_info("suspend_block: %s\n", blocker->name); >> > + >> > + � list_add(&blocker->link, &active_blockers); >> >> Here and in suspend_unblock(), you can use list_move() in place of >> list_del() followed by list_add(). > OK. > Indeed. �And the debug statement might be moved out of the critical section IMHO. > If I move the debug statements out of the critical section you could end entering suspend while the debug log claims a suspend blocker was active, but I can move the debug statement to the start of the critical section. -- Arve Hj�nnev�g -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arve Hjønnevåg on 28 Apr 2010 23:40 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: .... >> >> +/** >> + * � policy - set policy for state >> + */ >> + >> +static ssize_t policy_show(struct kobject *kobj, >> + � � � � � � � � � � � �struct kobj_attribute *attr, char *buf) >> +{ >> + � � char *s = buf; >> + � � int i; >> + >> + � � for (i = 0; i < ARRAY_SIZE(policies); i++) { >> + � � � � � � if (i == policy) >> + � � � � � � � � � � s += sprintf(s, "[%s] ", policies[i].name); >> + � � � � � � else >> + � � � � � � � � � � s += sprintf(s, "%s ", policies[i].name); >> + � � } >> + � � if (s != buf) >> + � � � � � � /* convert the last space to a newline */ >> + � � � � � � *(s-1) = '\n'; >> + � � return (s - buf); >> +} >> + >> +static ssize_t policy_store(struct kobject *kobj, >> + � � � � � � � � � � � � struct kobj_attribute *attr, >> + � � � � � � � � � � � � const char *buf, size_t n) >> +{ >> + � � const char *s; >> + � � char *p; >> + � � int len; >> + � � int i; >> + >> + � � p = memchr(buf, '\n', n); >> + � � len = p ? p - buf : n; >> + >> + � � for (i = 0; i < ARRAY_SIZE(policies); i++) { >> + � � � � � � s = policies[i].name; >> + � � � � � � if (s && len == strlen(s) && !strncmp(buf, s, len)) { >> + � � � � � � � � � � mutex_lock(&pm_mutex); >> + � � � � � � � � � � policies[policy].set_state(PM_SUSPEND_ON); >> + � � � � � � � � � � policy = i; >> + � � � � � � � � � � mutex_unlock(&pm_mutex); >> + � � � � � � � � � � return n; >> + � � � � � � } >> + � � } >> + � � return -EINVAL; >> +} >> + >> +power_attr(policy); >> + >> �#ifdef CONFIG_PM_TRACE >> �int pm_trace_enabled; >> > > Would you mind if I changed the above so that "policy" doesn't even show up > if CONFIG_OPPORTUNISTIC_SUSPEND is unset? > I don't mind, but It did not seem worth the trouble to hide it. It will only list the supported policies, and it is easy to add or remove policies this way. > ... >> +static void suspend_worker(struct work_struct *work) >> +{ >> + � � int ret; >> + � � int entry_event_num; >> + >> + � � enable_suspend_blockers = true; >> + � � while (!suspend_is_blocked()) { >> + � � � � � � entry_event_num = current_event_num; >> + >> + � � � � � � if (debug_mask & DEBUG_SUSPEND) >> + � � � � � � � � � � pr_info("suspend: enter suspend\n"); >> + >> + � � � � � � ret = pm_suspend(requested_suspend_state); >> + >> + � � � � � � if (debug_mask & DEBUG_EXIT_SUSPEND) >> + � � � � � � � � � � pr_info_time("suspend: exit suspend, ret = %d ", ret); >> + >> + � � � � � � if (current_event_num == entry_event_num) >> + � � � � � � � � � � pr_info("suspend: pm_suspend returned with no event\n"); > > Hmm, what exactly is this for? �It looks like a debug thing to me. �I'd use > pr_debug() here and in both debug printk()s above. �Would you agree? > If the driver that caused the wakeup does not use suspend blockers, we the only choice is to try to suspend again. I want to know if this happened. The stats patch disable this printk by default since it will show up in the stats, and the timeout patch (not included here) delays the retry. .... >> +EXPORT_SYMBOL(suspend_blocker_init); > > Is there a strong objection to changing that (and the other instances below) to > EXPORT_SYMBOL_GPL? > I don't know if it is a strong objection, but I prefer that this api is available to all drivers. I don't want to prevent a user from using opportunistic suspend because a non-gpl driver could not use suspend blockers. I changed the suspend blocking work functions to be gpl only though, since they are not required, and the workqueue api is available to gpl code anyway. .... >> +bool request_suspend_valid_state(suspend_state_t state) >> +{ >> + � � return (state == PM_SUSPEND_ON) || valid_state(state); >> +} >> + >> +int request_suspend_state(suspend_state_t state) >> +{ >> + � � unsigned long irqflags; >> + >> + � � if (!request_suspend_valid_state(state)) >> + � � � � � � return -ENODEV; >> + >> + � � spin_lock_irqsave(&state_lock, irqflags); >> + >> + � � if (debug_mask & DEBUG_USER_STATE) >> + � � � � � � pr_info_time("request_suspend_state: %s (%d->%d) at %lld ", >> + � � � � � � � � � � � � �state != PM_SUSPEND_ON ? "sleep" : "wakeup", >> + � � � � � � � � � � � � �requested_suspend_state, state, >> + � � � � � � � � � � � � �ktime_to_ns(ktime_get())); >> + >> + � � requested_suspend_state = state; >> + � � if (state == PM_SUSPEND_ON) >> + � � � � � � suspend_block(&main_suspend_blocker); >> + � � else >> + � � � � � � suspend_unblock(&main_suspend_blocker); >> + � � spin_unlock_irqrestore(&state_lock, irqflags); >> + � � return 0; >> +} > > I think the two functions above should be static, shouldn't they? No, they are used from main.c. > >> +static int __init suspend_block_init(void) >> +{ >> + � � suspend_work_queue = create_singlethread_workqueue("suspend"); >> + � � if (!suspend_work_queue) >> + � � � � � � return -ENOMEM; >> + >> + � � suspend_blocker_init(&main_suspend_blocker, "main"); >> + � � suspend_block(&main_suspend_blocker); >> + � � return 0; >> +} >> + >> +core_initcall(suspend_block_init); > > Hmm. �Why don't you want to put that initialization into pm_init() (in > kernel/power/main.c)? It was not needed before, but I changed pm_init to call suspend_block_init after creating pm_wq. > > Also, we already have one PM workqueue. �It is used for runtime PM, but I guess > it may be used just as well for the opportunistic suspend. �It is freezable, > but would it hurt? No, it works, the freezable flag is just ignored when I call pm_suspend and I don't run anything else on the workqueue while threads are frozen. It does need to be a single threaded workqueue though, so make sure you don't just change that. -- Arve Hj�nnev�g -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 30 Apr 2010 13:10
Hello, On 04/29/2010 11:16 PM, Rafael J. Wysocki wrote: >>> Also, we already have one PM workqueue. It is used for runtime PM, but I guess >>> it may be used just as well for the opportunistic suspend. It is freezable, >>> but would it hurt? >> >> No, it works, the freezable flag is just ignored when I call >> pm_suspend and I don't run anything else on the workqueue while >> threads are frozen. It does need to be a single threaded workqueue >> though, so make sure you don't just change that. > > Freezable workqueues have to be singlethread or else there will be unfixable > races, so you can safely assume things will stay as they are in this respect. Rafael, can you elaborate a bit more on this? Just in case I missed something while doing cmwq as it currently doesn't have such limit. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |