power: suspend: Replace dpm_watchdog by sleep timer
PATCH v2 1/1 power: suspend: Replace dpm_watchdog by sleep timer
Quoted 29 lines
Quoted 1 line
On Wed, Oct 21, 2020 at 4:09 AM Joseph Jang <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
Since
dpm_watchdogjust cover device power management, we proposedsleep
Quoted 4 lines
Quoted 4 lines
timer to cover not only device power management hang issues, but also core power management hand issue.
Add sleep timer and timeout handler to prevent device stuck during
suspend/
Quoted 5 lines
Quoted 4 lines
resume process. The timeout handler will dump disk sleep task at first round timeout and trigger kernel panic at second round timeout. The default timer for each round is defined in
CONFIG_PM_SLEEP_TIMER_TIMEOUT.Quoted 7 lines
Let me repeat the point that Greg has made already: Please don't replace the existing watchdog with something else, but try to extend it to cover your use case.
As it stands, the patch is not applicable IMV.
Thanks!
Thank you Rafael's promptly response.
Sincedmp_watchdogwas implement indrivers/base/power/main.cand it use stack memory for timer struct. It is a little bit hard to use its API inkernel/power/suspend.c.
And from the call flow, we prefer to start/stop watchog timer insuspend.conly. It will also cover device power management as well.
Quoted 7 lines
If you don't want to change
Kconfig, I can keep originalDPM_WATCHDOG. But it is not easy to extend driver base API into core power base. Am I correct?If you have some suggestion, please feel free to let me know, I will follow up.
Thank you,
Joseph.
Let me try to explain why cannot use current dpm_watchdog API, because it use stack memory and has limitation.
/**
* dpm_watchdog_set - Enable pm watchdog for given device.
* @wd: Watchdog. Must be allocated on the stack.
* @dev: Device to handle.
*/
static void dpm_watchdog_set(struct dpm_watchdog *wd, struct device *dev)
{
struct timer_list *timer = &wd->timer;
wd->dev = dev;
wd->tsk = current;
timer_setup_on_stack(timer, dpm_watchdog_handler, 0); <== setup timer on
stack memory ....
/* use same timeout value for both suspend and resume */
timer->expires = jiffies + HZ * CONFIG_DPM_WATCHDOG_TIMEOUT;
add_timer(timer);
}If you could take a look at my patch again, you can see we need to start watchdog at end of function like s2idle_enter(). We cannot use stack memory for timer because it will be free when go back to caller.
That is why I am creating new API to start/stop watchdog timer which use global memory.
/**
* pm_suspend_default_s2idle - Check if suspend-to-idle is the default
suspend.
*
@@ -89,6 +92,8 @@ static void s2idle_enter(void)
{
trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
+ stop_sleep_timer(&st);
+
raw_spin_lock_irq(&s2idle_lock); if (pm_wakeup_pending())
goto out;
@@ -114,6 +119,8 @@ static void s2idle_enter(void)
s2idle_state = S2IDLE_STATE_NONE; raw_spin_unlock_irq(&s2idle_lock);
+ start_sleep_timer(&st);
+
trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, false);
}
Base on that, could you help to review if we could create new API to start/stop watchdog timer for core power management?
Thank you,
Joseph.