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

[johannes . goede]

Hi,

On 14-Oct-26 17:06, Rafael J. Wysocki (Intel) wrote:
> Hi Hans,
> 
> On Wed, Oct 14, 2026 at 11:58 AM <johannes.goede@xxxxxxxxxxxxxxxx> wrote:
>>
>> Hi Rafael,
>>
>> On 13-Oct-26 19:20, Rafael J. Wysocki (Intel) wrote:
>>> Hi Hans,
>>>
>>> On Tue, Oct 13, 2026 at 3:36 PM <johannes.goede@xxxxxxxxxxxxxxxx> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> On 13-Oct-26 13:58, Rafael J. Wysocki (Intel) wrote:
>>>>> On Tue, Oct 13, 2026 at 11:33 AM <johannes.goede@xxxxxxxxxxxxxxxx> 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.
>>>
>>> That, of course, would require updating ACPICA code and I'm not sure
>>> if doing it upstream is useful.  For example, if some hosts already
>>> use locking around acpi_evaluate_object(), this modification won't
>>> help them and it will certainly be a change in behavior.
>>>
>>> This means that we'll likely end up with a Linux-specific divergence
>>> from the upstream which can be lived with, but it won't be nice.
>>
>> Ah, I see.
>>
>>>>> 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.
>>>
>>> While I think that your concern is valid, this can be done without
>>> touching the ACPICA code at all.
>>
>> Not needing to touch ACPICA is a valid concern. So I agree lets go
>> with a wrapper for paths which we know need to be serialized for now.
>>
>> And then add that around the battery methods + _TMP + UCSI methods
>> for starters I guess ?
> 
> Yes.
> 
>> ###
>>
>> Should we also stop releasing the interpreter lock in the EC code?
> 
> Which EC code do you mean in particular?

I meant the event-handling code, but I forgot the unlock is
actually in the ACPICA core, not in the Linux code.

Anyways I think this is nicely covered by Ali's latest series:

https://lore.kernel.org/all/20261014132703.93355-1-aliamemis@xxxxxxxxxxx/

Regards,

Hans