| Message ID | 20241211-power-supply-extensions-v6-4-9d9dc3f3d387@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: extension API | expand |
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(®->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(®->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(®->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); >
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(®->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(®->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(®->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
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(®->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(®->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(®->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(®->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(®->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(®->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
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(®->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(®->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(®->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);
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(-)