From a3f7047f00d2d283278b2d5448468596cfeea6ac Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 23 Oct 2023 21:02:25 +0800 Subject: [PATCH 1/4] hibernate-resume: add missing static for arg_info --- src/hibernate-resume/hibernate-resume.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hibernate-resume/hibernate-resume.c b/src/hibernate-resume/hibernate-resume.c index 48225201edf..9d81332f26c 100644 --- a/src/hibernate-resume/hibernate-resume.c +++ b/src/hibernate-resume/hibernate-resume.c @@ -12,7 +12,7 @@ #include "parse-util.h" #include "static-destruct.h" -HibernateInfo arg_info = {}; +static HibernateInfo arg_info = {}; STATIC_DESTRUCTOR_REGISTER(arg_info, hibernate_info_done); From 080105d77a6064026400d02ff66397e82698f187 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 21 Oct 2023 19:53:04 +0800 Subject: [PATCH 2/4] hibernate-util: add missing assertion --- src/shared/hibernate-util.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shared/hibernate-util.c b/src/shared/hibernate-util.c index 4a05320a40e..51f64aa2b19 100644 --- a/src/shared/hibernate-util.c +++ b/src/shared/hibernate-util.c @@ -408,6 +408,8 @@ static int get_proc_meminfo_active(unsigned long long *ret) { unsigned long long active; int r; + assert(ret); + r = get_proc_field("/proc/meminfo", "Active(anon)", WHITESPACE, &active_str); if (r < 0) return log_debug_errno(r, "Failed to retrieve Active(anon) from /proc/meminfo: %m"); From 1f82c21dceb2db0706c8f734ed5d8d7febc86477 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 21 Oct 2023 00:21:20 +0800 Subject: [PATCH 3/4] sleep-config: remove HibernateState= & HybridSleepState=, restrict SuspendState= not to include "disk" I don't know why these existed in the first place, but as I justified in the comments, it's simply not sensible to allow HibernateState= or HybridSleepState= to take values other than 'disk'. So let's just remove those options. Also, SuspendState= should not contain 'disk'. --- man/systemd-sleep.conf.xml | 26 ++++------- src/shared/sleep-config.c | 95 +++++++++++++++++++++++++++----------- src/sleep/sleep.conf | 3 -- 3 files changed, 77 insertions(+), 47 deletions(-) diff --git a/man/systemd-sleep.conf.xml b/man/systemd-sleep.conf.xml index 67933ebaf20..b33699b6c56 100644 --- a/man/systemd-sleep.conf.xml +++ b/man/systemd-sleep.conf.xml @@ -126,8 +126,8 @@ AllowSuspend= AllowHibernation= - AllowSuspendThenHibernate= AllowHybridSleep= + AllowSuspendThenHibernate= By default any power-saving mode is advertised if possible (i.e. the kernel supports that mode, the necessary resources are available). Those @@ -144,14 +144,12 @@ - SuspendMode= HibernateMode= HybridSleepMode= The string to be written to /sys/power/disk by, respectively, - systemd-suspend.service8, - systemd-hibernate.service8, - or + systemd-hibernate.service8 + and systemd-hybrid-sleep.service8. More than one value can be specified by separating multiple values with whitespace. They will be tried in turn, until one is written without error. If none of the writes succeed, the operation will @@ -162,6 +160,9 @@ url="https://www.kernel.org/doc/html/latest/admin-guide/pm/sleep-states.html#basic-sysfs-interfaces-for-system-suspend-and-hibernation">the kernel documentation for more details. + Note that hybrid sleep corresponds to the suspend disk mode. If HybridSleepMode= + is overridden, you might get plain hibernation instead. + systemd-suspend-then-hibernate.service8 uses the value of SuspendMode= when suspending and the value of @@ -172,18 +173,12 @@ SuspendState= - HibernateState= - HybridSleepState= - The string to be written to /sys/power/state by, respectively, - systemd-suspend.service8, - systemd-hibernate.service8, - or - systemd-hybrid-sleep.service8. + The string to be written to /sys/power/state by + systemd-suspend.service8. More than one value can be specified by separating multiple values with whitespace. They will be tried in turn, until one is written without error. If none of the writes succeed, the operation will - be aborted. - + be aborted. The allowed set of values is determined by the kernel and is shown in the file itself (use cat /sys/power/state to display). See systemd-suspend-then-hibernate.service8 - uses the value of SuspendState= when suspending and the value of - HibernateState= when hibernating. + uses this value when suspending. diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index deba2b1c588..03d04775e61 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -33,18 +33,55 @@ static const char* const sleep_operation_table[_SLEEP_OPERATION_MAX] = { DEFINE_STRING_TABLE_LOOKUP(sleep_operation, SleepOperation); +static char* const* const sleep_default_state_table[_SLEEP_OPERATION_CONFIG_MAX] = { + [SLEEP_SUSPEND] = STRV_MAKE("mem", "standby", "freeze"), + [SLEEP_HIBERNATE] = STRV_MAKE("disk"), + [SLEEP_HYBRID_SLEEP] = STRV_MAKE("disk"), +}; + +static char* const* const sleep_default_mode_table[_SLEEP_OPERATION_CONFIG_MAX] = { + /* Not used by SLEEP_SUSPEND */ + [SLEEP_HIBERNATE] = STRV_MAKE("platform", "shutdown"), + [SLEEP_HYBRID_SLEEP] = STRV_MAKE("suspend", "platform", "shutdown"), +}; + SleepConfig* sleep_config_free(SleepConfig *sc) { if (!sc) return NULL; for (SleepOperation i = 0; i < _SLEEP_OPERATION_CONFIG_MAX; i++) { - strv_free(sc->modes[i]); strv_free(sc->states[i]); + strv_free(sc->modes[i]); } return mfree(sc); } +static void sleep_config_validate_state_and_mode(SleepConfig *sc) { + assert(sc); + + /* So we should really not allow setting SuspendState= to 'disk', which means hibernation. We have + * SLEEP_HIBERNATE for proper hibernation support, which includes checks for resume support (through + * EFI variable or resume= kernel command line option). It's simply not sensible to call the suspend + * operation but eventually do an unsafe hibernation. */ + if (strv_contains(sc->states[SLEEP_SUSPEND], "disk")) { + strv_remove(sc->states[SLEEP_SUSPEND], "disk"); + log_warning("Sleep state 'disk' is not supported by operation %s, ignoring.", + sleep_operation_to_string(SLEEP_SUSPEND)); + } + assert(!sc->modes[SLEEP_SUSPEND]); + + /* People should use hybrid-sleep instead of setting HibernateMode=suspend. Warn about it but don't + * drop it in this case. */ + if (strv_contains(sc->modes[SLEEP_HIBERNATE], "suspend")) + log_warning("Sleep mode 'suspend' should not be used by operation %s. Please use %s instead.", + sleep_operation_to_string(SLEEP_HIBERNATE), sleep_operation_to_string(SLEEP_HYBRID_SLEEP)); + + if (!strv_contains(sc->modes[SLEEP_HYBRID_SLEEP], "suspend")) + log_warning("Sleep mode 'suspend' is not set for operation %s. This would likely result in a plain hibernation.", + sleep_operation_to_string(SLEEP_HYBRID_SLEEP)); +} + int parse_sleep_config(SleepConfig **ret) { _cleanup_(sleep_config_freep) SleepConfig *sc = NULL; int allow_suspend = -1, allow_hibernate = -1, allow_s2h = -1, allow_hybrid_sleep = -1; @@ -60,20 +97,22 @@ int parse_sleep_config(SleepConfig **ret) { }; const ConfigTableItem items[] = { - { "Sleep", "AllowSuspend", config_parse_tristate, 0, &allow_suspend }, - { "Sleep", "AllowHibernation", config_parse_tristate, 0, &allow_hibernate }, - { "Sleep", "AllowSuspendThenHibernate", config_parse_tristate, 0, &allow_s2h }, - { "Sleep", "AllowHybridSleep", config_parse_tristate, 0, &allow_hybrid_sleep }, + { "Sleep", "AllowSuspend", config_parse_tristate, 0, &allow_suspend }, + { "Sleep", "AllowHibernation", config_parse_tristate, 0, &allow_hibernate }, + { "Sleep", "AllowSuspendThenHibernate", config_parse_tristate, 0, &allow_s2h }, + { "Sleep", "AllowHybridSleep", config_parse_tristate, 0, &allow_hybrid_sleep }, - { "Sleep", "SuspendMode", config_parse_strv, 0, sc->modes + SLEEP_SUSPEND }, - { "Sleep", "SuspendState", config_parse_strv, 0, sc->states + SLEEP_SUSPEND }, - { "Sleep", "HibernateMode", config_parse_strv, 0, sc->modes + SLEEP_HIBERNATE }, - { "Sleep", "HibernateState", config_parse_strv, 0, sc->states + SLEEP_HIBERNATE }, - { "Sleep", "HybridSleepMode", config_parse_strv, 0, sc->modes + SLEEP_HYBRID_SLEEP }, - { "Sleep", "HybridSleepState", config_parse_strv, 0, sc->states + SLEEP_HYBRID_SLEEP }, + { "Sleep", "SuspendState", config_parse_strv, 0, sc->states + SLEEP_SUSPEND }, + { "Sleep", "SuspendMode", config_parse_warn_compat, DISABLED_LEGACY, NULL }, - { "Sleep", "HibernateDelaySec", config_parse_sec, 0, &sc->hibernate_delay_usec }, - { "Sleep", "SuspendEstimationSec", config_parse_sec, 0, &sc->suspend_estimation_usec }, + { "Sleep", "HibernateState", config_parse_warn_compat, DISABLED_LEGACY, NULL }, + { "Sleep", "HibernateMode", config_parse_strv, 0, sc->modes + SLEEP_HIBERNATE }, + + { "Sleep", "HybridSleepState", config_parse_warn_compat, DISABLED_LEGACY, NULL }, + { "Sleep", "HybridSleepMode", config_parse_strv, 0, sc->modes + SLEEP_HYBRID_SLEEP }, + + { "Sleep", "HibernateDelaySec", config_parse_sec, 0, &sc->hibernate_delay_usec }, + { "Sleep", "SuspendEstimationSec", config_parse_sec, 0, &sc->suspend_estimation_usec }, {} }; @@ -89,26 +128,26 @@ int parse_sleep_config(SleepConfig **ret) { sc->allow[SLEEP_SUSPEND_THEN_HIBERNATE] = allow_s2h >= 0 ? allow_s2h : (allow_suspend != 0 && allow_hibernate != 0); - if (!sc->states[SLEEP_SUSPEND]) - sc->states[SLEEP_SUSPEND] = strv_new("mem", "standby", "freeze"); - if (!sc->modes[SLEEP_HIBERNATE]) - sc->modes[SLEEP_HIBERNATE] = strv_new("platform", "shutdown"); - if (!sc->states[SLEEP_HIBERNATE]) - sc->states[SLEEP_HIBERNATE] = strv_new("disk"); - if (!sc->modes[SLEEP_HYBRID_SLEEP]) - sc->modes[SLEEP_HYBRID_SLEEP] = strv_new("suspend", "platform", "shutdown"); - if (!sc->states[SLEEP_HYBRID_SLEEP]) - sc->states[SLEEP_HYBRID_SLEEP] = strv_new("disk"); + for (SleepOperation i = 0; i < _SLEEP_OPERATION_CONFIG_MAX; i++) { + if (!sc->states[i] && sleep_default_state_table[i]) { + sc->states[i] = strv_copy(sleep_default_state_table[i]); + if (!sc->states[i]) + return log_oom(); + } + + if (!sc->modes[i] && sleep_default_mode_table[i]) { + sc->modes[i] = strv_copy(sleep_default_mode_table[i]); + if (!sc->modes[i]) + return log_oom(); + } + } + if (sc->suspend_estimation_usec == 0) sc->suspend_estimation_usec = DEFAULT_SUSPEND_ESTIMATION_USEC; - /* Ensure values set for all required fields */ - if (!sc->states[SLEEP_SUSPEND] || !sc->modes[SLEEP_HIBERNATE] - || !sc->states[SLEEP_HIBERNATE] || !sc->modes[SLEEP_HYBRID_SLEEP] || !sc->states[SLEEP_HYBRID_SLEEP]) - return log_oom(); + sleep_config_validate_state_and_mode(sc); *ret = TAKE_PTR(sc); - return 0; } diff --git a/src/sleep/sleep.conf b/src/sleep/sleep.conf index 8428821b525..06f7dc38a4c 100644 --- a/src/sleep/sleep.conf +++ b/src/sleep/sleep.conf @@ -21,11 +21,8 @@ #AllowHibernation=yes #AllowSuspendThenHibernate=yes #AllowHybridSleep=yes -#SuspendMode= #SuspendState=mem standby freeze #HibernateMode=platform shutdown -#HibernateState=disk #HybridSleepMode=suspend platform shutdown -#HybridSleepState=disk #HibernateDelaySec= #SuspendEstimationSec=60min From fefddffa538316defd0a136819f9b73e8d31ace3 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 23 Oct 2023 21:19:29 +0800 Subject: [PATCH 4/4] sleep-config: check sleep mode only when hibernation With the previous change, this should only be used when doing hibernation. --- src/shared/sleep-config.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 03d04775e61..b63071a6928 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -309,15 +309,15 @@ static int sleep_supported_internal( return false; } - r = sleep_mode_supported(sleep_config->modes[operation]); - if (r < 0) - return r; - if (r == 0) { - *ret_support = SLEEP_STATE_OR_MODE_NOT_SUPPORTED; - return false; - } - if (IN_SET(operation, SLEEP_HIBERNATE, SLEEP_HYBRID_SLEEP)) { + r = sleep_mode_supported(sleep_config->modes[operation]); + if (r < 0) + return r; + if (r == 0) { + *ret_support = SLEEP_STATE_OR_MODE_NOT_SUPPORTED; + return false; + } + r = hibernation_is_safe(); if (r == -ENOTRECOVERABLE) { *ret_support = SLEEP_RESUME_NOT_SUPPORTED; @@ -329,7 +329,8 @@ static int sleep_supported_internal( } if (r < 0) return r; - } + } else + assert(!sleep_config->modes[operation]); *ret_support = SLEEP_SUPPORTED; return true;