From 3d23df005e06b3616049686be82deff55788d3c4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 13 Nov 2022 23:59:49 +0900 Subject: [PATCH 1/9] sleep: rename hibernate_delay_sec -> _usec --- src/shared/sleep-config.c | 6 +++--- src/shared/sleep-config.h | 2 +- src/sleep/sleep.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index caf94b455c..260e8ccca4 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -82,7 +82,7 @@ int parse_sleep_config(SleepConfig **ret_sleep_config) { { "Sleep", "HybridSleepMode", config_parse_strv, 0, sc->modes + SLEEP_HYBRID_SLEEP }, { "Sleep", "HybridSleepState", config_parse_strv, 0, sc->states + SLEEP_HYBRID_SLEEP }, - { "Sleep", "HibernateDelaySec", config_parse_sec, 0, &sc->hibernate_delay_sec }, + { "Sleep", "HibernateDelaySec", config_parse_sec, 0, &sc->hibernate_delay_usec }, {} }; @@ -113,8 +113,8 @@ int parse_sleep_config(SleepConfig **ret_sleep_config) { sc->modes[SLEEP_HYBRID_SLEEP] = strv_new("suspend", "platform", "shutdown"); if (!sc->states[SLEEP_HYBRID_SLEEP]) sc->states[SLEEP_HYBRID_SLEEP] = strv_new("disk"); - if (sc->hibernate_delay_sec == 0) - sc->hibernate_delay_sec = 2 * USEC_PER_HOUR; + if (sc->hibernate_delay_usec == 0) + sc->hibernate_delay_usec = 2 * USEC_PER_HOUR; /* Ensure values set for all required fields */ if (!sc->states[SLEEP_SUSPEND] || !sc->modes[SLEEP_HIBERNATE] diff --git a/src/shared/sleep-config.h b/src/shared/sleep-config.h index af6947d982..813f33c7c0 100644 --- a/src/shared/sleep-config.h +++ b/src/shared/sleep-config.h @@ -19,7 +19,7 @@ typedef struct SleepConfig { bool allow[_SLEEP_OPERATION_MAX]; char **modes[_SLEEP_OPERATION_MAX]; char **states[_SLEEP_OPERATION_MAX]; - usec_t hibernate_delay_sec; + usec_t hibernate_delay_usec; } SleepConfig; SleepConfig* free_sleep_config(SleepConfig *sc); diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index c2c8c779ad..37d434bef3 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -275,7 +275,7 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { _cleanup_hashmap_free_ Hashmap *last_capacity = NULL, *current_capacity = NULL; _cleanup_close_ int tfd = -EBADF; struct itimerspec ts = {}; - usec_t suspend_interval = sleep_config->hibernate_delay_sec, before_timestamp = 0, after_timestamp = 0, total_suspend_interval; + usec_t suspend_interval = sleep_config->hibernate_delay_usec, before_timestamp = 0, after_timestamp = 0, total_suspend_interval; bool woken_by_timer; tfd = timerfd_create(CLOCK_BOOTTIME_ALARM, TFD_NONBLOCK|TFD_CLOEXEC); From d812e104c7c62648747d3ffe37db33dde319d15c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 08:31:09 +0900 Subject: [PATCH 2/9] sleep: fetch_batteries_capacity_by_name() does not return -ENOENT --- src/sleep/sleep.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index 37d434bef3..a1874c5844 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -275,21 +275,16 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { _cleanup_hashmap_free_ Hashmap *last_capacity = NULL, *current_capacity = NULL; _cleanup_close_ int tfd = -EBADF; struct itimerspec ts = {}; - usec_t suspend_interval = sleep_config->hibernate_delay_usec, before_timestamp = 0, after_timestamp = 0, total_suspend_interval; + usec_t suspend_interval = sleep_config->hibernate_delay_usec, total_suspend_interval; bool woken_by_timer; tfd = timerfd_create(CLOCK_BOOTTIME_ALARM, TFD_NONBLOCK|TFD_CLOEXEC); if (tfd < 0) return log_error_errno(errno, "Error creating timerfd: %m"); - /* Store current battery capacity and current time before suspension */ + /* Store current battery capacity before suspension */ r = fetch_batteries_capacity_by_name(&last_capacity); - if (r >= 0) - before_timestamp = now(CLOCK_BOOTTIME); - else if (r == -ENOENT) - /* In case of no battery, system suspend interval will be set to HibernateDelaySec=. */ - log_debug_errno(r, "Suspend Interval value set to %s: %m", FORMAT_TIMESPAN(suspend_interval, USEC_PER_SEC)); - else + if (r < 0) return log_error_errno(r, "Error fetching battery capacity percentage: %m"); r = get_total_suspend_interval(last_capacity, &total_suspend_interval); @@ -298,6 +293,8 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { else suspend_interval = total_suspend_interval; + usec_t before_timestamp = now(CLOCK_BOOTTIME); + log_debug("Set timerfd wake alarm for %s", FORMAT_TIMESPAN(suspend_interval, USEC_PER_SEC)); /* Wake alarm for system with or without battery to hibernate or estimate discharge rate whichever is applicable */ timespec_store(&ts.it_value, suspend_interval); @@ -316,17 +313,20 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { woken_by_timer = FLAGS_SET(r, POLLIN); r = fetch_batteries_capacity_by_name(¤t_capacity); - if (r < 0) { + if (r < 0 || hashmap_isempty(current_capacity)) { /* In case of no battery or error while getting charge level, no need to measure - * discharge rate. Instead system should wakeup if it is manual wakeup or - * hibernate if this is a timer wakeup. */ - log_debug_errno(r, "Battery capacity percentage unavailable, cannot estimate discharge rate: %m"); + * discharge rate. Instead the system should wake up if it is manual wakeup or + * hibernate if this is a timer wakeup. */ + if (r < 0) + log_debug_errno(r, "Battery capacity percentage unavailable, cannot estimate discharge rate: %m"); + else + log_debug("No battery found."); if (!woken_by_timer) return 0; break; } - after_timestamp = now(CLOCK_BOOTTIME); + usec_t after_timestamp = now(CLOCK_BOOTTIME); log_debug("Attempting to estimate battery discharge rate after wakeup from %s sleep", FORMAT_TIMESPAN(after_timestamp - before_timestamp, USEC_PER_HOUR)); From 2ed56afeb3c26596dbe44858559c92307778ff82 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 00:09:34 +0900 Subject: [PATCH 3/9] sleep: drop unnecessary temporal vaiable and initialization --- src/sleep/sleep.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index a1874c5844..d1c45e264d 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -275,7 +275,7 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { _cleanup_hashmap_free_ Hashmap *last_capacity = NULL, *current_capacity = NULL; _cleanup_close_ int tfd = -EBADF; struct itimerspec ts = {}; - usec_t suspend_interval = sleep_config->hibernate_delay_usec, total_suspend_interval; + usec_t suspend_interval; bool woken_by_timer; tfd = timerfd_create(CLOCK_BOOTTIME_ALARM, TFD_NONBLOCK|TFD_CLOEXEC); @@ -287,11 +287,12 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { if (r < 0) return log_error_errno(r, "Error fetching battery capacity percentage: %m"); - r = get_total_suspend_interval(last_capacity, &total_suspend_interval); - if (r < 0) + r = get_total_suspend_interval(last_capacity, &suspend_interval); + if (r < 0) { log_debug_errno(r, "Failed to estimate suspend interval using previous discharge rate, ignoring: %m"); - else - suspend_interval = total_suspend_interval; + /* In case of no battery or any errors, system suspend interval will be set to HibernateDelaySec=. */ + suspend_interval = sleep_config->hibernate_delay_usec; + } usec_t before_timestamp = now(CLOCK_BOOTTIME); From 4f58b656d92b09a953b7cffcfd1ee6d5136a57ed Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 02:08:05 +0900 Subject: [PATCH 4/9] sleep: introduce SuspendEstimationSec= Before v252, HibernateDelaySec= specifies the maximum timespan that the system in suspend state, and the system hibernate after the timespan. However, after 96d662fa4c8cab24da57523c5e49e6ef3967fc13, the setting is repurposed as the default interval to measure battery charge level and estimate the battery discharging late. And if the system has enough battery capacity, then the system will stay in suspend state and not hibernate even if the time passed. See issue #25269. To keep the backward compatibility, let's introduce another setting SuspendEstimationSec= for controlling the interval to measure battery charge level, and make HibernateDelaySec= work as of v251. This also drops implementation details from the man page. Fixes #25269. --- man/systemd-sleep.conf.xml | 58 ++++++++++++++++++++------------------ src/shared/sleep-config.c | 11 ++++++-- src/shared/sleep-config.h | 3 ++ src/sleep/sleep.c | 48 ++++++++++++++++++++++--------- src/sleep/sleep.conf | 3 +- 5 files changed, 77 insertions(+), 46 deletions(-) diff --git a/man/systemd-sleep.conf.xml b/man/systemd-sleep.conf.xml index be04f2cdf1..79ebef1fef 100644 --- a/man/systemd-sleep.conf.xml +++ b/man/systemd-sleep.conf.xml @@ -77,29 +77,16 @@ suspend-then-hibernate - A low power state where initially user.slice unit is freezed. - If the hardware supports low-battery alarms (ACPI _BTP), then the system is - first suspended (the state is stored in RAM) and then hibernates if the system - is woken up by the hardware via ACPI low-battery signal. Unit user.slice is - thawed when system returns from hibernation. If the hardware does not support - low-battery alarms (ACPI _BTP), then the system is suspended based on battery's - current percentage capacity. If the current battery capacity is higher than 5%, the - system suspends for interval calculated using battery discharge rate per hour or - HibernateDelaySec= - if former is not available. - Battery discharge rate per hour is stored in a file which is created after - initial suspend-resume cycle. The value is calculated using battery decreasing - charge level over a timespan for which system was suspended. For each battery - connected to the system, there is a unique entry. After RTC alarm wakeup from - suspend, battery discharge rate per hour is again estimated. If the current battery - charge level is equal to or less than 5%, the system will be hibernated (the state - is then stored on disk) else the system goes back to suspend for the interval - calculated using battery discharge rate per hour. - In case of manual wakeup, if the battery was discharged while the system was - suspended, the battery discharge rate is estimated and stored on the filesystem. - In case the system is woken up by the hardware via the ACPI low-battery signal, - then it hibernates. - + + A low power state where the system is initially suspended (the state is stored in + RAM). If the system supports low-battery alarms (ACPI _BTP), then the system will be woken up by + the ACPI low-battery signal and hibernated (the state is then stored on disk). Also, if not + interrupted within the timespan specified by HibernateDelaySec= or the estimated + timespan until the system battery charge level goes down to 5%, then the system will be woken up by the + RTC alarm and hibernated. The estimated timespan is calculated from the change of the battery + capacity level after the time specified by SuspendEstimationSec= or when + the system is woken up from the suspend. + @@ -189,13 +176,28 @@ uses the value of SuspendState= when suspending and the value of HibernateState= when hibernating. + HibernateDelaySec= - The amount of time the system spends in suspend mode - before the RTC alarm wakes the system, before the battery discharge rate - can be estimated and used instead to calculate the suspension interval. - systemd-suspend-then-hibernate.service8. Defaults - to 2h. + + + The amount of time the system spends in suspend mode before the system is + automatically put into hibernate mode. Only used by + systemd-suspend-then-hibernate.service8. + If the system has a battery, then defaults to the estimated timespan until the system battery charge level goes down to 5%. + If the system has no battery, then defaults to 2h. + + + + + SuspendEstimationSec= + + + The RTC alarm will wake the system after the specified timespan to measure the system battery + capacity level and estimate battery discharging rate, which is used for estimating timespan until the system battery charge + level goes down to 5%. Only used by + systemd-suspend-then-hibernate.service8. + Defaults to 2h. diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 260e8ccca4..45b78bdafa 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -65,10 +65,14 @@ int parse_sleep_config(SleepConfig **ret_sleep_config) { int allow_suspend = -1, allow_hibernate = -1, allow_s2h = -1, allow_hybrid_sleep = -1; - sc = new0(SleepConfig, 1); + sc = new(SleepConfig, 1); if (!sc) return log_oom(); + *sc = (SleepConfig) { + .hibernate_delay_usec = USEC_INFINITY, + }; + const ConfigTableItem items[] = { { "Sleep", "AllowSuspend", config_parse_tristate, 0, &allow_suspend }, { "Sleep", "AllowHibernation", config_parse_tristate, 0, &allow_hibernate }, @@ -83,6 +87,7 @@ int parse_sleep_config(SleepConfig **ret_sleep_config) { { "Sleep", "HybridSleepState", config_parse_strv, 0, sc->states + SLEEP_HYBRID_SLEEP }, { "Sleep", "HibernateDelaySec", config_parse_sec, 0, &sc->hibernate_delay_usec }, + { "Sleep", "SuspendEstimationSec", config_parse_sec, 0, &sc->suspend_estimation_usec }, {} }; @@ -113,8 +118,8 @@ int parse_sleep_config(SleepConfig **ret_sleep_config) { sc->modes[SLEEP_HYBRID_SLEEP] = strv_new("suspend", "platform", "shutdown"); if (!sc->states[SLEEP_HYBRID_SLEEP]) sc->states[SLEEP_HYBRID_SLEEP] = strv_new("disk"); - if (sc->hibernate_delay_usec == 0) - sc->hibernate_delay_usec = 2 * USEC_PER_HOUR; + 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] diff --git a/src/shared/sleep-config.h b/src/shared/sleep-config.h index 813f33c7c0..99423fe3d3 100644 --- a/src/shared/sleep-config.h +++ b/src/shared/sleep-config.h @@ -6,6 +6,8 @@ #include "hashmap.h" #include "time-util.h" +#define DEFAULT_SUSPEND_ESTIMATION_USEC (1 * USEC_PER_HOUR) + typedef enum SleepOperation { SLEEP_SUSPEND, SLEEP_HIBERNATE, @@ -20,6 +22,7 @@ typedef struct SleepConfig { char **modes[_SLEEP_OPERATION_MAX]; char **states[_SLEEP_OPERATION_MAX]; usec_t hibernate_delay_usec; + usec_t suspend_estimation_usec; } SleepConfig; SleepConfig* free_sleep_config(SleepConfig *sc); diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index d1c45e264d..0611cfb780 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -267,10 +267,13 @@ static int execute( } static int custom_timer_suspend(const SleepConfig *sleep_config) { + usec_t hibernate_timestamp; int r; assert(sleep_config); + hibernate_timestamp = usec_add(now(CLOCK_BOOTTIME), sleep_config->hibernate_delay_usec); + while (battery_is_low() == 0) { _cleanup_hashmap_free_ Hashmap *last_capacity = NULL, *current_capacity = NULL; _cleanup_close_ int tfd = -EBADF; @@ -287,14 +290,25 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { if (r < 0) return log_error_errno(r, "Error fetching battery capacity percentage: %m"); - r = get_total_suspend_interval(last_capacity, &suspend_interval); - if (r < 0) { - log_debug_errno(r, "Failed to estimate suspend interval using previous discharge rate, ignoring: %m"); - /* In case of no battery or any errors, system suspend interval will be set to HibernateDelaySec=. */ - suspend_interval = sleep_config->hibernate_delay_usec; + if (hashmap_isempty(last_capacity)) + /* In case of no battery, system suspend interval will be set to HibernateDelaySec= or 2 hours. */ + suspend_interval = timestamp_is_set(hibernate_timestamp) ? sleep_config->hibernate_delay_usec : DEFAULT_SUSPEND_ESTIMATION_USEC; + else { + r = get_total_suspend_interval(last_capacity, &suspend_interval); + if (r < 0) { + log_debug_errno(r, "Failed to estimate suspend interval using previous discharge rate, ignoring: %m"); + /* In case of any errors, especially when we do not know the battery + * discharging rate, system suspend interval will be set to + * SuspendEstimationSec=. */ + suspend_interval = sleep_config->suspend_estimation_usec; + } } + /* Do not suspend more than HibernateDelaySec= */ usec_t before_timestamp = now(CLOCK_BOOTTIME); + suspend_interval = MIN(suspend_interval, usec_sub_unsigned(hibernate_timestamp, before_timestamp)); + if (suspend_interval <= 0) + break; /* system should hibernate */ log_debug("Set timerfd wake alarm for %s", FORMAT_TIMESPAN(suspend_interval, USEC_PER_SEC)); /* Wake alarm for system with or without battery to hibernate or estimate discharge rate whichever is applicable */ @@ -380,7 +394,7 @@ static int freeze_thaw_user_slice(const char **method) { static int execute_s2h(const SleepConfig *sleep_config) { _unused_ _cleanup_(freeze_thaw_user_slice) const char *auto_method_thaw = "ThawUnit"; - int r, k; + int r; assert(sleep_config); @@ -388,15 +402,21 @@ static int execute_s2h(const SleepConfig *sleep_config) { if (r < 0) log_debug_errno(r, "Failed to freeze unit user.slice, ignoring: %m"); - r = check_wakeup_type(); - if (r < 0) - log_debug_errno(r, "Failed to check hardware wakeup type, ignoring: %m"); + /* Only check if we have automated battery alarms if HibernateDelaySec= is not set, as in that case + * we'll busy poll for the configured interval instead */ + if (!timestamp_is_set(sleep_config->hibernate_delay_usec)) { + r = check_wakeup_type(); + if (r < 0) + log_debug_errno(r, "Failed to check hardware wakeup type, ignoring: %m"); + else { + r = battery_trip_point_alarm_exists(); + if (r < 0) + log_debug_errno(r, "Failed to check whether acpi_btp support is enabled or not, ignoring: %m"); + } + } else + r = 0; /* Force fallback path */ - k = battery_trip_point_alarm_exists(); - if (k < 0) - log_debug_errno(k, "Failed to check whether acpi_btp support is enabled or not, ignoring: %m"); - - if (r >= 0 && k > 0) { + if (r > 0) { /* If we have both wakeup alarms and battery trip point support, use them */ log_debug("Attempting to suspend..."); r = execute(sleep_config, SLEEP_SUSPEND, NULL); if (r < 0) diff --git a/src/sleep/sleep.conf b/src/sleep/sleep.conf index a3d31140d8..4c8e8b9680 100644 --- a/src/sleep/sleep.conf +++ b/src/sleep/sleep.conf @@ -23,4 +23,5 @@ #HibernateState=disk #HybridSleepMode=suspend platform shutdown #HybridSleepState=disk -#HibernateDelaySec=120min +#HibernateDelaySec= +#SuspendEstimationSec=60min From 3c3f46013ed53aba1aad5b51844434713fa5a0e9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 02:44:13 +0900 Subject: [PATCH 5/9] sleep: coding style fixlets --- src/sleep/sleep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index 0611cfb780..9c51a3367f 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -431,9 +431,9 @@ static int execute_s2h(const SleepConfig *sleep_config) { return 0; } else { r = custom_timer_suspend(sleep_config); - if(r < 0) + if (r < 0) return log_debug_errno(r, "Suspend cycle with manual battery discharge rate estimation failed: %m"); - if(r == 0) + if (r == 0) /* manual wakeup */ return 0; } From 3d9ca76f368b7b198be3471dd28ed32b35114ace Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 02:46:53 +0900 Subject: [PATCH 6/9] sleep: simplify code a bit - use device_get_sysattr_int(), - drop redundant log message. --- src/shared/sleep-config.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 45b78bdafa..695e7fb9cf 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -22,6 +22,7 @@ #include "btrfs-util.h" #include "conf-parser.h" #include "constants.h" +#include "device-private.h" #include "device-util.h" #include "devnum-util.h" #include "env-util.h" @@ -170,18 +171,13 @@ int get_capacity_by_name(Hashmap *capacities_by_name, const char *name) { /* Battery percentage capacity fetched from capacity file and if in range 0-100 then returned */ static int read_battery_capacity_percentage(sd_device *dev) { - const char *power_supply_capacity; int battery_capacity, r; assert(dev); - r = sd_device_get_property_value(dev, "POWER_SUPPLY_CAPACITY", &power_supply_capacity); + r = device_get_sysattr_int(dev, "capacity", &battery_capacity); if (r < 0) - return log_device_debug_errno(dev, r, "Failed to get property POWER_SUPPLY_CAPACITY: %m"); - - r = safe_atoi(power_supply_capacity, &battery_capacity); - if (r < 0) - return log_device_debug_errno(dev, r, "Failed to parse property POWER_SUPPLY_CAPACITY: %m"); + return log_device_debug_errno(dev, r, "Failed to read/parse POWER_SUPPLY_CAPACITY: %m"); if (battery_capacity < 0 || battery_capacity > 100) return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ERANGE), "Invalid battery capacity"); @@ -203,15 +199,9 @@ int battery_is_low(void) { if (r < 0) return log_debug_errno(r, "Failed to initialize battery enumerator: %m"); - FOREACH_DEVICE(e, dev) { - r = read_battery_capacity_percentage(dev); - if (r < 0) { - log_device_debug_errno(dev, r, "Failed to get battery capacity, ignoring: %m"); - continue; - } - if (r > BATTERY_LOW_CAPACITY_LEVEL) + FOREACH_DEVICE(e, dev) + if (read_battery_capacity_percentage(dev) > BATTERY_LOW_CAPACITY_LEVEL) return false; - } return true; } From a7795a4ecfe5c51fc837d0cb2f27d892c83df7a4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 02:49:19 +0900 Subject: [PATCH 7/9] sleep: introduce siphash24_compress_id128() Also, rename get_battery_identifier() to siphash24_compress_device_sysattr(). This also makes any errors in sd_id128_get_machine() or id128_get_product() ignored. For the machine ID, the failure should not be significant unless the file stored in the discharge level is reused by another system, which is quite unusual. For the product ID, if the firmware provides useless ID (all zero or all 0xFF), then loading/storing the discharge rate becomes completely broken, that should be avoided. Note, now sysattrs are used instead of properties in uevent files, but both provide the same information, hence no functionality should be changed. --- src/shared/sleep-config.c | 62 ++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 695e7fb9cf..582f4cd9d2 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -254,61 +254,55 @@ int fetch_batteries_capacity_by_name(Hashmap **ret) { return 0; } -/* Read file path and return hash of value in that file */ -static int get_battery_identifier(sd_device *dev, const char *property, struct siphash *state) { +static int siphash24_compress_device_sysattr(sd_device *dev, const char *attr, struct siphash *state) { const char *x; int r; assert(dev); - assert(property); + assert(attr); assert(state); - r = sd_device_get_property_value(dev, property, &x); - if (r == -ENOENT) - log_device_debug_errno(dev, r, "Battery device property %s is unavailable, ignoring: %m", property); - else if (r < 0) - return log_device_debug_errno(dev, r, "Failed to get battery device property %s: %m", property); - else if (isempty(x)) - log_device_debug(dev, "Battery device property '%s' is empty.", property); - else - siphash24_compress_string(x, state); + r = sd_device_get_sysattr_value(dev, attr, &x); + if (r < 0) + return log_device_debug_errno(dev, r, "Failed to read '%s' attribute: %m", attr); + if (!isempty(x)) + siphash24_compress_string(x, state); + + return 0; +} + +static int siphash24_compress_id128(int (*getter)(sd_id128_t*), const char *name, struct siphash *state) { + sd_id128_t id; + int r; + + assert(getter); + assert(state); + + r = getter(&id); + if (r < 0) + return log_debug_errno(r, "Failed to get %s ID: %m", name); + + siphash24_compress(&id, sizeof(sd_id128_t), state); return 0; } /* Read system and battery identifier from specific location and generate hash of it */ static int get_system_battery_identifier_hash(sd_device *dev, uint64_t *ret) { struct siphash state; - sd_id128_t machine_id, product_id; - int r; assert(ret); assert(dev); siphash24_init(&state, BATTERY_DISCHARGE_RATE_HASH_KEY.bytes); - get_battery_identifier(dev, "POWER_SUPPLY_MANUFACTURER", &state); - get_battery_identifier(dev, "POWER_SUPPLY_MODEL_NAME", &state); - get_battery_identifier(dev, "POWER_SUPPLY_SERIAL_NUMBER", &state); - - r = sd_id128_get_machine(&machine_id); - if (r == -ENOENT) - log_debug_errno(r, "machine ID is unavailable: %m"); - else if (r < 0) - return log_debug_errno(r, "Failed to get machine ID: %m"); - else - siphash24_compress(&machine_id, sizeof(sd_id128_t), &state); - - r = id128_get_product(&product_id); - if (r == -ENOENT) - log_debug_errno(r, "product_id does not exist: %m"); - else if (r < 0) - return log_debug_errno(r, "Failed to get product ID: %m"); - else - siphash24_compress(&product_id, sizeof(sd_id128_t), &state); + (void) siphash24_compress_device_sysattr(dev, "manufacturer", &state); + (void) siphash24_compress_device_sysattr(dev, "model_name", &state); + (void) siphash24_compress_device_sysattr(dev, "serial_number", &state); + (void) siphash24_compress_id128(sd_id128_get_machine, "machine", &state); + (void) siphash24_compress_id128(id128_get_product, "product", &state); *ret = siphash24_finalize(&state); - return 0; } From 3332cfe1764e3c15d9af2ef68097d0f698fddb3d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 02:52:55 +0900 Subject: [PATCH 8/9] sleep: fix indentation --- src/shared/sleep-config.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 582f4cd9d2..056180fe90 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -380,11 +380,11 @@ static int put_battery_discharge_rate(int estimated_battery_discharge_rate, uint estimated_battery_discharge_rate); r = write_string_filef( - DISCHARGE_RATE_FILEPATH, - WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_MKDIR_0755 | (trunc ? WRITE_STRING_FILE_TRUNCATE : 0), - "%"PRIu64" %d", - system_hash_id, - estimated_battery_discharge_rate); + DISCHARGE_RATE_FILEPATH, + WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_MKDIR_0755 | (trunc ? WRITE_STRING_FILE_TRUNCATE : 0), + "%"PRIu64" %d", + system_hash_id, + estimated_battery_discharge_rate); if (r < 0) return log_debug_errno(r, "Failed to update %s: %m", DISCHARGE_RATE_FILEPATH); From fe8e0f8e7989fe5cead5ad0e225dc0888ff10140 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 14 Nov 2022 02:54:50 +0900 Subject: [PATCH 9/9] sleep: enumerate only existing and non-device batteries The enumerator is now mostly consistent with on_ac_power() in udev-util.c. --- src/shared/sleep-config.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 056180fe90..232d1db7f5 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -143,16 +143,27 @@ static int battery_enumerator_new(sd_device_enumerator **ret) { if (r < 0) return r; - r = sd_device_enumerator_add_match_subsystem(e, "power_supply", /* match= */ true); + r = sd_device_enumerator_add_match_subsystem(e, "power_supply", /* match = */ true); if (r < 0) return r; - r = sd_device_enumerator_add_match_property(e, "POWER_SUPPLY_TYPE", "Battery"); + r = sd_device_enumerator_allow_uninitialized(e); + if (r < 0) + return r; + + r = sd_device_enumerator_add_match_sysattr(e, "type", "Battery", /* match = */ true); + if (r < 0) + return r; + + r = sd_device_enumerator_add_match_sysattr(e, "present", "1", /* match = */ true); + if (r < 0) + return r; + + r = sd_device_enumerator_add_match_sysattr(e, "scope", "Device", /* match = */ false); if (r < 0) return r; *ret = TAKE_PTR(e); - return 0; }