| Message ID | 20240909080726.16811-1-xiongxin@kylinos.cn (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] PM: sleep: Optimize the pm_debug_messages_should_print() condition | expand |
On Mon, Sep 9, 2024 at 10:07 AM Xiong Xin <xiongxin@kylinos.cn> wrote: > > From: xiongxin <xiongxin@kylinos.cn> > > pm_pr_dbg() is useful when debugging suspend and hibernate processes. In > commit cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg > messages at suspend/resume") using pm_suspend_target_state to limits the > output range of pm_pr_dbg(), causes the original pm_pr_dbg() output > range to change. > > In the suspend process, pm_pr_dbg() is called before setting > pm_suspend_target_state. As a result, this part of the log cannot be > output. > > pm_pr_dbg() also outputs debug logs for hibernate, but > pm_suspend_target_state is not set, resulting in hibernate debug logs > can only be output through dynamic debug, which is very inconvenient. > > Expand the scope of the state variable in state_store() and add judgment > on it in pm_debug_messages_should_print() to extend the debugging output > of pm_pr_dbg() to suspend and hibernate processes. > > Fixes: cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"). > Signed-off-by: xiongxin <xiongxin@kylinos.cn> > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > --- > kernel/power/main.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/kernel/power/main.c b/kernel/power/main.c > index a9e0693aaf69..a376107efbb4 100644 > --- a/kernel/power/main.c > +++ b/kernel/power/main.c > @@ -559,6 +559,8 @@ late_initcall(pm_debugfs_init); > > #endif /* CONFIG_PM_SLEEP */ > > +static suspend_state_t pm_state = PM_SUSPEND_ON; > + > #ifdef CONFIG_PM_SLEEP_DEBUG > /* > * pm_print_times: print time taken by devices to suspend and resume. > @@ -613,7 +615,9 @@ bool pm_debug_messages_on __read_mostly; > > bool pm_debug_messages_should_print(void) > { > - return pm_debug_messages_on && pm_suspend_target_state != PM_SUSPEND_ON; > + return pm_debug_messages_on && > + (pm_suspend_target_state != PM_SUSPEND_ON || > + pm_state != PM_SUSPEND_ON); > } > EXPORT_SYMBOL_GPL(pm_debug_messages_should_print); > > @@ -715,7 +719,6 @@ static suspend_state_t decode_state(const char *buf, size_t n) > static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, > const char *buf, size_t n) > { > - suspend_state_t state; > int error; > > error = pm_autosleep_lock(); > @@ -727,18 +730,20 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, > goto out; > } > > - state = decode_state(buf, n); > - if (state < PM_SUSPEND_MAX) { > - if (state == PM_SUSPEND_MEM) > - state = mem_sleep_current; > + pm_state = decode_state(buf, n); > + if (pm_state < PM_SUSPEND_MAX) { > + if (pm_state == PM_SUSPEND_MEM) > + pm_state = mem_sleep_current; > > - error = pm_suspend(state); > - } else if (state == PM_SUSPEND_MAX) { > + error = pm_suspend(pm_state); > + } else if (pm_state == PM_SUSPEND_MAX) { > error = hibernate(); > } else { > error = -EINVAL; > } > > + pm_state = PM_SUSPEND_ON; > + > out: > pm_autosleep_unlock(); > return error ? error : n; > -- This would only work for transitions started via /sys/power/state AFAICS, but there are other ways to start them (eg. autosleep).
在 2024/9/11 02:08, Rafael J. Wysocki 写道: > On Mon, Sep 9, 2024 at 10:07 AM Xiong Xin <xiongxin@kylinos.cn> wrote: >> >> From: xiongxin <xiongxin@kylinos.cn> >> >> pm_pr_dbg() is useful when debugging suspend and hibernate processes. In >> commit cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg >> messages at suspend/resume") using pm_suspend_target_state to limits the >> output range of pm_pr_dbg(), causes the original pm_pr_dbg() output >> range to change. >> >> In the suspend process, pm_pr_dbg() is called before setting >> pm_suspend_target_state. As a result, this part of the log cannot be >> output. >> >> pm_pr_dbg() also outputs debug logs for hibernate, but >> pm_suspend_target_state is not set, resulting in hibernate debug logs >> can only be output through dynamic debug, which is very inconvenient. >> >> Expand the scope of the state variable in state_store() and add judgment >> on it in pm_debug_messages_should_print() to extend the debugging output >> of pm_pr_dbg() to suspend and hibernate processes. >> >> Fixes: cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"). >> Signed-off-by: xiongxin <xiongxin@kylinos.cn> >> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> kernel/power/main.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/kernel/power/main.c b/kernel/power/main.c >> index a9e0693aaf69..a376107efbb4 100644 >> --- a/kernel/power/main.c >> +++ b/kernel/power/main.c >> @@ -559,6 +559,8 @@ late_initcall(pm_debugfs_init); >> >> #endif /* CONFIG_PM_SLEEP */ >> >> +static suspend_state_t pm_state = PM_SUSPEND_ON; >> + >> #ifdef CONFIG_PM_SLEEP_DEBUG >> /* >> * pm_print_times: print time taken by devices to suspend and resume. >> @@ -613,7 +615,9 @@ bool pm_debug_messages_on __read_mostly; >> >> bool pm_debug_messages_should_print(void) >> { >> - return pm_debug_messages_on && pm_suspend_target_state != PM_SUSPEND_ON; >> + return pm_debug_messages_on && >> + (pm_suspend_target_state != PM_SUSPEND_ON || >> + pm_state != PM_SUSPEND_ON); >> } >> EXPORT_SYMBOL_GPL(pm_debug_messages_should_print); >> >> @@ -715,7 +719,6 @@ static suspend_state_t decode_state(const char *buf, size_t n) >> static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, >> const char *buf, size_t n) >> { >> - suspend_state_t state; >> int error; >> >> error = pm_autosleep_lock(); >> @@ -727,18 +730,20 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, >> goto out; >> } >> >> - state = decode_state(buf, n); >> - if (state < PM_SUSPEND_MAX) { >> - if (state == PM_SUSPEND_MEM) >> - state = mem_sleep_current; >> + pm_state = decode_state(buf, n); >> + if (pm_state < PM_SUSPEND_MAX) { >> + if (pm_state == PM_SUSPEND_MEM) >> + pm_state = mem_sleep_current; >> >> - error = pm_suspend(state); >> - } else if (state == PM_SUSPEND_MAX) { >> + error = pm_suspend(pm_state); >> + } else if (pm_state == PM_SUSPEND_MAX) { >> error = hibernate(); >> } else { >> error = -EINVAL; >> } >> >> + pm_state = PM_SUSPEND_ON; >> + >> out: >> pm_autosleep_unlock(); >> return error ? error : n; >> -- > > This would only work for transitions started via /sys/power/state > AFAICS, but there are other ways to start them (eg. autosleep). Yes, the current patch does not yet control the resume process in kernel/power/hibernate.c As suggested earlier, to define a global variable, such as bool is_pm_process, to determin whether in the suspend, hibernate, resume process?
diff --git a/kernel/power/main.c b/kernel/power/main.c index a9e0693aaf69..a376107efbb4 100644 --- a/kernel/power/main.c +++ b/kernel/power/main.c @@ -559,6 +559,8 @@ late_initcall(pm_debugfs_init); #endif /* CONFIG_PM_SLEEP */ +static suspend_state_t pm_state = PM_SUSPEND_ON; + #ifdef CONFIG_PM_SLEEP_DEBUG /* * pm_print_times: print time taken by devices to suspend and resume. @@ -613,7 +615,9 @@ bool pm_debug_messages_on __read_mostly; bool pm_debug_messages_should_print(void) { - return pm_debug_messages_on && pm_suspend_target_state != PM_SUSPEND_ON; + return pm_debug_messages_on && + (pm_suspend_target_state != PM_SUSPEND_ON || + pm_state != PM_SUSPEND_ON); } EXPORT_SYMBOL_GPL(pm_debug_messages_should_print); @@ -715,7 +719,6 @@ static suspend_state_t decode_state(const char *buf, size_t n) static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t n) { - suspend_state_t state; int error; error = pm_autosleep_lock(); @@ -727,18 +730,20 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr, goto out; } - state = decode_state(buf, n); - if (state < PM_SUSPEND_MAX) { - if (state == PM_SUSPEND_MEM) - state = mem_sleep_current; + pm_state = decode_state(buf, n); + if (pm_state < PM_SUSPEND_MAX) { + if (pm_state == PM_SUSPEND_MEM) + pm_state = mem_sleep_current; - error = pm_suspend(state); - } else if (state == PM_SUSPEND_MAX) { + error = pm_suspend(pm_state); + } else if (pm_state == PM_SUSPEND_MAX) { error = hibernate(); } else { error = -EINVAL; } + pm_state = PM_SUSPEND_ON; + out: pm_autosleep_unlock(); return error ? error : n;