Re: [PATCH] PM: remove device suspend and resume from "freeze" flow
From: Rafael J. Wysocki <hidden>
Date: 2013-11-14 17:08:29
Also in:
lkml
On 11/11/2013 6:18 AM, Yong Wang wrote:
"freeze" was originally designed to be equal to frozen processes + suspended devices + idle processors. Frankly speaking, "freeze" has not been widely adopted so far since it was introduced. Following might be some reasons of why that is the case. 1. As traditional s3 is going away, there will be more devices supporting connected standby instead. Unlike traditional s3, connected standby is a power state that devices enter and exit more frequently. Therefore, the entry and exit latency of connected standby state must be as short as possible in order to minimize the impact on average power and to achieve a decent battery life. With the current design of "freeze", device suspend and resume contribute to the overall entry and exit latency of "freeze" state significantly. Therefore removing device suspend and resume could cut the latency dramatically.
Yes, it could.
2. With the interaction between runtime PM and system PM, devices that have been runtime suspended before system enters "freeze" state will first be runtime resumed and then suspended again by their suspend callback. When system exits "freeze" state, all devices will be resumed despite the fact that only some devices need to participate in handling the wakeup event. Then devices that were previously runtime suspended will be runtime suspended again. Wouldn't it be better if we could leave those devices runtime suspended during "freeze" and only have necessary devices runtime resumed to handle wakeup event when system exits "freeze" state.
That only is correct for PCI devices at the moment, if I remember correctly. Moreover, it doesn't have to be this way and we may just remove that thing.
3. In practice, we have found many device drivers that will not put their devices into proper low power states because traditional s3 will yank the platform power as a whole as the final step of s3. Because many driver developers are still holding that assumption and assume that someone else will help do all the power setting correctly as the final step of s3, they simply leave their devices in a high power state. In contract, many driver developers are able to do the right thing with their runtime PM code because they know they are on their own and no one else is going to help them put their devices into a proper low power state. Due to the reasons listed above, I propose to remove device suspend and rsume from "freeze" flow and and make it equal to frozen processes + idle processors which I belive will make "freeze" more useful.
I'm generally fine with this change, but your point 2 above is quite arguable. Thanks, Rafael
quoted hunk
Signed-off-by: Yong Wang <redacted> --- kernel/power/suspend.c | 44 ++++++++++++++++++++++++++------------------ 1 files changed, 26 insertions(+), 18 deletions(-)diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 62ee437..4501cb9 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c@@ -184,10 +184,12 @@ static int suspend_enter(suspend_state_t state, bool *wakeup) goto Platform_finish; } - error = dpm_suspend_end(PMSG_SUSPEND); - if (error) { - printk(KERN_ERR "PM: Some devices failed to power down\n"); - goto Platform_finish; + if (need_suspend_ops(state)) { + error = dpm_suspend_end(PMSG_SUSPEND); + if (error) { + printk(KERN_ERR "PM: Some devices failed to power down\n"); + goto Platform_finish; + } } if (need_suspend_ops(state) && suspend_ops->prepare_late) {@@ -239,7 +241,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup) if (need_suspend_ops(state) && suspend_ops->wake) suspend_ops->wake(); - dpm_resume_start(PMSG_RESUME); + if (need_suspend_ops(state)) + dpm_resume_start(PMSG_RESUME); Platform_finish: if (need_suspend_ops(state) && suspend_ops->finish)@@ -266,16 +269,19 @@ int suspend_devices_and_enter(suspend_state_t state) if (error) goto Close; } - suspend_console(); - suspend_test_start(); - error = dpm_suspend_start(PMSG_SUSPEND); - if (error) { - pr_err("PM: Some devices failed to suspend, or early wake event detected\n"); - goto Recover_platform; + + if (need_suspend_ops(state)) { + suspend_console(); + suspend_test_start(); + error = dpm_suspend_start(PMSG_SUSPEND); + if (error) { + pr_err("PM: Some devices failed to suspend, or early wake event detected\n"); + goto Recover_platform; + } + suspend_test_finish("suspend devices"); + if (suspend_test(TEST_DEVICES)) + goto Recover_platform; } - suspend_test_finish("suspend devices"); - if (suspend_test(TEST_DEVICES)) - goto Recover_platform; do { error = suspend_enter(state, &wakeup);@@ -283,10 +289,12 @@ int suspend_devices_and_enter(suspend_state_t state) && suspend_ops->suspend_again && suspend_ops->suspend_again()); Resume_devices: - suspend_test_start(); - dpm_resume_end(PMSG_RESUME); - suspend_test_finish("resume devices"); - resume_console(); + if (need_suspend_ops(state)) { + suspend_test_start(); + dpm_resume_end(PMSG_RESUME); + suspend_test_finish("resume devices"); + resume_console(); + } Close: if (need_suspend_ops(state) && suspend_ops->end) suspend_ops->end();