Re: [PATCH 0/3] ACPI: Serialize battery methods and INT3403 _TMP

[Hans de Goede]

Hi,

On 15-Oct-26 15:48, Rong Zhang wrote:
> Hi Hans,
> 
> On Thu, 2026-10-15 at 15:13 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 15-Oct-26 13:45, Rong Zhang wrote:
>>> Hi Ali,
>>>
>>> Thanks for working on this!
>>>
>>> The series is generally in a good shape. I just have a few minor
>>> comments.
>>>
>>> On Wed, 2026-10-14 at 16:26 +0300, Ali Ahmet Memiş wrote:
>>>> As discussed in [1], add exclusive variants of acpi_evaluate_object()
>>>> and acpi_evaluate_integer() and use them for the battery methods and
>>>> INT3403 _TMP, which interleave badly on ThinkPad P1 Gen2 and P53.
>>>>
>>>> UCSI (bko221065) is not converted.  
>>>>
>>>
>>> Please leave a link to the bko bug, so that readers don't need to
>>> assemble the link themselves.
>>>
>>>> There the contention is on a
>>>
>>> After carefully auditing the ASL code again, I've just found the root
>>> cause of the mutex acquisition timeout.
>>>
>>> It's hard to notice, but \_SB.PCI0.LPC0.EC0.RDER being declared as
>>> "Serialized" causes an ABBA deadlock:
>>>
>>> Thread 1 (battery)                         Thread 2 (ideapad-laptop)
>>> -----------------------------------------  ---------------------------------------
>>> serialize \_SB.BAT0._BIX                   serialize \_SB.PCI0.LPC0.EC0.VPC0.GBMD
>>> evaluate \_SB.BAT0._BIX                    evaluate \_SB.PCI0.LPC0.EC0.VPC0.GBMD
>>>   acquire ECMT [A]
>>>   serialize \_SB.PCI0.LPC0.EC0.RDIF          serialize \_SB.PCI0.LPC0.EC0.RDER [B]
>>>   evaluate \_SB.PCI0.LPC0.EC0.RDIF           evaluate \_SB.PCI0.LPC0.EC0.RDER
>>>     serialize \_SB.PCI0.LPC0.EC0.RDER [B]      acquire ECMT [A]
>>>
>>> That's why merging battery notifications significantly reduced the
>>> frequency of mutex acquisition timeout.
>>>
>>>> firmware AML mutex that _Qxx EC queries also take, which a wrapper
>>>> around acpi_evaluate_object() cannot cover, and Rong's notification
>>>> merging in 7.3 already reduces it on the battery side.
>>>
>>> Lock contention is not the root cause, and EC queries and UCSI _DSM are
>>> just innocent victims of the ABBA deadlock.
>>
>> Hmm, ok so since \_SB.BAT0._BIX takes ECMT itself instead of relying
>> on \_SB.PCI0.LPC0.EC0.RDER taking it (1), 
>>
> 
> I guess the intent of holding ECMT in _BIX was to prevent race conditions
> in multi-threaded interpreters. IOW, the firmware authors probably took
> multi-threaded interpreter into their consideration.
> 
>         If ((Acquire (^^PCI0.LPC0.EC0.ECMT, 0x2000) == 0x00))
>         {
>             If (((^^PCI0.LPC0.EC0.BTDV && ^^PCI0.LPC0.EC0.BTFC) && ^^PCI0.LPC0.EC0.BTDC))
>             {
>                 BPK1 [0x01] = 0x00
>                 Local0 = ^^PCI0.LPC0.EC0.BTDC /* \_SB_.PCI0.LPC0.EC0_.BTDC */
>                 Local0 *= 0x0A
>                 BPK1 [0x02] = Local0
>                 Local0 = ^^PCI0.LPC0.EC0.BTFC /* \_SB_.PCI0.LPC0.EC0_.BTFC */
>                 Local0 *= 0x0A
>                 BPK1 [0x03] = Local0
>                 BPK1 [0x05] = ^^PCI0.LPC0.EC0.BTDV /* \_SB_.PCI0.LPC0.EC0_.BTDV */
>                 Local0 = ^^PCI0.LPC0.EC0.BTFC /* \_SB_.PCI0.LPC0.EC0_.BTFC */
>                 Local0 *= 0x0A
>                 Local0 *= 0x0A
>                 Divide (Local0, 0x64, Local1, Local2)
>                 BPK1 [0x06] = Local2
>                 Local0 = ^^PCI0.LPC0.EC0.BTFC /* \_SB_.PCI0.LPC0.EC0_.BTFC */
>                 Local0 *= 0x0A
>                 Local0 *= 0x02
>                 Divide (Local0, 0x64, Local1, Local2)
>                 BPK1 [0x07] = Local2
>                 BPK1 [0x08] = ^^PCI0.LPC0.EC0.BTCC /* \_SB_.PCI0.LPC0.EC0_.BTCC */
>                 BPK1 [0x09] = 0x251C
>             }
> 
>             BPK1 [0x10] = ToString (^^PCI0.LPC0.EC0.RDIF (0x01), Ones)
>             ToDecimalString (^^PCI0.LPC0.EC0.BTSN, Local0)
>             BPK1 [0x11] = ToBuffer (Local0)
>             BPK1 [0x12] = ToString (^^PCI0.LPC0.EC0.RDIF (0x02), Ones)
>             BPK1 [0x13] = ToString (^^PCI0.LPC0.EC0.RDIF (0x03), Ones)
>             Release (^^PCI0.LPC0.EC0.ECMT)
>         }
> 
>> this means that not just
>> the ideapad-laptop GBMD method call but anything calling
>> \_SB.PCI0.LPC0.EC0.RDER may cause this abba dead-lock, right?
>> Are we sure nothing else is calling  \_SB.PCI0.LPC0.EC0.RDER ?
> 
> I checked all code paths with ECMT involved, and concluded a call graph
> with all faulty methods:
> 
>                       ideapad-laptop
>                             |
>                             v
>    battery                 GBMD
>      |                      |
>      v                      v
>    _BIX -> ECMT -> RDIF -> RDER -> ECMT
>                     ^       ^
>                     |       |
>     +---------------+-------+
>     |
>    GSBI -> ECMT -> SMTE/SMTF -> ECMT
> 
> GSBI: \_SB.PCI0.LPC0.EC0.VPC0.GSBI
> SMTE: \_SB.PCI0.LPC0.EC0.VPC0.SMTE
> SMTF: \_SB.PCI0.LPC0.EC0.VPC0.SMTF
> 
> All methods in the graph are never called by any method or driver that is
> not in the graph.
> 
> So battery and ideapad-laptop are the only drivers that could potentially
> trigger the ABBA deadlock.

Great, thank you for checking this.

Given the above I agree with Rafael to drop the EC query and UCSI
patches for the time being.

Regards,

Hans