[PATCH RFC v3 0/9] power: supply: extension API

[PATCH RFC v3 0/9] power: supply: extension API

[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.

When testing, please enable lockdep to make sure the locking is correct.

Contents
--------

* Patch 1 to 5 are preparation patches.
* Patch 6 implements the extension API itself.
* Patch 7 implements a locking scheme for the extension API.
* Patch 8 adds extension support to test_power.c
* Patch 9 converts the in-tree cros_charge-control 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?
* Is an rw_semaphore acceptable?

[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 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 (9):
      power: supply: core: rename psy_has_property() to psy_desc_has_property()
      power: supply: core: register thermal zone for battery
      power: supply: hwmon: register battery properties
      power: supply: sysfs: register battery properties
      power: supply: sysfs: rework uevent property loop
      power: supply: core: implement extension API
      power: supply: core: add locking around extension access
      power: supply: test-power: implement a power supply extension
      power: supply: cros_charge-control: use power_supply extensions

 drivers/power/supply/cros_charge-control.c | 207 +++++++++++------------------
 drivers/power/supply/power_supply.h        |  18 +++
 drivers/power/supply/power_supply_core.c   | 169 +++++++++++++++++++++--
 drivers/power/supply/power_supply_hwmon.c  |  52 ++++----
 drivers/power/supply/power_supply_leds.c   |   2 +
 drivers/power/supply/power_supply_sysfs.c  |  65 +++++----
 drivers/power/supply/test_power.c          | 104 +++++++++++++++
 include/linux/power_supply.h               |  33 +++++
 8 files changed, 453 insertions(+), 197 deletions(-)
---
base-commit: d8abb73f584772eaafa95a447c90f1c02dba0dec
change-id: 20240602-power-supply-extensions-07d949f509d9

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH RFC v3 1/9] power: supply: core: rename psy_has_property() to psy_desc_has_property()

[Thomas Weißschuh]

The function only takes a desc as parameter, align the naming.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 8f6025acd10a..cff68c4fd63c 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)
+static bool psy_desc_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 (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);
@@ -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 (psy_desc_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 (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
 	    (!desc->usb_types || !desc->num_usb_types))
 		return ERR_PTR(-EINVAL);
 

-- 
2.46.0

Re: [PATCH RFC v3 1/9] power: supply: core: rename psy_has_property() to psy_desc_has_property()

[Hans de Goede]

Hi,

On 9/4/24 9:25 PM, Thomas Weißschuh wrote:
> The function only takes a desc as parameter, align the naming.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/power/supply/power_supply_core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 8f6025acd10a..cff68c4fd63c 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)
> +static bool psy_desc_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 (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);
> @@ -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 (psy_desc_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 (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
>  	    (!desc->usb_types || !desc->num_usb_types))
>  		return ERR_PTR(-EINVAL);
>  
> 

Re: [PATCH RFC v3 1/9] power: supply: core: rename psy_has_property() to psy_desc_has_property()

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]

Hi,

On Wed, Sep 04, 2024 at 09:25:34PM GMT, Thomas Weißschuh wrote:
> The function only takes a desc as parameter, align the naming.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

too late for 6.12, so

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/power/supply/power_supply_core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 8f6025acd10a..cff68c4fd63c 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)
> +static bool psy_desc_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 (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);
> @@ -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 (psy_desc_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 (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
>  	    (!desc->usb_types || !desc->num_usb_types))
>  		return ERR_PTR(-EINVAL);
>  
> 
> -- 
> 2.46.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RFC v3 2/9] power: supply: core: register thermal zone for battery

[Thomas Weißschuh]

power_supply_read_team() can also read the temperature from the battery.
But currently when registering the thermal zone, the battery is not
checked for POWER_SUPPLY_PROP_TEMP.
Introduce a helper which can check both the desc and the battery info
for property existence and use that.
Export the helper to the rest of the psy core because it will also be
used by different subcomponents.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply.h      |  3 +++
 drivers/power/supply/power_supply_core.c | 14 +++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 3cbafc58bdad..b01faeaf7827 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(struct power_supply *psy,
+				      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 cff68c4fd63c..dcb7e4853030 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1199,6 +1199,18 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
 	return found;
 }
 
+bool power_supply_has_property(struct power_supply *psy,
+			       enum power_supply_property psp)
+{
+	if (psy_desc_has_property(psy->desc, psp))
+		return true;
+
+	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
+		return true;
+
+	return false;
+}
+
 int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
@@ -1308,7 +1320,7 @@ static int psy_register_thermal(struct power_supply *psy)
 		return 0;
 
 	/* Register battery zone device psy reports temperature */
-	if (psy_desc_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)

-- 
2.46.0

Re: [PATCH RFC v3 2/9] power: supply: core: register thermal zone for battery

[Hans de Goede]

Hi,

On 9/4/24 9:25 PM, Thomas Weißschuh wrote:
> power_supply_read_team() can also read the temperature from the battery.
> But currently when registering the thermal zone, the battery is not
> checked for POWER_SUPPLY_PROP_TEMP.
> Introduce a helper which can check both the desc and the battery info
> for property existence and use that.
> Export the helper to the rest of the psy core because it will also be
> used by different subcomponents.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  drivers/power/supply/power_supply.h      |  3 +++
>  drivers/power/supply/power_supply_core.c | 14 +++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 3cbafc58bdad..b01faeaf7827 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(struct power_supply *psy,
> +				      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 cff68c4fd63c..dcb7e4853030 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1199,6 +1199,18 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
>  	return found;
>  }
>  
> +bool power_supply_has_property(struct power_supply *psy,
> +			       enum power_supply_property psp)
> +{
> +	if (psy_desc_has_property(psy->desc, psp))
> +		return true;
> +
> +	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> +		return true;
> +
> +	return false;
> +}
> +
>  int power_supply_get_property(struct power_supply *psy,
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val)
> @@ -1308,7 +1320,7 @@ static int psy_register_thermal(struct power_supply *psy)
>  		return 0;
>  
>  	/* Register battery zone device psy reports temperature */
> -	if (psy_desc_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)
> 

Re: [PATCH RFC v3 2/9] power: supply: core: register thermal zone for battery

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

Hi,

On Wed, Sep 04, 2024 at 09:25:35PM GMT, Thomas Weißschuh wrote:
> power_supply_read_team() can also read the temperature from the battery.
power_supply_read_temp()

> But currently when registering the thermal zone, the battery is
> not checked for POWER_SUPPLY_PROP_TEMP. Introduce a helper which
> can check both the desc and the battery info for property
> existence and use that. Export the helper to the rest of the psy
> core because it will also be used by different subcomponents.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

power_supply_battery_info contains constant battery information like
design capacity or maximum voltage, which is e.g. supplied by the
firmware and needed by fuel-gauges or chargers. The temperature is
not constant and cannot be part of power_supply_battery_info, so
this does not really make sense.

Greetings,

-- Sebastian

>  drivers/power/supply/power_supply.h      |  3 +++
>  drivers/power/supply/power_supply_core.c | 14 +++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 3cbafc58bdad..b01faeaf7827 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(struct power_supply *psy,
> +				      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 cff68c4fd63c..dcb7e4853030 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1199,6 +1199,18 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
>  	return found;
>  }
>  
> +bool power_supply_has_property(struct power_supply *psy,
> +			       enum power_supply_property psp)
> +{
> +	if (psy_desc_has_property(psy->desc, psp))
> +		return true;
> +
> +	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> +		return true;
> +
> +	return false;
> +}
> +
>  int power_supply_get_property(struct power_supply *psy,
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val)
> @@ -1308,7 +1320,7 @@ static int psy_register_thermal(struct power_supply *psy)
>  		return 0;
>  
>  	/* Register battery zone device psy reports temperature */
> -	if (psy_desc_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)
> 
> -- 
> 2.46.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC v3 2/9] power: supply: core: register thermal zone for battery

[Thomas Weißschuh]

Hi,

On 2024-09-14 11:29:04+0000, Sebastian Reichel wrote:
> On Wed, Sep 04, 2024 at 09:25:35PM GMT, Thomas Weißschuh wrote:
> > power_supply_read_team() can also read the temperature from the battery.
> power_supply_read_temp()
> 
> > But currently when registering the thermal zone, the battery is
> > not checked for POWER_SUPPLY_PROP_TEMP. Introduce a helper which
> > can check both the desc and the battery info for property
> > existence and use that. Export the helper to the rest of the psy
> > core because it will also be used by different subcomponents.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> 
> power_supply_battery_info contains constant battery information like
> design capacity or maximum voltage, which is e.g. supplied by the
> firmware and needed by fuel-gauges or chargers. The temperature is
> not constant and cannot be part of power_supply_battery_info, so
> this does not really make sense.

Good point. Also this code will run before the extensions will be
registered, so it doesn't make sense for that either.
I'll change the patch to only introduce power_supply_has_property().

> 
> Greetings,
> 
> -- Sebastian
> 
> >  drivers/power/supply/power_supply.h      |  3 +++
> >  drivers/power/supply/power_supply_core.c | 14 +++++++++++++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index 3cbafc58bdad..b01faeaf7827 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(struct power_supply *psy,
> > +				      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 cff68c4fd63c..dcb7e4853030 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -1199,6 +1199,18 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
> >  	return found;
> >  }
> >  
> > +bool power_supply_has_property(struct power_supply *psy,
> > +			       enum power_supply_property psp)
> > +{
> > +	if (psy_desc_has_property(psy->desc, psp))
> > +		return true;
> > +
> > +	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  int power_supply_get_property(struct power_supply *psy,
> >  			    enum power_supply_property psp,
> >  			    union power_supply_propval *val)
> > @@ -1308,7 +1320,7 @@ static int psy_register_thermal(struct power_supply *psy)
> >  		return 0;
> >  
> >  	/* Register battery zone device psy reports temperature */
> > -	if (psy_desc_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)
> > 
> > -- 
> > 2.46.0

[PATCH RFC v3 3/9] power: supply: hwmon: register battery properties

[Thomas Weißschuh]

Instead of only registering properties from the psy_desc itself,
also register the ones from the battery.
Use power_supply_has_property() for this test which makes the logic also
easier to read.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_hwmon.c | 52 +++++++++++++++----------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index baacefbdf768..1c1ad3e1d81f 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;
@@ -347,9 +349,28 @@ 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_POWER_AVG,
+	POWER_SUPPLY_PROP_POWER_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;
@@ -375,32 +396,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_POWER_AVG:
-		case POWER_SUPPLY_PROP_POWER_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, prop))
 			set_bit(prop, psyhw->props);
-			break;
-		default:
-			break;
-		}
 	}
 
 	name = psy->desc->name;

-- 
2.46.0

Re: [PATCH RFC v3 3/9] power: supply: hwmon: register battery properties

[Hans de Goede]

Hi,

On 9/4/24 9:25 PM, Thomas Weißschuh wrote:
> Instead of only registering properties from the psy_desc itself,
> also register the ones from the battery.
> Use power_supply_has_property() for this test which makes the logic also
> easier to read.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/power/supply/power_supply_hwmon.c | 52 +++++++++++++++----------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index baacefbdf768..1c1ad3e1d81f 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;
> @@ -347,9 +349,28 @@ 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_POWER_AVG,
> +	POWER_SUPPLY_PROP_POWER_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;
> @@ -375,32 +396,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_POWER_AVG:
> -		case POWER_SUPPLY_PROP_POWER_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, prop))
>  			set_bit(prop, psyhw->props);
> -			break;
> -		default:
> -			break;
> -		}
>  	}
>  
>  	name = psy->desc->name;
> 

Re: [PATCH RFC v3 3/9] power: supply: hwmon: register battery properties

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

Hi,

On Wed, Sep 04, 2024 at 09:25:36PM GMT, Thomas Weißschuh wrote:
> Instead of only registering properties from the psy_desc itself,
> also register the ones from the battery.
> Use power_supply_has_property() for this test which makes the logic also
> easier to read.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Greetings,

-- Sebastian

>  drivers/power/supply/power_supply_hwmon.c | 52 +++++++++++++++----------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index baacefbdf768..1c1ad3e1d81f 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;
> @@ -347,9 +349,28 @@ 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_POWER_AVG,
> +	POWER_SUPPLY_PROP_POWER_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;
> @@ -375,32 +396,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_POWER_AVG:
> -		case POWER_SUPPLY_PROP_POWER_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, prop))
>  			set_bit(prop, psyhw->props);
> -			break;
> -		default:
> -			break;
> -		}
>  	}
>  
>  	name = psy->desc->name;
> 
> -- 
> 2.46.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RFC v3 4/9] power: supply: sysfs: register battery properties

[Thomas Weißschuh]

Instead of only registering properties from the psy_desc itself,
also register the ones from the battery.
Use power_supply_has_property() for this test which makes the logic also
easier to read.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 3e63d165b2f7..4ab08386bcb7 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,19 +374,11 @@ 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_battery_info_has_prop(psy->battery_info, attrno))
+	if (power_supply_has_property(psy, attrno)) {
+		if (power_supply_property_is_writeable(psy, attrno) > 0)
+			mode |= S_IWUSR;
 		return mode;
+	}
 
 	return 0;
 }

-- 
2.46.0

Re: [PATCH RFC v3 4/9] power: supply: sysfs: register battery properties

[Hans de Goede]

Hi,

On 9/4/24 9:25 PM, Thomas Weißschuh wrote:
> Instead of only registering properties from the psy_desc itself,
> also register the ones from the battery.
> Use power_supply_has_property() for this test which makes the logic also
> easier to read.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/power/supply/power_supply_sysfs.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 3e63d165b2f7..4ab08386bcb7 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,19 +374,11 @@ 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_battery_info_has_prop(psy->battery_info, attrno))
> +	if (power_supply_has_property(psy, attrno)) {
> +		if (power_supply_property_is_writeable(psy, attrno) > 0)
> +			mode |= S_IWUSR;
>  		return mode;
> +	}
>  
>  	return 0;
>  }
> 

Re: [PATCH RFC v3 4/9] power: supply: sysfs: register battery properties

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

Hi,

On Wed, Sep 04, 2024 at 09:25:37PM GMT, Thomas Weißschuh wrote:
> Instead of only registering properties from the psy_desc itself,
> also register the ones from the battery.
> Use power_supply_has_property() for this test which makes the logic also
> easier to read.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

The properties from the battery_info are already registered, so this
"just" simplifies the logic. Otherwise LGTM.

Greetings,

-- Sebastian

>  drivers/power/supply/power_supply_sysfs.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 3e63d165b2f7..4ab08386bcb7 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,19 +374,11 @@ 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_battery_info_has_prop(psy->battery_info, attrno))
> +	if (power_supply_has_property(psy, attrno)) {
> +		if (power_supply_property_is_writeable(psy, attrno) > 0)
> +			mode |= S_IWUSR;
>  		return mode;
> +	}
>  
>  	return 0;
>  }
> 
> -- 
> 2.46.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RFC v3 5/9] power: supply: sysfs: rework uevent property loop

[Thomas Weißschuh]

Instead of looping through all properties known to be supported by the
psy, loop over all known properties and decide based on the return value
of power_supply_get_property() whether the property existed.

This makes the code shorter now and even more so when power supply
extensions are added.
It also simplifies the locking, as it can all happen inside
power_supply_get_property().

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 4ab08386bcb7..915a4ba62258 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -290,6 +290,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 				dev_dbg_ratelimited(dev,
 					"driver has no data for `%s' property\n",
 					attr->attr.name);
+			else if (ret == -EINVAL) /* property is not supported */
+				return -ENODATA;
 			else if (ret != -ENODEV && ret != -EAGAIN)
 				dev_err_ratelimited(dev,
 					"driver failed to report `%s' property: %zd\n",
@@ -451,11 +453,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);
-	const enum power_supply_property *battery_props =
-		power_supply_battery_info_properties;
-	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
-					 sizeof(unsigned long) + 1] = {0};
+	struct power_supply *psy = dev_get_drvdata(dev);
 	int ret = 0, j;
 	char *prop_buf;
 
@@ -483,22 +481,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	if (ret)
 		goto out;
 
-	for (j = 0; j < psy->desc->num_properties; j++) {
-		set_bit(psy->desc->properties[j], psy_drv_properties);
-		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
-				      prop_buf);
-		if (ret)
-			goto out;
-	}
-
-	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
-		if (test_bit(battery_props[j], psy_drv_properties))
-			continue;
-		if (!power_supply_battery_info_has_prop(psy->battery_info,
-				battery_props[j]))
-			continue;
-		ret = add_prop_uevent(dev, env, battery_props[j],
-			      prop_buf);
+	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
+		ret = add_prop_uevent(dev, env, j, prop_buf);
 		if (ret)
 			goto out;
 	}

-- 
2.46.0

Re: [PATCH RFC v3 5/9] power: supply: sysfs: rework uevent property loop

[Hans de Goede]

Hi,

On 9/4/24 9:25 PM, Thomas Weißschuh wrote:
> Instead of looping through all properties known to be supported by the
> psy, loop over all known properties and decide based on the return value
> of power_supply_get_property() whether the property existed.
> 
> This makes the code shorter now and even more so when power supply
> extensions are added.
> It also simplifies the locking, as it can all happen inside
> power_supply_get_property().
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


p.s.

Last review for me in this set. I'm afraid I don't have the bandwidth
atm to also review the actual extension API.





> ---
>  drivers/power/supply/power_supply_sysfs.c | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 4ab08386bcb7..915a4ba62258 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -290,6 +290,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  				dev_dbg_ratelimited(dev,
>  					"driver has no data for `%s' property\n",
>  					attr->attr.name);
> +			else if (ret == -EINVAL) /* property is not supported */
> +				return -ENODATA;
>  			else if (ret != -ENODEV && ret != -EAGAIN)
>  				dev_err_ratelimited(dev,
>  					"driver failed to report `%s' property: %zd\n",
> @@ -451,11 +453,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);
> -	const enum power_supply_property *battery_props =
> -		power_supply_battery_info_properties;
> -	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
> -					 sizeof(unsigned long) + 1] = {0};
> +	struct power_supply *psy = dev_get_drvdata(dev);
>  	int ret = 0, j;
>  	char *prop_buf;
>  
> @@ -483,22 +481,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>  	if (ret)
>  		goto out;
>  
> -	for (j = 0; j < psy->desc->num_properties; j++) {
> -		set_bit(psy->desc->properties[j], psy_drv_properties);
> -		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
> -				      prop_buf);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
> -		if (test_bit(battery_props[j], psy_drv_properties))
> -			continue;
> -		if (!power_supply_battery_info_has_prop(psy->battery_info,
> -				battery_props[j]))
> -			continue;
> -		ret = add_prop_uevent(dev, env, battery_props[j],
> -			      prop_buf);
> +	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
> +		ret = add_prop_uevent(dev, env, j, prop_buf);
>  		if (ret)
>  			goto out;
>  	}
> 

Re: [PATCH RFC v3 5/9] power: supply: sysfs: rework uevent property loop

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3150 bytes --]

Hi,

On Wed, Sep 04, 2024 at 09:25:38PM GMT, Thomas Weißschuh wrote:
> Instead of looping through all properties known to be supported by the
> psy, loop over all known properties and decide based on the return value
> of power_supply_get_property() whether the property existed.
> 
> This makes the code shorter now and even more so when power supply
> extensions are added.
> It also simplifies the locking, as it can all happen inside
> power_supply_get_property().
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/power/supply/power_supply_sysfs.c | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 4ab08386bcb7..915a4ba62258 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -290,6 +290,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  				dev_dbg_ratelimited(dev,
>  					"driver has no data for `%s' property\n",
>  					attr->attr.name);
> +			else if (ret == -EINVAL) /* property is not supported */
> +				return -ENODATA;

I think it's better to update the check in add_prop_uevent, so that
it also skips -EINVAL. That way sysfs still exposes the correct
error code.

Otherwise LGTM, even though I wonder about the performance impact of
this change. I suppose this is not called often enough to really
matter, though.

Greetings,

-- Sebastian

>  			else if (ret != -ENODEV && ret != -EAGAIN)
>  				dev_err_ratelimited(dev,
>  					"driver failed to report `%s' property: %zd\n",
> @@ -451,11 +453,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);
> -	const enum power_supply_property *battery_props =
> -		power_supply_battery_info_properties;
> -	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
> -					 sizeof(unsigned long) + 1] = {0};
> +	struct power_supply *psy = dev_get_drvdata(dev);
>  	int ret = 0, j;
>  	char *prop_buf;
>  
> @@ -483,22 +481,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>  	if (ret)
>  		goto out;
>  
> -	for (j = 0; j < psy->desc->num_properties; j++) {
> -		set_bit(psy->desc->properties[j], psy_drv_properties);
> -		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
> -				      prop_buf);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
> -		if (test_bit(battery_props[j], psy_drv_properties))
> -			continue;
> -		if (!power_supply_battery_info_has_prop(psy->battery_info,
> -				battery_props[j]))
> -			continue;
> -		ret = add_prop_uevent(dev, env, battery_props[j],
> -			      prop_buf);
> +	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
> +		ret = add_prop_uevent(dev, env, j, prop_buf);
>  		if (ret)
>  			goto out;
>  	}
> 
> -- 
> 2.46.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RFC v3 6/9] power: supply: core: implement extension API

[Thomas Weißschuh]

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply.h       |  12 ++++
 drivers/power/supply/power_supply_core.c  | 114 +++++++++++++++++++++++++++++-
 drivers/power/supply/power_supply_sysfs.c |  20 +++++-
 include/linux/power_supply.h              |  30 ++++++++
 4 files changed, 171 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index b01faeaf7827..fb45f0717bd1 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -15,6 +15,18 @@ struct power_supply;
 
 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) \
+	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 dcb7e4853030..db5e7af57e67 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1199,15 +1199,40 @@ 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;
+
+	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(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;
 }
 
@@ -1215,12 +1240,19 @@ 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;
 	}
 
+	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))
@@ -1234,7 +1266,21 @@ 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;
+
+	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);
@@ -1244,8 +1290,21 @@ 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)
+	struct power_supply_ext_registration *reg;
+
+	if (atomic_read(&psy->use_cnt) <= 0)
+		return -ENODEV;
+
+	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 -ENODEV;
+		}
+	}
+
+	if (!psy->desc->property_is_writeable)
 		return -ENODEV;
 
 	return psy->desc->property_is_writeable(psy, psp);
@@ -1268,6 +1327,54 @@ 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,
+				    void *ext_data)
+{
+	struct power_supply_ext_registration *reg;
+	size_t i;
+
+	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;
+	list_add(&reg->list_head, &psy->extensions);
+
+	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)
+{
+	struct power_supply_ext_registration *reg;
+
+	power_supply_for_each_extension(reg, psy) {
+		if (reg->ext == ext) {
+			list_del(&reg->list_head);
+			kfree(reg);
+			power_supply_update_groups(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);
@@ -1427,6 +1534,7 @@ __power_supply_register(struct device *parent,
 	}
 
 	spin_lock_init(&psy->changed_lock);
+	INIT_LIST_HEAD(&psy->extensions);
 	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 915a4ba62258..e7c306afd846 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
 	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;
+
+	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_show_property(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf) {
@@ -306,8 +323,7 @@ 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);
+		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);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 72dc7e45c90c..51788faf1d66 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/list.h>
 #include <linux/spinlock.h>
 #include <linux/notifier.h>
 
@@ -280,6 +281,28 @@ 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 *ext_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 *ext_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 *ext_data,
+				     enum power_supply_property psp);
+};
+
 struct power_supply {
 	const struct power_supply_desc *desc;
 
@@ -303,6 +326,7 @@ struct power_supply {
 	bool removing;
 	atomic_t use_cnt;
 	struct power_supply_battery_info *battery_info;
+	struct list_head extensions;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;
 	struct thermal_cooling_device *tcd;
@@ -887,6 +911,12 @@ 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,
+					   void *ext_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);

-- 
2.46.0

Re: [PATCH RFC v3 6/9] power: supply: core: implement extension API

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 12158 bytes --]

Hi,

On Wed, Sep 04, 2024 at 09:25:39PM GMT, Thomas Weißschuh wrote:
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

missing long description :)

In general I like the concept. It looks like a good candidate to
clean up the mess with the custom registration of properties done
by the x86 platform code.

Apart from ACPI with custom battery extensions mess, there is
something similar in the embedded world: Many charger chips have
rudimentary fuel-gauge support. Some drivers register a charger/usb
and a battery device. That results in two battery devices when the
machine also has a proper fuel gauge. Having two battery devices
exposed is bad, since that means the machine has two batteries
(like some Thinkpads). Not exposing the battery from the charger
device is possibly loosing information (depending on fuel gauge
features).

I think it should be possible to register the charger's rudimentary
battery device as a power_supply_ext, if it detects a proper fuel
gauge being available (e.g. via the DT "power-supplies" property).
But then the charger's properties are the preferred ones. I think
they should be fallback properties instead. A machine only gets its
own fuel-gauge, when that provides better values than the data
available from the charger :)

So the question is which property getter/setter should be used
when the same property is exposed multiple times. Your examples
only add new properties and does not run into this. The intuitive
thing would be for properties from extensions to be preferred,
but do we actually have a use case for that?

Greetings,

-- Sebastian

>  drivers/power/supply/power_supply.h       |  12 ++++
>  drivers/power/supply/power_supply_core.c  | 114 +++++++++++++++++++++++++++++-
>  drivers/power/supply/power_supply_sysfs.c |  20 +++++-
>  include/linux/power_supply.h              |  30 ++++++++
>  4 files changed, 171 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index b01faeaf7827..fb45f0717bd1 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -15,6 +15,18 @@ struct power_supply;
>  
>  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) \
> +	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 dcb7e4853030..db5e7af57e67 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1199,15 +1199,40 @@ 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;
> +
> +	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(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;
>  }
>  
> @@ -1215,12 +1240,19 @@ 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;
>  	}
>  
> +	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))
> @@ -1234,7 +1266,21 @@ 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;
> +
> +	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);
> @@ -1244,8 +1290,21 @@ 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)
> +	struct power_supply_ext_registration *reg;
> +
> +	if (atomic_read(&psy->use_cnt) <= 0)
> +		return -ENODEV;
> +
> +	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 -ENODEV;
> +		}
> +	}
> +
> +	if (!psy->desc->property_is_writeable)
>  		return -ENODEV;
>  
>  	return psy->desc->property_is_writeable(psy, psp);
> @@ -1268,6 +1327,54 @@ 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,
> +				    void *ext_data)
> +{
> +	struct power_supply_ext_registration *reg;
> +	size_t i;
> +
> +	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;
> +	list_add(&reg->list_head, &psy->extensions);
> +
> +	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)
> +{
> +	struct power_supply_ext_registration *reg;
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (reg->ext == ext) {
> +			list_del(&reg->list_head);
> +			kfree(reg);
> +			power_supply_update_groups(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);
> @@ -1427,6 +1534,7 @@ __power_supply_register(struct device *parent,
>  	}
>  
>  	spin_lock_init(&psy->changed_lock);
> +	INIT_LIST_HEAD(&psy->extensions);
>  	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 915a4ba62258..e7c306afd846 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>  	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;
> +
> +	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_show_property(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf) {
> @@ -306,8 +323,7 @@ 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);
> +		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);
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 72dc7e45c90c..51788faf1d66 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/list.h>
>  #include <linux/spinlock.h>
>  #include <linux/notifier.h>
>  
> @@ -280,6 +281,28 @@ 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 *ext_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 *ext_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 *ext_data,
> +				     enum power_supply_property psp);
> +};
> +
>  struct power_supply {
>  	const struct power_supply_desc *desc;
>  
> @@ -303,6 +326,7 @@ struct power_supply {
>  	bool removing;
>  	atomic_t use_cnt;
>  	struct power_supply_battery_info *battery_info;
> +	struct list_head extensions;
>  #ifdef CONFIG_THERMAL
>  	struct thermal_zone_device *tzd;
>  	struct thermal_cooling_device *tcd;
> @@ -887,6 +911,12 @@ 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,
> +					   void *ext_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);
> 
> -- 
> 2.46.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC v3 6/9] power: supply: core: implement extension API

[Thomas Weißschuh]

On 2024-09-14 12:50:57+0000, Sebastian Reichel wrote:
> On Wed, Sep 04, 2024 at 09:25:39PM GMT, Thomas Weißschuh wrote:
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> 
> missing long description :)

I'll paste the cover letter here for the next one :-)

> In general I like the concept. It looks like a good candidate to
> clean up the mess with the custom registration of properties done
> by the x86 platform code.
> 
> Apart from ACPI with custom battery extensions mess, there is
> something similar in the embedded world: Many charger chips have
> rudimentary fuel-gauge support. Some drivers register a charger/usb
> and a battery device. That results in two battery devices when the
> machine also has a proper fuel gauge. Having two battery devices
> exposed is bad, since that means the machine has two batteries
> (like some Thinkpads). Not exposing the battery from the charger
> device is possibly loosing information (depending on fuel gauge
> features).
> 
> I think it should be possible to register the charger's rudimentary
> battery device as a power_supply_ext, if it detects a proper fuel
> gauge being available (e.g. via the DT "power-supplies" property).
> But then the charger's properties are the preferred ones. I think
> they should be fallback properties instead. A machine only gets its
> own fuel-gauge, when that provides better values than the data
> available from the charger :)
> 
> So the question is which property getter/setter should be used
> when the same property is exposed multiple times. Your examples
> only add new properties and does not run into this. The intuitive
> thing would be for properties from extensions to be preferred,
> but do we actually have a use case for that?

Currently duplicate properties are straight up rejected.
With multiple extensions any order-based resolution of duplicates seems
to be prone for confusion in case the registration order changes.

The x86 platform and embedded usecases seem quite disjunct to me.
If we can keep them separate that would be easier to implement.

x86:

Don't allow any overriding, like the code currently does.

embedded:

Add different extension priorities. When registering a new one,
unregister all lower priority ones; do nothing if a higher priority is
already registered.
This makes the result predictable even if the order changes.
A fallback extension would need quite some bespoke logic I think.

Note:
I noticed that the locking for power_supply_set_property() is currently
broken. In case you are wondering about that.


Thomas

> Greetings,
> 
> -- Sebastian
> 
> >  drivers/power/supply/power_supply.h       |  12 ++++
> >  drivers/power/supply/power_supply_core.c  | 114 +++++++++++++++++++++++++++++-
> >  drivers/power/supply/power_supply_sysfs.c |  20 +++++-
> >  include/linux/power_supply.h              |  30 ++++++++
> >  4 files changed, 171 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index b01faeaf7827..fb45f0717bd1 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -15,6 +15,18 @@ struct power_supply;
> >  
> >  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) \
> > +	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 dcb7e4853030..db5e7af57e67 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -1199,15 +1199,40 @@ 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;
> > +
> > +	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(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;
> >  }
> >  
> > @@ -1215,12 +1240,19 @@ 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;
> >  	}
> >  
> > +	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))
> > @@ -1234,7 +1266,21 @@ 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;
> > +
> > +	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);
> > @@ -1244,8 +1290,21 @@ 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)
> > +	struct power_supply_ext_registration *reg;
> > +
> > +	if (atomic_read(&psy->use_cnt) <= 0)
> > +		return -ENODEV;
> > +
> > +	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 -ENODEV;
> > +		}
> > +	}
> > +
> > +	if (!psy->desc->property_is_writeable)
> >  		return -ENODEV;
> >  
> >  	return psy->desc->property_is_writeable(psy, psp);
> > @@ -1268,6 +1327,54 @@ 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,
> > +				    void *ext_data)
> > +{
> > +	struct power_supply_ext_registration *reg;
> > +	size_t i;
> > +
> > +	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;
> > +	list_add(&reg->list_head, &psy->extensions);
> > +
> > +	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)
> > +{
> > +	struct power_supply_ext_registration *reg;
> > +
> > +	power_supply_for_each_extension(reg, psy) {
> > +		if (reg->ext == ext) {
> > +			list_del(&reg->list_head);
> > +			kfree(reg);
> > +			power_supply_update_groups(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);
> > @@ -1427,6 +1534,7 @@ __power_supply_register(struct device *parent,
> >  	}
> >  
> >  	spin_lock_init(&psy->changed_lock);
> > +	INIT_LIST_HEAD(&psy->extensions);
> >  	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 915a4ba62258..e7c306afd846 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
> >  	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;
> > +
> > +	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_show_property(struct device *dev,
> >  					  struct device_attribute *attr,
> >  					  char *buf) {
> > @@ -306,8 +323,7 @@ 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);
> > +		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);
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index 72dc7e45c90c..51788faf1d66 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/list.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/notifier.h>
> >  
> > @@ -280,6 +281,28 @@ 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 *ext_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 *ext_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 *ext_data,
> > +				     enum power_supply_property psp);
> > +};
> > +
> >  struct power_supply {
> >  	const struct power_supply_desc *desc;
> >  
> > @@ -303,6 +326,7 @@ struct power_supply {
> >  	bool removing;
> >  	atomic_t use_cnt;
> >  	struct power_supply_battery_info *battery_info;
> > +	struct list_head extensions;
> >  #ifdef CONFIG_THERMAL
> >  	struct thermal_zone_device *tzd;
> >  	struct thermal_cooling_device *tcd;
> > @@ -887,6 +911,12 @@ 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,
> > +					   void *ext_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);
> > 
> > -- 
> > 2.46.0
> > 
> > 

Re: [PATCH RFC v3 6/9] power: supply: core: implement extension API

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

Hi,

On Sat, Sep 14, 2024 at 04:25:25PM GMT, Thomas Weißschuh wrote:
> On 2024-09-14 12:50:57+0000, Sebastian Reichel wrote:
> > On Wed, Sep 04, 2024 at 09:25:39PM GMT, Thomas Weißschuh wrote:
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > 
> > missing long description :)
> 
> I'll paste the cover letter here for the next one :-)
> 
> > In general I like the concept. It looks like a good candidate to
> > clean up the mess with the custom registration of properties done
> > by the x86 platform code.
> > 
> > Apart from ACPI with custom battery extensions mess, there is
> > something similar in the embedded world: Many charger chips have
> > rudimentary fuel-gauge support. Some drivers register a charger/usb
> > and a battery device. That results in two battery devices when the
> > machine also has a proper fuel gauge. Having two battery devices
> > exposed is bad, since that means the machine has two batteries
> > (like some Thinkpads). Not exposing the battery from the charger
> > device is possibly loosing information (depending on fuel gauge
> > features).
> > 
> > I think it should be possible to register the charger's rudimentary
> > battery device as a power_supply_ext, if it detects a proper fuel
> > gauge being available (e.g. via the DT "power-supplies" property).
> > But then the charger's properties are the preferred ones. I think
> > they should be fallback properties instead. A machine only gets its
> > own fuel-gauge, when that provides better values than the data
> > available from the charger :)
> > 
> > So the question is which property getter/setter should be used
> > when the same property is exposed multiple times. Your examples
> > only add new properties and does not run into this. The intuitive
> > thing would be for properties from extensions to be preferred,
> > but do we actually have a use case for that?
> 
> Currently duplicate properties are straight up rejected.
> With multiple extensions any order-based resolution of duplicates seems
> to be prone for confusion in case the registration order changes.

Ah, I missed the part about rejecting duplicates.

> The x86 platform and embedded usecases seem quite disjunct to me.
> If we can keep them separate that would be easier to implement.

I wouldn't say they are disjunct. The embedded use case is basically
a superset of the x86 variant.

> x86:
> 
> Don't allow any overriding, like the code currently does.
> 
> embedded:
> 
> Add different extension priorities. When registering a new one,
> unregister all lower priority ones; do nothing if a higher priority is
> already registered.
> This makes the result predictable even if the order changes.
> A fallback extension would need quite some bespoke logic I think.

Adding priorities seems sensible, but also requires assigning
priorities to the base device. I need to think more about this, but
it can wait until the basic version for x86 has landed

> Note:
> I noticed that the locking for power_supply_set_property() is currently
> broken. In case you are wondering about that.

ok.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RFC v3 7/9] power: supply: core: add locking around extension access

[Thomas Weißschuh]

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

---
This is only separate for easier review during the RFC phase.
It will be squashed into the actual power supply extension commit later.
---
 drivers/power/supply/power_supply.h       |  3 ++
 drivers/power/supply/power_supply_core.c  | 49 ++++++++++++++++++++++++++-----
 drivers/power/supply/power_supply_leds.c  |  2 ++
 drivers/power/supply/power_supply_sysfs.c |  6 ++++
 include/linux/power_supply.h              |  3 ++
 5 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index fb45f0717bd1..b8e14cc70fcf 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;
@@ -25,6 +27,7 @@ struct power_supply_ext_registration {
 };
 
 #define power_supply_for_each_extension(pos, psy) \
+	lockdep_assert_held(&(psy)->extensions_sem); \
 	list_for_each_entry(pos, &(psy)->extensions, list_head)
 
 
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index db5e7af57e67..4839be25e6be 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -11,6 +11,7 @@
 
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/cleanup.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
@@ -80,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);
@@ -87,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
@@ -1218,17 +1230,26 @@ bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
 	return found;
 }
 
-bool power_supply_has_property(struct power_supply *psy,
-			       enum power_supply_property psp)
+static bool psy_has_property_no_ext(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;
 
+	return false;
+}
+
+bool power_supply_has_property(struct power_supply *psy,
+			       enum power_supply_property psp)
+{
+	struct power_supply_ext_registration *reg;
+
+	if (psy_has_property_no_ext(psy, psp))
+		return true;
+
 	power_supply_for_each_extension(reg, psy)
 		if (power_supply_ext_has_property(reg->ext, psp))
 			return true;
@@ -1329,11 +1350,14 @@ EXPORT_SYMBOL_GPL(power_supply_powers);
 
 static int power_supply_update_groups(struct power_supply *psy)
 {
-	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&psy->changed_lock, flags);
+	psy->update_groups = true;
+	spin_unlock_irqrestore(&psy->changed_lock, flags);
 
-	ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
 	power_supply_changed(psy);
-	return ret;
+	return 0;
 }
 
 int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
@@ -1342,6 +1366,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
 	struct power_supply_ext_registration *reg;
 	size_t i;
 
+	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;
@@ -1362,6 +1388,8 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
 {
 	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);
@@ -1405,6 +1433,7 @@ static int power_supply_read_temp(struct thermal_zone_device *tzd,
 
 	WARN_ON(tzd == NULL);
 	psy = thermal_zone_device_priv(tzd);
+	guard(rwsem_read)(&psy->extensions_sem);
 	ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_TEMP, &val);
 	if (ret)
 		return ret;
@@ -1427,7 +1456,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, POWER_SUPPLY_PROP_TEMP)) {
+	if (psy_has_property_no_ext(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)
@@ -1534,7 +1563,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;
@@ -1547,6 +1578,8 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto register_thermal_failed;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	rc = power_supply_create_triggers(psy);
 	if (rc)
 		goto create_triggers_failed;
diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c
index f4a7e566bea1..09a6f3fe2f85 100644
--- a/drivers/power/supply/power_supply_leds.c
+++ b/drivers/power/supply/power_supply_leds.c
@@ -195,6 +195,8 @@ static void power_supply_remove_gen_triggers(struct power_supply *psy)
 
 void power_supply_update_leds(struct power_supply *psy)
 {
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	if (psy->desc->type == POWER_SUPPLY_TYPE_BATTERY)
 		power_supply_update_bat_leds(psy);
 	else
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index e7c306afd846..b56e0bd424f5 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -371,6 +371,8 @@ static ssize_t power_supply_store_property(struct device *dev,
 
 	value.intval = ret;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	ret = power_supply_set_property(psy, psp, &value);
 	if (ret < 0)
 		return ret;
@@ -392,6 +394,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;
@@ -497,6 +501,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	if (ret)
 		goto out;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
 		ret = add_prop_uevent(dev, env, j, prop_buf);
 		if (ret)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 51788faf1d66..87bc50698649 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/list.h>
 #include <linux/spinlock.h>
 #include <linux/notifier.h>
@@ -322,10 +323,12 @@ 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;

-- 
2.46.0

Re: [PATCH RFC v3 7/9] power: supply: core: add locking around extension access

[kernel test robot]

Hi Thomas,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on d8abb73f584772eaafa95a447c90f1c02dba0dec]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Wei-schuh/power-supply-core-rename-psy_has_property-to-psy_desc_has_property/20240905-032916
base:   d8abb73f584772eaafa95a447c90f1c02dba0dec
patch link:    https://lore.kernel.org/r/20240904-power-supply-extensions-v3-7-62efeb93f8ec%40weissschuh.net
patch subject: [PATCH RFC v3 7/9] power: supply: core: add locking around extension access
config: i386-buildonly-randconfig-001-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080331.btTYCUQL-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409080331.btTYCUQL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409080331.btTYCUQL-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/power/supply/power_supply_sysfs.c:502:3: error: cannot jump from this goto statement to its label
     502 |                 goto out;
         |                 ^
   drivers/power/supply/power_supply_sysfs.c:504:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
     504 |         guard(rwsem_read)(&psy->extensions_sem);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:75:1: note: expanded from here
      75 | __UNIQUE_ID_guard178
         | ^
   1 error generated.
--
>> drivers/power/supply/power_supply_core.c:1579:3: error: cannot jump from this goto statement to its label
    1579 |                 goto register_thermal_failed;
         |                 ^
   drivers/power/supply/power_supply_core.c:1581:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    1581 |         guard(rwsem_read)(&psy->extensions_sem);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:79:1: note: expanded from here
      79 | __UNIQUE_ID_guard216
         | ^
   drivers/power/supply/power_supply_core.c:1575:3: error: cannot jump from this goto statement to its label
    1575 |                 goto wakeup_init_failed;
         |                 ^
   drivers/power/supply/power_supply_core.c:1581:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    1581 |         guard(rwsem_read)(&psy->extensions_sem);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:79:1: note: expanded from here
      79 | __UNIQUE_ID_guard216
         | ^
   drivers/power/supply/power_supply_core.c:1571:3: error: cannot jump from this goto statement to its label
    1571 |                 goto device_add_failed;
         |                 ^
   drivers/power/supply/power_supply_core.c:1581:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    1581 |         guard(rwsem_read)(&psy->extensions_sem);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:79:1: note: expanded from here
      79 | __UNIQUE_ID_guard216
         | ^
   drivers/power/supply/power_supply_core.c:1562:4: error: cannot jump from this goto statement to its label
    1562 |                         goto check_supplies_failed;
         |                         ^
   drivers/power/supply/power_supply_core.c:1581:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    1581 |         guard(rwsem_read)(&psy->extensions_sem);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:79:1: note: expanded from here
      79 | __UNIQUE_ID_guard216
         | ^
   drivers/power/supply/power_supply_core.c:1551:3: error: cannot jump from this goto statement to its label
    1551 |                 goto check_supplies_failed;
         |                 ^
   drivers/power/supply/power_supply_core.c:1581:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    1581 |         guard(rwsem_read)(&psy->extensions_sem);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b


vim +502 drivers/power/supply/power_supply_sysfs.c

2ad3d74e3c69fb drivers/power/supply/power_supply_sysfs.c Mathew King         2020-05-04  473  
23680f0b7d7f67 drivers/power/supply/power_supply_sysfs.c Greg Kroah-Hartman  2022-11-23  474  int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  475  {
aeab24ab7f1d7d drivers/power/supply/power_supply_sysfs.c Thomas Weißschuh    2024-09-04  476  	struct power_supply *psy = dev_get_drvdata(dev);
7eff2e7a8b65c2 drivers/power/power_supply_sysfs.c        Kay Sievers         2007-08-14  477  	int ret = 0, j;
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  478  	char *prop_buf;
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  479  
297d716f6260cc drivers/power/power_supply_sysfs.c        Krzysztof Kozlowski 2015-03-12  480  	if (!psy || !psy->desc) {
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  481  		dev_dbg(dev, "No power supply yet\n");
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  482  		return ret;
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  483  	}
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  484  
297d716f6260cc drivers/power/power_supply_sysfs.c        Krzysztof Kozlowski 2015-03-12  485  	ret = add_uevent_var(env, "POWER_SUPPLY_NAME=%s", psy->desc->name);
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  486  	if (ret)
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  487  		return ret;
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  488  
3dc0bab23dba53 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-09-19  489  	/*
3dc0bab23dba53 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-09-19  490  	 * Kernel generates KOBJ_REMOVE uevent in device removal path, after
3dc0bab23dba53 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-09-19  491  	 * resources have been freed. Exit early to avoid use-after-free.
3dc0bab23dba53 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-09-19  492  	 */
3dc0bab23dba53 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-09-19  493  	if (psy->removing)
3dc0bab23dba53 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-09-19  494  		return 0;
3dc0bab23dba53 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-09-19  495  
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  496  	prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  497  	if (!prop_buf)
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  498  		return -ENOMEM;
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  499  
2ad3d74e3c69fb drivers/power/supply/power_supply_sysfs.c Mathew King         2020-05-04  500  	ret = add_prop_uevent(dev, env, POWER_SUPPLY_PROP_TYPE, prop_buf);
2ad3d74e3c69fb drivers/power/supply/power_supply_sysfs.c Mathew King         2020-05-04  501  	if (ret)
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04 @502  		goto out;
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  503  
b3fcf7aeab01f6 drivers/power/supply/power_supply_sysfs.c Thomas Weißschuh    2024-09-04  504  	guard(rwsem_read)(&psy->extensions_sem);
b3fcf7aeab01f6 drivers/power/supply/power_supply_sysfs.c Thomas Weißschuh    2024-09-04  505  
aeab24ab7f1d7d drivers/power/supply/power_supply_sysfs.c Thomas Weißschuh    2024-09-04  506  	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
aeab24ab7f1d7d drivers/power/supply/power_supply_sysfs.c Thomas Weißschuh    2024-09-04  507  		ret = add_prop_uevent(dev, env, j, prop_buf);
27a2195efa8d26 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-03-17  508  		if (ret)
27a2195efa8d26 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-03-17  509  			goto out;
27a2195efa8d26 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-03-17  510  	}
27a2195efa8d26 drivers/power/supply/power_supply_sysfs.c Sebastian Reichel   2023-03-17  511  
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  512  out:
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  513  	free_page((unsigned long)prop_buf);
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  514  
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  515  	return ret;
4a11b59d828366 drivers/power/power_supply_sysfs.c        Anton Vorontsov     2007-05-04  516  }
539b9c94ac8356 drivers/power/supply/power_supply_sysfs.c Thomas Weißschuh    2021-11-24  517  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH RFC v3 8/9] power: supply: test-power: implement a power supply extension

[Thomas Weißschuh]

Allow easy testing of the new power supply extension functionality.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/test_power.c | 104 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 442ceb7795e1..da1877f31929 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,82 @@ static const struct power_supply_config test_power_configs[] = {
 	},
 };
 
+static int power_supply_ext_manufacture_year = 1234;
+static const enum power_supply_property power_supply_ext_props[] = {
+	POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
+};
+
+static int power_supply_ext_get_property(struct power_supply *psy,
+					 const struct power_supply_ext *ext,
+					 void *ext_data,
+					 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;
+	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,
+					 const struct power_supply_ext *ext,
+					 void *ext_data,
+					 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,
+						  const struct power_supply_ext *ext,
+						  void *ext_data,
+						  enum power_supply_property psp)
+{
+	return true;
+}
+
+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, NULL)) {
+			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 +335,8 @@ static int __init test_power_init(void)
 		}
 	}
 
+	test_battery_configure_battery_hook(true);
+
 	module_initialized = true;
 	return 0;
 failed:
@@ -524,6 +603,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 +669,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 +684,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 +722,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 <cbouatmailru@gmail.com>");
 MODULE_LICENSE("GPL");

-- 
2.46.0

[PATCH RFC v3 9/9] power: supply: cros_charge-control: use power_supply extensions

[Thomas Weißschuh]

Power supply extensions provide an easier mechanism to implement
additional properties for existing power supplies.
Use that instead of reimplementing the sysfs attributes manually.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/cros_charge-control.c | 207 +++++++++++------------------
 1 file changed, 78 insertions(+), 129 deletions(-)

diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index 17c53591ce19..dbfd87f8eb1d 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -18,13 +18,6 @@
 					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)   | \
 					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
 
-enum CROS_CHCTL_ATTR {
-	CROS_CHCTL_ATTR_START_THRESHOLD,
-	CROS_CHCTL_ATTR_END_THRESHOLD,
-	CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR,
-	_CROS_CHCTL_ATTR_COUNT
-};
-
 /*
  * Semantics of data *returned* from the EC API and Linux sysfs differ
  * slightly, also the v1 API can not return any data.
@@ -41,13 +34,7 @@ struct cros_chctl_priv {
 	struct power_supply *hooked_battery;
 	u8 cmd_version;
 
-	/* The callbacks need to access this priv structure.
-	 * As neither the struct device nor power_supply are under the drivers
-	 * control, embed the attributes within priv to use with container_of().
-	 */
-	struct device_attribute device_attrs[_CROS_CHCTL_ATTR_COUNT];
-	struct attribute *attributes[_CROS_CHCTL_ATTR_COUNT];
-	struct attribute_group group;
+	struct power_supply_ext psy_ext;
 
 	enum power_supply_charge_behaviour current_behaviour;
 	u8 current_start_threshold, current_end_threshold;
@@ -114,123 +101,86 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
 	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
 }
 
-static struct cros_chctl_priv *cros_chctl_attr_to_priv(struct attribute *attr,
-						       enum CROS_CHCTL_ATTR idx)
-{
-	struct device_attribute *dev_attr = container_of(attr, struct device_attribute, attr);
+static const enum power_supply_property cros_chctl_psy_ext_props[] = {
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+};
 
-	return container_of(dev_attr, struct cros_chctl_priv, device_attrs[idx]);
-}
+static const enum power_supply_property cros_chctl_psy_ext_props_v1[] = {
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+};
 
-static ssize_t cros_chctl_store_threshold(struct device *dev, struct cros_chctl_priv *priv,
-					  int is_end_threshold, const char *buf, size_t count)
+static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
+				       const struct power_supply_ext *ext,
+				       void *data,
+				       enum power_supply_property psp,
+				       union power_supply_propval *val)
 {
-	int ret, val;
+	struct cros_chctl_priv *priv = data;
 
-	ret = kstrtoint(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-	if (val < 0 || val > 100)
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+		val->intval = priv->current_start_threshold;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		val->intval = priv->current_end_threshold;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		val->intval = priv->current_behaviour;
+		return 0;
+	default:
 		return -EINVAL;
-
-	if (is_end_threshold) {
-		if (val <= priv->current_start_threshold)
-			return -EINVAL;
-		priv->current_end_threshold = val;
-	} else {
-		if (val >= priv->current_end_threshold)
-			return -EINVAL;
-		priv->current_start_threshold = val;
-	}
-
-	if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) {
-		ret = cros_chctl_configure_ec(priv);
-		if (ret < 0)
-			return ret;
 	}
-
-	return count;
-}
-
-static ssize_t charge_control_start_threshold_show(struct device *dev,
-						   struct device_attribute *attr,
-						   char *buf)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_START_THRESHOLD);
-
-	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_start_threshold);
-}
-
-static ssize_t charge_control_start_threshold_store(struct device *dev,
-						    struct device_attribute *attr,
-						    const char *buf, size_t count)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_START_THRESHOLD);
-
-	return cros_chctl_store_threshold(dev, priv, 0, buf, count);
-}
-
-static ssize_t charge_control_end_threshold_show(struct device *dev, struct device_attribute *attr,
-						 char *buf)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_END_THRESHOLD);
-
-	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_end_threshold);
-}
-
-static ssize_t charge_control_end_threshold_store(struct device *dev, struct device_attribute *attr,
-						  const char *buf, size_t count)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_END_THRESHOLD);
-
-	return cros_chctl_store_threshold(dev, priv, 1, buf, count);
 }
 
-static ssize_t charge_behaviour_show(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
-
-	return power_supply_charge_behaviour_show(dev, EC_CHARGE_CONTROL_BEHAVIOURS,
-						  priv->current_behaviour, buf);
-}
 
-static ssize_t charge_behaviour_store(struct device *dev, struct device_attribute *attr,
-				      const char *buf, size_t count)
+static int cros_chctl_psy_ext_set_prop(struct power_supply *psy,
+				       const struct power_supply_ext *ext,
+				       void *data,
+				       enum power_supply_property psp,
+				       const union power_supply_propval *val)
 {
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
+	struct cros_chctl_priv *priv = data;
 	int ret;
 
-	ret = power_supply_charge_behaviour_parse(EC_CHARGE_CONTROL_BEHAVIOURS, buf);
-	if (ret < 0)
-		return ret;
-
-	priv->current_behaviour = ret;
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		if (val->intval < 0 || val->intval > 100)
+			return -EINVAL;
 
-	ret = cros_chctl_configure_ec(priv);
-	if (ret < 0)
-		return ret;
+		if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD) {
+			if (val->intval <= priv->current_start_threshold)
+				return -EINVAL;
+			priv->current_end_threshold = val->intval;
+		} else {
+			if (val->intval >= priv->current_end_threshold)
+				return -EINVAL;
+			priv->current_start_threshold = val->intval;
+		}
+
+		if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) {
+			ret = cros_chctl_configure_ec(priv);
+			if (ret < 0)
+				return ret;
+		}
 
-	return count;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		priv->current_behaviour = val->intval;
+		return cros_chctl_configure_ec(priv); /* FIXME > 0 */
+	default:
+		return -EINVAL;
+	}
 }
 
-static umode_t cros_chtl_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+static int cros_chctl_psy_prop_is_writeable(struct power_supply *psy,
+					    const struct power_supply_ext *ext,
+					    void *data,
+					    enum power_supply_property psp)
 {
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(attr, n);
-
-	if (priv->cmd_version < 2) {
-		if (n == CROS_CHCTL_ATTR_START_THRESHOLD)
-			return 0;
-		if (n == CROS_CHCTL_ATTR_END_THRESHOLD)
-			return 0;
-	}
-
-	return attr->mode;
+	return true;
 }
 
 static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
@@ -241,7 +191,7 @@ static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_batt
 		return 0;
 
 	priv->hooked_battery = battery;
-	return device_add_group(&battery->dev, &priv->group);
+	return power_supply_register_extension(battery, &priv->psy_ext, priv);
 }
 
 static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
@@ -249,7 +199,7 @@ static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_b
 	struct cros_chctl_priv *priv = container_of(hook, struct cros_chctl_priv, battery_hook);
 
 	if (priv->hooked_battery == battery) {
-		device_remove_group(&battery->dev, &priv->group);
+		power_supply_unregister_extension(battery, &priv->psy_ext);
 		priv->hooked_battery = NULL;
 	}
 
@@ -275,7 +225,6 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
 	struct cros_chctl_priv *priv;
-	size_t i;
 	int ret;
 
 	ret = cros_chctl_fwk_charge_control_versions(cros_ec);
@@ -305,23 +254,23 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	dev_dbg(dev, "Command version: %u\n", (unsigned int)priv->cmd_version);
 
 	priv->cros_ec = cros_ec;
-	priv->device_attrs[CROS_CHCTL_ATTR_START_THRESHOLD] =
-		(struct device_attribute)__ATTR_RW(charge_control_start_threshold);
-	priv->device_attrs[CROS_CHCTL_ATTR_END_THRESHOLD] =
-		(struct device_attribute)__ATTR_RW(charge_control_end_threshold);
-	priv->device_attrs[CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR] =
-		(struct device_attribute)__ATTR_RW(charge_behaviour);
-	for (i = 0; i < _CROS_CHCTL_ATTR_COUNT; i++) {
-		sysfs_attr_init(&priv->device_attrs[i].attr);
-		priv->attributes[i] = &priv->device_attrs[i].attr;
-	}
-	priv->group.is_visible = cros_chtl_attr_is_visible;
-	priv->group.attrs = priv->attributes;
 
 	priv->battery_hook.name = dev_name(dev);
 	priv->battery_hook.add_battery = cros_chctl_add_battery;
 	priv->battery_hook.remove_battery = cros_chctl_remove_battery;
 
+	if (priv->cmd_version == 1) {
+		priv->psy_ext.properties = cros_chctl_psy_ext_props_v1;
+		priv->psy_ext.num_properties = ARRAY_SIZE(cros_chctl_psy_ext_props_v1);
+	} else {
+		priv->psy_ext.properties = cros_chctl_psy_ext_props;
+		priv->psy_ext.num_properties = ARRAY_SIZE(cros_chctl_psy_ext_props);
+	}
+	priv->psy_ext.charge_behaviours = EC_CHARGE_CONTROL_BEHAVIOURS;
+	priv->psy_ext.get_property = cros_chctl_psy_ext_get_prop;
+	priv->psy_ext.set_property = cros_chctl_psy_ext_set_prop;
+	priv->psy_ext.property_is_writeable = cros_chctl_psy_prop_is_writeable;
+
 	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
 	priv->current_start_threshold = 0;
 	priv->current_end_threshold = 100;

-- 
2.46.0