[PATCH] ACPI: battery: Re-read battery information in the complete callback

[PATCH] ACPI: battery: Re-read battery information in the complete callback

[Ali Ahmet Memiş]

Since commit d33e89d12295 ("thermal: core: Suspend thermal zones later
and resume them earlier"), thermal zones are resumed from dpm_complete(),
before the PM_POST_SUSPEND notifier that re-reads _BIX and _BST. On
ThinkPad P1 Gen2 and P53, INT3403 _TMP calls \GTST which switches the EC
page selector (HIID) without taking the BATM mutex used by the battery
methods. ACPICA releases the interpreter lock around EC accesses so the
two methods interleave and _BIX often returns temperature page data
after resume.

Evaluate _BIX and _BST in the complete callback, which runs before
dpm_complete() resumes thermal zones and leave only the sysfs
re-registration to the notifier.

Fixes: d33e89d12295 ("thermal: core: Suspend thermal zones later and resume them earlier")
Reported-by: Charis Stratakis <c.stratakis@proton.me>
Closes: https://lore.kernel.org/all/37N_jFulf7n-Z55NfKfE71C_wVPxYvZ0xD_iRUg2fXu7TypDiF5PStQ6siSfVBih6fThg5WoPes8U5ZQpfVm_teMFcUGXFxz2DdMbfSWuws=@proton.me/
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>
---
Ran into this (report) while reading the EC firmware of my T490.
The P1 Gen2 EC has the same single page selector.

Tested on 7.3-rc7 with an SSDT that makes a thermal zone keep calling a
copy of the P1 Gen2 \GTST after resume.  _BIX was broken after 5 of 5
resumes without the patch and 0 of 5 with it.

Doing it in the resume callback also works but _BIX takes about 75 ms
there right after resume against about 35 ms here (and by the complete
callback all operation region providers have resumed too).

 drivers/acpi/battery.c | 62 ++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 670853ec3a4d..34f2e78c4314 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -107,6 +107,7 @@ struct acpi_battery {
 	struct notifier_block pm_nb;
 	struct list_head list;
 	unsigned long flags;
+	bool resume_reregister;
 
 	struct mutex property_lock; /* Protects properties below. */
 	unsigned long update_time;
@@ -1209,29 +1210,15 @@ static int battery_notify(struct notifier_block *nb,
 	if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
 		guard(mutex)(&battery->update_lock);
 
-		if (!acpi_battery_present(battery))
+		if (!battery->resume_reregister)
 			return 0;
 
-		if (battery->bat) {
-			acpi_battery_refresh(battery);
-		} else {
-			int result;
-
-			scoped_guard(mutex, &battery->property_lock) {
-				result = acpi_battery_get_info(battery);
-				if (result)
-					return result;
-			}
+		battery->resume_reregister = false;
 
-			result = sysfs_add_battery(battery);
-			if (result)
-				return result;
-		}
+		if (battery->bat)
+			sysfs_remove_battery(battery);
 
-		scoped_guard(mutex, &battery->property_lock) {
-			acpi_battery_init_alarm(battery);
-			acpi_battery_get_state(battery);
-		}
+		return sysfs_add_battery(battery);
 	}
 
 	return 0;
@@ -1450,7 +1437,42 @@ static int acpi_battery_resume(struct device *dev)
 	return 0;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, NULL, acpi_battery_resume);
+static void acpi_battery_complete(struct device *dev)
+{
+	struct acpi_battery *battery = dev_get_drvdata(dev);
+
+	if (!battery)
+		return;
+
+	guard(mutex)(&battery->update_lock);
+
+	if (!acpi_battery_present(battery))
+		return;
+
+	/*
+	 * Evaluate _BIX and _BST here before thermal zones are resumed, as
+	 * some firmware shares the EC window with _TMP without locking.
+	 */
+	guard(mutex)(&battery->property_lock);
+
+	if (battery->bat) {
+		int power_unit = battery->power_unit;
+
+		acpi_battery_get_info(battery);
+		if (power_unit != battery->power_unit)
+			battery->resume_reregister = true;
+	} else if (!acpi_battery_get_info(battery)) {
+		battery->resume_reregister = true;
+	}
+
+	acpi_battery_init_alarm(battery);
+	acpi_battery_get_state(battery);
+}
+
+static const struct dev_pm_ops acpi_battery_pm = {
+	SYSTEM_SLEEP_PM_OPS(NULL, acpi_battery_resume)
+	.complete = pm_sleep_ptr(acpi_battery_complete),
+};
 
 static struct platform_driver acpi_battery_driver = {
 	.probe = acpi_battery_probe,
-- 
2.55.0

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[Ali Ahmet Memiş]

Hi Charis, could you test this on the P1 Gen2?

(Resending with the lists in Cc, the first one only went to you.)

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[Charis Stratakis]

On Tuesday, October 13th, 2026 at 12:58 AM, Ali Ahmet Memiş <aliamemis@disroot.org> wrote:

> Hi Charis, could you test this on the P1 Gen2?
> 

I've rebuilt the Fedora 7.2.10 kernel with the patch for F44 to test it, the hunks are identical to 7.3-rc7. As before, no vanilla
kernel was installed, all testing on my laptop, with Claude's help.

The test is cycles of "rtcwake -m no -s 25; systemctl suspend", checking energy_full, voltage_min_design and serial_number
after each resume, with int3403_thermal loaded throughout. I ran it as ABA so that nothing else could drift:

  7.2.8-200.fc44, stock          10 of 10 resumes corrupted
  7.2.10 + this patch             0 of 20 resumes corrupted
  7.2.8-200.fc44, stock again     8 of 10 resumes corrupted

18 of 20 on stock against 0 of 20 patched.

Tested-by: Charalampos Stratakis <c.stratakis@proton.me>

Here is what Claude reports, hope it makes sense as I don't have much knowledge of the kernel's workings:


Both flavours from my report show up: energy_full = 328.1x Wh, which makes
capacity read ~20%, together with voltage_min_design = 32.81x V, and one
cycle with energy_now = 328960000, i.e. 424%, with energy_full intact. One
bad cycle is worth singling out because it supports the \GTST explanation,
it returned voltage_min_design correctly as 15360000 while energy_full was
328160000. So the page selector flips in the middle of _BIX and which
fields come back wrong depends on where in the read the _TMP burst lands,
rather than the whole structure being read from page 0x60.

I also traced the EC (ec.c dyndbg) on the patched kernel for one resume,
which shows the ordering the patch relies on:

  .223100  WR 81=01, 00, 02, 06, 04, 05 with the 0xA0-0xAF reads, i.e.
           _BIX and _BST from the complete callback
  .236664  last battery page selection
  .239670  WR 81=60, first \GTST
  .270916  last of 13 consecutive \GTST bursts

Nothing from the battery is interleaved among those bursts, and the next
battery access is 5.7s later from userspace polling. Worth noting that
"Restarting tasks: Done" (.240802) and "PM: suspend exit" (.251365) both
fall inside the \GTST burst, so PM_POST_SUSPEND, where the refresh used to
happen, is right in the middle of it.

The firmware bug itself is of course untouched, so a _BST that races a _TMP
at runtime can still return garbage. Those were the transient 424% readings
and they self-heal on the next read because of the 1s cache. Would a
plausibility check in acpi_battery_get_state() be welcome for that, re-
evaluating _BST once and warning FW_BUG when capacity_now exceeds
design_capacity by a wide margin? It would sit next to the existing
workarounds there. I realise it only catches the obviously absurd values
and not wrong ones that look plausible, so I am not sure it is worth it.

Regards,
Charalampos Stratakis

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[Ali Ahmet Memiş]

On Tue, Oct 13, 2026 at 01:40:17AM +0000, Charis Stratakis wrote:
> 18 of 20 on stock against 0 of 20 patched.
>
> Tested-by: Charalampos Stratakis <c.stratakis@proton.me>

Thanks for testing.

[...]

> Would a plausibility check in acpi_battery_get_state() be welcome for that, re-
> evaluating _BST once and warning FW_BUG when capacity_now exceeds
> design_capacity by a wide margin?

I'd rather not.  It only hides the absurd values and the plausible wrong
ones still get through.  If the runtime race turns out to matter, taking
BATM around the INT3403 _TMP on these two models would fix it where it
happens.

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[johannes.goede]

Hi,

On 13-Oct-26 00:46, Ali Ahmet Memiş wrote:
> Since commit d33e89d12295 ("thermal: core: Suspend thermal zones later
> and resume them earlier"), thermal zones are resumed from dpm_complete(),
> before the PM_POST_SUSPEND notifier that re-reads _BIX and _BST. On
> ThinkPad P1 Gen2 and P53, INT3403 _TMP calls \GTST which switches the EC
> page selector (HIID) without taking the BATM mutex used by the battery
> methods. ACPICA releases the interpreter lock around EC accesses

I'm wondering if this "ACPICA releases the interpreter lock around EC
accesses" is not the real problem here. Recently I've learned / heard
from a trusted source that Windows ACPI subsystem is very much single
threaded. So it is nice that ACPI has these mutexes and other stuff
to support multi-threading but as long as Windows ACPI interpreter is
not multi-threaded I think we may need to consider to copy Windows
behavior.

So maybe the EC accesses should keep the interpreter lock locked during
EC accesses.

And even go futher and maybe we need to introduce a big-acpi-lock,
which I guess the interpreter lock more or less is? And just serialize
all ACPI accesses since most ACPI tables clearly do not handle
multi-threaded access to ACPI methods well ?

Another example of multi-threaded ACPI calls causing problems is:

https://bugzilla.kernel.org/show_bug.cgi?id=221065

Regards,

Hans



> so the
> two methods interleave and _BIX often returns temperature page data
> after resume.
> > Evaluate _BIX and _BST in the complete callback, which runs before
> dpm_complete() resumes thermal zones and leave only the sysfs
> re-registration to the notifier.
> 
> Fixes: d33e89d12295 ("thermal: core: Suspend thermal zones later and resume them earlier")
> Reported-by: Charis Stratakis <c.stratakis@proton.me>
> Closes: https://lore.kernel.org/all/37N_jFulf7n-Z55NfKfE71C_wVPxYvZ0xD_iRUg2fXu7TypDiF5PStQ6siSfVBih6fThg5WoPes8U5ZQpfVm_teMFcUGXFxz2DdMbfSWuws=@proton.me/
> Cc: stable@vger.kernel.org
> Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>
> ---
> Ran into this (report) while reading the EC firmware of my T490.
> The P1 Gen2 EC has the same single page selector.
> 
> Tested on 7.3-rc7 with an SSDT that makes a thermal zone keep calling a
> copy of the P1 Gen2 \GTST after resume.  _BIX was broken after 5 of 5
> resumes without the patch and 0 of 5 with it.
> 
> Doing it in the resume callback also works but _BIX takes about 75 ms
> there right after resume against about 35 ms here (and by the complete
> callback all operation region providers have resumed too).
> 
>  drivers/acpi/battery.c | 62 ++++++++++++++++++++++++++++--------------
>  1 file changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 670853ec3a4d..34f2e78c4314 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -107,6 +107,7 @@ struct acpi_battery {
>  	struct notifier_block pm_nb;
>  	struct list_head list;
>  	unsigned long flags;
> +	bool resume_reregister;
>  
>  	struct mutex property_lock; /* Protects properties below. */
>  	unsigned long update_time;
> @@ -1209,29 +1210,15 @@ static int battery_notify(struct notifier_block *nb,
>  	if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
>  		guard(mutex)(&battery->update_lock);
>  
> -		if (!acpi_battery_present(battery))
> +		if (!battery->resume_reregister)
>  			return 0;
>  
> -		if (battery->bat) {
> -			acpi_battery_refresh(battery);
> -		} else {
> -			int result;
> -
> -			scoped_guard(mutex, &battery->property_lock) {
> -				result = acpi_battery_get_info(battery);
> -				if (result)
> -					return result;
> -			}
> +		battery->resume_reregister = false;
>  
> -			result = sysfs_add_battery(battery);
> -			if (result)
> -				return result;
> -		}
> +		if (battery->bat)
> +			sysfs_remove_battery(battery);
>  
> -		scoped_guard(mutex, &battery->property_lock) {
> -			acpi_battery_init_alarm(battery);
> -			acpi_battery_get_state(battery);
> -		}
> +		return sysfs_add_battery(battery);
>  	}
>  
>  	return 0;
> @@ -1450,7 +1437,42 @@ static int acpi_battery_resume(struct device *dev)
>  	return 0;
>  }
>  
> -static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, NULL, acpi_battery_resume);
> +static void acpi_battery_complete(struct device *dev)
> +{
> +	struct acpi_battery *battery = dev_get_drvdata(dev);
> +
> +	if (!battery)
> +		return;
> +
> +	guard(mutex)(&battery->update_lock);
> +
> +	if (!acpi_battery_present(battery))
> +		return;
> +
> +	/*
> +	 * Evaluate _BIX and _BST here before thermal zones are resumed, as
> +	 * some firmware shares the EC window with _TMP without locking.
> +	 */
> +	guard(mutex)(&battery->property_lock);
> +
> +	if (battery->bat) {
> +		int power_unit = battery->power_unit;
> +
> +		acpi_battery_get_info(battery);
> +		if (power_unit != battery->power_unit)
> +			battery->resume_reregister = true;
> +	} else if (!acpi_battery_get_info(battery)) {
> +		battery->resume_reregister = true;
> +	}
> +
> +	acpi_battery_init_alarm(battery);
> +	acpi_battery_get_state(battery);
> +}
> +
> +static const struct dev_pm_ops acpi_battery_pm = {
> +	SYSTEM_SLEEP_PM_OPS(NULL, acpi_battery_resume)
> +	.complete = pm_sleep_ptr(acpi_battery_complete),
> +};
>  
>  static struct platform_driver acpi_battery_driver = {
>  	.probe = acpi_battery_probe,

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[Rafael J. Wysocki (Intel)]

On Tue, Oct 13, 2026 at 11:33 AM <johannes.goede@oss.qualcomm.com> wrote:
>
> Hi,
>
> On 13-Oct-26 00:46, Ali Ahmet Memiş wrote:
> > Since commit d33e89d12295 ("thermal: core: Suspend thermal zones later
> > and resume them earlier"), thermal zones are resumed from dpm_complete(),
> > before the PM_POST_SUSPEND notifier that re-reads _BIX and _BST. On
> > ThinkPad P1 Gen2 and P53, INT3403 _TMP calls \GTST which switches the EC
> > page selector (HIID) without taking the BATM mutex used by the battery
> > methods. ACPICA releases the interpreter lock around EC accesses
>
> I'm wondering if this "ACPICA releases the interpreter lock around EC
> accesses" is not the real problem here.

Yes, it most likely is.

What actually happens is that ACPICA drops the interpreter lock and
the namespace lock in several cases listed in the
acpi_ex_exit_interpreter() documentation comment, the majority of
which involve having to wait.

The bottom line though is that the interpreter and namespace locks
cannot be expected to be held throughout AML evaluation because it is
not known in advance if any of the cases in question will appear.

> Recently I've learned / heard
> from a trusted source that Windows ACPI subsystem is very much single
> threaded. So it is nice that ACPI has these mutexes and other stuff
> to support multi-threading but as long as Windows ACPI interpreter is
> not multi-threaded I think we may need to consider to copy Windows
> behavior.

This is also my conclusion, at least to some extent.

The problem with it though is that in some cases parallel evaluation
of AML is actually useful.  For example, say two PCI devices are
powered up at the same time and AML waits for each of them.  The async
suspend/resume aligns those waits to take place simultaneously instead
of serializing them which allows some time to be saved.

> So maybe the EC accesses should keep the interpreter lock locked during
> EC accesses.

The problem here is that it is hard to say in advance whether or not
_BIX or _BST will access the EC and whether or not that will clash
with _TMP evaluation for a seemingly unrelated thermal zone.

> And even go further and maybe we need to introduce a big-acpi-lock,
> which I guess the interpreter lock more or less is? And just serialize
> all ACPI accesses since most ACPI tables clearly do not handle
> multi-threaded access to ACPI methods well ?

I would be inclined to do so except for device power management
methods, that is _ON, _OFF, and _PSx, that are not the problematic
ones, at least as far as I know.

> Another example of multi-threaded ACPI calls causing problems is:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=221065

Right.

So I guess the idea would be to add locking to acpi_evaluate_object()?

An alternative might be to introduce an "exclusive" variant of
acpi_evaluate_object() with built-in locking and use it in the cases
that are known-problematic.

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[Ali Ahmet Memiş]

On Tue, Oct 13, 2026 at 01:58:15PM +0200, Rafael J. Wysocki (Intel) wrote:
> An alternative might be to introduce an "exclusive" variant of
> acpi_evaluate_object() with built-in locking and use it in the cases
> that are known-problematic.

An exclusive variant would have to be used on both sides here, for
_BIX/_BST and for the INT3403 _TMP, since it is \GTST in _TMP that
switches the page.  I can do that and test it the same way.

Should this patch still go in for the regression meanwhile?

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[Rafael J. Wysocki (Intel)]

On Tue, Oct 13, 2026 at 2:31 PM Ali Ahmet Memiş <aliamemis@disroot.org> wrote:
>
> On Tue, Oct 13, 2026 at 01:58:15PM +0200, Rafael J. Wysocki (Intel) wrote:
> > An alternative might be to introduce an "exclusive" variant of
> > acpi_evaluate_object() with built-in locking and use it in the cases
> > that are known-problematic.
>
> An exclusive variant would have to be used on both sides here, for
> _BIX/_BST and for the INT3403 _TMP, since it is \GTST in _TMP that
> switches the page.  I can do that and test it the same way.

Sounds good.

> Should this patch still go in for the regression meanwhile?

No, it only addresses system resume while the same race may occur if
user space reads the thermal zone temperature during a battery
notification, for example.  All of that needs to be addressed.

Re: [PATCH] ACPI: battery: Re-read battery information in the complete callback

[johannes.goede]

Hi Rafael,

On 13-Oct-26 13:58, Rafael J. Wysocki (Intel) wrote:
> On Tue, Oct 13, 2026 at 11:33 AM <johannes.goede@oss.qualcomm.com> wrote:
>>
>> Hi,
>>
>> On 13-Oct-26 00:46, Ali Ahmet Memiş wrote:
>>> Since commit d33e89d12295 ("thermal: core: Suspend thermal zones later
>>> and resume them earlier"), thermal zones are resumed from dpm_complete(),
>>> before the PM_POST_SUSPEND notifier that re-reads _BIX and _BST. On
>>> ThinkPad P1 Gen2 and P53, INT3403 _TMP calls \GTST which switches the EC
>>> page selector (HIID) without taking the BATM mutex used by the battery
>>> methods. ACPICA releases the interpreter lock around EC accesses
>>
>> I'm wondering if this "ACPICA releases the interpreter lock around EC
>> accesses" is not the real problem here.
> 
> Yes, it most likely is.
> 
> What actually happens is that ACPICA drops the interpreter lock and
> the namespace lock in several cases listed in the
> acpi_ex_exit_interpreter() documentation comment, the majority of
> which involve having to wait.
> 
> The bottom line though is that the interpreter and namespace locks
> cannot be expected to be held throughout AML evaluation because it is
> not known in advance if any of the cases in question will appear.
> 
>> Recently I've learned / heard
>> from a trusted source that Windows ACPI subsystem is very much single
>> threaded. So it is nice that ACPI has these mutexes and other stuff
>> to support multi-threading but as long as Windows ACPI interpreter is
>> not multi-threaded I think we may need to consider to copy Windows
>> behavior.
> 
> This is also my conclusion, at least to some extent.
> 
> The problem with it though is that in some cases parallel evaluation
> of AML is actually useful.  For example, say two PCI devices are
> powered up at the same time and AML waits for each of them.  The async
> suspend/resume aligns those waits to take place simultaneously instead
> of serializing them which allows some time to be saved.
> 
>> So maybe the EC accesses should keep the interpreter lock locked during
>> EC accesses.
> 
> The problem here is that it is hard to say in advance whether or not
> _BIX or _BST will access the EC and whether or not that will clash
> with _TMP evaluation for a seemingly unrelated thermal zone.
> 
>> And even go further and maybe we need to introduce a big-acpi-lock,
>> which I guess the interpreter lock more or less is? And just serialize
>> all ACPI accesses since most ACPI tables clearly do not handle
>> multi-threaded access to ACPI methods well ?
> 
> I would be inclined to do so except for device power management
> methods, that is _ON, _OFF, and _PSx, that are not the problematic
> ones, at least as far as I know.

That sounds like a reasonable compromise which should fix both
this case as well as help with bko221065 .

>> Another example of multi-threaded ACPI calls causing problems is:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=221065
> 
> Right.
> 
> So I guess the idea would be to add locking to acpi_evaluate_object()?

Yes that seems like a nice single point of entry to do this where
all the various helpers eventually end up.

> An alternative might be to introduce an "exclusive" variant of
> acpi_evaluate_object() with built-in locking and use it in the cases
> that are known-problematic.

So here we have _TMP and _BIX / _BST and in bko221065 we have
a bunch of again batteyr methods + UCSI racing. I'm afraid if
we add something like acpi_evaluate_object_exclusive() we'll end
up eventually using that almost everywhere.

Maybe do things the other way around and serializing all the calls
by default and adding an acpi_evaluate_object_unlocked() to be used
in cases where we want the speedup of multi-threading like
the _ON, _OFF and _PSx you mentioned above ?

Regards,

Hans