Re: [PATCH v4 3/3] ACPI: battery: Protect all properties with a separated mutex

Hello,

On Wed, Jul 22, 2026 at 8:25 PM Rong Zhang <[email protected]> wrote:
>
> Hi Rafael,
>
> On Tue, 2026-07-21 at 22:54 +0200, Rafael J. Wysocki (Intel) wrote:
> > On Sat, Jul 18, 2026 at 1:11 AM Rong Zhang <[email protected]> wrote:
> > >
> > > The acpi_battery_get_property() callback calls acpi_battery_get_state()
> > > without any lock held, which could lead to race conditions, e.g., when
> > > multiple tasks read power supply properties simultaneously, or when
> > > other callbacks 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.
> > >
> > > The said "other synchronized methods" are protected by update_lock,
> > > leaving acpi_battery_get_property() to be the last desynchronized code
> > > path. Unfortunately, update_lock is not applicapable for
> > > acpi_battery_get_property(), as it protects too many fields, far more
> > > than necessary. What's worse, some code path could call or wait for
> > > acpi_battery_get_property() while an outer functions holding
> > > update_lock.
> > >
> > > Therefore, introduce a mutex to protect all accesses to battery
> > > properties, so that acpi_battery_get_property() can take the advantage
> > > of the mutex and synchronize itself. 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: Avraham Hollander <[email protected]>
> > > Reported-by: Rick <[email protected]>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221065
> > > Signed-off-by: Rong Zhang <[email protected]>
> >
> > I have queued up the other 2 patches in the series for 7.3,
> >
>
> Thanks a lot ;-)
>
> > but this
> > one is not quite ready IMV.
> >
> > In the first place, it is not quite clear from the changelog what
> > exactly the role of the new mutex is.
>
> It aims to eliminate lock-free access to battery properties. Especially,
> synchronize the fragile update_time-based cache.
>
> I agreed that the changelog is somehow hard to read. I was struggling to
> summarize the debugging findings, and it turned out to be too
> complicated. I will explain them below.
>
> >
> > If there are any real race conditions it prevents from occurring, it
> > would be good to give examples.
>
> This pattern can be reliably reproduced on Rick's device. I had a
> detailed analysis to the sysrq stacktraces on his device, see
> https://bugzilla.kernel.org/show_bug.cgi?id=221065#c85

So please add a Link: tag pointing to this.

> >
> > Moreover, the exact mechanism of the _BST synchronization failure is
> > unclear to me because in Linux all AML evaluation is synchronized by
> > the namespace and interpreter mutexes.
> >
>
> Well, _BST itself has no synchronization failure. It's always evaluated
> serialized. However, the frequency of its evaluation matters on buggy
> devices.

Yes, it does, because it isn't really serialized.

Please see the changelog of this patch:

https://lore.kernel.org/linux-acpi/[email protected]/

and the documentation comment of acpi_ex_exit_interpreter().  ACPICA
drops the namespace and interpreter locks when AML is acquiring a
mutex, so effectively you can get two (or more) _BST evaluations
running at the same time and all of them except for one will be
blocking on the AML mutex.  Moreover, this appears to somehow confuse
the interpreter and it attempts to release the AML mutex in a thread
that hasn't acquired it (see the debug info in this message:
https://lore.kernel.org/linux-acpi/CAP1mzZReJCn6df5DwEPu-JCQUyr=Pu1cg5xKCMttWZkHCQtVmQ@mail.gmail.com/).

So obviously the frequency of _BST evaluation matters and the devices
in question may not in fact be buggy.

> > If that is not sufficient, it
> > would be good to explain why because as it stands, it is not clear
> > whether or not the approach used in this patch is adequate.
>
> The issue is about the synchronization failure of battery->update_time
> due to acpi_battery_get_property() calling acpi_battery_get_state()
> without any mutex held, which results in _BST being evaluated too
> frequently.
>
> The _BST method on buggy devices acquires an ACPI mutex called ECMT. As
> the name suggests, the mutex protects all sorts of EC stuff. The mutex
> is heavily used, but very fragile. It is prone to some sorts of lock
> starvation (or deadlock? I am unsure as I don't have a buggy device)
> once its acquired too frequently. Lock starvation causes mutex
> acquisition timeout in other methods, which is handled by the firmware
> poorly, and thus leads to catastrophic results.
>
> Once battery->update_time has expired, multiple get_property() requests
> originated from userspace will all have to try to evaluate the _BST
> method. The evaluation is indeed serialized, but still causing to much
> pressure on the mutex.
>
> The patch fixes the synchronization failure of the unfortunate
> combination of battery->update_time and acpi_battery_get_property(), and
> thus always ensures the 1000ms gap (i.e., cache_time).
>
>
> BTW, even not thinking about the buggy device, the race condition of
> property access still exists. Consider:
>
>                 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()
>                                             [up to date]
>                                           [read capacity_now]
>                                         return
>     [fix capacity_now due to quirk]
>     ...
>
> where CPU1 reads raw capacity_now before CPU0 fixes it to a meaningful
> value.
>
> Does my explanation make sense to you? If so, I will refine the
> changelog and resubmit the patch.

Yes, please.