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

[Rong Zhang]

Hi Rafael,

于 2026年7月7日 GMT+08:00 02:22:19，"Rafael J. Wysocki (Intel)" <rafael@xxxxxxxxxx> 写道：
>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.

Thanks for your review!

I originally would like to minimize the critical section here, thus schedule_delayed_work() was moved out.

It also makes sense to move it back for readability and simplicity. I will resubmit a v4 with the change.

Thanks,
Rong

>
>> +       } 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
>>