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

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);

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);

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

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.

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

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

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);
>