[PATCH 1/2] ACPI: battery: Synchronize get_property() callback

[Rong Zhang]

The acpi_battery_get_property() callback calls acpi_battery_get_state()
without battery->update_lock held, which could lead to race conditions,
e.g., when multiple tasks read power supply properties simultaneously,
or when other synchronized methods are called during its execution.

Moreover, some devices' _BST method relies on a heavily shared ACPI
mutex which protects EC accesses, so it cannot tolerate too much
pressure or else other methods will time out. The lack of
synchronization sometimes nullifies the cache mechanism of
acpi_battery_get_state() when multiple processes read power supply
properties simultaneously, which usually happens after a uevent.
Normally, emitting a uevent implies that the cache must have been
refreshed due to power_supply_uevent() reading all properties, so the
mentioned processes should have seen cache hits. Unfortunately, these
fragile devices' power_supply_ext properties are somehow slow to read
after battery events, resulting in cache expiration before
power_supply_uevent() finishes. Hence, once the uevent reaches
userspace, the _BST method will be executed multiple times within a
short period due to userspace processes reading all properties again.
The coincidence causes lock starvation, resulting in a catastrophic
situation that a lot of ACPI methods fail to acquire the shared ACPI
mutex due to timeout and return garbage data thanks to the firmware's
poorly designed error paths.

Protect acpi_battery_get_property() with update_lock to synchronize it.
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.

Tested-by: Jeffrey Wälti <jeffrey@xxxxxxxxxx>
Fixes: 399dbcadc01e ("ACPI: battery: Add synchronization between interface updates")
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>
---
 drivers/acpi/battery.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index b82dd67d98c9..5f06841b48a1 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -180,20 +180,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)
@@ -201,15 +187,35 @@ 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);
 
+	mutex_lock(&battery->update_lock);
+
 	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)
+	} else if (psp != POWER_SUPPLY_PROP_PRESENT) {
+		mutex_unlock(&battery->update_lock);
 		return -ENODEV;
+	}
 	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->update_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)
 			/* Validate the status by checking the current. */
 			if (battery->rate_now != ACPI_BATTERY_VALUE_UNKNOWN &&
@@ -311,6 +317,8 @@ static int acpi_battery_get_property(struct power_supply *psy,
 	default:
 		ret = -EINVAL;
 	}
+
+	mutex_unlock(&battery->update_lock);
 	return ret;
 }
 

-- 
2.53.0