charge thresholds: read and write them where they actually live

On a split gauge/charger the threshold attributes are on the charger, not
the battery, and upstream only looks at the battery — so a settable ceiling
reported as unsupported, and a write went to a path that does not exist.
Same supplier walk charge_type already uses. Also report the live value when
no hwdb CHARGE_LIMIT entry recommends a pair.

src/linux/up-device-supply-battery.c

@@ -165,17 +165,99 @@ up_device_supply_battery_convert_to_double (const gchar *str_value, gdouble *val
 	return TRUE;
 }
 
+/**
+ * up_device_supply_battery_attr_dir:
+ *
+ * The directory carrying @attr for @native — the battery itself, or the
+ * charger that supplies it (see up_device_supply_find_attr_dir), with the
+ * UMOCKDEV_DIR prefix applied the same way up_device_supply_device_path()
+ * applies it, so the test harness keeps working.
+ */
+static gchar *
+up_device_supply_battery_attr_dir (GUdevDevice *native, const gchar *attr)
+{
+	g_autofree gchar *dir = NULL;
+	const char *root;
+
+	dir = up_device_supply_find_attr_dir (native, attr);
+	if (dir == NULL)
+		return NULL;
+
+	root = g_getenv ("UMOCKDEV_DIR");
+	if (!root || *root == '\0')
+		return g_steal_pointer (&dir);
+
+	return g_build_filename (root, dir, NULL);
+}
+
+/**
+ * up_device_supply_battery_read_threshold:
+ *
+ * Reads a threshold attribute from wherever it lives. %G_MAXUINT when absent
+ * or unreadable, which is the value the rest of this file already uses for
+ * "this end of the range is not settable".
+ */
+static gdouble
+up_device_supply_battery_read_threshold (GUdevDevice *native, const gchar *attr)
+{
+	g_autofree gchar *dir = NULL;
+	g_autofree gchar *path = NULL;
+	g_autofree gchar *contents = NULL;
+	gdouble value;
+
+	dir = up_device_supply_battery_attr_dir (native, attr);
+	if (dir == NULL)
+		return G_MAXUINT;
+
+	path = g_build_filename (dir, attr, NULL);
+	if (!g_file_get_contents (path, &contents, NULL, NULL))
+		return G_MAXUINT;
+
+	g_strstrip (contents);
+	if (!up_device_supply_battery_convert_to_double (contents, &value))
+		return G_MAXUINT;
+
+	return value;
+}
+
 static gboolean
 up_device_supply_battery_get_charge_control_limits (GUdevDevice *native, UpBatteryInfo *info)
 {
 	const gchar *charge_limit;
 	g_auto(GStrv) pairs = NULL;
+	g_autofree gchar *start_dir = NULL;
+	g_autofree gchar *end_dir = NULL;
 	gdouble charge_control_start_threshold;
 	gdouble charge_control_end_threshold;
 
+	start_dir = up_device_supply_battery_attr_dir (native, "charge_control_start_threshold");
+	end_dir = up_device_supply_battery_attr_dir (native, "charge_control_end_threshold");
+
 	charge_limit = g_udev_device_get_property (native, "CHARGE_LIMIT");
-	if (charge_limit == NULL)
-		return FALSE;
+	if (charge_limit == NULL) {
+		/* No hwdb entry recommending a pair. Upstream stops here and
+		 * reports the whole feature unsupported — but hwdb coverage is
+		 * a list of laptops somebody added, not a statement about the
+		 * hardware. If the kernel exposes a threshold, it is settable,
+		 * and the honest thing to report is what it is set to now.
+		 *
+		 * This is what makes the ceiling reachable on blueline: the
+		 * attribute is on pmi8998-charger, reads 99, and takes writes. */
+		if (start_dir == NULL && end_dir == NULL)
+			return FALSE;
+
+		info->charge_control_start_threshold =
+			up_device_supply_battery_read_threshold (native, "charge_control_start_threshold");
+		info->charge_control_end_threshold =
+			up_device_supply_battery_read_threshold (native, "charge_control_end_threshold");
+
+		if (start_dir != NULL)
+			info->charge_threshold_settings |= UP_DEVICE_SUPPLY_BATTERY_CHARGE_THRESHOLD_SETTINGS_CHARGE_CONTROL_START_THRESHOLD;
+		if (end_dir != NULL)
+			info->charge_threshold_settings |= UP_DEVICE_SUPPLY_BATTERY_CHARGE_THRESHOLD_SETTINGS_CHARGE_CONTROL_END_THRESHOLD;
+
+		return TRUE;
+	}
 
 	pairs = g_strsplit (charge_limit, ",", 0);
 	if (g_strv_length (pairs) != 2) {
@@ -202,9 +284,9 @@ up_device_supply_battery_get_charge_control_limits (GUdevDevice *native, UpBatte
 	info->charge_control_start_threshold = charge_control_start_threshold;
 	info->charge_control_end_threshold = charge_control_end_threshold;
 
-	if (g_udev_device_has_sysfs_attr (native, "charge_control_start_threshold"))
+	if (start_dir != NULL)
 		info->charge_threshold_settings |= UP_DEVICE_SUPPLY_BATTERY_CHARGE_THRESHOLD_SETTINGS_CHARGE_CONTROL_START_THRESHOLD;
-	if (g_udev_device_has_sysfs_attr (native, "charge_control_end_threshold"))
+	if (end_dir != NULL)
 		info->charge_threshold_settings |= UP_DEVICE_SUPPLY_BATTERY_CHARGE_THRESHOLD_SETTINGS_CHARGE_CONTROL_END_THRESHOLD;
 
 	return TRUE;
@@ -356,10 +438,18 @@ up_device_supply_battery_is_charge_threshold_by_charge_type (UpDevice *device) {
 	native = G_UDEV_DEVICE (up_device_get_native (device));
 
 	/* if the charge_control_start_threshold or charge_control_end_threshold is found,
-	 * then the charge threshold is not controlled by charge_types. */
-	if (g_udev_device_has_sysfs_attr (native, "charge_control_start_threshold") ||
-	    g_udev_device_has_sysfs_attr (native, "charge_control_end_threshold"))
-		return FALSE;
+	 * then the charge threshold is not controlled by charge_types. Resolved
+	 * through the supplier walk: on a split gauge/charger the thresholds are
+	 * real, they are just not on this device node. */
+	{
+		g_autofree gchar *start_dir = NULL;
+		g_autofree gchar *end_dir = NULL;
+
+		start_dir = up_device_supply_battery_attr_dir (native, "charge_control_start_threshold");
+		end_dir = up_device_supply_battery_attr_dir (native, "charge_control_end_threshold");
+		if (start_dir != NULL || end_dir != NULL)
+			return FALSE;
+	}
 
 	if (self->supported_charge_types & UP_DEVICE_SUPPLY_BATTERY_CHARGE_TYPES_LONG_LIFE) {
 		if (self->supported_charge_types & (UP_DEVICE_SUPPLY_BATTERY_CHARGE_TYPES_STANDARD |
@@ -699,7 +789,8 @@ up_device_supply_battery_set_battery_charge_thresholds(UpDevice *device, guint s
 	guint err_count = 0;
 	GUdevDevice *native;
 	UpDeviceSupplyBattery *self = UP_DEVICE_SUPPLY_BATTERY (device);
-	g_autofree gchar *native_path = NULL;
+	g_autofree gchar *start_dir = NULL;
+	g_autofree gchar *end_dir = NULL;
 	g_autofree gchar *start_filename = NULL;
 	g_autofree gchar *end_filename = NULL;
 	g_autoptr (GString) start_str = g_string_new (NULL);
@@ -707,9 +798,16 @@ up_device_supply_battery_set_battery_charge_thresholds(UpDevice *device, guint s
 	UpDeviceSupplyBatteryChargeTypes charge_type_enum;
 
 	native = G_UDEV_DEVICE (up_device_get_native (device));
-	native_path = up_device_supply_device_path (native);
-	start_filename = g_build_filename (native_path, "charge_control_start_threshold", NULL);
-	end_filename = g_build_filename (native_path, "charge_control_end_threshold", NULL);
+	/* Write where the attribute actually is. On a split gauge/charger that
+	 * is the charger, not this device — building the path from the battery
+	 * node produced a filename that does not exist, and
+	 * G_FILE_SET_CONTENTS_ONLY_EXISTING turned that into a silent no-op. */
+	start_dir = up_device_supply_battery_attr_dir (native, "charge_control_start_threshold");
+	end_dir = up_device_supply_battery_attr_dir (native, "charge_control_end_threshold");
+	if (start_dir != NULL)
+		start_filename = g_build_filename (start_dir, "charge_control_start_threshold", NULL);
+	if (end_dir != NULL)
+		end_filename = g_build_filename (end_dir, "charge_control_end_threshold", NULL);
 
 	/* if the charge threshold is controlled by charge_types,
 	 * the charge_types will be set to Long_life when enabling the charge threshold.
@@ -731,7 +829,8 @@ up_device_supply_battery_set_battery_charge_thresholds(UpDevice *device, guint s
 
 	if (start != G_MAXUINT) {
 		g_string_printf (start_str, "%d", CLAMP (start, 0, 100));
-		if (!g_file_set_contents_full (start_filename, start_str->str, start_str->len,
+		if (start_filename == NULL ||
+		    !g_file_set_contents_full (start_filename, start_str->str, start_str->len,
 					  G_FILE_SET_CONTENTS_ONLY_EXISTING, 0644, NULL))
 			err_count++;
 	} else {
@@ -740,7 +839,8 @@ up_device_supply_battery_set_battery_charge_thresholds(UpDevice *device, guint s
 
 	if (end != G_MAXUINT) {
 		g_string_printf (end_str, "%d", CLAMP (end, 0, 100));
-		if (!g_file_set_contents_full (end_filename, end_str->str, end_str->len,
+		if (end_filename == NULL ||
+		    !g_file_set_contents_full (end_filename, end_str->str, end_str->len,
 					  G_FILE_SET_CONTENTS_ONLY_EXISTING, 0644, NULL))
 			err_count++;
 	} else {

src/linux/up-device-supply.c

@@ -222,11 +222,71 @@ up_device_supply_get_state (GUdevDevice *native)
  **/
 static gchar *
 up_device_supply_get_supplier_charge_type_str (GUdevDevice *native)
+{
+	g_autofree gchar *dir = NULL;
+	g_autofree gchar *attr_path = NULL;
+	gchar *contents = NULL;
+
+	dir = up_device_supply_find_attr_dir (native, "charge_type");
+	if (dir == NULL)
+		return NULL;
+
+	attr_path = g_build_filename (dir, "charge_type", NULL);
+	if (!g_file_get_contents (attr_path, &contents, NULL, NULL))
+		return NULL;
+
+	g_strstrip (contents);
+	if (contents[0] == '\0') {
+		g_free (contents);
+		return NULL;
+	}
+
+	return contents;
+}
+
+/**
+ * up_device_supply_find_attr_dir:
+ *
+ * Returns the power-supply directory that actually carries @attr for @native:
+ * @native's own sysfs path when it has the attribute, otherwise the power
+ * supply that *supplies* @native, found by following the kernel's device
+ * links. %NULL when nothing in that chain exposes it.
+ *
+ * On a discrete-PMIC platform the fuel gauge and the charger are separate
+ * devices, and the charger owns everything about the charge regime. Qualcomm
+ * SDM845 is the case in hand: `qcom-battery` is `fuel-gauge@4000` and has
+ * neither `charge_type` nor `charge_control_end_threshold`, while
+ * `pmi8998-charger` is `charger@1000` and has both. Reading only @native's own
+ * attributes reports "unsupported" forever on such a device — which is what
+ * upstream does, and why a writable charge ceiling looked absent here when it
+ * was sitting one device link away the whole time.
+ *
+ * The kernel publishes the relation, so follow it rather than guessing which
+ * sibling supply is the charger:
+ *
+ *   <native>/device/supplier:<...>  ->  /sys/devices/virtual/devlink/<s>--<c>
+ *   <devlink>/supplier              ->  the supplying device
+ *   <supplier>/power_supply/<name>/<attr>
+ *
+ * Returns the directory path (caller frees), or %NULL.
+ **/
+gchar *
+up_device_supply_find_attr_dir (GUdevDevice *native, const gchar *attr)
 {
 	g_autoptr(GUdevDevice) parent = NULL;
 	g_autoptr(GDir) dir = NULL;
 	const gchar *parent_path;
 	const gchar *entry;
+	const gchar *native_path;
+
+	g_return_val_if_fail (attr != NULL, NULL);
+
+	/* The battery's own attribute wins whenever it has one — a laptop
+	 * where the gauge and the charger are one device must keep behaving
+	 * exactly as it did before this walk existed. */
+	native_path = g_udev_device_get_sysfs_path (native);
+	if (native_path != NULL && g_udev_device_has_sysfs_attr (native, attr))
+		return g_strdup (native_path);
 
 	parent = g_udev_device_get_parent (native);
 	if (parent == NULL)
@@ -276,17 +336,13 @@ up_device_supply_get_supplier_charge_type_str (GUdevDevice *native)
 			continue;
 
 		while ((ps_entry = g_dir_read_name (ps_dir)) != NULL) {
+			g_autofree gchar *supply_dir = NULL;
 			g_autofree gchar *attr_path = NULL;
-			gchar *contents = NULL;
 
-			attr_path = g_build_filename (ps_path, ps_entry, "charge_type", NULL);
-			if (!g_file_get_contents (attr_path, &contents, NULL, NULL))
-				continue;
-
-			g_strstrip (contents);
-			if (contents[0] != '\0')
-				return contents;
-			g_free (contents);
+			supply_dir = g_build_filename (ps_path, ps_entry, NULL);
+			attr_path = g_build_filename (supply_dir, attr, NULL);
+			if (g_file_test (attr_path, G_FILE_TEST_EXISTS))
+				return g_steal_pointer (&supply_dir);
 		}
 	}
 

src/linux/up-device-supply.h

@@ -55,6 +55,8 @@ UpDeviceChargeType up_device_supply_get_charge_type (GUdevDevice *native);
 
 gboolean up_device_supply_percentage_is_trusted (GUdevDevice *native);
 
+gchar *up_device_supply_find_attr_dir (GUdevDevice *native, const gchar *attr);
+
 G_END_DECLS
 
 #endif /* __UP_DEVICE_SUPPLY_H__ */