[RFC,0/6] power: supply: extension API

Introduce a mechanism for drivers to extend the properties implemented
by a power supply.

Motivation
----------

Various drivers, mostly in platform/x86 extend the ACPI battery driver
with additional sysfs attributes to implement more UAPIs than are
exposed through ACPI by using various side-channels, like WMI,
nonstandard ACPI or EC communication.

While the created sysfs attributes look similar to the attributes
provided by the powersupply core, there are various deficiencies:

* They don't show up in uevent payload.
* They can't be queried with the standard in-kernel APIs.
* They don't work with triggers.
* The extending driver has to reimplement all of the parsing,
  formatting and sysfs display logic.
* Writing a extension driver is completely different from writing a
  normal power supply driver.
* Properties can not be properly overriden.

The proposed extension API avoids all of these issues.
An extension is just a "struct power_supply_ext" with the same kind of
callbacks as in a normal "struct power_supply_desc".

The API is meant to be used via battery_hook_register(), the same way as
the current extensions.

For example my upcoming cros_ec charge control driver[0] saves 80 lines
of code with this patchset.

Contents
--------

* Patch 1 and 2 are generic preparation patches, that probably make
  sense without this series.
* Patch 3 implements the extension API itself.
* Patch 4 implements a PoC locking scheme for the extension API.
* Patch 5 adds extension support to test_power.c
* Patch 6 converts the in-tree platform/x86/system76 driver to the
  extension API.

Open issues
-----------

* Newly registered properties will not show up in hwmon.
  To do that properly would require some changes in the hwmon core.
  As far as I know, no current driver would extend the hwmon properties anyways.
* As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
  it also be gated behind this or another config?
* Only one extension can be used at a time.
  So far this should be enough, having more would complicate the
  implementation.
* Is an rw_semaphore acceptable?

[0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (6):
      power: supply: sysfs: use power_supply_property_is_writeable()
      power: supply: core: avoid iterating properties directly
      power: supply: core: implement extension API
      power: supply: core: add locking around extension access
      power: supply: test-power: implement a power supply extension
      platform/x86: system76: Use power_supply extension API

 drivers/platform/x86/system76_acpi.c      |  83 +++++++++---------
 drivers/power/supply/power_supply.h       |   9 ++
 drivers/power/supply/power_supply_core.c  | 136 ++++++++++++++++++++++++++++--
 drivers/power/supply/power_supply_hwmon.c |  48 +++++------
 drivers/power/supply/power_supply_sysfs.c |  39 ++++++---
 drivers/power/supply/test_power.c         | 102 ++++++++++++++++++++++
 include/linux/power_supply.h              |  25 ++++++
 7 files changed, 357 insertions(+), 85 deletions(-)
---
base-commit: 2df0193e62cf887f373995fb8a91068562784adc
change-id: 20240602-power-supply-extensions-07d949f509d9

Best regards,

Am 06.06.24 um 16:50 schrieb Thomas Weißschuh:

> Introduce a mechanism for drivers to extend the properties implemented
> by a power supply.
>
> Motivation
> ----------
>
> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
>
> While the created sysfs attributes look similar to the attributes
> provided by the powersupply core, there are various deficiencies:
>
> * They don't show up in uevent payload.
> * They can't be queried with the standard in-kernel APIs.
> * They don't work with triggers.
> * The extending driver has to reimplement all of the parsing,
>    formatting and sysfs display logic.
> * Writing a extension driver is completely different from writing a
>    normal power supply driver.
> * Properties can not be properly overriden.
>
> The proposed extension API avoids all of these issues.
> An extension is just a "struct power_supply_ext" with the same kind of
> callbacks as in a normal "struct power_supply_desc".
>
> The API is meant to be used via battery_hook_register(), the same way as
> the current extensions.
>
> For example my upcoming cros_ec charge control driver[0] saves 80 lines
> of code with this patchset.
>
> Contents
> --------
>
> * Patch 1 and 2 are generic preparation patches, that probably make
>    sense without this series.
> * Patch 3 implements the extension API itself.
> * Patch 4 implements a PoC locking scheme for the extension API.
> * Patch 5 adds extension support to test_power.c
> * Patch 6 converts the in-tree platform/x86/system76 driver to the
>    extension API.
>
> Open issues
> -----------
>
> * Newly registered properties will not show up in hwmon.
>    To do that properly would require some changes in the hwmon core.
>    As far as I know, no current driver would extend the hwmon properties anyways.
> * As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
>    it also be gated behind this or another config?
> * Only one extension can be used at a time.
>    So far this should be enough, having more would complicate the
>    implementation.
> * Is an rw_semaphore acceptable?
>
> [0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/

Nice, i love this proposal!

I agree that the hwmon update functionality will need some changes in the hwmon core to work,
but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
support for this at a later point in time.

The possibility of registering multiple power supply extensions on a single power supply will
be necessary to support battery charge control on Dell notebooks in the future. This is because
there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
dell-laptop (when support for battery charge control is supported someday).

How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
this later when the need arises.

Thanks,
Armin Wolf

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Thomas Weißschuh (6):
>        power: supply: sysfs: use power_supply_property_is_writeable()
>        power: supply: core: avoid iterating properties directly
>        power: supply: core: implement extension API
>        power: supply: core: add locking around extension access
>        power: supply: test-power: implement a power supply extension
>        platform/x86: system76: Use power_supply extension API
>
>   drivers/platform/x86/system76_acpi.c      |  83 +++++++++---------
>   drivers/power/supply/power_supply.h       |   9 ++
>   drivers/power/supply/power_supply_core.c  | 136 ++++++++++++++++++++++++++++--
>   drivers/power/supply/power_supply_hwmon.c |  48 +++++------
>   drivers/power/supply/power_supply_sysfs.c |  39 ++++++---
>   drivers/power/supply/test_power.c         | 102 ++++++++++++++++++++++
>   include/linux/power_supply.h              |  25 ++++++
>   7 files changed, 357 insertions(+), 85 deletions(-)
> ---
> base-commit: 2df0193e62cf887f373995fb8a91068562784adc
> change-id: 20240602-power-supply-extensions-07d949f509d9
>
> Best regards,

On 2024-06-07 01:10:02+0000, Armin Wolf wrote:
> Am 06.06.24 um 16:50 schrieb Thomas Weißschuh:
> 
> > Introduce a mechanism for drivers to extend the properties implemented
> > by a power supply.
> > 
> > Motivation
> > ----------
> > 
> > Various drivers, mostly in platform/x86 extend the ACPI battery driver
> > with additional sysfs attributes to implement more UAPIs than are
> > exposed through ACPI by using various side-channels, like WMI,
> > nonstandard ACPI or EC communication.
> > 
> > While the created sysfs attributes look similar to the attributes
> > provided by the powersupply core, there are various deficiencies:
> > 
> > * They don't show up in uevent payload.
> > * They can't be queried with the standard in-kernel APIs.
> > * They don't work with triggers.
> > * The extending driver has to reimplement all of the parsing,
> >    formatting and sysfs display logic.
> > * Writing a extension driver is completely different from writing a
> >    normal power supply driver.
> > * Properties can not be properly overriden.
> > 
> > The proposed extension API avoids all of these issues.
> > An extension is just a "struct power_supply_ext" with the same kind of
> > callbacks as in a normal "struct power_supply_desc".
> > 
> > The API is meant to be used via battery_hook_register(), the same way as
> > the current extensions.
> > 
> > For example my upcoming cros_ec charge control driver[0] saves 80 lines
> > of code with this patchset.
> > 
> > Contents
> > --------
> > 
> > * Patch 1 and 2 are generic preparation patches, that probably make
> >    sense without this series.
> > * Patch 3 implements the extension API itself.
> > * Patch 4 implements a PoC locking scheme for the extension API.
> > * Patch 5 adds extension support to test_power.c
> > * Patch 6 converts the in-tree platform/x86/system76 driver to the
> >    extension API.
> > 
> > Open issues
> > -----------
> > 
> > * Newly registered properties will not show up in hwmon.
> >    To do that properly would require some changes in the hwmon core.
> >    As far as I know, no current driver would extend the hwmon properties anyways.
> > * As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
> >    it also be gated behind this or another config?
> > * Only one extension can be used at a time.
> >    So far this should be enough, having more would complicate the
> >    implementation.
> > * Is an rw_semaphore acceptable?
> > 
> > [0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/
> 
> Nice, i love this proposal!

Good to hear!

> I agree that the hwmon update functionality will need some changes in the hwmon core to work,
> but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
> support for this at a later point in time.

Surely. Alternatively we could re-register the hwmon device after an
extension was added.

> The possibility of registering multiple power supply extensions on a single power supply will
> be necessary to support battery charge control on Dell notebooks in the future. This is because
> there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
> dell-laptop (when support for battery charge control is supported someday).
> 
> How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
> this later when the need arises.

It's not really difficult. The problem is in the callback functions
going from a 'struct power_supply' back to the correct extension struct
for use with container_of() to access the drivers private data.

But we can add a marker member to 'struct power_supply_ext' with which
the callback can figure out which of the registered extensions is its
own. Something like "led_hw_trigger_type" in the LED subsystem.

And some documentation about how conflicts are to be resolved.

Thomas

> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > Thomas Weißschuh (6):
> >        power: supply: sysfs: use power_supply_property_is_writeable()
> >        power: supply: core: avoid iterating properties directly
> >        power: supply: core: implement extension API
> >        power: supply: core: add locking around extension access
> >        power: supply: test-power: implement a power supply extension
> >        platform/x86: system76: Use power_supply extension API
> > 
> >   drivers/platform/x86/system76_acpi.c      |  83 +++++++++---------
> >   drivers/power/supply/power_supply.h       |   9 ++
> >   drivers/power/supply/power_supply_core.c  | 136 ++++++++++++++++++++++++++++--
> >   drivers/power/supply/power_supply_hwmon.c |  48 +++++------
> >   drivers/power/supply/power_supply_sysfs.c |  39 ++++++---
> >   drivers/power/supply/test_power.c         | 102 ++++++++++++++++++++++
> >   include/linux/power_supply.h              |  25 ++++++
> >   7 files changed, 357 insertions(+), 85 deletions(-)
> > ---
> > base-commit: 2df0193e62cf887f373995fb8a91068562784adc
> > change-id: 20240602-power-supply-extensions-07d949f509d9

Am 07.06.24 um 12:26 schrieb Thomas Weißschuh:

> On 2024-06-07 01:10:02+0000, Armin Wolf wrote:
>> Am 06.06.24 um 16:50 schrieb Thomas Weißschuh:
>>
>>> Introduce a mechanism for drivers to extend the properties implemented
>>> by a power supply.
>>>
>>> Motivation
>>> ----------
>>>
>>> Various drivers, mostly in platform/x86 extend the ACPI battery driver
>>> with additional sysfs attributes to implement more UAPIs than are
>>> exposed through ACPI by using various side-channels, like WMI,
>>> nonstandard ACPI or EC communication.
>>>
>>> While the created sysfs attributes look similar to the attributes
>>> provided by the powersupply core, there are various deficiencies:
>>>
>>> * They don't show up in uevent payload.
>>> * They can't be queried with the standard in-kernel APIs.
>>> * They don't work with triggers.
>>> * The extending driver has to reimplement all of the parsing,
>>>     formatting and sysfs display logic.
>>> * Writing a extension driver is completely different from writing a
>>>     normal power supply driver.
>>> * Properties can not be properly overriden.
>>>
>>> The proposed extension API avoids all of these issues.
>>> An extension is just a "struct power_supply_ext" with the same kind of
>>> callbacks as in a normal "struct power_supply_desc".
>>>
>>> The API is meant to be used via battery_hook_register(), the same way as
>>> the current extensions.
>>>
>>> For example my upcoming cros_ec charge control driver[0] saves 80 lines
>>> of code with this patchset.
>>>
>>> Contents
>>> --------
>>>
>>> * Patch 1 and 2 are generic preparation patches, that probably make
>>>     sense without this series.
>>> * Patch 3 implements the extension API itself.
>>> * Patch 4 implements a PoC locking scheme for the extension API.
>>> * Patch 5 adds extension support to test_power.c
>>> * Patch 6 converts the in-tree platform/x86/system76 driver to the
>>>     extension API.
>>>
>>> Open issues
>>> -----------
>>>
>>> * Newly registered properties will not show up in hwmon.
>>>     To do that properly would require some changes in the hwmon core.
>>>     As far as I know, no current driver would extend the hwmon properties anyways.
>>> * As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
>>>     it also be gated behind this or another config?
>>> * Only one extension can be used at a time.
>>>     So far this should be enough, having more would complicate the
>>>     implementation.
>>> * Is an rw_semaphore acceptable?
>>>
>>> [0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/
>> Nice, i love this proposal!
> Good to hear!
>
>> I agree that the hwmon update functionality will need some changes in the hwmon core to work,
>> but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
>> support for this at a later point in time.
> Surely. Alternatively we could re-register the hwmon device after an
> extension was added.
>
>> The possibility of registering multiple power supply extensions on a single power supply will
>> be necessary to support battery charge control on Dell notebooks in the future. This is because
>> there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
>> dell-laptop (when support for battery charge control is supported someday).
>>
>> How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
>> this later when the need arises.
> It's not really difficult. The problem is in the callback functions
> going from a 'struct power_supply' back to the correct extension struct
> for use with container_of() to access the drivers private data.
>
> But we can add a marker member to 'struct power_supply_ext' with which
> the callback can figure out which of the registered extensions is its
> own. Something like "led_hw_trigger_type" in the LED subsystem.

Maybe we can do the same thing as the battery hook API and just pass a pointer to
the power_supply_ext instance to the callbacks. They then can use container_of()
to access the drivers private data if the struct power_supply_ext is embedded
inside the private data struct.

>
> And some documentation about how conflicts are to be resolved.
>
> Thomas

Sound like a plan, i suggest that extensions be prevented from registering with
a power supply containing conflicting properties or containing extensions with
conflicting properties.

As a side note, maybe there is a way to make power_supply_update_groups() available
for other power supply drivers? Afaik the ACPI battery driver would benefit from this too.

Thanks,
Armin Wolf

>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>> Thomas Weißschuh (6):
>>>         power: supply: sysfs: use power_supply_property_is_writeable()
>>>         power: supply: core: avoid iterating properties directly
>>>         power: supply: core: implement extension API
>>>         power: supply: core: add locking around extension access
>>>         power: supply: test-power: implement a power supply extension
>>>         platform/x86: system76: Use power_supply extension API
>>>
>>>    drivers/platform/x86/system76_acpi.c      |  83 +++++++++---------
>>>    drivers/power/supply/power_supply.h       |   9 ++
>>>    drivers/power/supply/power_supply_core.c  | 136 ++++++++++++++++++++++++++++--
>>>    drivers/power/supply/power_supply_hwmon.c |  48 +++++------
>>>    drivers/power/supply/power_supply_sysfs.c |  39 ++++++---
>>>    drivers/power/supply/test_power.c         | 102 ++++++++++++++++++++++
>>>    include/linux/power_supply.h              |  25 ++++++
>>>    7 files changed, 357 insertions(+), 85 deletions(-)
>>> ---
>>> base-commit: 2df0193e62cf887f373995fb8a91068562784adc
>>> change-id: 20240602-power-supply-extensions-07d949f509d9

On 2024-06-08 18:27:07+0000, Armin Wolf wrote:
> Am 07.06.24 um 12:26 schrieb Thomas Weißschuh:
> 
> > On 2024-06-07 01:10:02+0000, Armin Wolf wrote:
> > > Am 06.06.24 um 16:50 schrieb Thomas Weißschuh:
> > > 
> > > > Introduce a mechanism for drivers to extend the properties implemented
> > > > by a power supply.

<snip>

> > > > 
> > > > [0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/
> > > Nice, i love this proposal!
> > Good to hear!
> > 
> > > I agree that the hwmon update functionality will need some changes in the hwmon core to work,
> > > but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
> > > support for this at a later point in time.
> > Surely. Alternatively we could re-register the hwmon device after an
> > extension was added.
> > 
> > > The possibility of registering multiple power supply extensions on a single power supply will
> > > be necessary to support battery charge control on Dell notebooks in the future. This is because
> > > there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
> > > dell-laptop (when support for battery charge control is supported someday).
> > > 
> > > How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
> > > this later when the need arises.
> > It's not really difficult. The problem is in the callback functions
> > going from a 'struct power_supply' back to the correct extension struct
> > for use with container_of() to access the drivers private data.
> > 
> > But we can add a marker member to 'struct power_supply_ext' with which
> > the callback can figure out which of the registered extensions is its
> > own. Something like "led_hw_trigger_type" in the LED subsystem.
> 
> Maybe we can do the same thing as the battery hook API and just pass a pointer to
> the power_supply_ext instance to the callbacks. They then can use container_of()
> to access the drivers private data if the struct power_supply_ext is embedded
> inside the private data struct.

That indeed sounds like the obvious thing to do.
I tried very hard to keep the callback signatures exactly the same as in
power_supply_desc and didn't even see this possibility.

> 
> > 
> > And some documentation about how conflicts are to be resolved.
> > 
> > Thomas
> 
> Sound like a plan, i suggest that extensions be prevented from registering with
> a power supply containing conflicting properties or containing extensions with
> conflicting properties.

Ack.

> As a side note, maybe there is a way to make power_supply_update_groups() available
> for other power supply drivers? Afaik the ACPI battery driver would benefit from this too.

I'll take a look and spin that into its own series.
Or as you seem to know that driver better, I'd be happy if you did.

> Thanks,
> Armin Wolf
> 
> > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > ---
> > > > Thomas Weißschuh (6):
> > > >         power: supply: sysfs: use power_supply_property_is_writeable()
> > > >         power: supply: core: avoid iterating properties directly
> > > >         power: supply: core: implement extension API
> > > >         power: supply: core: add locking around extension access
> > > >         power: supply: test-power: implement a power supply extension
> > > >         platform/x86: system76: Use power_supply extension API
> > > > 
> > > >    drivers/platform/x86/system76_acpi.c      |  83 +++++++++---------
> > > >    drivers/power/supply/power_supply.h       |   9 ++
> > > >    drivers/power/supply/power_supply_core.c  | 136 ++++++++++++++++++++++++++++--
> > > >    drivers/power/supply/power_supply_hwmon.c |  48 +++++------
> > > >    drivers/power/supply/power_supply_sysfs.c |  39 ++++++---
> > > >    drivers/power/supply/test_power.c         | 102 ++++++++++++++++++++++
> > > >    include/linux/power_supply.h              |  25 ++++++
> > > >    7 files changed, 357 insertions(+), 85 deletions(-)
> > > > ---
> > > > base-commit: 2df0193e62cf887f373995fb8a91068562784adc
> > > > change-id: 20240602-power-supply-extensions-07d949f509d9