Re: [PATCH v3 1/3] ACPI: battery: Merge consecutive battery notifications

[Rafael J. Wysocki (Intel)]

On Wed, Jun 10, 2026 at 10:11 PM Rong Zhang <i@xxxxxxxx> wrote:
>
> It's a very common pattern to emit consecutive battery notifications,
> for example:
>
>     Method (_Qxx, 0, NotSerialized)
>     {
>         Notify (BAT0, 0x80) // Status Change
>         Notify (BAT0, 0x81) // Information Change
>     }
>
> In this case, the current code path will update battery state twice
> within a short period, which is not optimal, as the same data are
> fetched twice. Moreover, both notifications are likely to call
> power_supply_changed(), causing power_supply_uevent() to read all
> battery properties in order to assemble uevents. Even worse, after the
> first uevent reaches userspace, some userspace processes start to read
> all battery properties in order to refresh their internal states, which
> competes with the second notification's handling and uevent assembling.
>
> This generates significant pressure on _STA, _BST and _BIX/_BIF methods.
> Not only that, power_supply_ext properties may also rely on some other
> ACPI methods, so both uevent assembling and userspace processes call
> them. It becomes a nightmare when all these methods share the same ACPI
> mutex protecting EC accesses and hence vulnerable to lock starvation.
> This is exactly the case of some Lenovo devices, where the mentioned EC
> query pattern eventually leads to a catastrophic situation that a bunch
> of ACPI methods (including but not limited to the mentioned ones) fail
> to acquire the same mutex due to timeout. These devices don't handle
> mutex acquisition failure gracefully and return garbage data, causing
> even more chaos.
>
> Improve battery notification handling by merging consecutive battery
> notifications within 10ms using a delayed work, so that they only
> refresh and/or update battery state once. ACPI netlink event and
> notifier call chain are still triggered multiple times in order not to
> break other components. Finally, call power_supply_changed() once and
> lead to a single uevent instead of a bunch, preventing userspace
> programs from causing too much pressure on power supply properties and
> underlying ACPI methods.
>
> Tested-by: Jeffrey Wälti <jeffrey@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Rick <rickk1166@xxxxxxxxx>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221065
> Signed-off-by: Rong Zhang <i@xxxxxxxx>
> ---
> Changes in v2:
> - Address Sashiko's concerns:
>   - Return from acpi_battery_notification_worker() early when the fifo
>     is empty
>   - Use pr_err_ratelimited() for potential event storms
>   - Add missing `\n' in a printk message
>   - https://sashiko.dev/#/patchset/20260527-b4-acpi-battery-notification-v1-0-2303bed8ec0b%40rong.moe
> - Minimalize the critical section of acpi_battery_notify()
> ---
>  drivers/acpi/battery.c | 80 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index b82dd67d98c9..5f476c074c68 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -14,6 +14,7 @@
>  #include <linux/dmi.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
> +#include <linux/kfifo.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -21,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
>
>  #include <linux/unaligned.h>
>
> @@ -43,6 +45,9 @@
>
>  #define MAX_STRING_LENGTH      64
>
> +#define MAX_QUEUED_EVENTS      16
> +#define NOTIF_MERGING_MS       10
> +
>  MODULE_AUTHOR("Paul Diefenbaugh");
>  MODULE_AUTHOR("Alexey Starikovskiy <astarikovskiy@xxxxxxx>");
>  MODULE_DESCRIPTION("ACPI Battery Driver");
> @@ -95,6 +100,8 @@ struct acpi_battery {
>         struct power_supply_desc bat_desc;
>         struct acpi_device *device;
>         struct device *phys_dev;
> +       struct kfifo acpi_notif_fifo;
> +       struct delayed_work acpi_notif_dwork;
>         struct notifier_block pm_nb;
>         struct list_head list;
>         unsigned long update_time;
> @@ -1059,14 +1066,24 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
>  }
>
>  /* Driver Interface */
> -static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
> +static void acpi_battery_notification_worker(struct work_struct *work)
>  {
> -       struct acpi_battery *battery = data;
> +       struct acpi_battery *battery = container_of(work, struct acpi_battery,
> +                                                   acpi_notif_dwork.work);
>         struct acpi_device *device = battery->device;
> +       u32 events[MAX_QUEUED_EVENTS];
>         struct power_supply *old;
> +       unsigned int count, i;
>
>         guard(mutex)(&battery->update_lock);
>
> +       count = kfifo_out(&battery->acpi_notif_fifo, events, sizeof(events));
> +       count /= sizeof(events[0]);
> +       if (!count)
> +               return;
> +
> +       pr_debug("merged %u battery notifications within %dms\n", count, NOTIF_MERGING_MS);
> +
>         old = battery->bat;
>         /*
>          * On Acer Aspire V5-573G notifications are sometimes triggered too
> @@ -1076,19 +1093,50 @@ static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
>          */
>         if (battery_notification_delay_ms > 0)
>                 msleep(battery_notification_delay_ms);
> -       if (event == ACPI_BATTERY_NOTIFY_INFO)
> -               acpi_battery_refresh(battery);
> +
> +       for (i = 0; i < count; i++) {
> +               if (events[i] == ACPI_BATTERY_NOTIFY_INFO) {
> +                       acpi_battery_refresh(battery);
> +                       break;
> +               }
> +       }
> +
>         acpi_battery_update(battery, false);
> -       acpi_bus_generate_netlink_event(ACPI_BATTERY_CLASS,
> -                                       dev_name(&device->dev), event,
> -                                       acpi_battery_present(battery));
> -       acpi_notifier_call_chain(ACPI_BATTERY_CLASS, acpi_device_bid(device),
> -                                event, acpi_battery_present(battery));
> +
> +       for (i = 0; i < count; i++) {
> +               acpi_bus_generate_netlink_event(ACPI_BATTERY_CLASS,
> +                                               dev_name(&device->dev), events[i],
> +                                               acpi_battery_present(battery));
> +               acpi_notifier_call_chain(ACPI_BATTERY_CLASS, acpi_device_bid(device),
> +                                        events[i], acpi_battery_present(battery));
> +       }
> +
>         /* acpi_battery_update could remove power_supply object */
>         if (old && battery->bat)
>                 power_supply_changed(battery->bat);
>  }
>
> +static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
> +{
> +       struct acpi_battery *battery = data;
> +       bool queued = false;
> +
> +       scoped_guard(mutex, &battery->update_lock) {
> +               if (kfifo_avail(&battery->acpi_notif_fifo) >= sizeof(event)) {
> +                       kfifo_in(&battery->acpi_notif_fifo, &event, sizeof(event));
> +                       queued = true;
> +               }
> +       }
> +
> +       if (queued) {
> +               schedule_delayed_work(&battery->acpi_notif_dwork,
> +                                     msecs_to_jiffies(NOTIF_MERGING_MS));

Why can't this be done under update_lock?

It looks like the lock could be held across the entire function and
the "queued" variable could be dropped.

> +       } else {
> +               pr_err_ratelimited("too many battery notifications within %dms\n",
> +                                  NOTIF_MERGING_MS);
> +       }
> +}
> +
>  static int battery_notify(struct notifier_block *nb,
>                           unsigned long mode, void *_unused)
>  {
> @@ -1256,13 +1304,22 @@ static int acpi_battery_probe(struct platform_device *pdev)
>
>         device_init_wakeup(&pdev->dev, true);
>
> +       result = kfifo_alloc(&battery->acpi_notif_fifo,
> +                            MAX_QUEUED_EVENTS * sizeof(u32), GFP_KERNEL);
> +       if (result)
> +               goto fail_pm;
> +
> +       INIT_DELAYED_WORK(&battery->acpi_notif_dwork, acpi_battery_notification_worker);
> +
>         result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY,
>                                                  acpi_battery_notify, battery);
>         if (result)
> -               goto fail_pm;
> +               goto fail_kfifo;
>
>         return 0;
>
> +fail_kfifo:
> +       kfifo_free(&battery->acpi_notif_fifo);
>  fail_pm:
>         device_init_wakeup(&pdev->dev, false);
>         unregister_pm_notifier(&battery->pm_nb);
> @@ -1279,6 +1336,9 @@ static void acpi_battery_remove(struct platform_device *pdev)
>         acpi_dev_remove_notify_handler(battery->device, ACPI_ALL_NOTIFY,
>                                        acpi_battery_notify);
>
> +       cancel_delayed_work_sync(&battery->acpi_notif_dwork);
> +       kfifo_free(&battery->acpi_notif_fifo);
> +
>         device_init_wakeup(&pdev->dev, false);
>         unregister_pm_notifier(&battery->pm_nb);
>
>
> --
> 2.53.0
>