From 406fbeca32d9abacdcc28b0cc00d2beefcc4feb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 23 Aug 2022 14:18:41 +0200 Subject: [PATCH 1/4] shared/udev-util: say "ignoring device", not "ignoring" The short form sounds like we're ignoring the error. --- src/shared/udev-util.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index 0c6dea4264..db968a17c4 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -683,7 +683,7 @@ int on_ac_power(void) { r = sd_device_get_sysattr_value(d, "type", &val); if (r < 0) { - log_device_debug_errno(d, r, "Failed to read 'type' sysfs attribute, ignoring: %m"); + log_device_debug_errno(d, r, "Failed to read 'type' sysfs attribute, ignoring device: %m"); continue; } @@ -693,7 +693,7 @@ int on_ac_power(void) { * https://docs.kernel.org/admin-guide/abi-testing.html#abi-file-testing-sysfs-class-power */ if (streq(val, "Battery")) { found_battery = true; - log_device_debug(d, "The power supply is battery, ignoring."); + log_device_debug(d, "The power supply is battery, ignoring device."); continue; } @@ -702,22 +702,22 @@ int on_ac_power(void) { r = device_is_power_sink(d); if (r <= 0) { if (r < 0) - log_device_debug_errno(d, r, "Failed to determine the current power role, ignoring: %m"); + log_device_debug_errno(d, r, "Failed to determine the current power role, ignoring device: %m"); else - log_device_debug(d, "USB power supply is in source mode, ignoring."); + log_device_debug(d, "USB power supply is in source mode, ignoring device."); continue; } } r = sd_device_get_sysattr_value(d, "online", &val); if (r < 0) { - log_device_debug_errno(d, r, "Failed to read 'online' sysfs attribute, ignoring: %m"); + log_device_debug_errno(d, r, "Failed to read 'online' sysfs attribute, ignoring device: %m"); continue; } r = safe_atou(val, &v); if (r < 0) { - log_device_debug_errno(d, r, "Failed to parse 'online' attribute, ignoring: %m"); + log_device_debug_errno(d, r, "Failed to parse 'online' attribute, ignoring device: %m"); continue; } From 48a511cf92cbf202e9ef6064a9b5ebd1f497e1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 23 Aug 2022 16:24:02 +0200 Subject: [PATCH 2/4] sd-device: add helper to read a unsigned int attribute There are dozens of places where this could be used, but I don't want to do the conversion now because it's quite a bit of work. I think we could export this function later on, because reading numerical attributes is so common. But for now, I'm just adding the helper to use it one place. --- src/libsystemd/sd-device/device-private.h | 1 + src/libsystemd/sd-device/sd-device.c | 19 +++++++++++++++++++ src/libsystemd/sd-device/test-sd-device.c | 7 +++++++ 3 files changed, 27 insertions(+) diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 3d670a3afc..d993b77b49 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -18,6 +18,7 @@ static inline int device_new_from_watch_handle(sd_device **ret, int wd) { } int device_get_property_bool(sd_device *device, const char *key); +int device_get_sysattr_unsigned(sd_device *device, const char *sysattr, unsigned *ret_value); int device_get_sysattr_bool(sd_device *device, const char *sysattr); int device_get_device_id(sd_device *device, const char **ret); int device_get_devlink_priority(sd_device *device, int *ret); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 6bc4e6a019..51bee24d51 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -2206,6 +2206,25 @@ _public_ int sd_device_get_sysattr_value(sd_device *device, const char *sysattr, return 0; } +int device_get_sysattr_unsigned(sd_device *device, const char *sysattr, unsigned *ret_value) { + const char *value; + int r; + + r = sd_device_get_sysattr_value(device, sysattr, &value); + if (r < 0) + return r; + + unsigned v; + r = safe_atou(value, &v); + if (r < 0) + return log_device_debug_errno(device, r, "Failed to parse '%s' attribute: %m", sysattr); + + if (ret_value) + *ret_value = v; + /* We return "true" if the value is positive. */ + return v > 0; +} + int device_get_sysattr_bool(sd_device *device, const char *sysattr) { const char *value; int r; diff --git a/src/libsystemd/sd-device/test-sd-device.c b/src/libsystemd/sd-device/test-sd-device.c index 8172c64e45..3deb9beaac 100644 --- a/src/libsystemd/sd-device/test-sd-device.c +++ b/src/libsystemd/sd-device/test-sd-device.c @@ -180,6 +180,13 @@ static void test_sd_device_one(sd_device *d) { r = sd_device_get_sysattr_value(d, "name_assign_type", &val); assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r) || IN_SET(r, -ENOENT, -EINVAL)); + + if (r > 0) { + unsigned x; + + assert_se(device_get_sysattr_unsigned(d, "name_assign_type", NULL) >= 0); + assert_se(device_get_sysattr_unsigned(d, "name_assign_type", &x) >= 0); + } } TEST(sd_device_enumerator_devices) { From 4a52514b371bf8013e89c421dfc2405a443feef8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 23 Aug 2022 16:30:05 +0200 Subject: [PATCH 3/4] on-ac-power: rework logic History of the function: 96788d2aa4f4b0b49874b4a240ce47d9e8485d1b assume system is running on AC power when no battery found 795e86b4f1e8a1fd440f8c817621779c6aedbdb5 ignore USB-C ports in power source mode when detecting system is running on AC power c19a51bec40ae5e5073464e72411e7920d05d683 invert ac_power() source type check 6d89003462484c8656b698e07b9cf0a337e3818e assume ac when /sys/class/power_supply is missing 240dbaa44f8e5ad51775c776fc3ce9cd2f19f037 add ConditionACPower= Interestingly, the return condition 'on_ac_power == found_online || !found_offline' was there from the very beginning, and even Yu's latest change doesn't change this, but only extends it to 'on_ac_power == found_online || !found_offline || !found_battery'. This means that any system with no AC power supply will be unconditionally classified as on_ac_power. Let's change the logic: if we have an online AC supply, answer is "yes". If no supplies, but we have a battery, answer is "no". Otherwise, assume "yes", based on the assumption that presense of a battery would at least be always reported, even if an AC power supply might not be. Fixes #24407. It also shouldn't impact previous fixes: assume ac when /sys/class/power_supply is missing, ignore USB-C ports in power source mode, assume system is running on AC power when no battery found. --- src/shared/udev-util.c | 62 ++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index db968a17c4..8892d2bc9a 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -661,7 +661,7 @@ static int device_is_power_sink(sd_device *device) { int on_ac_power(void) { _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; - bool found_offline = false, found_online = false, found_battery = false; + bool found_ac_online = false, found_battery = false; sd_device *d; int r; @@ -678,22 +678,15 @@ int on_ac_power(void) { return r; FOREACH_DEVICE(e, d) { - const char *val; - unsigned v; - - r = sd_device_get_sysattr_value(d, "type", &val); - if (r < 0) { - log_device_debug_errno(d, r, "Failed to read 'type' sysfs attribute, ignoring device: %m"); - continue; - } - - /* We assume every power source is AC, except for batteries. See + /* See * https://github.com/torvalds/linux/blob/4eef766b7d4d88f0b984781bc1bcb574a6eafdc7/include/linux/power_supply.h#L176 * for defined power source types. Also see: * https://docs.kernel.org/admin-guide/abi-testing.html#abi-file-testing-sysfs-class-power */ - if (streq(val, "Battery")) { - found_battery = true; - log_device_debug(d, "The power supply is battery, ignoring device."); + + const char *val; + r = sd_device_get_sysattr_value(d, "type", &val); + if (r < 0) { + log_device_debug_errno(d, r, "Failed to read 'type' sysfs attribute, ignoring device: %m"); continue; } @@ -709,36 +702,33 @@ int on_ac_power(void) { } } - r = sd_device_get_sysattr_value(d, "online", &val); - if (r < 0) { - log_device_debug_errno(d, r, "Failed to read 'online' sysfs attribute, ignoring device: %m"); + bool is_battery = streq(val, "Battery"); + if (is_battery) { + found_battery = true; + log_device_debug(d, "The power supply is battery."); continue; } - r = safe_atou(val, &v); + r = device_get_sysattr_unsigned(d, "online", NULL); if (r < 0) { - log_device_debug_errno(d, r, "Failed to parse 'online' attribute, ignoring device: %m"); + log_device_debug_errno(d, r, "Failed to query 'online' sysfs attribute: %m"); continue; - } + } else if (r > 0) /* At least 1 and 2 are defined as different types of 'online' */ + found_ac_online = true; - if (v > 0) /* At least 1 and 2 are defined as different types of 'online' */ - found_online = true; - else - found_offline = true; - - log_device_debug(d, "The power supply is currently %s.", v > 0 ? "online" : "offline"); + log_device_debug(d, "The power supply is currently %s.", r > 0 ? "online" : "offline"); } - if (found_online) - log_debug("Found at least one online non-battery power supply, system is running on AC power."); - else if (!found_offline) - log_debug("Found no offline non-battery power supply, assuming system is running on AC power."); - else if (!found_battery) - log_debug("Found no battery, assuming system is running on AC power."); - else - log_debug("All non-battery power supplies are offline, assuming system is running with battery."); - - return found_online || !found_offline || !found_battery; + if (found_ac_online) { + log_debug("Found at least one online non-battery power supply, system is running on AC."); + return true; + } else if (found_battery) { + log_debug("Found battery and no online power sources, assuming system is running from battery."); + return false; + } else { + log_debug("No power supply reported online and no battery, assuming system is running on AC."); + return true; + } } bool udev_available(void) { From 3c69e94a5ce8076fa1240028f24d9b4ba1b67408 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 23 Aug 2022 16:48:26 +0200 Subject: [PATCH 4/4] on-ac-power: ignore devices with scope==Device My mouse is reported as: P: /devices/pci0000:00/0000:00:14.0/usb1/1-4/1-4.4/1-4.4:1.2/0003:046D:C52B.001E/0003:046D:4051.001F/power_supply/hidpp_battery_4 M: hidpp_battery_4 R: 4 U: power_supply E: DEVPATH=/devices/pci0000:00/0000:00:14.0/usb1/1-4/1-4.4/1-4.4:1.2/0003:046D:C52B.001E/0003:046D:4051.001F/power_supply/hidpp_battery_4 E: SUBSYSTEM=power_supply E: POWER_SUPPLY_NAME=hidpp_battery_4 E: POWER_SUPPLY_TYPE=Battery E: POWER_SUPPLY_ONLINE=1 E: POWER_SUPPLY_STATUS=Discharging E: POWER_SUPPLY_SCOPE=Device E: POWER_SUPPLY_MODEL_NAME=Wireless Mouse M510 E: POWER_SUPPLY_MANUFACTURER=Logitech E: POWER_SUPPLY_SERIAL_NUMBER=4051-bc-cd-d2-5b E: POWER_SUPPLY_CAPACITY_LEVEL=Normal See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=25a0bc2dfc2ea732f40af2dae52426ead66ae76e Effectively, "System" and "Unkown" are passed through, "Device" is rejected. --- src/shared/udev-util.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index 8892d2bc9a..2845e62c79 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -704,6 +704,14 @@ int on_ac_power(void) { bool is_battery = streq(val, "Battery"); if (is_battery) { + r = sd_device_get_sysattr_value(d, "scope", &val); + if (r < 0) + log_device_debug_errno(d, r, "Failed to read 'scope' sysfs attribute, ignoring: %m"); + else if (streq(val, "Device")) { + log_device_debug(d, "The power supply is a device battery, ignoring."); + continue; + } + found_battery = true; log_device_debug(d, "The power supply is battery."); continue;