[PATCH v5] ACPI: battery: Protect all properties with a separated mutex
From: Rong Zhang
Date: Sat Aug 08 2026 - 19:44:36 EST
The acpi_battery_get_property() callback calls acpi_battery_get_state()
without any lock held. On some devices, it happens that the property
cache has expired before a uevent reaches userspace, triggering
simultaneous attempts to evaluate _BST. See [1] for an analysis to sysrq
stacktraces on one of the these devices.
In a few cases, including when the AML is sleeping or acquiring a mutex,
ACPICA drops the namespace and interpreter locks and allows the
evaluation of _BST to start while another task is still evaluating it.
This could somehow confuse the interpreter and lead to chaos in AML
mutexes on some devices, see [2] for an example.
Not holding the lock is also prone to race conditions, for example:
CPU0 | CPU1
acpi_battery_get_property() |
acpi_battery_get_state() |
[update_time expired] |
extract_package() | acpi_battery_get_property()
battery->update_time = jiffies | acpi_battery_get_state()
kfree() | [up to date]
| [read capacity_now]
[fix capacity_now due to quirk] |
where CPU1 gets raw capacity_now before CPU0 fixes it to a meaningful
value.
The existing mutex update_lock is not applicapable for
acpi_battery_get_property(), as some code path could call or wait for
acpi_battery_get_property() while holding update_lock.
Therefore, introduce a mutex called property_lock to protect all
accesses to battery properties, so that acpi_battery_get_property() can
take the advantage of the mutex and synchronize itself. With the mutex,
acpi_battery_get_state() are synchronized in all code paths calling it,
and its cache mechanism can always clamp the frequency of _BST
evaluations according to cache_time.
The helper function acpi_battery_handle_discharging() for quirky devices
has to be inlined due to the change, as the mutex must be unlocked
before calling the expensive power_supply_is_system_supplied() helper
function.
Fixes: 86bfd21a0baf ("ACPI: battery: Drop redundant locking")
Tested-by: Avraham Hollander <anhollander516@xxxxxxxxx>
Reported-by: Rick <rickk1166@xxxxxxxxx>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221065#c85 [1]
Reported-by: Avraham Hollander <anhollander516@xxxxxxxxx>
Closes: https://lore.kernel.org/linux-acpi/CAP1mzZReJCn6df5DwEPu-JCQUyr=Pu1cg5xKCMttWZkHCQtVmQ@xxxxxxxxxxxxxx [2]
Signed-off-by: Rong Zhang <i@xxxxxxxx>
---
Changes in v5:
- Reword commit message (thanks Rafael J. Wysocki)
- Rebase onto linux-pm after other patches in the series have been
applied
- Link to v4: https://patch.msgid.link/20260718-b4-acpi-battery-notification-v4-0-599c8ed1072f@xxxxxxxx
Changes in v4:
- Rebase and adopt devres-based resource management
- Refactor acpi_battery_notify() to hold the mutex across the entire
function to improve readability and drop unnecessary variables (thanks
Rafael J. Wysocki)
- Link to v3: https://patch.msgid.link/20260611-b4-acpi-battery-notification-v3-0-f9390382c5a4@xxxxxxxx
Changes in v3:
- Address Sashiko's concerns on my last-minute changes:
- Set the number base to 10 in order not to break the ABI
- Do not overwrite the initial value of `ret' in
acpi_battery_get_property()
- https://sashiko.dev/#/patchset/20260611-b4-acpi-battery-notification-v2-0-4e8ed651a151%40rong.moe
- Link to v2: https://patch.msgid.link/20260611-b4-acpi-battery-notification-v2-0-4e8ed651a151@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
- Use a separated mutex to protect all properties instead of reusing
update_lock
- https://sashiko.dev/#/patchset/20260527-b4-acpi-battery-notification-v1-0-2303bed8ec0b%40rong.moe
- Minimalize the critical section of acpi_battery_notify()
- Rearrange the series
- Dropped Tested-by from patch 3 due to massive rewrite
- Link to v1: https://patch.msgid.link/20260527-b4-acpi-battery-notification-v1-0-2303bed8ec0b@xxxxxxxx
---
drivers/acpi/battery.c | 147 +++++++++++++++++++++++++++++++++----------------
1 file changed, 101 insertions(+), 46 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 0084f308b790..670853ec3a4d 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/kfifo.h>
#include <linux/list.h>
+#include <linux/lockdep.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/platform_device.h>
@@ -105,6 +106,9 @@ struct acpi_battery {
struct delayed_work acpi_notif_dwork;
struct notifier_block pm_nb;
struct list_head list;
+ unsigned long flags;
+
+ struct mutex property_lock; /* Protects properties below. */
unsigned long update_time;
int revision;
int rate_now;
@@ -131,7 +135,6 @@ struct acpi_battery {
char oem_info[MAX_STRING_LENGTH];
int state;
int power_unit;
- unsigned long flags;
};
#define to_acpi_battery(x) power_supply_get_drvdata(x)
@@ -189,20 +192,6 @@ static bool acpi_battery_is_degraded(struct acpi_battery *battery)
battery->full_charge_capacity < battery->design_capacity;
}
-static int acpi_battery_handle_discharging(struct acpi_battery *battery)
-{
- /*
- * Some devices wrongly report discharging if the battery's charge level
- * was above the device's start charging threshold atm the AC adapter
- * was plugged in and the device thus did not start a new charge cycle.
- */
- if ((battery_ac_is_broken || power_supply_is_system_supplied()) &&
- battery->rate_now == 0)
- return POWER_SUPPLY_STATUS_NOT_CHARGING;
-
- return POWER_SUPPLY_STATUS_DISCHARGING;
-}
-
static int acpi_battery_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
@@ -210,15 +199,41 @@ static int acpi_battery_get_property(struct power_supply *psy,
int full_capacity = ACPI_BATTERY_VALUE_UNKNOWN, ret = 0;
struct acpi_battery *battery = to_acpi_battery(psy);
- if (acpi_battery_present(battery)) {
- /* run battery update only if it is present */
- acpi_battery_get_state(battery);
- } else if (psp != POWER_SUPPLY_PROP_PRESENT)
- return -ENODEV;
+ /* run battery update only if it is present */
+ if (!acpi_battery_present(battery)) {
+ switch (psp) {
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = 0;
+ return 0;
+ default:
+ return -ENODEV;
+ }
+ }
+
+ mutex_lock(&battery->property_lock);
+
+ acpi_battery_get_state(battery);
+
switch (psp) {
case POWER_SUPPLY_PROP_STATUS:
+ /*
+ * Some devices wrongly report discharging if the battery's charge level
+ * was above the device's start charging threshold atm the AC adapter
+ * was plugged in and the device thus did not start a new charge cycle.
+ */
if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
- val->intval = acpi_battery_handle_discharging(battery);
+ if (battery->rate_now != 0) {
+ val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ } else if (battery_ac_is_broken) {
+ val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ } else {
+ mutex_unlock(&battery->property_lock);
+
+ val->intval = power_supply_is_system_supplied()
+ ? POWER_SUPPLY_STATUS_NOT_CHARGING
+ : POWER_SUPPLY_STATUS_DISCHARGING;
+ return 0;
+ }
else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
/* Check the rate and capacity to validate the status. */
if (!acpi_battery_is_full(battery) ||
@@ -321,6 +336,8 @@ static int acpi_battery_get_property(struct power_supply *psy,
default:
ret = -EINVAL;
}
+
+ mutex_unlock(&battery->property_lock);
return ret;
}
@@ -556,6 +573,8 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
int use_bix;
int result = -ENODEV;
+ lockdep_assert_held(&battery->property_lock);
+
if (!acpi_battery_present(battery))
return 0;
@@ -595,6 +614,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
acpi_status status = 0;
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ lockdep_assert_held(&battery->property_lock);
+
if (!acpi_battery_present(battery))
return 0;
@@ -648,6 +669,8 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
{
acpi_status status = 0;
+ lockdep_assert_held(&battery->property_lock);
+
if (!acpi_battery_present(battery) ||
!test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags))
return -ENODEV;
@@ -665,6 +688,8 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
static int acpi_battery_init_alarm(struct acpi_battery *battery)
{
+ lockdep_assert_held(&battery->property_lock);
+
/* See if alarms are supported, and if so, set default */
if (!acpi_has_method(battery->device->handle, "_BTP")) {
clear_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags);
@@ -682,6 +707,8 @@ static ssize_t acpi_battery_alarm_show(struct device *dev,
{
struct acpi_battery *battery = to_acpi_battery(dev_get_drvdata(dev));
+ guard(mutex)(&battery->property_lock);
+
return sysfs_emit(buf, "%d\n", battery->alarm * 1000);
}
@@ -697,6 +724,8 @@ static ssize_t acpi_battery_alarm_store(struct device *dev,
if (err)
return err;
+ guard(mutex)(&battery->property_lock);
+
battery->alarm = x / 1000;
if (acpi_battery_present(battery))
acpi_battery_set_alarm(battery);
@@ -881,12 +910,17 @@ static int sysfs_add_battery(struct acpi_battery *battery)
.no_wakeup_source = true,
};
bool full_cap_broken = false;
+ int power_unit;
- if (!ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity) &&
- !ACPI_BATTERY_CAPACITY_VALID(battery->design_capacity))
- full_cap_broken = true;
+ scoped_guard(mutex, &battery->property_lock) {
+ power_unit = battery->power_unit;
- if (battery->power_unit == ACPI_BATTERY_POWER_UNIT_MA) {
+ if (!ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity) &&
+ !ACPI_BATTERY_CAPACITY_VALID(battery->design_capacity))
+ full_cap_broken = true;
+ }
+
+ if (power_unit == ACPI_BATTERY_POWER_UNIT_MA) {
if (full_cap_broken) {
battery->bat_desc.properties =
charge_battery_full_cap_broken_props;
@@ -940,6 +974,9 @@ static void sysfs_remove_battery(struct acpi_battery *battery)
static void find_battery(const struct dmi_header *dm, void *private)
{
struct acpi_battery *battery = (struct acpi_battery *)private;
+
+ lockdep_assert_held(&battery->property_lock);
+
/* Note: the hardcoded offsets below have been extracted from
* the source code of dmidecode.
*/
@@ -971,6 +1008,8 @@ static void find_battery(const struct dmi_header *dm, void *private)
*/
static void acpi_battery_quirks(struct acpi_battery *battery)
{
+ lockdep_assert_held(&battery->property_lock);
+
if (test_bit(ACPI_BATTERY_QUIRK_PERCENTAGE_CAPACITY, &battery->flags))
return;
@@ -1023,30 +1062,38 @@ static void acpi_battery_quirks(struct acpi_battery *battery)
static int acpi_battery_update(struct acpi_battery *battery, bool resume)
{
int result = acpi_battery_get_status(battery);
+ bool wakeup;
if (result)
return result;
if (!acpi_battery_present(battery)) {
sysfs_remove_battery(battery);
- battery->update_time = 0;
+ scoped_guard(mutex, &battery->property_lock)
+ battery->update_time = 0;
return 0;
}
if (resume)
return 0;
- if (!battery->update_time) {
- result = acpi_battery_get_info(battery);
+ scoped_guard(mutex, &battery->property_lock) {
+ if (!battery->update_time) {
+ result = acpi_battery_get_info(battery);
+ if (result)
+ return result;
+ acpi_battery_init_alarm(battery);
+ }
+
+ result = acpi_battery_get_state(battery);
if (result)
return result;
- acpi_battery_init_alarm(battery);
- }
+ acpi_battery_quirks(battery);
- result = acpi_battery_get_state(battery);
- if (result)
- return result;
- acpi_battery_quirks(battery);
+ wakeup = ((battery->state & ACPI_BATTERY_STATE_CRITICAL) ||
+ (test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags) &&
+ (battery->capacity_now <= battery->alarm)));
+ }
if (!battery->bat) {
result = sysfs_add_battery(battery);
@@ -1058,9 +1105,7 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
* Wakeup the system if battery is critical low
* or lower than the alarm level
*/
- if ((battery->state & ACPI_BATTERY_STATE_CRITICAL) ||
- (test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags) &&
- (battery->capacity_now <= battery->alarm)))
+ if (wakeup)
acpi_pm_wakeup_event(battery->phys_dev);
return result;
@@ -1073,12 +1118,14 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
if (!battery->bat)
return;
- power_unit = battery->power_unit;
+ scoped_guard(mutex, &battery->property_lock) {
+ power_unit = battery->power_unit;
- acpi_battery_get_info(battery);
+ acpi_battery_get_info(battery);
- if (power_unit == battery->power_unit)
- return;
+ if (power_unit == battery->power_unit)
+ return;
+ }
/* The battery has changed its reporting units. */
sysfs_remove_battery(battery);
@@ -1170,17 +1217,21 @@ static int battery_notify(struct notifier_block *nb,
} else {
int result;
- result = acpi_battery_get_info(battery);
- if (result)
- return result;
+ scoped_guard(mutex, &battery->property_lock) {
+ result = acpi_battery_get_info(battery);
+ if (result)
+ return result;
+ }
result = sysfs_add_battery(battery);
if (result)
return result;
}
- acpi_battery_init_alarm(battery);
- acpi_battery_get_state(battery);
+ scoped_guard(mutex, &battery->property_lock) {
+ acpi_battery_init_alarm(battery);
+ acpi_battery_get_state(battery);
+ }
}
return 0;
@@ -1345,6 +1396,10 @@ static int acpi_battery_probe(struct platform_device *pdev)
if (result)
return result;
+ result = devm_mutex_init(&pdev->dev, &battery->property_lock);
+ if (result)
+ return result;
+
if (acpi_has_method(battery->device->handle, "_BIX"))
set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
---
base-commit: 92ec461acad4a92722634aafab38cfd3236e884d
change-id: 20260520-b4-acpi-battery-notification-90d781a3f217
Thanks,
Rong