| Message ID | 20241211-power-supply-extensions-v6-0-9d9dc3f3d387@weissschuh.net |
|---|---|
| Headers | show |
| Series | power: supply: extension API | expand |
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,
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. [..]
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(®->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(®->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(®->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); >
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 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,
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
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,
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,