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.
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.
For example my upcoming cros_ec charge control driver[0] saves 80 lines
of code with this patchset.
Contents
--------
* Patch 1 and 2 are generic preparation patches, that probably make
sense without this series.
* Patch 3 implements the extension API itself.
* Patch 4 implements a PoC locking scheme for the extension API.
* Patch 5 adds extension support to test_power.c
* Patch 6 converts the in-tree platform/x86/system76 driver to the
extension API.
Open issues
-----------
* Newly registered properties will not show up in hwmon.
To do that properly would require some changes in the hwmon core.
As far as I know, no current driver would extend the hwmon properties anyways.
* As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
it also be gated behind this or another config?
* Only one extension can be used at a time.
So far this should be enough, having more would complicate the
implementation.
* Is an rw_semaphore acceptable?
[0] https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Thomas Weißschuh <[email protected]>
---
Thomas Weißschuh (6):
power: supply: sysfs: use power_supply_property_is_writeable()
power: supply: core: avoid iterating properties directly
power: supply: core: implement extension API
power: supply: core: add locking around extension access
power: supply: test-power: implement a power supply extension
platform/x86: system76: Use power_supply extension API
drivers/platform/x86/system76_acpi.c | 83 +++++++++---------
drivers/power/supply/power_supply.h | 9 ++
drivers/power/supply/power_supply_core.c | 136 ++++++++++++++++++++++++++++--
drivers/power/supply/power_supply_hwmon.c | 48 +++++------
drivers/power/supply/power_supply_sysfs.c | 39 ++++++---
drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++
include/linux/power_supply.h | 25 ++++++
7 files changed, 357 insertions(+), 85 deletions(-)
---
base-commit: 2df0193e62cf887f373995fb8a91068562784adc
change-id: 20240602-power-supply-extensions-07d949f509d9
Best regards,
--
Thomas Weißschuh <[email protected]>
Instead of open-coding the helper use it directly.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply_sysfs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index b86e11bdc07e..3e63d165b2f7 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -379,8 +379,7 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
int property = psy->desc->properties[i];
if (property == attrno) {
- if (psy->desc->property_is_writeable &&
- psy->desc->property_is_writeable(psy, property) > 0)
+ if (power_supply_property_is_writeable(psy, property) > 0)
mode |= S_IWUSR;
return mode;
--
2.45.2
With the introduction of power supply extension, it will not be enough
to iterate the properties on the struct power_supply directly.
Instead introduce a helper power_supply_has_property() which ill handle
properties added by extensions.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply.h | 3 ++
drivers/power/supply/power_supply_core.c | 10 +++----
drivers/power/supply/power_supply_hwmon.c | 48 +++++++++++++++----------------
drivers/power/supply/power_supply_sysfs.c | 14 +++------
4 files changed, 36 insertions(+), 39 deletions(-)
diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 3cbafc58bdad..622be1f0a180 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -13,6 +13,9 @@ struct device;
struct device_type;
struct power_supply;
+extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
+ enum power_supply_property psp);
+
#ifdef CONFIG_SYSFS
extern void power_supply_init_attrs(void);
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index fefe938c9342..d57ecdd966e0 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1183,8 +1183,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
}
EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
-static bool psy_has_property(const struct power_supply_desc *psy_desc,
- enum power_supply_property psp)
+bool power_supply_has_property(const struct power_supply_desc *psy_desc,
+ enum power_supply_property psp)
{
bool found = false;
int i;
@@ -1209,7 +1209,7 @@ int power_supply_get_property(struct power_supply *psy,
return -ENODEV;
}
- if (psy_has_property(psy->desc, psp))
+ if (power_supply_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))
return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
@@ -1308,7 +1308,7 @@ static int psy_register_thermal(struct power_supply *psy)
return 0;
/* Register battery zone device psy reports temperature */
- if (psy_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
+ if (power_supply_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
/* Prefer our hwmon device and avoid duplicates */
struct thermal_zone_params tzp = {
.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
@@ -1361,7 +1361,7 @@ __power_supply_register(struct device *parent,
pr_warn("%s: Expected proper parent device for '%s'\n",
__func__, desc->name);
- if (psy_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
+ if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
(!desc->usb_types || !desc->num_usb_types))
return ERR_PTR(-EINVAL);
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index c97893d4c25e..2ecbe4a74c25 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -8,6 +8,8 @@
#include <linux/power_supply.h>
#include <linux/slab.h>
+#include "power_supply.h"
+
struct power_supply_hwmon {
struct power_supply *psy;
unsigned long *props;
@@ -324,9 +326,26 @@ static const struct hwmon_chip_info power_supply_hwmon_chip_info = {
.info = power_supply_hwmon_info,
};
+static const enum power_supply_property power_supply_hwmon_props[] = {
+ POWER_SUPPLY_PROP_CURRENT_AVG,
+ POWER_SUPPLY_PROP_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_TEMP,
+ POWER_SUPPLY_PROP_TEMP_MAX,
+ POWER_SUPPLY_PROP_TEMP_MIN,
+ POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
+ POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
+ POWER_SUPPLY_PROP_VOLTAGE_AVG,
+ POWER_SUPPLY_PROP_VOLTAGE_MIN,
+ POWER_SUPPLY_PROP_VOLTAGE_MAX,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+};
+
int power_supply_add_hwmon_sysfs(struct power_supply *psy)
{
- const struct power_supply_desc *desc = psy->desc;
struct power_supply_hwmon *psyhw;
struct device *dev = &psy->dev;
struct device *hwmon;
@@ -352,30 +371,11 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
goto error;
}
- for (i = 0; i < desc->num_properties; i++) {
- const enum power_supply_property prop = desc->properties[i];
-
- switch (prop) {
- case POWER_SUPPLY_PROP_CURRENT_AVG:
- case POWER_SUPPLY_PROP_CURRENT_MAX:
- case POWER_SUPPLY_PROP_CURRENT_NOW:
- case POWER_SUPPLY_PROP_TEMP:
- case POWER_SUPPLY_PROP_TEMP_MAX:
- case POWER_SUPPLY_PROP_TEMP_MIN:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
- case POWER_SUPPLY_PROP_TEMP_AMBIENT:
- case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
- case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
- case POWER_SUPPLY_PROP_VOLTAGE_AVG:
- case POWER_SUPPLY_PROP_VOLTAGE_MIN:
- case POWER_SUPPLY_PROP_VOLTAGE_MAX:
- case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ for (i = 0; i < ARRAY_SIZE(power_supply_hwmon_props); i++) {
+ const enum power_supply_property prop = power_supply_hwmon_props[i];
+
+ if (power_supply_has_property(psy->desc, prop))
set_bit(prop, psyhw->props);
- break;
- default:
- break;
- }
}
name = psy->desc->name;
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 3e63d165b2f7..abd44ebfe6fe 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -367,7 +367,6 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
struct device *dev = kobj_to_dev(kobj);
struct power_supply *psy = dev_get_drvdata(dev);
umode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
- int i;
if (!power_supply_attrs[attrno].prop_name)
return 0;
@@ -375,15 +374,10 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
if (attrno == POWER_SUPPLY_PROP_TYPE)
return mode;
- for (i = 0; i < psy->desc->num_properties; i++) {
- int property = psy->desc->properties[i];
-
- if (property == attrno) {
- if (power_supply_property_is_writeable(psy, property) > 0)
- mode |= S_IWUSR;
-
- return mode;
- }
+ if (power_supply_has_property(psy->desc, attrno)) {
+ if (power_supply_property_is_writeable(psy, attrno) > 0)
+ mode |= S_IWUSR;
+ return mode;
}
if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
--
2.45.2
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply.h | 6 +-
drivers/power/supply/power_supply_core.c | 93 ++++++++++++++++++++++++++++---
drivers/power/supply/power_supply_hwmon.c | 2 +-
drivers/power/supply/power_supply_sysfs.c | 22 +++++++-
include/linux/power_supply.h | 23 ++++++++
5 files changed, 132 insertions(+), 14 deletions(-)
diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 622be1f0a180..7f9e139064cc 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -13,8 +13,10 @@ struct device;
struct device_type;
struct power_supply;
-extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
- enum power_supply_property psp);
+bool power_supply_has_property(const struct power_supply *psy,
+ enum power_supply_property psp);
+bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
+ enum power_supply_property psp);
#ifdef CONFIG_SYSFS
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index d57ecdd966e0..b32bbca9a848 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1183,8 +1183,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
}
EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
-bool power_supply_has_property(const struct power_supply_desc *psy_desc,
- enum power_supply_property psp)
+static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
+ enum power_supply_property psp)
{
bool found = false;
int i;
@@ -1199,6 +1199,33 @@ bool power_supply_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)
+{
+ bool found = false;
+ int i;
+
+ if (!psy_ext)
+ return false;
+
+ for (i = 0; i < psy_ext->num_properties; i++) {
+ if (psy_ext->properties[i] == psp) {
+ found = true;
+ break;
+ }
+ }
+
+ return found;
+}
+
+bool power_supply_has_property(const struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ if (power_supply_ext_has_property(psy->ext, psp))
+ return true;
+ return psy_desc_has_property(psy->desc, psp);
+}
+
int power_supply_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
@@ -1209,7 +1236,9 @@ int power_supply_get_property(struct power_supply *psy,
return -ENODEV;
}
- if (power_supply_has_property(psy->desc, psp))
+ if (power_supply_ext_has_property(psy->ext, psp))
+ return psy->ext->get_property(psy, psp, val);
+ else 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))
return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
@@ -1222,7 +1251,17 @@ 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)
+ if (atomic_read(&psy->use_cnt) <= 0)
+ return -ENODEV;
+
+ if (power_supply_ext_has_property(psy->ext, psp)) {
+ if (psy->ext->set_property)
+ return psy->ext->set_property(psy, psp, val);
+ else
+ return -ENODEV;
+ }
+
+ if (!psy->desc->set_property)
return -ENODEV;
return psy->desc->set_property(psy, psp, val);
@@ -1232,8 +1271,17 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
int power_supply_property_is_writeable(struct power_supply *psy,
enum power_supply_property psp)
{
- if (atomic_read(&psy->use_cnt) <= 0 ||
- !psy->desc->property_is_writeable)
+ if (atomic_read(&psy->use_cnt) <= 0)
+ return -ENODEV;
+
+ if (power_supply_ext_has_property(psy->ext, psp)) {
+ if (psy->ext->property_is_writeable)
+ return psy->ext->property_is_writeable(psy, psp);
+ else
+ return -ENODEV;
+ }
+
+ if (!psy->desc->property_is_writeable)
return -ENODEV;
return psy->desc->property_is_writeable(psy, psp);
@@ -1256,6 +1304,35 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
}
EXPORT_SYMBOL_GPL(power_supply_powers);
+static int power_supply_update_groups(struct power_supply *psy)
+{
+ int ret;
+
+ ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
+ power_supply_changed(psy);
+ return ret;
+}
+
+int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+ if (psy->ext)
+ return -EEXIST;
+
+ psy->ext = ext;
+ return power_supply_update_groups(psy);
+}
+EXPORT_SYMBOL_GPL(power_supply_register_extension);
+
+void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+ if (psy->ext != ext)
+ dev_warn(&psy->dev, "Trying to unregister invalid extension");
+
+ psy->ext = NULL;
+ power_supply_update_groups(psy);
+}
+EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
+
static void power_supply_dev_release(struct device *dev)
{
struct power_supply *psy = to_power_supply(dev);
@@ -1308,7 +1385,7 @@ static int psy_register_thermal(struct power_supply *psy)
return 0;
/* Register battery zone device psy reports temperature */
- if (power_supply_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
+ if (power_supply_has_property(psy, POWER_SUPPLY_PROP_TEMP)) {
/* Prefer our hwmon device and avoid duplicates */
struct thermal_zone_params tzp = {
.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
@@ -1361,7 +1438,7 @@ __power_supply_register(struct device *parent,
pr_warn("%s: Expected proper parent device for '%s'\n",
__func__, desc->name);
- if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
+ if (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
(!desc->usb_types || !desc->num_usb_types))
return ERR_PTR(-EINVAL);
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index 2ecbe4a74c25..8cb852a734b1 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -374,7 +374,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
for (i = 0; i < ARRAY_SIZE(power_supply_hwmon_props); i++) {
const enum power_supply_property prop = power_supply_hwmon_props[i];
- if (power_supply_has_property(psy->desc, prop))
+ if (power_supply_has_property(psy, prop))
set_bit(prop, psyhw->props);
}
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index abd44ebfe6fe..3487f161e9bf 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -304,8 +304,12 @@ static ssize_t power_supply_show_property(struct device *dev,
&value, buf);
break;
case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
- ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
- value.intval, buf);
+ if (power_supply_ext_has_property(psy->ext, POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
+ ret = power_supply_charge_behaviour_show(dev, psy->ext->charge_behaviours,
+ value.intval, buf);
+ else
+ ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
+ value.intval, buf);
break;
case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
ret = sysfs_emit(buf, "%s\n", value.strval);
@@ -374,7 +378,7 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
if (attrno == POWER_SUPPLY_PROP_TYPE)
return mode;
- if (power_supply_has_property(psy->desc, attrno)) {
+ if (power_supply_has_property(psy, attrno)) {
if (power_supply_property_is_writeable(psy, attrno) > 0)
mode |= S_IWUSR;
return mode;
@@ -486,7 +490,19 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
if (ret)
goto out;
+ if (psy->ext) {
+ for (j = 0; j < psy->ext->num_properties; j++) {
+ set_bit(psy->ext->properties[j], psy_drv_properties);
+ ret = add_prop_uevent(dev, env, psy->ext->properties[j],
+ prop_buf);
+ if (ret)
+ goto out;
+ }
+ }
+
for (j = 0; j < psy->desc->num_properties; j++) {
+ if (test_bit(psy->desc->properties[j], psy_drv_properties))
+ continue;
set_bit(psy->desc->properties[j], psy_drv_properties);
ret = add_prop_uevent(dev, env, psy->desc->properties[j],
prop_buf);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 8e5705a56b85..0c1e23081d2a 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -280,6 +280,22 @@ 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,
+ enum power_supply_property psp,
+ union power_supply_propval *val);
+ int (*set_property)(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val);
+
+ int (*property_is_writeable)(struct power_supply *psy,
+ enum power_supply_property psp);
+};
+
struct power_supply {
const struct power_supply_desc *desc;
@@ -290,6 +306,8 @@ struct power_supply {
size_t num_supplies;
struct device_node *of_node;
+ const struct power_supply_ext *ext;
+
/* Driver private data */
void *drv_data;
@@ -892,6 +910,11 @@ devm_power_supply_register_no_ws(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 power_supply_register_extension(struct power_supply *psy,
+ const struct power_supply_ext *ext);
+extern void power_supply_unregister_extension(struct power_supply *psy,
+ const struct power_supply_ext *ext);
+
#define to_power_supply(device) container_of(device, struct power_supply, dev)
extern void *power_supply_get_drvdata(struct power_supply *psy);
--
2.45.2
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply.h | 6 ++-
drivers/power/supply/power_supply_core.c | 65 +++++++++++++++++++++++++------
drivers/power/supply/power_supply_sysfs.c | 22 ++++++-----
include/linux/power_supply.h | 2 +
4 files changed, 73 insertions(+), 22 deletions(-)
diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 7f9e139064cc..b469a9719045 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -13,10 +13,14 @@ struct device;
struct device_type;
struct power_supply;
-bool power_supply_has_property(const struct power_supply *psy,
+bool power_supply_has_property(struct power_supply *psy,
enum power_supply_property psp);
+bool power_supply_has_property_nolock(struct power_supply *psy,
+ enum power_supply_property psp);
bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
enum power_supply_property psp);
+int power_supply_property_is_writeable_nolock(struct power_supply *psy,
+ enum power_supply_property psp);
#ifdef CONFIG_SYSFS
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index b32bbca9a848..ded9748be550 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1218,14 +1218,26 @@ bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
return found;
}
-bool power_supply_has_property(const struct power_supply *psy,
- enum power_supply_property psp)
+bool power_supply_has_property_nolock(struct power_supply *psy,
+ enum power_supply_property psp)
{
+ lockdep_assert_held(&psy->ext_lock);
+
if (power_supply_ext_has_property(psy->ext, psp))
return true;
return psy_desc_has_property(psy->desc, psp);
}
+bool power_supply_has_property(struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ scoped_guard(rwsem_read, &psy->ext_lock) {
+ if (power_supply_ext_has_property(psy->ext, psp))
+ return true;
+ }
+ return psy_desc_has_property(psy->desc, psp);
+}
+
int power_supply_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
@@ -1236,9 +1248,12 @@ int power_supply_get_property(struct power_supply *psy,
return -ENODEV;
}
- if (power_supply_ext_has_property(psy->ext, psp))
- return psy->ext->get_property(psy, psp, val);
- else if (psy_desc_has_property(psy->desc, psp))
+ scoped_guard(rwsem_read, &psy->ext_lock) {
+ if (power_supply_ext_has_property(psy->ext, psp))
+ return psy->ext->get_property(psy, 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))
return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
@@ -1254,11 +1269,13 @@ int power_supply_set_property(struct power_supply *psy,
if (atomic_read(&psy->use_cnt) <= 0)
return -ENODEV;
- if (power_supply_ext_has_property(psy->ext, psp)) {
- if (psy->ext->set_property)
- return psy->ext->set_property(psy, psp, val);
- else
- return -ENODEV;
+ scoped_guard(rwsem_read, &psy->ext_lock) {
+ if (power_supply_ext_has_property(psy->ext, psp)) {
+ if (psy->ext->set_property)
+ return psy->ext->set_property(psy, psp, val);
+ else
+ return -ENODEV;
+ }
}
if (!psy->desc->set_property)
@@ -1274,6 +1291,28 @@ int power_supply_property_is_writeable(struct power_supply *psy,
if (atomic_read(&psy->use_cnt) <= 0)
return -ENODEV;
+ scoped_guard(rwsem_read, &psy->ext_lock) {
+ if (power_supply_ext_has_property(psy->ext, psp)) {
+ if (psy->ext->property_is_writeable)
+ return psy->ext->property_is_writeable(psy, psp);
+ else
+ return -ENODEV;
+ }
+ }
+
+ if (!psy->desc->property_is_writeable)
+ return -ENODEV;
+
+ return psy->desc->property_is_writeable(psy, psp);
+}
+EXPORT_SYMBOL_GPL(power_supply_property_is_writeable);
+
+int power_supply_property_is_writeable_nolock(struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ if (atomic_read(&psy->use_cnt) <= 0)
+ return -ENODEV;
+
if (power_supply_ext_has_property(psy->ext, psp)) {
if (psy->ext->property_is_writeable)
return psy->ext->property_is_writeable(psy, psp);
@@ -1286,7 +1325,6 @@ int power_supply_property_is_writeable(struct power_supply *psy,
return psy->desc->property_is_writeable(psy, psp);
}
-EXPORT_SYMBOL_GPL(power_supply_property_is_writeable);
void power_supply_external_power_changed(struct power_supply *psy)
{
@@ -1315,6 +1353,8 @@ static int power_supply_update_groups(struct power_supply *psy)
int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext)
{
+ guard(rwsem_write)(&psy->ext_lock);
+
if (psy->ext)
return -EEXIST;
@@ -1325,6 +1365,8 @@ EXPORT_SYMBOL_GPL(power_supply_register_extension);
void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
{
+ guard(rwsem_write)(&psy->ext_lock);
+
if (psy->ext != ext)
dev_warn(&psy->dev, "Trying to unregister invalid extension");
@@ -1492,6 +1534,7 @@ __power_supply_register(struct device *parent,
}
spin_lock_init(&psy->changed_lock);
+ init_rwsem(&psy->ext_lock);
rc = device_add(dev);
if (rc)
goto device_add_failed;
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 3487f161e9bf..d47804cbb500 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -378,8 +378,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
if (attrno == POWER_SUPPLY_PROP_TYPE)
return mode;
- if (power_supply_has_property(psy, attrno)) {
- if (power_supply_property_is_writeable(psy, attrno) > 0)
+ if (power_supply_has_property_nolock(psy, attrno)) {
+ if (power_supply_property_is_writeable_nolock(psy, attrno) > 0)
mode |= S_IWUSR;
return mode;
}
@@ -458,7 +458,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
{
- const struct power_supply *psy = dev_get_drvdata(dev);
+ struct power_supply *psy = dev_get_drvdata(dev);
const enum power_supply_property *battery_props =
power_supply_battery_info_properties;
unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
@@ -490,13 +490,15 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
if (ret)
goto out;
- if (psy->ext) {
- for (j = 0; j < psy->ext->num_properties; j++) {
- set_bit(psy->ext->properties[j], psy_drv_properties);
- ret = add_prop_uevent(dev, env, psy->ext->properties[j],
- prop_buf);
- if (ret)
- goto out;
+ scoped_guard(rwsem_read, &psy->ext_lock) {
+ if (psy->ext) {
+ for (j = 0; j < psy->ext->num_properties; j++) {
+ set_bit(psy->ext->properties[j], psy_drv_properties);
+ ret = add_prop_uevent(dev, env, psy->ext->properties[j],
+ prop_buf);
+ if (ret)
+ goto out;
+ }
}
}
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 0c1e23081d2a..6bba7e6ab161 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -15,6 +15,7 @@
#include <linux/device.h>
#include <linux/workqueue.h>
#include <linux/leds.h>
+#include <linux/rwsem.h>
#include <linux/spinlock.h>
#include <linux/notifier.h>
@@ -306,6 +307,7 @@ struct power_supply {
size_t num_supplies;
struct device_node *of_node;
+ struct rw_semaphore ext_lock;
const struct power_supply_ext *ext;
/* Driver private data */
--
2.45.2
Allow easy testing of the new power supply extension functionality.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++++++++++++++++++
1 file changed, 102 insertions(+)
diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 442ceb7795e1..5730cbff6159 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -37,6 +37,7 @@ static int battery_charge_counter = -1000;
static int battery_current = -1600;
static enum power_supply_charge_behaviour battery_charge_behaviour =
POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+static bool battery_hook;
static bool module_initialized;
@@ -238,6 +239,80 @@ static const struct power_supply_config test_power_configs[] = {
},
};
+static int power_supply_ext_manufacture_year = 1234;
+static enum power_supply_property power_supply_ext_props[] = {
+ POWER_SUPPLY_PROP_CHARGE_COUNTER, /* overridden from base supply */
+ POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
+};
+
+static int power_supply_ext_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ switch (psp) {
+ case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
+ val->intval = power_supply_ext_manufacture_year;
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_COUNTER:
+ val->intval = 1234;
+ break;
+ default:
+ pr_info("%s: some properties deliberately report errors.\n",
+ __func__);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int power_supply_ext_set_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ switch (psp) {
+ case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
+ power_supply_ext_manufacture_year = val->intval;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int power_supply_ext_property_is_writeable(struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ return psp == POWER_SUPPLY_PROP_MANUFACTURE_YEAR;
+}
+
+static const struct power_supply_ext power_supply_ext = {
+ .properties = power_supply_ext_props,
+ .num_properties = ARRAY_SIZE(power_supply_ext_props),
+ .get_property = power_supply_ext_get_property,
+ .set_property = power_supply_ext_set_property,
+ .property_is_writeable = power_supply_ext_property_is_writeable,
+};
+
+static void test_battery_configure_battery_hook(bool enable)
+{
+ struct power_supply *psy;
+
+ if (battery_hook == enable)
+ return;
+
+ psy = test_power_supplies[TEST_BATTERY];
+
+ if (enable) {
+ if (power_supply_register_extension(psy, &power_supply_ext)) {
+ pr_err("registering battery extension failed\n");
+ return;
+ }
+ } else {
+ power_supply_unregister_extension(psy, &power_supply_ext);
+ }
+
+ battery_hook = enable;
+}
+
static int __init test_power_init(void)
{
int i;
@@ -258,6 +333,8 @@ static int __init test_power_init(void)
}
}
+ test_battery_configure_battery_hook(true);
+
module_initialized = true;
return 0;
failed:
@@ -524,6 +601,22 @@ static int param_set_battery_current(const char *key,
#define param_get_battery_current param_get_int
+static int param_set_battery_hook(const char *key,
+ const struct kernel_param *kp)
+{
+ int tmp;
+
+ if (1 != sscanf(key, "%d", &tmp))
+ return -EINVAL;
+ if (tmp != 1 && tmp != 0)
+ return -EINVAL;
+
+ test_battery_configure_battery_hook(tmp);
+ return 0;
+}
+
+#define param_get_battery_hook param_get_int
+
static const struct kernel_param_ops param_ops_ac_online = {
.set = param_set_ac_online,
.get = param_get_ac_online,
@@ -574,6 +667,11 @@ static const struct kernel_param_ops param_ops_battery_current = {
.get = param_get_battery_current,
};
+static const struct kernel_param_ops param_ops_battery_hook = {
+ .set = param_set_battery_hook,
+ .get = param_get_battery_hook,
+};
+
#define param_check_ac_online(name, p) __param_check(name, p, void);
#define param_check_usb_online(name, p) __param_check(name, p, void);
#define param_check_battery_status(name, p) __param_check(name, p, void);
@@ -584,6 +682,7 @@ static const struct kernel_param_ops param_ops_battery_current = {
#define param_check_battery_voltage(name, p) __param_check(name, p, void);
#define param_check_battery_charge_counter(name, p) __param_check(name, p, void);
#define param_check_battery_current(name, p) __param_check(name, p, void);
+#define param_check_battery_hook(name, p) __param_check(name, p, void);
module_param(ac_online, ac_online, 0644);
@@ -621,6 +720,9 @@ MODULE_PARM_DESC(battery_charge_counter,
module_param(battery_current, battery_current, 0644);
MODULE_PARM_DESC(battery_current, "battery current (milliampere)");
+module_param(battery_hook, battery_hook, 0644);
+MODULE_PARM_DESC(battery_hook, "battery hook");
+
MODULE_DESCRIPTION("Power supply driver for testing");
MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
MODULE_LICENSE("GPL");
--
2.45.2
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/platform/x86/system76_acpi.c | 83 +++++++++++++++++++-----------------
1 file changed, 44 insertions(+), 39 deletions(-)
diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 3da753b3d00d..c26f42c917dd 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -162,7 +162,7 @@ enum {
THRESHOLD_END,
};
-static ssize_t battery_get_threshold(int which, char *buf)
+static int battery_get_threshold(int which, int *val)
{
struct acpi_object_list input;
union acpi_object param;
@@ -186,29 +186,21 @@ static ssize_t battery_get_threshold(int which, char *buf)
if (ret == BATTERY_THRESHOLD_INVALID)
return -EINVAL;
- return sysfs_emit(buf, "%d\n", (int)ret);
+ *val = ret;
+ return 0;
}
-static ssize_t battery_set_threshold(int which, const char *buf, size_t count)
+static int battery_set_threshold(int which, unsigned int value)
{
struct acpi_object_list input;
union acpi_object params[2];
acpi_handle handle;
acpi_status status;
- unsigned int value;
- int ret;
handle = ec_get_handle();
if (!handle)
return -ENODEV;
- ret = kstrtouint(buf, 10, &value);
- if (ret)
- return ret;
-
- if (value > 100)
- return -EINVAL;
-
input.count = 2;
input.pointer = params;
// Start/stop selection
@@ -222,52 +214,65 @@ static ssize_t battery_set_threshold(int which, const char *buf, size_t count)
if (ACPI_FAILURE(status))
return -EIO;
- return count;
+ return 0;
}
-static ssize_t charge_control_start_threshold_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return battery_get_threshold(THRESHOLD_START, buf);
-}
+static const enum power_supply_property system76_battery_properties[] = {
+ POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD,
+ POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
+};
-static ssize_t charge_control_start_threshold_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
+static int system76_property_is_writeable(struct power_supply *psy,
+ enum power_supply_property psp)
{
- return battery_set_threshold(THRESHOLD_START, buf, count);
+ return true;
}
-static DEVICE_ATTR_RW(charge_control_start_threshold);
-
-static ssize_t charge_control_end_threshold_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static int system76_get_property(struct power_supply *psy, enum power_supply_property psp,
+ union power_supply_propval *val)
{
- return battery_get_threshold(THRESHOLD_END, buf);
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+ return battery_get_threshold(THRESHOLD_START, &val->intval);
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+ return battery_get_threshold(THRESHOLD_END, &val->intval);
+ default:
+ return -EINVAL;
+ };
}
-static ssize_t charge_control_end_threshold_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
+static int system76_set_property(struct power_supply *psy, enum power_supply_property psp,
+ const union power_supply_propval *val)
{
- return battery_set_threshold(THRESHOLD_END, buf, count);
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+ if (val->intval < 0 || val->intval > 100)
+ return -EINVAL;
+ return battery_set_threshold(THRESHOLD_START, val->intval);
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+ if (val->intval < 0 || val->intval > 100)
+ return -EINVAL;
+ return battery_set_threshold(THRESHOLD_END, val->intval);
+ default:
+ return -EINVAL;
+ };
}
-static DEVICE_ATTR_RW(charge_control_end_threshold);
-
-static struct attribute *system76_battery_attrs[] = {
- &dev_attr_charge_control_start_threshold.attr,
- &dev_attr_charge_control_end_threshold.attr,
- NULL,
+static const struct power_supply_ext system76_power_supply_ext = {
+ .properties = system76_battery_properties,
+ .num_properties = ARRAY_SIZE(system76_battery_properties),
+ .property_is_writeable = system76_property_is_writeable,
+ .get_property = system76_get_property,
+ .set_property = system76_set_property,
};
-ATTRIBUTE_GROUPS(system76_battery);
-
static int system76_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
{
// System76 EC only supports 1 battery
if (strcmp(battery->desc->name, "BAT0") != 0)
return -ENODEV;
- if (device_add_groups(&battery->dev, system76_battery_groups))
+ if (power_supply_register_extension(battery, &system76_power_supply_ext))
return -ENODEV;
return 0;
@@ -275,7 +280,7 @@ static int system76_battery_add(struct power_supply *battery, struct acpi_batter
static int system76_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
{
- device_remove_groups(&battery->dev, system76_battery_groups);
+ power_supply_unregister_extension(battery, &system76_power_supply_ext);
return 0;
}
--
2.45.2
Am 06.06.24 um 16:50 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.
>
> 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.
>
> For example my upcoming cros_ec charge control driver[0] saves 80 lines
> of code with this patchset.
>
> Contents
> --------
>
> * Patch 1 and 2 are generic preparation patches, that probably make
> sense without this series.
> * Patch 3 implements the extension API itself.
> * Patch 4 implements a PoC locking scheme for the extension API.
> * Patch 5 adds extension support to test_power.c
> * Patch 6 converts the in-tree platform/x86/system76 driver to the
> extension API.
>
> Open issues
> -----------
>
> * Newly registered properties will not show up in hwmon.
> To do that properly would require some changes in the hwmon core.
> As far as I know, no current driver would extend the hwmon properties anyways.
> * As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
> it also be gated behind this or another config?
> * Only one extension can be used at a time.
> So far this should be enough, having more would complicate the
> implementation.
> * Is an rw_semaphore acceptable?
>
> [0] https://lore.kernel.org/lkml/[email protected]/
Nice, i love this proposal!
I agree that the hwmon update functionality will need some changes in the hwmon core to work,
but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
support for this at a later point in time.
The possibility of registering multiple power supply extensions on a single power supply will
be necessary to support battery charge control on Dell notebooks in the future. This is because
there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
dell-laptop (when support for battery charge control is supported someday).
How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
this later when the need arises.
Thanks,
Armin Wolf
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> Thomas Weißschuh (6):
> power: supply: sysfs: use power_supply_property_is_writeable()
> power: supply: core: avoid iterating properties directly
> power: supply: core: implement extension API
> power: supply: core: add locking around extension access
> power: supply: test-power: implement a power supply extension
> platform/x86: system76: Use power_supply extension API
>
> drivers/platform/x86/system76_acpi.c | 83 +++++++++---------
> drivers/power/supply/power_supply.h | 9 ++
> drivers/power/supply/power_supply_core.c | 136 ++++++++++++++++++++++++++++--
> drivers/power/supply/power_supply_hwmon.c | 48 +++++------
> drivers/power/supply/power_supply_sysfs.c | 39 ++++++---
> drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++
> include/linux/power_supply.h | 25 ++++++
> 7 files changed, 357 insertions(+), 85 deletions(-)
> ---
> base-commit: 2df0193e62cf887f373995fb8a91068562784adc
> change-id: 20240602-power-supply-extensions-07d949f509d9
>
> Best regards,
On 2024-06-07 01:10:02+0000, Armin Wolf wrote:
> Am 06.06.24 um 16:50 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.
> >
> > 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.
> >
> > For example my upcoming cros_ec charge control driver[0] saves 80 lines
> > of code with this patchset.
> >
> > Contents
> > --------
> >
> > * Patch 1 and 2 are generic preparation patches, that probably make
> > sense without this series.
> > * Patch 3 implements the extension API itself.
> > * Patch 4 implements a PoC locking scheme for the extension API.
> > * Patch 5 adds extension support to test_power.c
> > * Patch 6 converts the in-tree platform/x86/system76 driver to the
> > extension API.
> >
> > Open issues
> > -----------
> >
> > * Newly registered properties will not show up in hwmon.
> > To do that properly would require some changes in the hwmon core.
> > As far as I know, no current driver would extend the hwmon properties anyways.
> > * As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
> > it also be gated behind this or another config?
> > * Only one extension can be used at a time.
> > So far this should be enough, having more would complicate the
> > implementation.
> > * Is an rw_semaphore acceptable?
> >
> > [0] https://lore.kernel.org/lkml/[email protected]/
>
> Nice, i love this proposal!
Good to hear!
> I agree that the hwmon update functionality will need some changes in the hwmon core to work,
> but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
> support for this at a later point in time.
Surely. Alternatively we could re-register the hwmon device after an
extension was added.
> The possibility of registering multiple power supply extensions on a single power supply will
> be necessary to support battery charge control on Dell notebooks in the future. This is because
> there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
> dell-laptop (when support for battery charge control is supported someday).
>
> How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
> this later when the need arises.
It's not really difficult. The problem is in the callback functions
going from a 'struct power_supply' back to the correct extension struct
for use with container_of() to access the drivers private data.
But we can add a marker member to 'struct power_supply_ext' with which
the callback can figure out which of the registered extensions is its
own. Something like "led_hw_trigger_type" in the LED subsystem.
And some documentation about how conflicts are to be resolved.
Thomas
> > Signed-off-by: Thomas Weißschuh <[email protected]>
> > ---
> > Thomas Weißschuh (6):
> > power: supply: sysfs: use power_supply_property_is_writeable()
> > power: supply: core: avoid iterating properties directly
> > power: supply: core: implement extension API
> > power: supply: core: add locking around extension access
> > power: supply: test-power: implement a power supply extension
> > platform/x86: system76: Use power_supply extension API
> >
> > drivers/platform/x86/system76_acpi.c | 83 +++++++++---------
> > drivers/power/supply/power_supply.h | 9 ++
> > drivers/power/supply/power_supply_core.c | 136 ++++++++++++++++++++++++++++--
> > drivers/power/supply/power_supply_hwmon.c | 48 +++++------
> > drivers/power/supply/power_supply_sysfs.c | 39 ++++++---
> > drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++
> > include/linux/power_supply.h | 25 ++++++
> > 7 files changed, 357 insertions(+), 85 deletions(-)
> > ---
> > base-commit: 2df0193e62cf887f373995fb8a91068562784adc
> > change-id: 20240602-power-supply-extensions-07d949f509d9
Am 07.06.24 um 12:26 schrieb Thomas Weißschuh:
> On 2024-06-07 01:10:02+0000, Armin Wolf wrote:
>> Am 06.06.24 um 16:50 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.
>>>
>>> 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.
>>>
>>> For example my upcoming cros_ec charge control driver[0] saves 80 lines
>>> of code with this patchset.
>>>
>>> Contents
>>> --------
>>>
>>> * Patch 1 and 2 are generic preparation patches, that probably make
>>> sense without this series.
>>> * Patch 3 implements the extension API itself.
>>> * Patch 4 implements a PoC locking scheme for the extension API.
>>> * Patch 5 adds extension support to test_power.c
>>> * Patch 6 converts the in-tree platform/x86/system76 driver to the
>>> extension API.
>>>
>>> Open issues
>>> -----------
>>>
>>> * Newly registered properties will not show up in hwmon.
>>> To do that properly would require some changes in the hwmon core.
>>> As far as I know, no current driver would extend the hwmon properties anyways.
>>> * As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
>>> it also be gated behind this or another config?
>>> * Only one extension can be used at a time.
>>> So far this should be enough, having more would complicate the
>>> implementation.
>>> * Is an rw_semaphore acceptable?
>>>
>>> [0] https://lore.kernel.org/lkml/[email protected]/
>> Nice, i love this proposal!
> Good to hear!
>
>> I agree that the hwmon update functionality will need some changes in the hwmon core to work,
>> but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
>> support for this at a later point in time.
> Surely. Alternatively we could re-register the hwmon device after an
> extension was added.
>
>> The possibility of registering multiple power supply extensions on a single power supply will
>> be necessary to support battery charge control on Dell notebooks in the future. This is because
>> there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
>> dell-laptop (when support for battery charge control is supported someday).
>>
>> How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
>> this later when the need arises.
> It's not really difficult. The problem is in the callback functions
> going from a 'struct power_supply' back to the correct extension struct
> for use with container_of() to access the drivers private data.
>
> But we can add a marker member to 'struct power_supply_ext' with which
> the callback can figure out which of the registered extensions is its
> own. Something like "led_hw_trigger_type" in the LED subsystem.
Maybe we can do the same thing as the battery hook API and just pass a pointer to
the power_supply_ext instance to the callbacks. They then can use container_of()
to access the drivers private data if the struct power_supply_ext is embedded
inside the private data struct.
>
> And some documentation about how conflicts are to be resolved.
>
> Thomas
Sound like a plan, i suggest that extensions be prevented from registering with
a power supply containing conflicting properties or containing extensions with
conflicting properties.
As a side note, maybe there is a way to make power_supply_update_groups() available
for other power supply drivers? Afaik the ACPI battery driver would benefit from this too.
Thanks,
Armin Wolf
>>> Signed-off-by: Thomas Weißschuh <[email protected]>
>>> ---
>>> Thomas Weißschuh (6):
>>> power: supply: sysfs: use power_supply_property_is_writeable()
>>> power: supply: core: avoid iterating properties directly
>>> power: supply: core: implement extension API
>>> power: supply: core: add locking around extension access
>>> power: supply: test-power: implement a power supply extension
>>> platform/x86: system76: Use power_supply extension API
>>>
>>> drivers/platform/x86/system76_acpi.c | 83 +++++++++---------
>>> drivers/power/supply/power_supply.h | 9 ++
>>> drivers/power/supply/power_supply_core.c | 136 ++++++++++++++++++++++++++++--
>>> drivers/power/supply/power_supply_hwmon.c | 48 +++++------
>>> drivers/power/supply/power_supply_sysfs.c | 39 ++++++---
>>> drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++
>>> include/linux/power_supply.h | 25 ++++++
>>> 7 files changed, 357 insertions(+), 85 deletions(-)
>>> ---
>>> base-commit: 2df0193e62cf887f373995fb8a91068562784adc
>>> change-id: 20240602-power-supply-extensions-07d949f509d9
On 2024-06-08 18:27:07+0000, Armin Wolf wrote:
> Am 07.06.24 um 12:26 schrieb Thomas Weißschuh:
>
> > On 2024-06-07 01:10:02+0000, Armin Wolf wrote:
> > > Am 06.06.24 um 16:50 schrieb Thomas Weißschuh:
> > >
> > > > Introduce a mechanism for drivers to extend the properties implemented
> > > > by a power supply.
<snip>
> > > >
> > > > [0] https://lore.kernel.org/lkml/[email protected]/
> > > Nice, i love this proposal!
> > Good to hear!
> >
> > > I agree that the hwmon update functionality will need some changes in the hwmon core to work,
> > > but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
> > > support for this at a later point in time.
> > Surely. Alternatively we could re-register the hwmon device after an
> > extension was added.
> >
> > > The possibility of registering multiple power supply extensions on a single power supply will
> > > be necessary to support battery charge control on Dell notebooks in the future. This is because
> > > there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
> > > dell-laptop (when support for battery charge control is supported someday).
> > >
> > > How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
> > > this later when the need arises.
> > It's not really difficult. The problem is in the callback functions
> > going from a 'struct power_supply' back to the correct extension struct
> > for use with container_of() to access the drivers private data.
> >
> > But we can add a marker member to 'struct power_supply_ext' with which
> > the callback can figure out which of the registered extensions is its
> > own. Something like "led_hw_trigger_type" in the LED subsystem.
>
> Maybe we can do the same thing as the battery hook API and just pass a pointer to
> the power_supply_ext instance to the callbacks. They then can use container_of()
> to access the drivers private data if the struct power_supply_ext is embedded
> inside the private data struct.
That indeed sounds like the obvious thing to do.
I tried very hard to keep the callback signatures exactly the same as in
power_supply_desc and didn't even see this possibility.
>
> >
> > And some documentation about how conflicts are to be resolved.
> >
> > Thomas
>
> Sound like a plan, i suggest that extensions be prevented from registering with
> a power supply containing conflicting properties or containing extensions with
> conflicting properties.
Ack.
> As a side note, maybe there is a way to make power_supply_update_groups() available
> for other power supply drivers? Afaik the ACPI battery driver would benefit from this too.
I'll take a look and spin that into its own series.
Or as you seem to know that driver better, I'd be happy if you did.
> Thanks,
> Armin Wolf
>
> > > > Signed-off-by: Thomas Weißschuh <[email protected]>
> > > > ---
> > > > Thomas Weißschuh (6):
> > > > power: supply: sysfs: use power_supply_property_is_writeable()
> > > > power: supply: core: avoid iterating properties directly
> > > > power: supply: core: implement extension API
> > > > power: supply: core: add locking around extension access
> > > > power: supply: test-power: implement a power supply extension
> > > > platform/x86: system76: Use power_supply extension API
> > > >
> > > > drivers/platform/x86/system76_acpi.c | 83 +++++++++---------
> > > > drivers/power/supply/power_supply.h | 9 ++
> > > > drivers/power/supply/power_supply_core.c | 136 ++++++++++++++++++++++++++++--
> > > > drivers/power/supply/power_supply_hwmon.c | 48 +++++------
> > > > drivers/power/supply/power_supply_sysfs.c | 39 ++++++---
> > > > drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++
> > > > include/linux/power_supply.h | 25 ++++++
> > > > 7 files changed, 357 insertions(+), 85 deletions(-)
> > > > ---
> > > > base-commit: 2df0193e62cf887f373995fb8a91068562784adc
> > > > change-id: 20240602-power-supply-extensions-07d949f509d9