Message-ID: <13eb4319-b323-463a-99af-db3fa9e5508b@gmx.de>
Date: Sun, 24 Nov 2024 19:15:45 +0100
From: Armin Wolf <W_Armin@....de>
To: Thomas Weißschuh <linux@...ssschuh.net>,
 Sebastian Reichel <sre@...nel.org>, Hans de Goede <hdegoede@...hat.com>,
 Thomas Weißschuh <thomas@...ssschuh.net>,
 Benson Leung <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
 chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v4 7/9] power: supply: core: implement extension API

Am 11.11.24 um 22:40 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.
>
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> ---
>   drivers/power/supply/power_supply.h       |  14 +++
>   drivers/power/supply/power_supply_core.c  | 155 ++++++++++++++++++++++++++++--
>   drivers/power/supply/power_supply_sysfs.c |  22 ++++-
>   include/linux/power_supply.h              |  32 ++++++
>   4 files changed, 213 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 5dabbd895538003096b62d03fdd0201b82b090e6..4c3e602c416cec556173a8eb1a3114c13ded71b7 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,18 @@ 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;
> +};
> +
> +#define power_supply_for_each_extension(pos, psy) \
> +	lockdep_assert_held(&(psy)->extensions_sem); \
> +	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..bf3054ed034e091adefcdbf98873a108b4c90fde 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,37 @@ 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)
> +{
> +	bool found = false;
> +	int i;
> +
> +	for (i = 0; i < psy_ext->num_properties; i++) {
> +		if (psy_ext->properties[i] == psp) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	return found;

Can we just return false here and directly return true when the property is found?

> +}
> +
>   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 +1245,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;
>   	}
>
> +	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);
> +	}

Maybe we can use scoped_guard() here?

> +
>   	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 +1273,23 @@ 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;
> +
> +	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;
> +		}
> +	}

Same as above.

> +
> +	if (!psy->desc->set_property)
>   		return -ENODEV;
>
>   	return psy->desc->set_property(psy, psp, val);
> @@ -1241,7 +1299,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) {

Missing guard here.

> +		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 -ENODEV;
> +		}
> +	}
> +
> +	if (!psy->desc->property_is_writeable)
> +		return -ENODEV;
> +
> +	return psy->desc->property_is_writeable(psy, psp);
>   }
>
>   void power_supply_external_power_changed(struct power_supply *psy)
> @@ -1260,6 +1333,67 @@ 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);

Do we need some locking here or is this ok?

> +}
> +
> +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;
> +
> +	guard(rwsem_write)(&psy->extensions_sem);
> +
> +	power_supply_for_each_extension(reg, psy)
> +		if (reg->ext == ext)
> +			return -EEXIST;
> +
> +	for (i = 0; i < ext->num_properties; i++)
> +		if (power_supply_has_property(psy, ext->properties[i]))
> +			return -EEXIST;
> +
> +	reg = kmalloc(sizeof(*reg), GFP_KERNEL);
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	reg->ext = ext;
> +	reg->data = data;
> +	list_add(&reg->list_head, &psy->extensions);
> +
> +	return power_supply_update_sysfs_and_hwmon(psy);

We need to clean up *reg here should power_supply_update_sysfs_and_hwmon() fail.

> +}
> +EXPORT_SYMBOL_GPL(power_supply_register_extension);
> +
> +void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> +{
> +	struct power_supply_ext_registration *reg;
> +
> +	guard(rwsem_write)(&psy->extensions_sem);
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (reg->ext == ext) {
> +			list_del(&reg->list_head);
> +			kfree(reg);
> +			power_supply_update_sysfs_and_hwmon(psy);
> +			return;
> +		}
> +	}
> +
> +	dev_warn(&psy->dev, "Trying to unregister invalid extension");
> +}
> +EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
> +
>   static void power_supply_dev_release(struct device *dev)
>   {
>   	struct power_supply *psy = to_power_supply(dev);
> @@ -1414,6 +1548,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 +1563,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..2cf25bacd7a1bb66e5a72629bffaa6d16bfbf3be 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -268,6 +268,23 @@ 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;
> +

Missing guard here.

Thanks,
Armin Wolf

> +	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 +324,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 +401,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..016e44cb3eb5eb7ace01a032661f65a5d81a522f 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,12 @@ 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 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);
>

Powered by blists - more mailing lists