[PATCH v6 0/4] power: supply: extension API

[PATCH v6 0/4] power: supply: extension API

[Thomas Weißschuh]

Introduce a mechanism for drivers to extend the properties implemented
by a power supply.

Motivation
----------

Various drivers, mostly in platform/x86 extend the ACPI battery driver
with additional sysfs attributes to implement more UAPIs than are
exposed through ACPI by using various side-channels, like WMI,
nonstandard ACPI or EC communication.

While the created sysfs attributes look similar to the attributes
provided by the powersupply core, there are various deficiencies:

* They don't show up in uevent payload.
* They can't be queried with the standard in-kernel APIs.
* They don't work with triggers.
* The extending driver has to reimplement all of the parsing,
  formatting and sysfs display logic.
* Writing a extension driver is completely different from writing a
  normal power supply driver.
* ~Properties can not be properly overriden.~
  (Overriding is now explicitly forbidden)

The proposed extension API avoids all of these issues.
An extension is just a "struct power_supply_ext" with the same kind of
callbacks as in a normal "struct power_supply_desc".

The API is meant to be used via battery_hook_register(), the same way as
the current extensions.
Further usecases are fuel gauges and the existing battery_info
properties.

When testing, please enable lockdep to make sure the locking is correct.

The series is based on the linux-power-supply/for-next branch.
It also depends on some recent fixes not yet available in the for-next
branch [0].

[0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v6:
- Drop alreay picked up ACPI battery hook rename patch
- Only return bool from power_supply_property_is_writeable()
- Improve naming for test_power symbols
- Integrate cros_charge-control fixes from the psy/fixes branch
- Add sysfs UAPI for extension discovery
- Use __must_check on API
- Make power_supply_for_each_extension() safer.
  (And uglier, ideas welcome)
- Link to v5: https://lore.kernel.org/r/20241205-power-supply-extensions-v5-0-f0f996db4347@weissschuh.net

Changes in v5:
- Drop already picked up patches
- Simplify power_supply_ext_has_property()
- Handle failure of power_supply_update_sysfs_and_hwmon()
- Reduce some locking scopes
- Add missing locking to power_supply_show_charge_behaviour()
- Improve sanity checks in power_supply_register_extension()
- Implement writeable property in test_power battery
- Rename ACPI battery hook messages for clarity
- Link to v4: https://lore.kernel.org/r/20241111-power-supply-extensions-v4-0-7240144daa8e@weissschuh.net

Changes in v4:
- Drop RFC state
- Integrate locking commit
- Reregister hwmon device
- Link to v3: https://lore.kernel.org/r/20240904-power-supply-extensions-v3-0-62efeb93f8ec@weissschuh.net

Changes in v3:
- Make naming more consistent
- Readd locking
- Allow multiple active extensions
- Allow passing a "void *ext_data" when registering
- Switch example driver from system76 to cros_charge-control
- Link to v2: https://lore.kernel.org/r/20240608-power-supply-extensions-v2-0-2dcd35b012ad@weissschuh.net

Changes in v2:
- Drop locking patch, let's figure out the API first
- Allow registration of multiple extensions
- Pass extension to extension callbacks as parameter
- Disallow property overlap between extensions and core psy
- Drop system76/pdx86 maintainers, as the system76 changes are only RFC
  state anyways
- Link to v1: https://lore.kernel.org/r/20240606-power-supply-extensions-v1-0-b45669290bdc@weissschuh.net

---
Thomas Weißschuh (4):
      power: supply: core: implement extension API
      power: supply: test-power: implement a power supply extension
      power: supply: cros_charge-control: implement a power supply extension
      power: supply: core: add UAPI to discover currently used extensions

 Documentation/ABI/testing/sysfs-class-power |   9 ++
 drivers/power/supply/cros_charge-control.c  | 200 ++++++++++++----------------
 drivers/power/supply/power_supply.h         |  19 +++
 drivers/power/supply/power_supply_core.c    | 177 ++++++++++++++++++++++--
 drivers/power/supply/power_supply_sysfs.c   |  36 ++++-
 drivers/power/supply/test_power.c           | 113 ++++++++++++++++
 include/linux/power_supply.h                |  35 +++++
 7 files changed, 467 insertions(+), 122 deletions(-)
---
base-commit: 810dde9dad8222f3b831cf5179927fc66fc6a006
change-id: 20240602-power-supply-extensions-07d949f509d9

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v6 1/4] power: supply: core: implement extension API

[Thomas Weißschuh]

Various drivers, mostly in platform/x86 extend the ACPI battery driver
with additional sysfs attributes to implement more UAPIs than are
exposed through ACPI by using various side-channels, like WMI,
nonstandard ACPI or EC communication.

While the created sysfs attributes look similar to the attributes
provided by the powersupply core, there are various deficiencies:

* They don't show up in uevent payload.
* They can't be queried with the standard in-kernel APIs.
* They don't work with triggers.
* The extending driver has to reimplement all of the parsing,
formatting and sysfs display logic.
* Writing a extension driver is completely different from writing a
normal power supply driver.

This extension API avoids all of these issues.
An extension is just a "struct power_supply_ext" with the same kind of
callbacks as in a normal "struct power_supply_desc".

The API is meant to be used via battery_hook_register(), the same way as
the current extensions.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply.h       |  17 ++++
 drivers/power/supply/power_supply_core.c  | 162 ++++++++++++++++++++++++++++--
 drivers/power/supply/power_supply_sysfs.c |  26 ++++-
 include/linux/power_supply.h              |  33 ++++++
 4 files changed, 228 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 5dabbd895538003096b62d03fdd0201b82b090e6..531785516d2ac31f9a7f73a58e15e64cb81820ed 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -9,6 +9,8 @@
  *  Modified: 2004, Oct     Szabolcs Gyurko
  */
 
+#include <linux/lockdep.h>
+
 struct device;
 struct device_type;
 struct power_supply;
@@ -17,6 +19,21 @@ extern int power_supply_property_is_writeable(struct power_supply *psy,
 					      enum power_supply_property psp);
 extern bool power_supply_has_property(struct power_supply *psy,
 				      enum power_supply_property psp);
+extern bool power_supply_ext_has_property(const struct power_supply_ext *ext,
+					  enum power_supply_property psp);
+
+struct power_supply_ext_registration {
+	struct list_head list_head;
+	const struct power_supply_ext *ext;
+	void *data;
+};
+
+/* Make sure that the macro is a single expression */
+#define power_supply_for_each_extension(pos, psy)			\
+	if ( ({ lockdep_assert_held(&(psy)->extensions_sem); 0; }) )	\
+		;							\
+	else								\
+		list_for_each_entry(pos, &(psy)->extensions, list_head)	\
 
 #ifdef CONFIG_SYSFS
 
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 502b07468b93dfb7f5a6c2092588d931a7d015f2..bc22edbd0e6a02c27500132075f5c98d814a7330 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -81,6 +81,7 @@ static int __power_supply_changed_work(struct device *dev, void *data)
 
 static void power_supply_changed_work(struct work_struct *work)
 {
+	int ret;
 	unsigned long flags;
 	struct power_supply *psy = container_of(work, struct power_supply,
 						changed_work);
@@ -88,6 +89,16 @@ static void power_supply_changed_work(struct work_struct *work)
 	dev_dbg(&psy->dev, "%s\n", __func__);
 
 	spin_lock_irqsave(&psy->changed_lock, flags);
+
+	if (unlikely(psy->update_groups)) {
+		psy->update_groups = false;
+		spin_unlock_irqrestore(&psy->changed_lock, flags);
+		ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
+		if (ret)
+			dev_warn(&psy->dev, "failed to update sysfs groups: %pe\n", ERR_PTR(ret));
+		spin_lock_irqsave(&psy->changed_lock, flags);
+	}
+
 	/*
 	 * Check 'changed' here to avoid issues due to race between
 	 * power_supply_changed() and this routine. In worst case
@@ -1196,15 +1207,34 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
 	return found;
 }
 
+bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
+				   enum power_supply_property psp)
+{
+	int i;
+
+	for (i = 0; i < psy_ext->num_properties; i++)
+		if (psy_ext->properties[i] == psp)
+			return true;
+
+	return false;
+}
+
 bool power_supply_has_property(struct power_supply *psy,
 			       enum power_supply_property psp)
 {
+	struct power_supply_ext_registration *reg;
+
 	if (psy_desc_has_property(psy->desc, psp))
 		return true;
 
 	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
 		return true;
 
+	power_supply_for_each_extension(reg, psy) {
+		if (power_supply_ext_has_property(reg->ext, psp))
+			return true;
+	}
+
 	return false;
 }
 
@@ -1212,12 +1242,21 @@ int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
 {
+	struct power_supply_ext_registration *reg;
+
 	if (atomic_read(&psy->use_cnt) <= 0) {
 		if (!psy->initialized)
 			return -EAGAIN;
 		return -ENODEV;
 	}
 
+	scoped_guard(rwsem_read, &psy->extensions_sem) {
+		power_supply_for_each_extension(reg, psy) {
+			if (power_supply_ext_has_property(reg->ext, psp))
+				return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
+		}
+	}
+
 	if (psy_desc_has_property(psy->desc, psp))
 		return psy->desc->get_property(psy, psp, val);
 	else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
@@ -1231,7 +1270,24 @@ int power_supply_set_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    const union power_supply_propval *val)
 {
-	if (atomic_read(&psy->use_cnt) <= 0 || !psy->desc->set_property)
+	struct power_supply_ext_registration *reg;
+
+	if (atomic_read(&psy->use_cnt) <= 0)
+		return -ENODEV;
+
+	scoped_guard(rwsem_read, &psy->extensions_sem) {
+		power_supply_for_each_extension(reg, psy) {
+			if (power_supply_ext_has_property(reg->ext, psp)) {
+				if (reg->ext->set_property)
+					return reg->ext->set_property(psy, reg->ext, reg->data,
+								      psp, val);
+				else
+					return -ENODEV;
+			}
+		}
+	}
+
+	if (!psy->desc->set_property)
 		return -ENODEV;
 
 	return psy->desc->set_property(psy, psp, val);
@@ -1241,7 +1297,22 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
 int power_supply_property_is_writeable(struct power_supply *psy,
 					enum power_supply_property psp)
 {
-	return psy->desc->property_is_writeable && psy->desc->property_is_writeable(psy, psp);
+	struct power_supply_ext_registration *reg;
+
+	power_supply_for_each_extension(reg, psy) {
+		if (power_supply_ext_has_property(reg->ext, psp)) {
+			if (reg->ext->property_is_writeable)
+				return reg->ext->property_is_writeable(psy, reg->ext,
+								       reg->data, psp);
+			else
+				return 0;
+		}
+	}
+
+	if (!psy->desc->property_is_writeable)
+		return 0;
+
+	return psy->desc->property_is_writeable(psy, psp);
 }
 
 void power_supply_external_power_changed(struct power_supply *psy)
@@ -1260,6 +1331,76 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(power_supply_powers);
 
+static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&psy->changed_lock, flags);
+	psy->update_groups = true;
+	spin_unlock_irqrestore(&psy->changed_lock, flags);
+
+	power_supply_changed(psy);
+
+	power_supply_remove_hwmon_sysfs(psy);
+	return power_supply_add_hwmon_sysfs(psy);
+}
+
+int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
+				    void *data)
+{
+	struct power_supply_ext_registration *reg;
+	size_t i;
+	int ret;
+
+	if (!psy || !ext || !ext->properties || !ext->num_properties)
+		return -EINVAL;
+
+	guard(rwsem_write)(&psy->extensions_sem);
+
+	for (i = 0; i < ext->num_properties; i++)
+		if (power_supply_has_property(psy, ext->properties[i]))
+			return -EEXIST;
+
+	reg = kmalloc(sizeof(*reg), GFP_KERNEL);
+	if (!reg)
+		return -ENOMEM;
+
+	reg->ext = ext;
+	reg->data = data;
+	list_add(&reg->list_head, &psy->extensions);
+
+	ret = power_supply_update_sysfs_and_hwmon(psy);
+	if (ret)
+		goto sysfs_hwmon_failed;
+
+	return 0;
+
+sysfs_hwmon_failed:
+	list_del(&reg->list_head);
+	kfree(reg);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(power_supply_register_extension);
+
+void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+	struct power_supply_ext_registration *reg;
+
+	guard(rwsem_write)(&psy->extensions_sem);
+
+	power_supply_for_each_extension(reg, psy) {
+		if (reg->ext == ext) {
+			list_del(&reg->list_head);
+			kfree(reg);
+			power_supply_update_sysfs_and_hwmon(psy);
+			return;
+		}
+	}
+
+	dev_warn(&psy->dev, "Trying to unregister invalid extension");
+}
+EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
+
 static void power_supply_dev_release(struct device *dev)
 {
 	struct power_supply *psy = to_power_supply(dev);
@@ -1414,6 +1555,9 @@ __power_supply_register(struct device *parent,
 	}
 
 	spin_lock_init(&psy->changed_lock);
+	init_rwsem(&psy->extensions_sem);
+	INIT_LIST_HEAD(&psy->extensions);
+
 	rc = device_add(dev);
 	if (rc)
 		goto device_add_failed;
@@ -1426,13 +1570,15 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto register_thermal_failed;
 
-	rc = power_supply_create_triggers(psy);
-	if (rc)
-		goto create_triggers_failed;
+	scoped_guard(rwsem_read, &psy->extensions_sem) {
+		rc = power_supply_create_triggers(psy);
+		if (rc)
+			goto create_triggers_failed;
 
-	rc = power_supply_add_hwmon_sysfs(psy);
-	if (rc)
-		goto add_hwmon_sysfs_failed;
+		rc = power_supply_add_hwmon_sysfs(psy);
+		if (rc)
+			goto add_hwmon_sysfs_failed;
+	}
 
 	/*
 	 * Update use_cnt after any uevents (most notably from device_add()).
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 99bfe1f03eb8326d38c4e2831c9670313b42e425..927ddb9d83bb7259809ba695cb9398d1ad654b46 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -268,6 +268,27 @@ static ssize_t power_supply_show_enum_with_available(
 	return count;
 }
 
+static ssize_t power_supply_show_charge_behaviour(struct device *dev,
+						  struct power_supply *psy,
+						  union power_supply_propval *value,
+						  char *buf)
+{
+	struct power_supply_ext_registration *reg;
+
+	scoped_guard(rwsem_read, &psy->extensions_sem) {
+		power_supply_for_each_extension(reg, psy) {
+			if (power_supply_ext_has_property(reg->ext,
+							  POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
+				return power_supply_charge_behaviour_show(dev,
+						reg->ext->charge_behaviours,
+						value->intval, buf);
+		}
+	}
+
+	return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
+						  value->intval, buf);
+}
+
 static ssize_t power_supply_format_property(struct device *dev,
 					    bool uevent,
 					    struct device_attribute *attr,
@@ -307,8 +328,7 @@ static ssize_t power_supply_format_property(struct device *dev,
 	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
 		if (uevent) /* no possible values in uevents */
 			goto default_format;
-		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
-							 value.intval, buf);
+		ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = sysfs_emit(buf, "%s\n", value.strval);
@@ -385,6 +405,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 	if (attrno == POWER_SUPPLY_PROP_TYPE)
 		return mode;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	if (power_supply_has_property(psy, attrno)) {
 		if (power_supply_property_is_writeable(psy, attrno) > 0)
 			mode |= S_IWUSR;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index b98106e1a90f34bce5129317a099f363248342b9..e434516086f032cdb4698005bb1a99eda303a307 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -15,6 +15,8 @@
 #include <linux/device.h>
 #include <linux/workqueue.h>
 #include <linux/leds.h>
+#include <linux/rwsem.h>
+#include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/notifier.h>
 
@@ -281,6 +283,27 @@ struct power_supply_desc {
 	int use_for_apm;
 };
 
+struct power_supply_ext {
+	u8 charge_behaviours;
+	const enum power_supply_property *properties;
+	size_t num_properties;
+
+	int (*get_property)(struct power_supply *psy,
+			    const struct power_supply_ext *ext,
+			    void *data,
+			    enum power_supply_property psp,
+			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply *psy,
+			    const struct power_supply_ext *ext,
+			    void *data,
+			    enum power_supply_property psp,
+			    const union power_supply_propval *val);
+	int (*property_is_writeable)(struct power_supply *psy,
+				     const struct power_supply_ext *ext,
+				     void *data,
+				     enum power_supply_property psp);
+};
+
 struct power_supply {
 	const struct power_supply_desc *desc;
 
@@ -300,10 +323,13 @@ struct power_supply {
 	struct delayed_work deferred_register_work;
 	spinlock_t changed_lock;
 	bool changed;
+	bool update_groups;
 	bool initialized;
 	bool removing;
 	atomic_t use_cnt;
 	struct power_supply_battery_info *battery_info;
+	struct rw_semaphore extensions_sem; /* protects "extensions" */
+	struct list_head extensions;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;
 	struct thermal_cooling_device *tcd;
@@ -878,6 +904,13 @@ devm_power_supply_register(struct device *parent,
 extern void power_supply_unregister(struct power_supply *psy);
 extern int power_supply_powers(struct power_supply *psy, struct device *dev);
 
+extern int __must_check
+power_supply_register_extension(struct power_supply *psy,
+				const struct power_supply_ext *ext,
+				void *data);
+extern void power_supply_unregister_extension(struct power_supply *psy,
+					      const struct power_supply_ext *ext);
+
 #define to_power_supply(device) container_of(device, struct power_supply, dev)
 
 extern void *power_supply_get_drvdata(struct power_supply *psy);

-- 
2.47.1

[PATCH v6 2/4] power: supply: test-power: implement a power supply extension

[Thomas Weißschuh]

Allow easy testing of the new power supply extension functionality.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/test_power.c | 111 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 442ceb7795e1d84e34da2801d228d53fb67e08d9..66f9ef52e0f3e6e6e6bebcfd438c2acd421284ec 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -37,6 +37,7 @@ static int battery_charge_counter	= -1000;
 static int battery_current		= -1600;
 static enum power_supply_charge_behaviour battery_charge_behaviour =
 	POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+static bool battery_extension;
 
 static bool module_initialized;
 
@@ -238,6 +239,85 @@ static const struct power_supply_config test_power_configs[] = {
 	},
 };
 
+static int test_power_battery_extmanufacture_year = 1234;
+static int test_power_battery_exttemp_max = 1000;
+static const enum power_supply_property test_power_battery_extprops[] = {
+	POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
+	POWER_SUPPLY_PROP_TEMP_MAX,
+};
+
+static int test_power_battery_extget_property(struct power_supply *psy,
+					      const struct power_supply_ext *ext,
+					      void *ext_data,
+					      enum power_supply_property psp,
+					      union power_supply_propval *val)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
+		val->intval = test_power_battery_extmanufacture_year;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MAX:
+		val->intval = test_power_battery_exttemp_max;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int test_power_battery_extset_property(struct power_supply *psy,
+					      const struct power_supply_ext *ext,
+					      void *ext_data,
+					      enum power_supply_property psp,
+					      const union power_supply_propval *val)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
+		test_power_battery_extmanufacture_year = val->intval;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MAX:
+		test_power_battery_exttemp_max = val->intval;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int test_power_battery_extproperty_is_writeable(struct power_supply *psy,
+						       const struct power_supply_ext *ext,
+						       void *ext_data,
+						       enum power_supply_property psp)
+{
+	return true;
+}
+
+static const struct power_supply_ext test_power_battery_ext = {
+	.properties		= test_power_battery_extprops,
+	.num_properties		= ARRAY_SIZE(test_power_battery_extprops),
+	.get_property		= test_power_battery_extget_property,
+	.set_property		= test_power_battery_extset_property,
+	.property_is_writeable	= test_power_battery_extproperty_is_writeable,
+};
+
+static void test_power_configure_battery_extension(bool enable)
+{
+	struct power_supply *psy;
+
+	psy = test_power_supplies[TEST_BATTERY];
+
+	if (enable) {
+		if (power_supply_register_extension(psy, &test_power_battery_ext, NULL)) {
+			pr_err("registering battery extension failed\n");
+			return;
+		}
+	} else {
+		power_supply_unregister_extension(psy, &test_power_battery_ext);
+	}
+
+	battery_extension = enable;
+}
+
 static int __init test_power_init(void)
 {
 	int i;
@@ -258,6 +338,8 @@ static int __init test_power_init(void)
 		}
 	}
 
+	test_power_configure_battery_extension(true);
+
 	module_initialized = true;
 	return 0;
 failed:
@@ -524,6 +606,26 @@ static int param_set_battery_current(const char *key,
 
 #define param_get_battery_current param_get_int
 
+static int param_set_battery_extension(const char *key,
+				       const struct kernel_param *kp)
+{
+	bool prev_battery_extension;
+	int ret;
+
+	prev_battery_extension = battery_extension;
+
+	ret = param_set_bool(key, kp);
+	if (ret)
+		return ret;
+
+	if (prev_battery_extension != battery_extension)
+		test_power_configure_battery_extension(battery_extension);
+
+	return 0;
+}
+
+#define param_get_battery_extension param_get_bool
+
 static const struct kernel_param_ops param_ops_ac_online = {
 	.set = param_set_ac_online,
 	.get = param_get_ac_online,
@@ -574,6 +676,11 @@ static const struct kernel_param_ops param_ops_battery_current = {
 	.get = param_get_battery_current,
 };
 
+static const struct kernel_param_ops param_ops_battery_extension = {
+	.set = param_set_battery_extension,
+	.get = param_get_battery_extension,
+};
+
 #define param_check_ac_online(name, p) __param_check(name, p, void);
 #define param_check_usb_online(name, p) __param_check(name, p, void);
 #define param_check_battery_status(name, p) __param_check(name, p, void);
@@ -584,6 +691,7 @@ static const struct kernel_param_ops param_ops_battery_current = {
 #define param_check_battery_voltage(name, p) __param_check(name, p, void);
 #define param_check_battery_charge_counter(name, p) __param_check(name, p, void);
 #define param_check_battery_current(name, p) __param_check(name, p, void);
+#define param_check_battery_extension(name, p) __param_check(name, p, void);
 
 
 module_param(ac_online, ac_online, 0644);
@@ -621,6 +729,9 @@ MODULE_PARM_DESC(battery_charge_counter,
 module_param(battery_current, battery_current, 0644);
 MODULE_PARM_DESC(battery_current, "battery current (milliampere)");
 
+module_param(battery_extension, battery_extension, 0644);
+MODULE_PARM_DESC(battery_extension, "battery extension");
+
 MODULE_DESCRIPTION("Power supply driver for testing");
 MODULE_AUTHOR("Anton Vorontsov <cbouatmailru@gmail.com>");
 MODULE_LICENSE("GPL");

-- 
2.47.1

[PATCH v6 3/4] power: supply: cros_charge-control: implement a power supply extension

[Thomas Weißschuh]

Power supply extensions provide an easier mechanism to implement
additional properties for existing power supplies.
Use that instead of reimplementing the sysfs attributes manually.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/cros_charge-control.c | 197 +++++++++++++----------------
 1 file changed, 85 insertions(+), 112 deletions(-)

diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index 9b0a7500296b4d7eb8cd53153e148926bb98aec1..fb4af232721dec1d4f0090f6616922848812b2a2 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -20,13 +20,6 @@
 					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)   | \
 					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
 
-enum CROS_CHCTL_ATTR {
-	CROS_CHCTL_ATTR_START_THRESHOLD,
-	CROS_CHCTL_ATTR_END_THRESHOLD,
-	CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR,
-	_CROS_CHCTL_ATTR_COUNT
-};
-
 /*
  * Semantics of data *returned* from the EC API and Linux sysfs differ
  * slightly, also the v1 API can not return any data.
@@ -43,13 +36,7 @@ struct cros_chctl_priv {
 	struct power_supply *hooked_battery;
 	u8 cmd_version;
 
-	/* The callbacks need to access this priv structure.
-	 * As neither the struct device nor power_supply are under the drivers
-	 * control, embed the attributes within priv to use with container_of().
-	 */
-	struct device_attribute device_attrs[_CROS_CHCTL_ATTR_COUNT];
-	struct attribute *attributes[_CROS_CHCTL_ATTR_COUNT];
-	struct attribute_group group;
+	const struct power_supply_ext *psy_ext;
 
 	struct mutex lock; /* protects fields below and cros_ec */
 	enum power_supply_charge_behaviour current_behaviour;
@@ -119,26 +106,39 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
 	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
 }
 
-static struct cros_chctl_priv *cros_chctl_attr_to_priv(struct attribute *attr,
-						       enum CROS_CHCTL_ATTR idx)
+static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
+				       const struct power_supply_ext *ext,
+				       void *data,
+				       enum power_supply_property psp,
+				       union power_supply_propval *val)
 {
-	struct device_attribute *dev_attr = container_of(attr, struct device_attribute, attr);
+	struct cros_chctl_priv *priv = data;
 
-	return container_of(dev_attr, struct cros_chctl_priv, device_attrs[idx]);
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+		val->intval = priv->current_start_threshold;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		val->intval = priv->current_end_threshold;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		val->intval = priv->current_behaviour;
+		return 0;
+	default:
+		return -EINVAL;
+	}
 }
 
-static ssize_t cros_chctl_store_threshold(struct device *dev, struct cros_chctl_priv *priv,
-					  int is_end_threshold, const char *buf, size_t count)
+static int cros_chctl_psy_ext_set_threshold(struct cros_chctl_priv *priv,
+					    enum power_supply_property psp,
+					    int val)
 {
-	int ret, val;
+	int ret;
 
-	ret = kstrtoint(buf, 10, &val);
-	if (ret < 0)
-		return ret;
 	if (val < 0 || val > 100)
 		return -EINVAL;
 
-	if (is_end_threshold) {
+	if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD) {
 		/* Start threshold is not exposed, use fixed value */
 		if (priv->cmd_version == 2)
 			priv->current_start_threshold = val == 100 ? 0 : val;
@@ -158,93 +158,72 @@ static ssize_t cros_chctl_store_threshold(struct device *dev, struct cros_chctl_
 			return ret;
 	}
 
-	return count;
-}
-
-static ssize_t charge_control_start_threshold_show(struct device *dev,
-						   struct device_attribute *attr,
-						   char *buf)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_START_THRESHOLD);
-
-	guard(mutex)(&priv->lock);
-	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_start_threshold);
+	return 0;
 }
 
-static ssize_t charge_control_start_threshold_store(struct device *dev,
-						    struct device_attribute *attr,
-						    const char *buf, size_t count)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_START_THRESHOLD);
-
-	guard(mutex)(&priv->lock);
-	return cros_chctl_store_threshold(dev, priv, 0, buf, count);
-}
 
-static ssize_t charge_control_end_threshold_show(struct device *dev, struct device_attribute *attr,
-						 char *buf)
+static int cros_chctl_psy_ext_set_prop(struct power_supply *psy,
+				       const struct power_supply_ext *ext,
+				       void *data,
+				       enum power_supply_property psp,
+				       const union power_supply_propval *val)
 {
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_END_THRESHOLD);
+	struct cros_chctl_priv *priv = data;
+	int ret;
 
 	guard(mutex)(&priv->lock);
-	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_end_threshold);
-}
-
-static ssize_t charge_control_end_threshold_store(struct device *dev, struct device_attribute *attr,
-						  const char *buf, size_t count)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_END_THRESHOLD);
 
-	guard(mutex)(&priv->lock);
-	return cros_chctl_store_threshold(dev, priv, 1, buf, count);
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		return cros_chctl_psy_ext_set_threshold(priv, psp, val->intval);
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		priv->current_behaviour = val->intval;
+		ret = cros_chctl_configure_ec(priv);
+		if (ret < 0)
+			return ret;
+		return 0;
+	default:
+		return -EINVAL;
+	}
 }
 
-static ssize_t charge_behaviour_show(struct device *dev, struct device_attribute *attr, char *buf)
+static int cros_chctl_psy_prop_is_writeable(struct power_supply *psy,
+					    const struct power_supply_ext *ext,
+					    void *data,
+					    enum power_supply_property psp)
 {
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
-
-	guard(mutex)(&priv->lock);
-	return power_supply_charge_behaviour_show(dev, EC_CHARGE_CONTROL_BEHAVIOURS,
-						  priv->current_behaviour, buf);
+	return true;
 }
 
-static ssize_t charge_behaviour_store(struct device *dev, struct device_attribute *attr,
-				      const char *buf, size_t count)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
-	int ret;
-
-	ret = power_supply_charge_behaviour_parse(EC_CHARGE_CONTROL_BEHAVIOURS, buf);
-	if (ret < 0)
-		return ret;
-
-	guard(mutex)(&priv->lock);
-	priv->current_behaviour = ret;
+#define DEFINE_CROS_CHCTL_POWER_SUPPLY_EXTENSION(_name, ...)			\
+	static const enum power_supply_property _name ## _props[] = {		\
+		__VA_ARGS__,							\
+	};									\
+										\
+	static const struct power_supply_ext _name = {				\
+		.properties		= _name ## _props,			\
+		.num_properties		= ARRAY_SIZE(_name ## _props),		\
+		.charge_behaviours	= EC_CHARGE_CONTROL_BEHAVIOURS,		\
+		.get_property		= cros_chctl_psy_ext_get_prop,		\
+		.set_property		= cros_chctl_psy_ext_set_prop,		\
+		.property_is_writeable	= cros_chctl_psy_prop_is_writeable,	\
+	}
 
-	ret = cros_chctl_configure_ec(priv);
-	if (ret < 0)
-		return ret;
+DEFINE_CROS_CHCTL_POWER_SUPPLY_EXTENSION(cros_chctl_psy_ext_v1,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
+);
 
-	return count;
-}
+DEFINE_CROS_CHCTL_POWER_SUPPLY_EXTENSION(cros_chctl_psy_ext_v2,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD
+);
 
-static umode_t cros_chtl_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(attr, n);
-
-	if (n == CROS_CHCTL_ATTR_START_THRESHOLD && priv->cmd_version < 3)
-		return 0;
-	else if (n == CROS_CHCTL_ATTR_END_THRESHOLD && priv->cmd_version < 2)
-		return 0;
-
-	return attr->mode;
-}
+DEFINE_CROS_CHCTL_POWER_SUPPLY_EXTENSION(cros_chctl_psy_ext_v3,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD
+);
 
 static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
@@ -254,7 +233,7 @@ static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_batt
 		return 0;
 
 	priv->hooked_battery = battery;
-	return device_add_group(&battery->dev, &priv->group);
+	return power_supply_register_extension(battery, priv->psy_ext, priv);
 }
 
 static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
@@ -262,7 +241,7 @@ static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_b
 	struct cros_chctl_priv *priv = container_of(hook, struct cros_chctl_priv, battery_hook);
 
 	if (priv->hooked_battery == battery) {
-		device_remove_group(&battery->dev, &priv->group);
+		power_supply_unregister_extension(battery, priv->psy_ext);
 		priv->hooked_battery = NULL;
 	}
 
@@ -288,7 +267,6 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
 	struct cros_chctl_priv *priv;
-	size_t i;
 	int ret;
 
 	ret = cros_chctl_fwk_charge_control_versions(cros_ec);
@@ -322,18 +300,13 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	dev_dbg(dev, "Command version: %u\n", (unsigned int)priv->cmd_version);
 
 	priv->cros_ec = cros_ec;
-	priv->device_attrs[CROS_CHCTL_ATTR_START_THRESHOLD] =
-		(struct device_attribute)__ATTR_RW(charge_control_start_threshold);
-	priv->device_attrs[CROS_CHCTL_ATTR_END_THRESHOLD] =
-		(struct device_attribute)__ATTR_RW(charge_control_end_threshold);
-	priv->device_attrs[CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR] =
-		(struct device_attribute)__ATTR_RW(charge_behaviour);
-	for (i = 0; i < _CROS_CHCTL_ATTR_COUNT; i++) {
-		sysfs_attr_init(&priv->device_attrs[i].attr);
-		priv->attributes[i] = &priv->device_attrs[i].attr;
-	}
-	priv->group.is_visible = cros_chtl_attr_is_visible;
-	priv->group.attrs = priv->attributes;
+
+	if (priv->cmd_version == 1)
+		priv->psy_ext = &cros_chctl_psy_ext_v1;
+	else if (priv->cmd_version == 2)
+		priv->psy_ext = &cros_chctl_psy_ext_v2;
+	else
+		priv->psy_ext = &cros_chctl_psy_ext_v3;
 
 	priv->battery_hook.name = dev_name(dev);
 	priv->battery_hook.add_battery = cros_chctl_add_battery;

-- 
2.47.1

[PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Thomas Weißschuh]

Userspace wants to now about the used power supply extensions,
for example to handle a device extended by a certain extension
differently or to discover information about the extending device.

Add a sysfs directory to the power supply device.
This directory contains links which are named after the used extension
and point to the device implementing that extension.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 Documentation/ABI/testing/sysfs-class-power |  9 +++++++++
 drivers/power/supply/cros_charge-control.c  |  5 ++++-
 drivers/power/supply/power_supply.h         |  2 ++
 drivers/power/supply/power_supply_core.c    | 19 +++++++++++++++++--
 drivers/power/supply/power_supply_sysfs.c   | 10 ++++++++++
 drivers/power/supply/test_power.c           |  4 +++-
 include/linux/power_supply.h                |  2 ++
 7 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index 45180b62d42686c8999eda54f38435cb6c74a879..31e8b33d849cbe99dc93a4ba3723a43440ac3103 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -793,3 +793,12 @@ Description:
 
 		Access: Read
 		Valid values: 1-31
+
+What:		/sys/class/power_supply/<supply_name>/extensions/<extension_name>
+Date:		March 2025
+Contact:	linux-pm@vger.kernel.org
+Description:
+		Reports the extensions registered to the power supply.
+		Each entry is a link to the device which registered the extension.
+
+		Access: Read
diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index fb4af232721dec1d4f0090f6616922848812b2a2..02d5bdbe2e8d45108dd8f2d3ab6a927b94864b9e 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -31,6 +31,7 @@
  */
 
 struct cros_chctl_priv {
+	struct device *dev;
 	struct cros_ec_device *cros_ec;
 	struct acpi_battery_hook battery_hook;
 	struct power_supply *hooked_battery;
@@ -202,6 +203,7 @@ static int cros_chctl_psy_prop_is_writeable(struct power_supply *psy,
 	};									\
 										\
 	static const struct power_supply_ext _name = {				\
+		.name			= "cros-charge-control",		\
 		.properties		= _name ## _props,			\
 		.num_properties		= ARRAY_SIZE(_name ## _props),		\
 		.charge_behaviours	= EC_CHARGE_CONTROL_BEHAVIOURS,		\
@@ -233,7 +235,7 @@ static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_batt
 		return 0;
 
 	priv->hooked_battery = battery;
-	return power_supply_register_extension(battery, priv->psy_ext, priv);
+	return power_supply_register_extension(battery, priv->psy_ext, priv->dev, priv);
 }
 
 static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
@@ -299,6 +301,7 @@ static int cros_chctl_probe(struct platform_device *pdev)
 
 	dev_dbg(dev, "Command version: %u\n", (unsigned int)priv->cmd_version);
 
+	priv->dev = dev;
 	priv->cros_ec = cros_ec;
 
 	if (priv->cmd_version == 1)
diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 531785516d2ac31f9a7f73a58e15e64cb81820ed..9ed749cd09369f0f13017847687509736b30aae8 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -25,6 +25,7 @@ extern bool power_supply_ext_has_property(const struct power_supply_ext *ext,
 struct power_supply_ext_registration {
 	struct list_head list_head;
 	const struct power_supply_ext *ext;
+	struct device *dev;
 	void *data;
 };
 
@@ -39,6 +40,7 @@ struct power_supply_ext_registration {
 
 extern void __init power_supply_init_attrs(void);
 extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
+extern const struct attribute_group power_supply_extension_group;
 extern const struct attribute_group *power_supply_attr_groups[];
 
 #else
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index bc22edbd0e6a02c27500132075f5c98d814a7330..5142fbd580ee3d629a2aae7d0b9bcd5709162129 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1346,17 +1346,21 @@ static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
 }
 
 int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
-				    void *data)
+				    struct device *dev, void *data)
 {
 	struct power_supply_ext_registration *reg;
 	size_t i;
 	int ret;
 
-	if (!psy || !ext || !ext->properties || !ext->num_properties)
+	if (!psy || !dev || !ext || !ext->name || !ext->properties || !ext->num_properties)
 		return -EINVAL;
 
 	guard(rwsem_write)(&psy->extensions_sem);
 
+	power_supply_for_each_extension(reg, psy)
+		if (strcmp(ext->name, reg->ext->name) == 0)
+			return -EEXIST;
+
 	for (i = 0; i < ext->num_properties; i++)
 		if (power_supply_has_property(psy, ext->properties[i]))
 			return -EEXIST;
@@ -1366,9 +1370,15 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
 		return -ENOMEM;
 
 	reg->ext = ext;
+	reg->dev = dev;
 	reg->data = data;
 	list_add(&reg->list_head, &psy->extensions);
 
+	ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
+				      &dev->kobj, ext->name);
+	if (ret)
+		goto sysfs_link_failed;
+
 	ret = power_supply_update_sysfs_and_hwmon(psy);
 	if (ret)
 		goto sysfs_hwmon_failed;
@@ -1376,6 +1386,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
 	return 0;
 
 sysfs_hwmon_failed:
+	sysfs_remove_link_from_group(&psy->dev.kobj, power_supply_extension_group.name, ext->name);
+sysfs_link_failed:
 	list_del(&reg->list_head);
 	kfree(reg);
 	return ret;
@@ -1392,6 +1404,9 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
 		if (reg->ext == ext) {
 			list_del(&reg->list_head);
 			kfree(reg);
+			sysfs_remove_link_from_group(&psy->dev.kobj,
+						     power_supply_extension_group.name,
+						     reg->ext->name);
 			power_supply_update_sysfs_and_hwmon(psy);
 			return;
 		}
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 927ddb9d83bb7259809ba695cb9398d1ad654b46..aadc41ca741d8acd27a83f6bd01d578d7877e7c2 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -421,8 +421,18 @@ static const struct attribute_group power_supply_attr_group = {
 	.is_visible = power_supply_attr_is_visible,
 };
 
+static struct attribute *power_supply_extension_attrs[] = {
+	NULL
+};
+
+const struct attribute_group power_supply_extension_group = {
+	.name = "extensions",
+	.attrs = power_supply_extension_attrs,
+};
+
 const struct attribute_group *power_supply_attr_groups[] = {
 	&power_supply_attr_group,
+	&power_supply_extension_group,
 	NULL
 };
 
diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 66f9ef52e0f3e6e6e6bebcfd438c2acd421284ec..2a975a110f4859a77f7689369675f2008816d704 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -293,6 +293,7 @@ static int test_power_battery_extproperty_is_writeable(struct power_supply *psy,
 }
 
 static const struct power_supply_ext test_power_battery_ext = {
+	.name			= "test_power",
 	.properties		= test_power_battery_extprops,
 	.num_properties		= ARRAY_SIZE(test_power_battery_extprops),
 	.get_property		= test_power_battery_extget_property,
@@ -307,7 +308,8 @@ static void test_power_configure_battery_extension(bool enable)
 	psy = test_power_supplies[TEST_BATTERY];
 
 	if (enable) {
-		if (power_supply_register_extension(psy, &test_power_battery_ext, NULL)) {
+		if (power_supply_register_extension(psy, &test_power_battery_ext, &psy->dev,
+						    NULL)) {
 			pr_err("registering battery extension failed\n");
 			return;
 		}
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index e434516086f032cdb4698005bb1a99eda303a307..88a7bd34c8a74d694013aaaebd30269b30509e8b 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -284,6 +284,7 @@ struct power_supply_desc {
 };
 
 struct power_supply_ext {
+	const char *const name;
 	u8 charge_behaviours;
 	const enum power_supply_property *properties;
 	size_t num_properties;
@@ -907,6 +908,7 @@ extern int power_supply_powers(struct power_supply *psy, struct device *dev);
 extern int __must_check
 power_supply_register_extension(struct power_supply *psy,
 				const struct power_supply_ext *ext,
+				struct device *dev,
 				void *data);
 extern void power_supply_unregister_extension(struct power_supply *psy,
 					      const struct power_supply_ext *ext);

-- 
2.47.1

Re: [PATCH v6 0/4] power: supply: extension API

[Armin Wolf]

Am 11.12.24 um 20:57 schrieb Thomas Weißschuh:

> Introduce a mechanism for drivers to extend the properties implemented
> by a power supply.
>
> Motivation
> ----------
>
> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
>
> While the created sysfs attributes look similar to the attributes
> provided by the powersupply core, there are various deficiencies:
>
> * They don't show up in uevent payload.
> * They can't be queried with the standard in-kernel APIs.
> * They don't work with triggers.
> * The extending driver has to reimplement all of the parsing,
>    formatting and sysfs display logic.
> * Writing a extension driver is completely different from writing a
>    normal power supply driver.
> * ~Properties can not be properly overriden.~
>    (Overriding is now explicitly forbidden)
>
> The proposed extension API avoids all of these issues.
> An extension is just a "struct power_supply_ext" with the same kind of
> callbacks as in a normal "struct power_supply_desc".
>
> The API is meant to be used via battery_hook_register(), the same way as
> the current extensions.
> Further usecases are fuel gauges and the existing battery_info
> properties.
>
> When testing, please enable lockdep to make sure the locking is correct.
>
> The series is based on the linux-power-supply/for-next branch.
> It also depends on some recent fixes not yet available in the for-next
> branch [0].
>
> [0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Changes in v6:
> - Drop alreay picked up ACPI battery hook rename patch
> - Only return bool from power_supply_property_is_writeable()
> - Improve naming for test_power symbols
> - Integrate cros_charge-control fixes from the psy/fixes branch
> - Add sysfs UAPI for extension discovery
> - Use __must_check on API
> - Make power_supply_for_each_extension() safer.
>    (And uglier, ideas welcome)

Maybe we can use a do { ... } while (0) construct here.

> - Link to v5: https://lore.kernel.org/r/20241205-power-supply-extensions-v5-0-f0f996db4347@weissschuh.net
>
> Changes in v5:
> - Drop already picked up patches
> - Simplify power_supply_ext_has_property()
> - Handle failure of power_supply_update_sysfs_and_hwmon()
> - Reduce some locking scopes
> - Add missing locking to power_supply_show_charge_behaviour()
> - Improve sanity checks in power_supply_register_extension()
> - Implement writeable property in test_power battery
> - Rename ACPI battery hook messages for clarity
> - Link to v4: https://lore.kernel.org/r/20241111-power-supply-extensions-v4-0-7240144daa8e@weissschuh.net
>
> Changes in v4:
> - Drop RFC state
> - Integrate locking commit
> - Reregister hwmon device
> - Link to v3: https://lore.kernel.org/r/20240904-power-supply-extensions-v3-0-62efeb93f8ec@weissschuh.net
>
> Changes in v3:
> - Make naming more consistent
> - Readd locking
> - Allow multiple active extensions
> - Allow passing a "void *ext_data" when registering
> - Switch example driver from system76 to cros_charge-control
> - Link to v2: https://lore.kernel.org/r/20240608-power-supply-extensions-v2-0-2dcd35b012ad@weissschuh.net
>
> Changes in v2:
> - Drop locking patch, let's figure out the API first
> - Allow registration of multiple extensions
> - Pass extension to extension callbacks as parameter
> - Disallow property overlap between extensions and core psy
> - Drop system76/pdx86 maintainers, as the system76 changes are only RFC
>    state anyways
> - Link to v1: https://lore.kernel.org/r/20240606-power-supply-extensions-v1-0-b45669290bdc@weissschuh.net
>
> ---
> Thomas Weißschuh (4):
>        power: supply: core: implement extension API
>        power: supply: test-power: implement a power supply extension
>        power: supply: cros_charge-control: implement a power supply extension
>        power: supply: core: add UAPI to discover currently used extensions
>
>   Documentation/ABI/testing/sysfs-class-power |   9 ++
>   drivers/power/supply/cros_charge-control.c  | 200 ++++++++++++----------------
>   drivers/power/supply/power_supply.h         |  19 +++
>   drivers/power/supply/power_supply_core.c    | 177 ++++++++++++++++++++++--
>   drivers/power/supply/power_supply_sysfs.c   |  36 ++++-
>   drivers/power/supply/test_power.c           | 113 ++++++++++++++++
>   include/linux/power_supply.h                |  35 +++++
>   7 files changed, 467 insertions(+), 122 deletions(-)
> ---
> base-commit: 810dde9dad8222f3b831cf5179927fc66fc6a006
> change-id: 20240602-power-supply-extensions-07d949f509d9
>
> Best regards,

Re: [PATCH v6 0/4] power: supply: extension API

[Thomas Weißschuh]

On 2024-12-12 15:27:52+0100, Armin Wolf wrote:
> Am 11.12.24 um 20:57 schrieb Thomas Weißschuh:
> 
> > Introduce a mechanism for drivers to extend the properties implemented
> > by a power supply.

[..]

> > ---
> > Changes in v6:
> > - Drop alreay picked up ACPI battery hook rename patch
> > - Only return bool from power_supply_property_is_writeable()
> > - Improve naming for test_power symbols
> > - Integrate cros_charge-control fixes from the psy/fixes branch
> > - Add sysfs UAPI for extension discovery
> > - Use __must_check on API
> > - Make power_supply_for_each_extension() safer.
> >    (And uglier, ideas welcome)
> 
> Maybe we can use a do { ... } while (0) construct here.

I don't think so. The macro needs to expand to a dangling loop
condition. So whatever statement comes after gets executed in the loop.

[..]

Re: [PATCH v6 1/4] power: supply: core: implement extension API

[Armin Wolf]

Am 11.12.24 um 20:57 schrieb Thomas Weißschuh:

> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
>
> While the created sysfs attributes look similar to the attributes
> provided by the powersupply core, there are various deficiencies:
>
> * They don't show up in uevent payload.
> * They can't be queried with the standard in-kernel APIs.
> * They don't work with triggers.
> * The extending driver has to reimplement all of the parsing,
> formatting and sysfs display logic.
> * Writing a extension driver is completely different from writing a
> normal power supply driver.
>
> This extension API avoids all of these issues.
> An extension is just a "struct power_supply_ext" with the same kind of
> callbacks as in a normal "struct power_supply_desc".
>
> The API is meant to be used via battery_hook_register(), the same way as
> the current extensions.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/power/supply/power_supply.h       |  17 ++++
>   drivers/power/supply/power_supply_core.c  | 162 ++++++++++++++++++++++++++++--
>   drivers/power/supply/power_supply_sysfs.c |  26 ++++-
>   include/linux/power_supply.h              |  33 ++++++
>   4 files changed, 228 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 5dabbd895538003096b62d03fdd0201b82b090e6..531785516d2ac31f9a7f73a58e15e64cb81820ed 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -9,6 +9,8 @@
>    *  Modified: 2004, Oct     Szabolcs Gyurko
>    */
>
> +#include <linux/lockdep.h>
> +
>   struct device;
>   struct device_type;
>   struct power_supply;
> @@ -17,6 +19,21 @@ extern int power_supply_property_is_writeable(struct power_supply *psy,
>   					      enum power_supply_property psp);
>   extern bool power_supply_has_property(struct power_supply *psy,
>   				      enum power_supply_property psp);
> +extern bool power_supply_ext_has_property(const struct power_supply_ext *ext,
> +					  enum power_supply_property psp);
> +
> +struct power_supply_ext_registration {
> +	struct list_head list_head;
> +	const struct power_supply_ext *ext;
> +	void *data;
> +};
> +
> +/* Make sure that the macro is a single expression */
> +#define power_supply_for_each_extension(pos, psy)			\
> +	if ( ({ lockdep_assert_held(&(psy)->extensions_sem); 0; }) )	\
> +		;							\
> +	else								\
> +		list_for_each_entry(pos, &(psy)->extensions, list_head)	\
>
>   #ifdef CONFIG_SYSFS
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 502b07468b93dfb7f5a6c2092588d931a7d015f2..bc22edbd0e6a02c27500132075f5c98d814a7330 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -81,6 +81,7 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>
>   static void power_supply_changed_work(struct work_struct *work)
>   {
> +	int ret;
>   	unsigned long flags;
>   	struct power_supply *psy = container_of(work, struct power_supply,
>   						changed_work);
> @@ -88,6 +89,16 @@ static void power_supply_changed_work(struct work_struct *work)
>   	dev_dbg(&psy->dev, "%s\n", __func__);
>
>   	spin_lock_irqsave(&psy->changed_lock, flags);
> +
> +	if (unlikely(psy->update_groups)) {
> +		psy->update_groups = false;
> +		spin_unlock_irqrestore(&psy->changed_lock, flags);
> +		ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
> +		if (ret)
> +			dev_warn(&psy->dev, "failed to update sysfs groups: %pe\n", ERR_PTR(ret));
> +		spin_lock_irqsave(&psy->changed_lock, flags);
> +	}
> +
>   	/*
>   	 * Check 'changed' here to avoid issues due to race between
>   	 * power_supply_changed() and this routine. In worst case
> @@ -1196,15 +1207,34 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
>   	return found;
>   }
>
> +bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
> +				   enum power_supply_property psp)
> +{
> +	int i;
> +
> +	for (i = 0; i < psy_ext->num_properties; i++)
> +		if (psy_ext->properties[i] == psp)
> +			return true;
> +
> +	return false;
> +}
> +
>   bool power_supply_has_property(struct power_supply *psy,
>   			       enum power_supply_property psp)
>   {
> +	struct power_supply_ext_registration *reg;
> +
>   	if (psy_desc_has_property(psy->desc, psp))
>   		return true;
>
>   	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
>   		return true;
>
> +	power_supply_for_each_extension(reg, psy) {
> +		if (power_supply_ext_has_property(reg->ext, psp))
> +			return true;
> +	}
> +
>   	return false;
>   }
>
> @@ -1212,12 +1242,21 @@ int power_supply_get_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
>   			    union power_supply_propval *val)
>   {
> +	struct power_supply_ext_registration *reg;
> +
>   	if (atomic_read(&psy->use_cnt) <= 0) {
>   		if (!psy->initialized)
>   			return -EAGAIN;
>   		return -ENODEV;
>   	}
>
> +	scoped_guard(rwsem_read, &psy->extensions_sem) {
> +		power_supply_for_each_extension(reg, psy) {
> +			if (power_supply_ext_has_property(reg->ext, psp))
> +				return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
> +		}
> +	}
> +
>   	if (psy_desc_has_property(psy->desc, psp))
>   		return psy->desc->get_property(psy, psp, val);
>   	else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> @@ -1231,7 +1270,24 @@ int power_supply_set_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
>   			    const union power_supply_propval *val)
>   {
> -	if (atomic_read(&psy->use_cnt) <= 0 || !psy->desc->set_property)
> +	struct power_supply_ext_registration *reg;
> +
> +	if (atomic_read(&psy->use_cnt) <= 0)
> +		return -ENODEV;
> +
> +	scoped_guard(rwsem_read, &psy->extensions_sem) {
> +		power_supply_for_each_extension(reg, psy) {
> +			if (power_supply_ext_has_property(reg->ext, psp)) {
> +				if (reg->ext->set_property)
> +					return reg->ext->set_property(psy, reg->ext, reg->data,
> +								      psp, val);
> +				else
> +					return -ENODEV;
> +			}
> +		}
> +	}
> +
> +	if (!psy->desc->set_property)
>   		return -ENODEV;
>
>   	return psy->desc->set_property(psy, psp, val);
> @@ -1241,7 +1297,22 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
>   int power_supply_property_is_writeable(struct power_supply *psy,
>   					enum power_supply_property psp)
>   {
> -	return psy->desc->property_is_writeable && psy->desc->property_is_writeable(psy, psp);
> +	struct power_supply_ext_registration *reg;
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (power_supply_ext_has_property(reg->ext, psp)) {
> +			if (reg->ext->property_is_writeable)
> +				return reg->ext->property_is_writeable(psy, reg->ext,
> +								       reg->data, psp);
> +			else
> +				return 0;
> +		}
> +	}
> +
> +	if (!psy->desc->property_is_writeable)
> +		return 0;
> +
> +	return psy->desc->property_is_writeable(psy, psp);
>   }
>
>   void power_supply_external_power_changed(struct power_supply *psy)
> @@ -1260,6 +1331,76 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(power_supply_powers);
>
> +static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&psy->changed_lock, flags);
> +	psy->update_groups = true;
> +	spin_unlock_irqrestore(&psy->changed_lock, flags);
> +
> +	power_supply_changed(psy);
> +
> +	power_supply_remove_hwmon_sysfs(psy);
> +	return power_supply_add_hwmon_sysfs(psy);
> +}
> +
> +int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> +				    void *data)
> +{
> +	struct power_supply_ext_registration *reg;
> +	size_t i;
> +	int ret;
> +
> +	if (!psy || !ext || !ext->properties || !ext->num_properties)
> +		return -EINVAL;
> +
> +	guard(rwsem_write)(&psy->extensions_sem);
> +
> +	for (i = 0; i < ext->num_properties; i++)
> +		if (power_supply_has_property(psy, ext->properties[i]))
> +			return -EEXIST;
> +
> +	reg = kmalloc(sizeof(*reg), GFP_KERNEL);
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	reg->ext = ext;
> +	reg->data = data;
> +	list_add(&reg->list_head, &psy->extensions);
> +
> +	ret = power_supply_update_sysfs_and_hwmon(psy);
> +	if (ret)
> +		goto sysfs_hwmon_failed;
> +
> +	return 0;
> +
> +sysfs_hwmon_failed:
> +	list_del(&reg->list_head);
> +	kfree(reg);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(power_supply_register_extension);
> +
> +void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> +{
> +	struct power_supply_ext_registration *reg;
> +
> +	guard(rwsem_write)(&psy->extensions_sem);
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (reg->ext == ext) {
> +			list_del(&reg->list_head);
> +			kfree(reg);
> +			power_supply_update_sysfs_and_hwmon(psy);
> +			return;
> +		}
> +	}
> +
> +	dev_warn(&psy->dev, "Trying to unregister invalid extension");
> +}
> +EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
> +
>   static void power_supply_dev_release(struct device *dev)
>   {
>   	struct power_supply *psy = to_power_supply(dev);
> @@ -1414,6 +1555,9 @@ __power_supply_register(struct device *parent,
>   	}
>
>   	spin_lock_init(&psy->changed_lock);
> +	init_rwsem(&psy->extensions_sem);
> +	INIT_LIST_HEAD(&psy->extensions);
> +
>   	rc = device_add(dev);
>   	if (rc)
>   		goto device_add_failed;
> @@ -1426,13 +1570,15 @@ __power_supply_register(struct device *parent,
>   	if (rc)
>   		goto register_thermal_failed;
>
> -	rc = power_supply_create_triggers(psy);
> -	if (rc)
> -		goto create_triggers_failed;
> +	scoped_guard(rwsem_read, &psy->extensions_sem) {
> +		rc = power_supply_create_triggers(psy);
> +		if (rc)
> +			goto create_triggers_failed;
>
> -	rc = power_supply_add_hwmon_sysfs(psy);
> -	if (rc)
> -		goto add_hwmon_sysfs_failed;
> +		rc = power_supply_add_hwmon_sysfs(psy);
> +		if (rc)
> +			goto add_hwmon_sysfs_failed;
> +	}
>
>   	/*
>   	 * Update use_cnt after any uevents (most notably from device_add()).
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 99bfe1f03eb8326d38c4e2831c9670313b42e425..927ddb9d83bb7259809ba695cb9398d1ad654b46 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -268,6 +268,27 @@ static ssize_t power_supply_show_enum_with_available(
>   	return count;
>   }
>
> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> +						  struct power_supply *psy,
> +						  union power_supply_propval *value,
> +						  char *buf)
> +{
> +	struct power_supply_ext_registration *reg;
> +
> +	scoped_guard(rwsem_read, &psy->extensions_sem) {
> +		power_supply_for_each_extension(reg, psy) {
> +			if (power_supply_ext_has_property(reg->ext,
> +							  POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
> +				return power_supply_charge_behaviour_show(dev,
> +						reg->ext->charge_behaviours,
> +						value->intval, buf);
> +		}
> +	}
> +
> +	return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> +						  value->intval, buf);
> +}
> +
>   static ssize_t power_supply_format_property(struct device *dev,
>   					    bool uevent,
>   					    struct device_attribute *attr,
> @@ -307,8 +328,7 @@ static ssize_t power_supply_format_property(struct device *dev,
>   	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
>   		if (uevent) /* no possible values in uevents */
>   			goto default_format;
> -		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> -							 value.intval, buf);
> +		ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
>   		break;
>   	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
>   		ret = sysfs_emit(buf, "%s\n", value.strval);
> @@ -385,6 +405,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>   	if (attrno == POWER_SUPPLY_PROP_TYPE)
>   		return mode;
>
> +	guard(rwsem_read)(&psy->extensions_sem);
> +
>   	if (power_supply_has_property(psy, attrno)) {
>   		if (power_supply_property_is_writeable(psy, attrno) > 0)
>   			mode |= S_IWUSR;
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index b98106e1a90f34bce5129317a099f363248342b9..e434516086f032cdb4698005bb1a99eda303a307 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -15,6 +15,8 @@
>   #include <linux/device.h>
>   #include <linux/workqueue.h>
>   #include <linux/leds.h>
> +#include <linux/rwsem.h>
> +#include <linux/list.h>
>   #include <linux/spinlock.h>
>   #include <linux/notifier.h>
>
> @@ -281,6 +283,27 @@ struct power_supply_desc {
>   	int use_for_apm;
>   };
>
> +struct power_supply_ext {
> +	u8 charge_behaviours;
> +	const enum power_supply_property *properties;
> +	size_t num_properties;
> +
> +	int (*get_property)(struct power_supply *psy,
> +			    const struct power_supply_ext *ext,
> +			    void *data,
> +			    enum power_supply_property psp,
> +			    union power_supply_propval *val);
> +	int (*set_property)(struct power_supply *psy,
> +			    const struct power_supply_ext *ext,
> +			    void *data,
> +			    enum power_supply_property psp,
> +			    const union power_supply_propval *val);
> +	int (*property_is_writeable)(struct power_supply *psy,
> +				     const struct power_supply_ext *ext,
> +				     void *data,
> +				     enum power_supply_property psp);
> +};
> +
>   struct power_supply {
>   	const struct power_supply_desc *desc;
>
> @@ -300,10 +323,13 @@ struct power_supply {
>   	struct delayed_work deferred_register_work;
>   	spinlock_t changed_lock;
>   	bool changed;
> +	bool update_groups;
>   	bool initialized;
>   	bool removing;
>   	atomic_t use_cnt;
>   	struct power_supply_battery_info *battery_info;
> +	struct rw_semaphore extensions_sem; /* protects "extensions" */
> +	struct list_head extensions;
>   #ifdef CONFIG_THERMAL
>   	struct thermal_zone_device *tzd;
>   	struct thermal_cooling_device *tcd;
> @@ -878,6 +904,13 @@ devm_power_supply_register(struct device *parent,
>   extern void power_supply_unregister(struct power_supply *psy);
>   extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +extern int __must_check
> +power_supply_register_extension(struct power_supply *psy,
> +				const struct power_supply_ext *ext,
> +				void *data);
> +extern void power_supply_unregister_extension(struct power_supply *psy,
> +					      const struct power_supply_ext *ext);
> +
>   #define to_power_supply(device) container_of(device, struct power_supply, dev)
>
>   extern void *power_supply_get_drvdata(struct power_supply *psy);
>

Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Armin Wolf]

Am 11.12.24 um 20:57 schrieb Thomas Weißschuh:

> Userspace wants to now about the used power supply extensions,
> for example to handle a device extended by a certain extension
> differently or to discover information about the extending device.
>
> Add a sysfs directory to the power supply device.
> This directory contains links which are named after the used extension
> and point to the device implementing that extension.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   Documentation/ABI/testing/sysfs-class-power |  9 +++++++++
>   drivers/power/supply/cros_charge-control.c  |  5 ++++-
>   drivers/power/supply/power_supply.h         |  2 ++
>   drivers/power/supply/power_supply_core.c    | 19 +++++++++++++++++--
>   drivers/power/supply/power_supply_sysfs.c   | 10 ++++++++++
>   drivers/power/supply/test_power.c           |  4 +++-
>   include/linux/power_supply.h                |  2 ++
>   7 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> index 45180b62d42686c8999eda54f38435cb6c74a879..31e8b33d849cbe99dc93a4ba3723a43440ac3103 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -793,3 +793,12 @@ Description:
>
>   		Access: Read
>   		Valid values: 1-31
> +
> +What:		/sys/class/power_supply/<supply_name>/extensions/<extension_name>
> +Date:		March 2025
> +Contact:	linux-pm@vger.kernel.org
> +Description:
> +		Reports the extensions registered to the power supply.
> +		Each entry is a link to the device which registered the extension.
> +
> +		Access: Read
> diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
> index fb4af232721dec1d4f0090f6616922848812b2a2..02d5bdbe2e8d45108dd8f2d3ab6a927b94864b9e 100644
> --- a/drivers/power/supply/cros_charge-control.c
> +++ b/drivers/power/supply/cros_charge-control.c
> @@ -31,6 +31,7 @@
>    */
>
>   struct cros_chctl_priv {
> +	struct device *dev;
>   	struct cros_ec_device *cros_ec;
>   	struct acpi_battery_hook battery_hook;
>   	struct power_supply *hooked_battery;
> @@ -202,6 +203,7 @@ static int cros_chctl_psy_prop_is_writeable(struct power_supply *psy,
>   	};									\
>   										\
>   	static const struct power_supply_ext _name = {				\
> +		.name			= "cros-charge-control",		\
>   		.properties		= _name ## _props,			\
>   		.num_properties		= ARRAY_SIZE(_name ## _props),		\
>   		.charge_behaviours	= EC_CHARGE_CONTROL_BEHAVIOURS,		\
> @@ -233,7 +235,7 @@ static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_batt
>   		return 0;
>
>   	priv->hooked_battery = battery;
> -	return power_supply_register_extension(battery, priv->psy_ext, priv);
> +	return power_supply_register_extension(battery, priv->psy_ext, priv->dev, priv);
>   }
>
>   static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
> @@ -299,6 +301,7 @@ static int cros_chctl_probe(struct platform_device *pdev)
>
>   	dev_dbg(dev, "Command version: %u\n", (unsigned int)priv->cmd_version);
>
> +	priv->dev = dev;
>   	priv->cros_ec = cros_ec;
>
>   	if (priv->cmd_version == 1)
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 531785516d2ac31f9a7f73a58e15e64cb81820ed..9ed749cd09369f0f13017847687509736b30aae8 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -25,6 +25,7 @@ extern bool power_supply_ext_has_property(const struct power_supply_ext *ext,
>   struct power_supply_ext_registration {
>   	struct list_head list_head;
>   	const struct power_supply_ext *ext;
> +	struct device *dev;
>   	void *data;
>   };
>
> @@ -39,6 +40,7 @@ struct power_supply_ext_registration {
>
>   extern void __init power_supply_init_attrs(void);
>   extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
> +extern const struct attribute_group power_supply_extension_group;
>   extern const struct attribute_group *power_supply_attr_groups[];
>
>   #else
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index bc22edbd0e6a02c27500132075f5c98d814a7330..5142fbd580ee3d629a2aae7d0b9bcd5709162129 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1346,17 +1346,21 @@ static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
>   }
>
>   int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> -				    void *data)
> +				    struct device *dev, void *data)
>   {
>   	struct power_supply_ext_registration *reg;
>   	size_t i;
>   	int ret;
>
> -	if (!psy || !ext || !ext->properties || !ext->num_properties)
> +	if (!psy || !dev || !ext || !ext->name || !ext->properties || !ext->num_properties)
>   		return -EINVAL;
>
>   	guard(rwsem_write)(&psy->extensions_sem);
>
> +	power_supply_for_each_extension(reg, psy)
> +		if (strcmp(ext->name, reg->ext->name) == 0)
> +			return -EEXIST;
> +
>   	for (i = 0; i < ext->num_properties; i++)
>   		if (power_supply_has_property(psy, ext->properties[i]))
>   			return -EEXIST;
> @@ -1366,9 +1370,15 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>   		return -ENOMEM;
>
>   	reg->ext = ext;
> +	reg->dev = dev;
>   	reg->data = data;
>   	list_add(&reg->list_head, &psy->extensions);
>
> +	ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> +				      &dev->kobj, ext->name);
> +	if (ret)
> +		goto sysfs_link_failed;
> +
>   	ret = power_supply_update_sysfs_and_hwmon(psy);
>   	if (ret)
>   		goto sysfs_hwmon_failed;
> @@ -1376,6 +1386,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>   	return 0;
>
>   sysfs_hwmon_failed:
> +	sysfs_remove_link_from_group(&psy->dev.kobj, power_supply_extension_group.name, ext->name);
> +sysfs_link_failed:
>   	list_del(&reg->list_head);
>   	kfree(reg);
>   	return ret;
> @@ -1392,6 +1404,9 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
>   		if (reg->ext == ext) {
>   			list_del(&reg->list_head);
>   			kfree(reg);
> +			sysfs_remove_link_from_group(&psy->dev.kobj,
> +						     power_supply_extension_group.name,
> +						     reg->ext->name);
>   			power_supply_update_sysfs_and_hwmon(psy);
>   			return;
>   		}
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 927ddb9d83bb7259809ba695cb9398d1ad654b46..aadc41ca741d8acd27a83f6bd01d578d7877e7c2 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -421,8 +421,18 @@ static const struct attribute_group power_supply_attr_group = {
>   	.is_visible = power_supply_attr_is_visible,
>   };
>
> +static struct attribute *power_supply_extension_attrs[] = {
> +	NULL
> +};
> +
> +const struct attribute_group power_supply_extension_group = {
> +	.name = "extensions",
> +	.attrs = power_supply_extension_attrs,
> +};
> +
>   const struct attribute_group *power_supply_attr_groups[] = {
>   	&power_supply_attr_group,
> +	&power_supply_extension_group,
>   	NULL
>   };
>
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 66f9ef52e0f3e6e6e6bebcfd438c2acd421284ec..2a975a110f4859a77f7689369675f2008816d704 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -293,6 +293,7 @@ static int test_power_battery_extproperty_is_writeable(struct power_supply *psy,
>   }
>
>   static const struct power_supply_ext test_power_battery_ext = {
> +	.name			= "test_power",
>   	.properties		= test_power_battery_extprops,
>   	.num_properties		= ARRAY_SIZE(test_power_battery_extprops),
>   	.get_property		= test_power_battery_extget_property,
> @@ -307,7 +308,8 @@ static void test_power_configure_battery_extension(bool enable)
>   	psy = test_power_supplies[TEST_BATTERY];
>
>   	if (enable) {
> -		if (power_supply_register_extension(psy, &test_power_battery_ext, NULL)) {
> +		if (power_supply_register_extension(psy, &test_power_battery_ext, &psy->dev,
> +						    NULL)) {
>   			pr_err("registering battery extension failed\n");
>   			return;
>   		}
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index e434516086f032cdb4698005bb1a99eda303a307..88a7bd34c8a74d694013aaaebd30269b30509e8b 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -284,6 +284,7 @@ struct power_supply_desc {
>   };
>
>   struct power_supply_ext {
> +	const char *const name;
>   	u8 charge_behaviours;
>   	const enum power_supply_property *properties;
>   	size_t num_properties;
> @@ -907,6 +908,7 @@ extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>   extern int __must_check
>   power_supply_register_extension(struct power_supply *psy,
>   				const struct power_supply_ext *ext,
> +				struct device *dev,
>   				void *data);
>   extern void power_supply_unregister_extension(struct power_supply *psy,
>   					      const struct power_supply_ext *ext);
>

Re: (subset) [PATCH v6 0/4] power: supply: extension API

[Sebastian Reichel]

On Wed, 11 Dec 2024 20:57:54 +0100, Thomas Weißschuh wrote:
> Introduce a mechanism for drivers to extend the properties implemented
> by a power supply.
> 
> Motivation
> ----------
> 
> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
> 
> [...]

Applied, thanks!

[1/4] power: supply: core: implement extension API
      commit: 6037802bbae892f3ad0c7b4c4faee39b967e32b0
[2/4] power: supply: test-power: implement a power supply extension
      commit: 9d76d5de87bbf03c6e483565030b562dc42c7bff

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Thomas Weißschuh]

On 2024-12-11 20:57:58+0100, Thomas Weißschuh wrote:
> Userspace wants to now about the used power supply extensions,
> for example to handle a device extended by a certain extension
> differently or to discover information about the extending device.
> 
> Add a sysfs directory to the power supply device.
> This directory contains links which are named after the used extension
> and point to the device implementing that extension.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  Documentation/ABI/testing/sysfs-class-power |  9 +++++++++
>  drivers/power/supply/cros_charge-control.c  |  5 ++++-
>  drivers/power/supply/power_supply.h         |  2 ++
>  drivers/power/supply/power_supply_core.c    | 19 +++++++++++++++++--
>  drivers/power/supply/power_supply_sysfs.c   | 10 ++++++++++
>  drivers/power/supply/test_power.c           |  4 +++-
>  include/linux/power_supply.h                |  2 ++
>  7 files changed, 47 insertions(+), 4 deletions(-)

[..]

> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1346,17 +1346,21 @@ static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
>  }
>  
>  int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> -				    void *data)
> +				    struct device *dev, void *data)
>  {
>  	struct power_supply_ext_registration *reg;
>  	size_t i;
>  	int ret;
>  
> -	if (!psy || !ext || !ext->properties || !ext->num_properties)
> +	if (!psy || !dev || !ext || !ext->name || !ext->properties || !ext->num_properties)
>  		return -EINVAL;
>  
>  	guard(rwsem_write)(&psy->extensions_sem);
>  
> +	power_supply_for_each_extension(reg, psy)
> +		if (strcmp(ext->name, reg->ext->name) == 0)
> +			return -EEXIST;
> +
>  	for (i = 0; i < ext->num_properties; i++)
>  		if (power_supply_has_property(psy, ext->properties[i]))
>  			return -EEXIST;
> @@ -1366,9 +1370,15 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>  		return -ENOMEM;
>  
>  	reg->ext = ext;
> +	reg->dev = dev;
>  	reg->data = data;
>  	list_add(&reg->list_head, &psy->extensions);
>  
> +	ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> +				      &dev->kobj, ext->name);
> +	if (ret)
> +		goto sysfs_link_failed;
> +
>  	ret = power_supply_update_sysfs_and_hwmon(psy);
>  	if (ret)
>  		goto sysfs_hwmon_failed;
> @@ -1376,6 +1386,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>  	return 0;
>  
>  sysfs_hwmon_failed:
> +	sysfs_remove_link_from_group(&psy->dev.kobj, power_supply_extension_group.name, ext->name);
> +sysfs_link_failed:
>  	list_del(&reg->list_head);
>  	kfree(reg);
>  	return ret;
> @@ -1392,6 +1404,9 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
>  		if (reg->ext == ext) {
>  			list_del(&reg->list_head);
>  			kfree(reg);
> +			sysfs_remove_link_from_group(&psy->dev.kobj,
> +						     power_supply_extension_group.name,
> +						     reg->ext->name);

Dan Carpenter's sparse bot detected a use after free here.
Could you move the call to sysfs_remove_link_from_group() before the
kfree() when applying?

(Or switch reg->ext->name to ext->name)

>  			power_supply_update_sysfs_and_hwmon(psy);
>  			return;
>  		}

[..]


Thanks,
Thomas

Re: (subset) [PATCH v6 0/4] power: supply: extension API

[Sebastian Reichel]

On Wed, 11 Dec 2024 20:57:54 +0100, Thomas Weißschuh wrote:
> Introduce a mechanism for drivers to extend the properties implemented
> by a power supply.
> 
> Motivation
> ----------
> 
> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
> 
> [...]

Applied, thanks!

[4/4] power: supply: core: add UAPI to discover currently used extensions
      commit: 288a2cabcf6bb35532e8b2708829bdc2b85bc690

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Nathan Chancellor]

Hi Thomas,

On Wed, Dec 11, 2024 at 08:57:58PM +0100, Thomas Weißschuh wrote:
> Userspace wants to now about the used power supply extensions,
> for example to handle a device extended by a certain extension
> differently or to discover information about the extending device.
> 
> Add a sysfs directory to the power supply device.
> This directory contains links which are named after the used extension
> and point to the device implementing that extension.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  Documentation/ABI/testing/sysfs-class-power |  9 +++++++++
>  drivers/power/supply/cros_charge-control.c  |  5 ++++-
>  drivers/power/supply/power_supply.h         |  2 ++
>  drivers/power/supply/power_supply_core.c    | 19 +++++++++++++++++--
>  drivers/power/supply/power_supply_sysfs.c   | 10 ++++++++++
>  drivers/power/supply/test_power.c           |  4 +++-
>  include/linux/power_supply.h                |  2 ++
>  7 files changed, 47 insertions(+), 4 deletions(-)
...
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 531785516d2ac31f9a7f73a58e15e64cb81820ed..9ed749cd09369f0f13017847687509736b30aae8 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -39,6 +40,7 @@ struct power_supply_ext_registration {
>  
>  extern void __init power_supply_init_attrs(void);
>  extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
> +extern const struct attribute_group power_supply_extension_group;
>  extern const struct attribute_group *power_supply_attr_groups[];
>  
>  #else
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index bc22edbd0e6a02c27500132075f5c98d814a7330..5142fbd580ee3d629a2aae7d0b9bcd5709162129 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1346,17 +1346,21 @@ static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
>  }
>  
>  int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> -				    void *data)
> +				    struct device *dev, void *data)
>  {
>  	struct power_supply_ext_registration *reg;
>  	size_t i;
>  	int ret;
>  
> -	if (!psy || !ext || !ext->properties || !ext->num_properties)
> +	if (!psy || !dev || !ext || !ext->name || !ext->properties || !ext->num_properties)
>  		return -EINVAL;
>  
>  	guard(rwsem_write)(&psy->extensions_sem);
>  
> +	power_supply_for_each_extension(reg, psy)
> +		if (strcmp(ext->name, reg->ext->name) == 0)
> +			return -EEXIST;
> +
>  	for (i = 0; i < ext->num_properties; i++)
>  		if (power_supply_has_property(psy, ext->properties[i]))
>  			return -EEXIST;
> @@ -1366,9 +1370,15 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>  		return -ENOMEM;
>  
>  	reg->ext = ext;
> +	reg->dev = dev;
>  	reg->data = data;
>  	list_add(&reg->list_head, &psy->extensions);
>  
> +	ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> +				      &dev->kobj, ext->name);
> +	if (ret)
> +		goto sysfs_link_failed;
> +
>  	ret = power_supply_update_sysfs_and_hwmon(psy);
>  	if (ret)
>  		goto sysfs_hwmon_failed;
> @@ -1376,6 +1386,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>  	return 0;
>  
>  sysfs_hwmon_failed:
> +	sysfs_remove_link_from_group(&psy->dev.kobj, power_supply_extension_group.name, ext->name);
> +sysfs_link_failed:
>  	list_del(&reg->list_head);
>  	kfree(reg);
>  	return ret;
> @@ -1392,6 +1404,9 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
>  		if (reg->ext == ext) {
>  			list_del(&reg->list_head);
>  			kfree(reg);
> +			sysfs_remove_link_from_group(&psy->dev.kobj,
> +						     power_supply_extension_group.name,
> +						     reg->ext->name);
>  			power_supply_update_sysfs_and_hwmon(psy);
>  			return;
>  		}

I am seeing a build failure in certain configurations because
power_supply_extension_group is only declared under a CONFIG_SYSFS ifdef
but this code can be built without CONFIG_SYSFS.

  $ echo 'CONFIG_EXPERT=y
  CONFIG_SYSFS=n' >allno.config

  $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- KCONFIG_ALLCONFIG=1 mrproper allnoconfig drivers/power/supply/power_supply_core.o
  drivers/power/supply/power_supply_core.c: In function 'power_supply_register_extension':
  drivers/power/supply/power_supply_core.c:1389:55: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
   1389 |         ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
        |                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
        |                                                       power_supply_attr_groups
  drivers/power/supply/power_supply_core.c:1389:55: note: each undeclared identifier is reported only once for each function it appears in
  drivers/power/supply/power_supply_core.c: In function 'power_supply_unregister_extension':
  drivers/power/supply/power_supply_core.c:1419:54: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
   1419 |                                                      power_supply_extension_group.name,
        |                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
        |                                                      power_supply_attr_groups

Should the declaration be moved out from the ifdef or is there some
other solution I am not seeing?

Cheers,
Nathan

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 9ed749cd0936..6fc9939145fc 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -40,7 +40,6 @@ struct power_supply_ext_registration {
 
 extern void __init power_supply_init_attrs(void);
 extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
-extern const struct attribute_group power_supply_extension_group;
 extern const struct attribute_group *power_supply_attr_groups[];
 
 #else
@@ -51,6 +50,8 @@ static inline void power_supply_init_attrs(void) {}
 
 #endif /* CONFIG_SYSFS */
 
+extern const struct attribute_group power_supply_extension_group;
+
 #ifdef CONFIG_LEDS_TRIGGERS
 
 extern void power_supply_update_leds(struct power_supply *psy);

Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Thomas Weißschuh]

Hi Nathan,

On 2024-12-18 12:52:29-0700, Nathan Chancellor wrote:
> On Wed, Dec 11, 2024 at 08:57:58PM +0100, Thomas Weißschuh wrote:
> > Userspace wants to now about the used power supply extensions,
> > for example to handle a device extended by a certain extension
> > differently or to discover information about the extending device.
> > 
> > Add a sysfs directory to the power supply device.
> > This directory contains links which are named after the used extension
> > and point to the device implementing that extension.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  Documentation/ABI/testing/sysfs-class-power |  9 +++++++++
> >  drivers/power/supply/cros_charge-control.c  |  5 ++++-
> >  drivers/power/supply/power_supply.h         |  2 ++
> >  drivers/power/supply/power_supply_core.c    | 19 +++++++++++++++++--
> >  drivers/power/supply/power_supply_sysfs.c   | 10 ++++++++++
> >  drivers/power/supply/test_power.c           |  4 +++-
> >  include/linux/power_supply.h                |  2 ++
> >  7 files changed, 47 insertions(+), 4 deletions(-)
> ...
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index 531785516d2ac31f9a7f73a58e15e64cb81820ed..9ed749cd09369f0f13017847687509736b30aae8 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -39,6 +40,7 @@ struct power_supply_ext_registration {
> >  
> >  extern void __init power_supply_init_attrs(void);
> >  extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
> > +extern const struct attribute_group power_supply_extension_group;
> >  extern const struct attribute_group *power_supply_attr_groups[];
> >  
> >  #else
> > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> > index bc22edbd0e6a02c27500132075f5c98d814a7330..5142fbd580ee3d629a2aae7d0b9bcd5709162129 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -1346,17 +1346,21 @@ static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
> >  }
> >  
> >  int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> > -				    void *data)
> > +				    struct device *dev, void *data)
> >  {
> >  	struct power_supply_ext_registration *reg;
> >  	size_t i;
> >  	int ret;
> >  
> > -	if (!psy || !ext || !ext->properties || !ext->num_properties)
> > +	if (!psy || !dev || !ext || !ext->name || !ext->properties || !ext->num_properties)
> >  		return -EINVAL;
> >  
> >  	guard(rwsem_write)(&psy->extensions_sem);
> >  
> > +	power_supply_for_each_extension(reg, psy)
> > +		if (strcmp(ext->name, reg->ext->name) == 0)
> > +			return -EEXIST;
> > +
> >  	for (i = 0; i < ext->num_properties; i++)
> >  		if (power_supply_has_property(psy, ext->properties[i]))
> >  			return -EEXIST;
> > @@ -1366,9 +1370,15 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
> >  		return -ENOMEM;
> >  
> >  	reg->ext = ext;
> > +	reg->dev = dev;
> >  	reg->data = data;
> >  	list_add(&reg->list_head, &psy->extensions);
> >  
> > +	ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> > +				      &dev->kobj, ext->name);
> > +	if (ret)
> > +		goto sysfs_link_failed;
> > +
> >  	ret = power_supply_update_sysfs_and_hwmon(psy);
> >  	if (ret)
> >  		goto sysfs_hwmon_failed;
> > @@ -1376,6 +1386,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
> >  	return 0;
> >  
> >  sysfs_hwmon_failed:
> > +	sysfs_remove_link_from_group(&psy->dev.kobj, power_supply_extension_group.name, ext->name);
> > +sysfs_link_failed:
> >  	list_del(&reg->list_head);
> >  	kfree(reg);
> >  	return ret;
> > @@ -1392,6 +1404,9 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
> >  		if (reg->ext == ext) {
> >  			list_del(&reg->list_head);
> >  			kfree(reg);
> > +			sysfs_remove_link_from_group(&psy->dev.kobj,
> > +						     power_supply_extension_group.name,
> > +						     reg->ext->name);
> >  			power_supply_update_sysfs_and_hwmon(psy);
> >  			return;
> >  		}
> 
> I am seeing a build failure in certain configurations because
> power_supply_extension_group is only declared under a CONFIG_SYSFS ifdef
> but this code can be built without CONFIG_SYSFS.

Thanks for the report.

>   $ echo 'CONFIG_EXPERT=y
>   CONFIG_SYSFS=n' >allno.config
> 
>   $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- KCONFIG_ALLCONFIG=1 mrproper allnoconfig drivers/power/supply/power_supply_core.o
>   drivers/power/supply/power_supply_core.c: In function 'power_supply_register_extension':
>   drivers/power/supply/power_supply_core.c:1389:55: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
>    1389 |         ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
>         |                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>         |                                                       power_supply_attr_groups
>   drivers/power/supply/power_supply_core.c:1389:55: note: each undeclared identifier is reported only once for each function it appears in
>   drivers/power/supply/power_supply_core.c: In function 'power_supply_unregister_extension':
>   drivers/power/supply/power_supply_core.c:1419:54: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
>    1419 |                                                      power_supply_extension_group.name,
>         |                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>         |                                                      power_supply_attr_groups

The reproducer doesn't actually enable CONFIG_POWER_SUPPLY, when I use it
I get a whole array of errors.

> Should the declaration be moved out from the ifdef or is there some
> other solution I am not seeing?

This, inline constants or a #define.

Sebastian, do you want me to send a patch?

> Cheers,
> Nathan
> 
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 9ed749cd0936..6fc9939145fc 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -40,7 +40,6 @@ struct power_supply_ext_registration {
>  
>  extern void __init power_supply_init_attrs(void);
>  extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
> -extern const struct attribute_group power_supply_extension_group;
>  extern const struct attribute_group *power_supply_attr_groups[];
>  
>  #else
> @@ -51,6 +50,8 @@ static inline void power_supply_init_attrs(void) {}
>  
>  #endif /* CONFIG_SYSFS */
>  
> +extern const struct attribute_group power_supply_extension_group;
> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  
>  extern void power_supply_update_leds(struct power_supply *psy);

Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

Hi,

On Wed, Dec 18, 2024 at 09:29:31PM +0100, Thomas Weißschuh wrote:
> On 2024-12-18 12:52:29-0700, Nathan Chancellor wrote:
> > I am seeing a build failure in certain configurations because
> > power_supply_extension_group is only declared under a CONFIG_SYSFS ifdef
> > but this code can be built without CONFIG_SYSFS.
> 
> Thanks for the report.
> 
> >   $ echo 'CONFIG_EXPERT=y
> >   CONFIG_SYSFS=n' >allno.config
> > 
> >   $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- KCONFIG_ALLCONFIG=1 mrproper allnoconfig drivers/power/supply/power_supply_core.o
> >   drivers/power/supply/power_supply_core.c: In function 'power_supply_register_extension':
> >   drivers/power/supply/power_supply_core.c:1389:55: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> >    1389 |         ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> >         |                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >         |                                                       power_supply_attr_groups
> >   drivers/power/supply/power_supply_core.c:1389:55: note: each undeclared identifier is reported only once for each function it appears in
> >   drivers/power/supply/power_supply_core.c: In function 'power_supply_unregister_extension':
> >   drivers/power/supply/power_supply_core.c:1419:54: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> >    1419 |                                                      power_supply_extension_group.name,
> >         |                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >         |                                                      power_supply_attr_groups
> 
> The reproducer doesn't actually enable CONFIG_POWER_SUPPLY, when I use it
> I get a whole array of errors.
> 
> > Should the declaration be moved out from the ifdef or is there some
> > other solution I am not seeing?
> 
> This, inline constants or a #define.
> 
> Sebastian, do you want me to send a patch?

Yes, please send a patch. I suppose a define next to the NULL define
for power_supply_attr_groups should be good enough and consistent
with existing handling of this problem in the subsystem. 

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Thomas Weißschuh]

On 2024-12-18 23:11:35+0100, Sebastian Reichel wrote:
> On Wed, Dec 18, 2024 at 09:29:31PM +0100, Thomas Weißschuh wrote:
> > On 2024-12-18 12:52:29-0700, Nathan Chancellor wrote:
> > > I am seeing a build failure in certain configurations because
> > > power_supply_extension_group is only declared under a CONFIG_SYSFS ifdef
> > > but this code can be built without CONFIG_SYSFS.
> > 
> > Thanks for the report.
> > 
> > >   $ echo 'CONFIG_EXPERT=y
> > >   CONFIG_SYSFS=n' >allno.config
> > > 
> > >   $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- KCONFIG_ALLCONFIG=1 mrproper allnoconfig drivers/power/supply/power_supply_core.o
> > >   drivers/power/supply/power_supply_core.c: In function 'power_supply_register_extension':
> > >   drivers/power/supply/power_supply_core.c:1389:55: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> > >    1389 |         ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> > >         |                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >         |                                                       power_supply_attr_groups
> > >   drivers/power/supply/power_supply_core.c:1389:55: note: each undeclared identifier is reported only once for each function it appears in
> > >   drivers/power/supply/power_supply_core.c: In function 'power_supply_unregister_extension':
> > >   drivers/power/supply/power_supply_core.c:1419:54: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> > >    1419 |                                                      power_supply_extension_group.name,
> > >         |                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >         |                                                      power_supply_attr_groups
> > 
> > The reproducer doesn't actually enable CONFIG_POWER_SUPPLY, when I use it
> > I get a whole array of errors.
> > 
> > > Should the declaration be moved out from the ifdef or is there some
> > > other solution I am not seeing?
> > 
> > This, inline constants or a #define.
> > 
> > Sebastian, do you want me to send a patch?
> 
> Yes, please send a patch. I suppose a define next to the NULL define
> for power_supply_attr_groups should be good enough and consistent
> with existing handling of this problem in the subsystem. 

That doesn't work because of the usage of the member "name" of the
symbol power_supply_extension_group.

Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]

Hi,

On Wed, Dec 18, 2024 at 11:16:16PM +0100, Thomas Weißschuh wrote:
> On 2024-12-18 23:11:35+0100, Sebastian Reichel wrote:
> > On Wed, Dec 18, 2024 at 09:29:31PM +0100, Thomas Weißschuh wrote:
> > > On 2024-12-18 12:52:29-0700, Nathan Chancellor wrote:
> > > > I am seeing a build failure in certain configurations because
> > > > power_supply_extension_group is only declared under a CONFIG_SYSFS ifdef
> > > > but this code can be built without CONFIG_SYSFS.
> > > 
> > > Thanks for the report.
> > > 
> > > >   $ echo 'CONFIG_EXPERT=y
> > > >   CONFIG_SYSFS=n' >allno.config
> > > > 
> > > >   $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- KCONFIG_ALLCONFIG=1 mrproper allnoconfig drivers/power/supply/power_supply_core.o
> > > >   drivers/power/supply/power_supply_core.c: In function 'power_supply_register_extension':
> > > >   drivers/power/supply/power_supply_core.c:1389:55: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> > > >    1389 |         ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> > > >         |                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >         |                                                       power_supply_attr_groups
> > > >   drivers/power/supply/power_supply_core.c:1389:55: note: each undeclared identifier is reported only once for each function it appears in
> > > >   drivers/power/supply/power_supply_core.c: In function 'power_supply_unregister_extension':
> > > >   drivers/power/supply/power_supply_core.c:1419:54: error: 'power_supply_extension_group' undeclared (first use in this function); did you mean 'power_supply_attr_groups'?
> > > >    1419 |                                                      power_supply_extension_group.name,
> > > >         |                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >         |                                                      power_supply_attr_groups
> > > 
> > > The reproducer doesn't actually enable CONFIG_POWER_SUPPLY, when I use it
> > > I get a whole array of errors.
> > > 
> > > > Should the declaration be moved out from the ifdef or is there some
> > > > other solution I am not seeing?
> > > 
> > > This, inline constants or a #define.
> > > 
> > > Sebastian, do you want me to send a patch?
> > 
> > Yes, please send a patch. I suppose a define next to the NULL define
> > for power_supply_attr_groups should be good enough and consistent
> > with existing handling of this problem in the subsystem. 
> 
> That doesn't work because of the usage of the member "name" of the
> symbol power_supply_extension_group.

Right. Maybe make the power_supply_extension_group static to
power_supply_sysfs.c and instead offer power_supply_add_extension_sysfs()
as a wrapper to sysfs_add_link_to_group(). Then power_supply.h can
provide a dummy for !CONFIG_SYSFS making this similar to the
power_supply_add_hwmon_sysfs() handling.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]