Re: [PATCH v2 1/2] ACPI: battery: add unified battery hook mechanism for ACPI and SBS batteries

[Rafael J. Wysocki (Intel)]

On Fri, Sep 18, 2026 at 7:51 PM Jordan Brough <jordan@xxxxxxxxxx> wrote:
>
> drivers/acpi/battery.c provides a battery_hook_register()/unregister()
> mechanism that lets other drivers (e.g. platform and hwmon drivers
> exposing vendor-specific charge control) attach extra power_supply
> properties to an ACPI Control Method Battery (HID PNP0C0A).
>
> Some machines instead expose their battery through the ACPI Smart
> Battery System (HID ACPI0002, "SBS") driver in drivers/acpi/sbs.c,
> which has had no equivalent hook point.
>
> Extract the battery hooking implementation out of drivers/acpi/battery.c
> into a shared helper in drivers/acpi/battery_hook.c, built directly into
> the ACPI core (acpi.o). Both drivers/acpi/battery.c and drivers/acpi/sbs.c
> now register their power_supply devices with this shared hook
> infrastructure via battery_hook_add_battery() and
> battery_hook_remove_battery().
>
> This unifies the hooking interface across both battery drivers so
> callers only need to register a single struct acpi_battery_hook without
> needing to know or care whether the underlying hardware exposes a Control
> Method Battery or an SBS battery.
>
> Suggested-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> Signed-off-by: Jordan Brough <jordan@xxxxxxxxxx>
> ---
>  drivers/acpi/Makefile       |   1 +
>  drivers/acpi/battery.c      | 165 +-----------------------------------
>  drivers/acpi/battery_hook.c | 112 ++++++++++++++++++++++++
>  drivers/acpi/sbs.c          |   8 +-
>  include/acpi/battery.h      |  10 +++
>  5 files changed, 133 insertions(+), 163 deletions(-)
>  create mode 100644 drivers/acpi/battery_hook.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index d1b0affb844..57413f2a09e 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_ACPI)            += acpi.o \
>  # All the builtin files are in the "acpi." module_param namespace.
>  acpi-y                         += osi.o osl.o utils.o reboot.o
>  acpi-y                         += nvs.o
> +acpi-y                         += battery_hook.o

s/battery_hook/battery_hooks/

And surely it need not be compiled in without any users built at least
as modules.

>
>  # Power management related files
>  acpi-y                         += wakeup.o
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 670853ec3a4..9b6af528bfe 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -105,7 +105,7 @@ struct acpi_battery {
>         struct kfifo acpi_notif_fifo;
>         struct delayed_work acpi_notif_dwork;
>         struct notifier_block pm_nb;
> -       struct list_head list;
> +       struct acpi_battery_hook_device hook_dev;

struct acpi_battery_hooks_list_entry hooks_list_entry;

>         unsigned long flags;
>
>         struct mutex property_lock; /* Protects properties below. */
> @@ -744,164 +744,6 @@ static struct attribute *acpi_battery_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(acpi_battery);
>
> -/*
> - * The Battery Hooking API
> - *
> - * This API is used inside other drivers that need to expose
> - * platform-specific behaviour within the generic driver in a
> - * generic way.
> - *
> - */
> -
> -static LIST_HEAD(acpi_battery_list);
> -static LIST_HEAD(battery_hook_list);
> -static DEFINE_MUTEX(hook_mutex);
> -
> -static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook)
> -{
> -       struct acpi_battery *battery;
> -
> -       /*
> -        * In order to remove a hook, we first need to
> -        * de-register all the batteries that are registered.
> -        */
> -       list_for_each_entry(battery, &acpi_battery_list, list) {
> -               if (!hook->remove_battery(battery->bat, hook))
> -                       power_supply_changed(battery->bat);
> -       }
> -       list_del_init(&hook->list);
> -
> -       pr_info("hook unregistered: %s\n", hook->name);
> -}
> -
> -void battery_hook_unregister(struct acpi_battery_hook *hook)
> -{
> -       mutex_lock(&hook_mutex);
> -       /*
> -        * Ignore already unregistered battery hooks. This might happen
> -        * if a battery hook was previously unloaded due to an error when
> -        * adding a new battery.
> -        */
> -       if (!list_empty(&hook->list))
> -               battery_hook_unregister_unlocked(hook);
> -
> -       mutex_unlock(&hook_mutex);
> -}
> -EXPORT_SYMBOL_GPL(battery_hook_unregister);
> -
> -void battery_hook_register(struct acpi_battery_hook *hook)
> -{
> -       struct acpi_battery *battery;
> -
> -       mutex_lock(&hook_mutex);
> -       list_add(&hook->list, &battery_hook_list);
> -       /*
> -        * Now that the driver is registered, we need
> -        * to notify the hook that a battery is available
> -        * for each battery, so that the driver may add
> -        * its attributes.
> -        */
> -       list_for_each_entry(battery, &acpi_battery_list, list) {
> -               if (hook->add_battery(battery->bat, hook)) {
> -                       /*
> -                        * If a add-battery returns non-zero,
> -                        * the registration of the hook has failed,
> -                        * and we will not add it to the list of loaded
> -                        * hooks.
> -                        */
> -                       pr_err("hook failed to load: %s", hook->name);
> -                       battery_hook_unregister_unlocked(hook);
> -                       goto end;
> -               }
> -
> -               power_supply_changed(battery->bat);
> -       }
> -       pr_info("new hook: %s\n", hook->name);
> -end:
> -       mutex_unlock(&hook_mutex);
> -}
> -EXPORT_SYMBOL_GPL(battery_hook_register);
> -
> -static void devm_battery_hook_unregister(void *data)
> -{
> -       struct acpi_battery_hook *hook = data;
> -
> -       battery_hook_unregister(hook);
> -}
> -
> -int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook)
> -{
> -       battery_hook_register(hook);
> -
> -       return devm_add_action_or_reset(dev, devm_battery_hook_unregister, hook);
> -}
> -EXPORT_SYMBOL_GPL(devm_battery_hook_register);
> -
> -/*
> - * This function gets called right after the battery sysfs
> - * attributes have been added, so that the drivers that
> - * define custom sysfs attributes can add their own.
> - */
> -static void battery_hook_add_battery(struct acpi_battery *battery)
> -{
> -       struct acpi_battery_hook *hook_node, *tmp;
> -
> -       mutex_lock(&hook_mutex);
> -       INIT_LIST_HEAD(&battery->list);
> -       list_add(&battery->list, &acpi_battery_list);
> -       /*
> -        * Since we added a new battery to the list, we need to
> -        * iterate over the hooks and call add_battery for each
> -        * hook that was registered. This usually happens
> -        * when a battery gets hotplugged or initialized
> -        * during the battery module initialization.
> -        */
> -       list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
> -               if (hook_node->add_battery(battery->bat, hook_node)) {
> -                       /*
> -                        * The notification of the hook has failed, to
> -                        * prevent further errors we will unload the hook.
> -                        */
> -                       pr_err("error in hook, unloading: %s",
> -                                       hook_node->name);
> -                       battery_hook_unregister_unlocked(hook_node);
> -               }
> -       }
> -       mutex_unlock(&hook_mutex);
> -}
> -
> -static void battery_hook_remove_battery(struct acpi_battery *battery)
> -{
> -       struct acpi_battery_hook *hook;
> -
> -       mutex_lock(&hook_mutex);
> -       /*
> -        * Before removing the hook, we need to remove all
> -        * custom attributes from the battery.
> -        */
> -       list_for_each_entry(hook, &battery_hook_list, list) {
> -               hook->remove_battery(battery->bat, hook);
> -       }
> -       /* Then, just remove the battery from the list */
> -       list_del(&battery->list);
> -       mutex_unlock(&hook_mutex);
> -}
> -
> -static void __exit battery_hook_exit(void)
> -{
> -       struct acpi_battery_hook *hook;
> -       struct acpi_battery_hook *ptr;
> -       /*
> -        * At this point, the acpi_bus_unregister_driver()
> -        * has called remove for all batteries. We just
> -        * need to remove the hooks.
> -        */
> -       list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
> -               battery_hook_unregister(hook);
> -       }
> -       mutex_destroy(&hook_mutex);
> -}
> -
>  static int sysfs_add_battery(struct acpi_battery *battery)
>  {
>         struct power_supply_config psy_cfg = {
> @@ -957,7 +799,7 @@ static int sysfs_add_battery(struct acpi_battery *battery)
>                 battery->bat = NULL;
>                 return result;
>         }
> -       battery_hook_add_battery(battery);
> +       battery_hook_add_battery(&battery->hook_dev, battery->bat);

acpi_battery_add_hooks(&battery->hooks_list_entry, battery->bat);

>         return 0;
>  }
>
> @@ -966,7 +808,7 @@ static void sysfs_remove_battery(struct acpi_battery *battery)
>         if (!battery->bat)
>                 return;
>
> -       battery_hook_remove_battery(battery);
> +       battery_hook_remove_battery(&battery->hook_dev);

acpi_battery_remove_hooks(...)

>         power_supply_unregister(battery->bat);
>         battery->bat = NULL;
>  }
> @@ -1476,7 +1318,6 @@ static int __init acpi_battery_init(void)
>  static void __exit acpi_battery_exit(void)
>  {
>         platform_driver_unregister(&acpi_battery_driver);
> -       battery_hook_exit();
>  }
>
>  module_init(acpi_battery_init);
> diff --git a/drivers/acpi/battery_hook.c b/drivers/acpi/battery_hook.c
> new file mode 100644
> index 00000000000..308d10c940e
> --- /dev/null
> +++ b/drivers/acpi/battery_hook.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ACPI Battery Hooking API

s/Hooking/Hooks/

> + *
> + * Provides a common registration and notification framework for drivers
> + * that attach platform-specific properties or extensions to ACPI-registered
> + * batteries (both ACPI Control Method batteries and Smart Battery Systems).

"Provides helpers for registering and unregistering battery hooks for
drivers that use platform-specific extensions to ACPI-enumerated
batteries (both ACPI Control Method batteries and Smart Battery System
batteries)."

> + */
> +
> +#include <linux/device.h>
> +#include <linux/export.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/power_supply.h>
> +#include <acpi/battery.h>
> +
> +static LIST_HEAD(acpi_battery_list);
> +static LIST_HEAD(battery_hook_list);
> +static DEFINE_MUTEX(hook_mutex);
> +
> +static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook)

Please add acpi_ prefix to all function names here and below.

> +{
> +       struct acpi_battery_hook_device *hook_dev;

struct acpi_battery_hooks_list_entry *entry;

and analogously below.

> +
> +       list_for_each_entry(hook_dev, &acpi_battery_list, list) {
> +               if (!hook->remove_battery(hook_dev->battery, hook))
> +                       power_supply_changed(hook_dev->battery);
> +       }
> +       list_del_init(&hook->list);
> +
> +       pr_info("hook unregistered: %s\n", hook->name);
> +}
> +
> +void battery_hook_unregister(struct acpi_battery_hook *hook)
> +{
> +       mutex_lock(&hook_mutex);

This code is different from the code removed from battery.c, so please
use mutex locking guards in it.

> +       if (!list_empty(&hook->list))
> +               battery_hook_unregister_unlocked(hook);
> +       mutex_unlock(&hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(battery_hook_unregister);
> +
> +void battery_hook_register(struct acpi_battery_hook *hook)
> +{
> +       struct acpi_battery_hook_device *hook_dev;
> +
> +       mutex_lock(&hook_mutex);
> +       list_add(&hook->list, &battery_hook_list);
> +
> +       list_for_each_entry(hook_dev, &acpi_battery_list, list) {
> +               if (hook->add_battery(hook_dev->battery, hook)) {
> +                       pr_err("hook failed to load: %s\n", hook->name);
> +                       battery_hook_unregister_unlocked(hook);
> +                       goto end;
> +               }
> +
> +               power_supply_changed(hook_dev->battery);
> +       }
> +       pr_info("new hook: %s\n", hook->name);
> +end:
> +       mutex_unlock(&hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(battery_hook_register);
> +
> +static void devm_battery_hook_unregister(void *data)
> +{
> +       struct acpi_battery_hook *hook = data;
> +
> +       battery_hook_unregister(hook);
> +}
> +
> +int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook)

devm_acpi_battery_hook_register(...)

> +{
> +       battery_hook_register(hook);
> +
> +       return devm_add_action_or_reset(dev, devm_battery_hook_unregister, hook);
> +}
> +EXPORT_SYMBOL_GPL(devm_battery_hook_register);
> +
> +void battery_hook_add_battery(struct acpi_battery_hook_device *hook_dev,
> +                             struct power_supply *battery)

void acpi_battery_add_hooks(struct acpi_battery_hooks_list_entry *entry,

and analogously for the removal function below.

> +{
> +       struct acpi_battery_hook *hook_node, *tmp;
> +
> +       hook_dev->battery = battery;
> +       mutex_lock(&hook_mutex);
> +       INIT_LIST_HEAD(&hook_dev->list);
> +       list_add(&hook_dev->list, &acpi_battery_list);
> +
> +       list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
> +               if (hook_node->add_battery(hook_dev->battery, hook_node)) {
> +                       pr_err("error in hook, unloading: %s\n", hook_node->name);
> +                       battery_hook_unregister_unlocked(hook_node);
> +               }
> +       }
> +       mutex_unlock(&hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(battery_hook_add_battery);

Use a symbol export namespace for this.

> +
> +void battery_hook_remove_battery(struct acpi_battery_hook_device *hook_dev)
> +{
> +       struct acpi_battery_hook *hook;
> +
> +       mutex_lock(&hook_mutex);
> +       list_for_each_entry(hook, &battery_hook_list, list)
> +               hook->remove_battery(hook_dev->battery, hook);
> +
> +       list_del(&hook_dev->list);
> +       hook_dev->battery = NULL;
> +       mutex_unlock(&hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(battery_hook_remove_battery);

And for this too.

> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index 86b7c797585..a2f27c0d233 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c
> @@ -10,6 +10,7 @@
>  #define pr_fmt(fmt) "ACPI: " fmt
>
>  #include <linux/init.h>
> +#include <linux/list.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> @@ -54,6 +55,7 @@ struct acpi_battery {
>         struct power_supply *bat;
>         struct power_supply_desc bat_desc;
>         struct acpi_sbs *sbs;
> +       struct acpi_battery_hook_device hook_dev;

struct acpi_battery_hooks_list_entry hooks_list_entry;

>         unsigned long update_time;
>         char name[8];
>         char manufacturer_name[ACPI_SBS_BLOCK_MAX];
> @@ -555,6 +557,8 @@ static int acpi_battery_add(struct acpi_sbs *sbs, int id)
>                 goto end;
>         }
>
> +       battery_hook_add_battery(&battery->hook_dev, battery->bat);

acpi_battery_add_hooks(&battery->hook_list_entry, battery->bat);

> +
>        end:
>         pr_info("%s [%s]: Battery Slot [%s] (battery %s)\n",
>                ACPI_SBS_DEVICE_NAME, acpi_device_bid(sbs->device),
> @@ -566,8 +570,10 @@ static void acpi_battery_remove(struct acpi_sbs *sbs, int id)
>  {
>         struct acpi_battery *battery = &sbs->battery[id];
>
> -       if (battery->bat)
> +       if (battery->bat) {
> +               battery_hook_remove_battery(&battery->hook_dev);

acpi_battery_remove_hooks(...);

>                 power_supply_unregister(battery->bat);
> +       }
>  }
>
>  static int acpi_charger_add(struct acpi_sbs *sbs)
> diff --git a/include/acpi/battery.h b/include/acpi/battery.h
> index c93f16dfb94..df5087c0529 100644
> --- a/include/acpi/battery.h
> +++ b/include/acpi/battery.h
> @@ -3,6 +3,7 @@
>  #define __ACPI_BATTERY_H
>
>  #include <linux/device.h>
> +#include <linux/list.h>
>  #include <linux/power_supply.h>
>
>  #define ACPI_BATTERY_CLASS "battery"
> @@ -18,8 +19,17 @@ struct acpi_battery_hook {
>         struct list_head list;
>  };
>
> +struct acpi_battery_hook_device {

struct acpi_battery_hooks_list_entry {

> +       struct power_supply *battery;
> +       struct list_head list;

struct list_head list_entry;
struct power_supply *battery;

> +};
> +
>  void battery_hook_register(struct acpi_battery_hook *hook);
>  void battery_hook_unregister(struct acpi_battery_hook *hook);
>  int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
>
> +void battery_hook_add_battery(struct acpi_battery_hook_device *hook_dev,
> +                             struct power_supply *battery);
> +void battery_hook_remove_battery(struct acpi_battery_hook_device *hook_dev);
> +
>  #endif
> --

HTH