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

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

[Ali Ahmet Memiş]

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.  There the contention is on a
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.

[1] https://lore.kernel.org/all/CAJZ5v0gS4agVUkCO6M=VW4Wi2y1-s+k23bpzmX9aAZ0Rsv4z9Q@mail.gmail.com/

Ali Ahmet Memiş (3):
  ACPI: utils: Add exclusive AML evaluation helpers
  ACPI: battery: Evaluate battery methods exclusively
  thermal: intel: int340x: Evaluate _TMP exclusively

 drivers/acpi/battery.c                        | 17 +++---
 drivers/acpi/utils.c                          | 56 +++++++++++++++++--
 .../int340x_thermal/int340x_thermal_zone.c    |  3 +-
 include/acpi/acpi_bus.h                       |  8 +++
 4 files changed, 71 insertions(+), 13 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.55.0

[PATCH 1/3] ACPI: utils: Add exclusive AML evaluation helpers

[Ali Ahmet Memiş]

ACPICA drops the interpreter lock while AML waits, for example around EC
accesses, so two methods can run interleaved. Some firmware relies on
methods not interleaving, for instance by switching an EC page selector
without taking the AML mutex that the other users of that page hold.

Add acpi_evaluate_object_exclusive() and acpi_evaluate_integer_exclusive()
which serialize the evaluation under a global mutex, for callers that are
known to be affected.

Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>
---
 drivers/acpi/utils.c    | 56 +++++++++++++++++++++++++++++++++++++----
 include/acpi/acpi_bus.h |  8 ++++++
 2 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index d499b72574ab..f28680424f14 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -10,6 +10,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/types.h>
@@ -243,10 +244,34 @@ acpi_extract_package(union acpi_object *package,
 
 EXPORT_SYMBOL(acpi_extract_package);
 
-acpi_status
-acpi_evaluate_integer(acpi_handle handle,
-		      acpi_string pathname,
-		      struct acpi_object_list *arguments, unsigned long long *data)
+static DEFINE_MUTEX(acpi_exclusive_eval_lock);
+
+/**
+ * acpi_evaluate_object_exclusive - Evaluate an object without interleaving
+ * @handle: ACPI handle of the object or its scope.
+ * @pathname: Path of the object relative to @handle.
+ * @arguments: Arguments to pass, or NULL.
+ * @buffer: Buffer for the return value, or NULL.
+ *
+ * Same as acpi_evaluate_object(), but serialized with other exclusive
+ * evaluations.  Use it only for methods known to break when interleaved.
+ */
+acpi_status acpi_evaluate_object_exclusive(acpi_handle handle,
+					   acpi_string pathname,
+					   struct acpi_object_list *arguments,
+					   struct acpi_buffer *buffer)
+{
+	guard(mutex)(&acpi_exclusive_eval_lock);
+
+	return acpi_evaluate_object(handle, pathname, arguments, buffer);
+}
+EXPORT_SYMBOL_GPL(acpi_evaluate_object_exclusive);
+
+static acpi_status __acpi_evaluate_integer(acpi_handle handle,
+					   acpi_string pathname,
+					   struct acpi_object_list *arguments,
+					   unsigned long long *data,
+					   bool exclusive)
 {
 	acpi_status status = AE_OK;
 	union acpi_object element;
@@ -257,7 +282,12 @@ acpi_evaluate_integer(acpi_handle handle,
 
 	buffer.length = sizeof(union acpi_object);
 	buffer.pointer = &element;
-	status = acpi_evaluate_object(handle, pathname, arguments, &buffer);
+	if (exclusive)
+		status = acpi_evaluate_object_exclusive(handle, pathname,
+							arguments, &buffer);
+	else
+		status = acpi_evaluate_object(handle, pathname, arguments,
+					      &buffer);
 	if (ACPI_FAILURE(status)) {
 		acpi_util_eval_error(handle, pathname, status);
 		return status;
@@ -275,8 +305,24 @@ acpi_evaluate_integer(acpi_handle handle,
 	return AE_OK;
 }
 
+acpi_status
+acpi_evaluate_integer(acpi_handle handle,
+		      acpi_string pathname,
+		      struct acpi_object_list *arguments, unsigned long long *data)
+{
+	return __acpi_evaluate_integer(handle, pathname, arguments, data, false);
+}
 EXPORT_SYMBOL(acpi_evaluate_integer);
 
+acpi_status acpi_evaluate_integer_exclusive(acpi_handle handle,
+					    acpi_string pathname,
+					    struct acpi_object_list *arguments,
+					    unsigned long long *data)
+{
+	return __acpi_evaluate_integer(handle, pathname, arguments, data, true);
+}
+EXPORT_SYMBOL_GPL(acpi_evaluate_integer_exclusive);
+
 int acpi_get_local_u64_address(acpi_handle handle, u64 *addr)
 {
 	acpi_status status;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a10a591c18b2..e7a671dba624 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -32,6 +32,14 @@ acpi_status
 acpi_evaluate_integer(acpi_handle handle,
 		      acpi_string pathname,
 		      struct acpi_object_list *arguments, unsigned long long *data);
+acpi_status acpi_evaluate_object_exclusive(acpi_handle handle,
+					   acpi_string pathname,
+					   struct acpi_object_list *arguments,
+					   struct acpi_buffer *buffer);
+acpi_status acpi_evaluate_integer_exclusive(acpi_handle handle,
+					    acpi_string pathname,
+					    struct acpi_object_list *arguments,
+					    unsigned long long *data);
 bool acpi_evaluate_reference(acpi_handle handle, acpi_string pathname,
 			     struct acpi_object_list *arguments,
 			     struct acpi_handle_list *list);

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.55.0

[PATCH 2/3] ACPI: battery: Evaluate battery methods exclusively

[Ali Ahmet Memiş]

On ThinkPad P1 Gen2 and P53, INT3403 _TMP switches the EC page selector
without taking the AML mutex the battery methods use, so _BIX and _BST
running at the same time can read the wrong page. Since commit d33e89d12295
("thermal: core: Suspend thermal zones later and resume them earlier")
this happens on most resumes.

Evaluate the battery methods with acpi_evaluate_object_exclusive().

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>
---
 drivers/acpi/battery.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 670853ec3a4d..61de3f5be51f 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -583,9 +583,9 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
 		struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 		acpi_status status = AE_ERROR;
 
-		status = acpi_evaluate_object(battery->device->handle,
-					      use_bix ? "_BIX":"_BIF",
-					      NULL, &buffer);
+		status = acpi_evaluate_object_exclusive(battery->device->handle,
+							use_bix ? "_BIX":"_BIF",
+							NULL, &buffer);
 
 		if (ACPI_FAILURE(status)) {
 			acpi_handle_info(battery->device->handle,
@@ -624,8 +624,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
 			msecs_to_jiffies(cache_time)))
 		return 0;
 
-	status = acpi_evaluate_object(battery->device->handle, "_BST",
-				      NULL, &buffer);
+	status = acpi_evaluate_object_exclusive(battery->device->handle, "_BST",
+						NULL, &buffer);
 	if (ACPI_FAILURE(status)) {
 		acpi_handle_info(battery->device->handle,
 				 "_BST evaluation failed: %s",
@@ -667,6 +667,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
 
 static int acpi_battery_set_alarm(struct acpi_battery *battery)
 {
+	union acpi_object arg = { .type = ACPI_TYPE_INTEGER };
+	struct acpi_object_list args = { .count = 1, .pointer = &arg };
 	acpi_status status = 0;
 
 	lockdep_assert_held(&battery->property_lock);
@@ -675,8 +677,9 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
 	    !test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags))
 		return -ENODEV;
 
-	status = acpi_execute_simple_method(battery->device->handle, "_BTP",
-					    battery->alarm);
+	arg.integer.value = battery->alarm;
+	status = acpi_evaluate_object_exclusive(battery->device->handle, "_BTP",
+						&args, NULL);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-- 
2.55.0

[PATCH 3/3] thermal: intel: int340x: Evaluate _TMP exclusively

[Ali Ahmet Memiş]

On ThinkPad P1 Gen2 and P53, INT3403 _TMP switches the EC page selector
without taking the mutex the battery methods use, so it must not run
interleaved with them.

Evaluate _TMP with acpi_evaluate_integer_exclusive().

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>
---
 .../thermal/intel/int340x_thermal/int340x_thermal_zone.c   | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
index 965700ee6962..de0a1fc7b817 100644
--- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
+++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
@@ -18,7 +18,12 @@ static int int340x_thermal_get_zone_temp(struct thermal_zone_device *zone,
 	unsigned long long tmp;
 	acpi_status status;
 
-	status = acpi_evaluate_integer(d->adev->handle, "_TMP", NULL, &tmp);
+	/*
+	 * On some ThinkPads _TMP switches the EC page selector without
+	 * taking the mutex the battery methods use.
+	 */
+	status = acpi_evaluate_integer_exclusive(d->adev->handle, "_TMP", NULL,
+						 &tmp);
 	if (ACPI_FAILURE(status))
 		return -EIO;
 
-- 
2.55.0

Re: [PATCH 1/3] ACPI: utils: Add exclusive AML evaluation helpers

[Hans de Goede]

Hi,

On 14-Oct-26 15:30, Ali Ahmet Memiş wrote:
> ACPICA drops the interpreter lock while AML waits, for example around EC
> accesses, so two methods can run interleaved. Some firmware relies on
> methods not interleaving, for instance by switching an EC page selector
> without taking the AML mutex that the other users of that page hold.
> 
> Add acpi_evaluate_object_exclusive() and acpi_evaluate_integer_exclusive()
> which serialize the evaluation under a global mutex, for callers that are
> known to be affected.
> 
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans


> ---
>  drivers/acpi/utils.c    | 56 +++++++++++++++++++++++++++++++++++++----
>  include/acpi/acpi_bus.h |  8 ++++++
>  2 files changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index d499b72574ab..f28680424f14 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/types.h>
> @@ -243,10 +244,34 @@ acpi_extract_package(union acpi_object *package,
>  
>  EXPORT_SYMBOL(acpi_extract_package);
>  
> -acpi_status
> -acpi_evaluate_integer(acpi_handle handle,
> -		      acpi_string pathname,
> -		      struct acpi_object_list *arguments, unsigned long long *data)
> +static DEFINE_MUTEX(acpi_exclusive_eval_lock);
> +
> +/**
> + * acpi_evaluate_object_exclusive - Evaluate an object without interleaving
> + * @handle: ACPI handle of the object or its scope.
> + * @pathname: Path of the object relative to @handle.
> + * @arguments: Arguments to pass, or NULL.
> + * @buffer: Buffer for the return value, or NULL.
> + *
> + * Same as acpi_evaluate_object(), but serialized with other exclusive
> + * evaluations.  Use it only for methods known to break when interleaved.
> + */
> +acpi_status acpi_evaluate_object_exclusive(acpi_handle handle,
> +					   acpi_string pathname,
> +					   struct acpi_object_list *arguments,
> +					   struct acpi_buffer *buffer)
> +{
> +	guard(mutex)(&acpi_exclusive_eval_lock);
> +
> +	return acpi_evaluate_object(handle, pathname, arguments, buffer);
> +}
> +EXPORT_SYMBOL_GPL(acpi_evaluate_object_exclusive);
> +
> +static acpi_status __acpi_evaluate_integer(acpi_handle handle,
> +					   acpi_string pathname,
> +					   struct acpi_object_list *arguments,
> +					   unsigned long long *data,
> +					   bool exclusive)
>  {
>  	acpi_status status = AE_OK;
>  	union acpi_object element;
> @@ -257,7 +282,12 @@ acpi_evaluate_integer(acpi_handle handle,
>  
>  	buffer.length = sizeof(union acpi_object);
>  	buffer.pointer = &element;
> -	status = acpi_evaluate_object(handle, pathname, arguments, &buffer);
> +	if (exclusive)
> +		status = acpi_evaluate_object_exclusive(handle, pathname,
> +							arguments, &buffer);
> +	else
> +		status = acpi_evaluate_object(handle, pathname, arguments,
> +					      &buffer);
>  	if (ACPI_FAILURE(status)) {
>  		acpi_util_eval_error(handle, pathname, status);
>  		return status;
> @@ -275,8 +305,24 @@ acpi_evaluate_integer(acpi_handle handle,
>  	return AE_OK;
>  }
>  
> +acpi_status
> +acpi_evaluate_integer(acpi_handle handle,
> +		      acpi_string pathname,
> +		      struct acpi_object_list *arguments, unsigned long long *data)
> +{
> +	return __acpi_evaluate_integer(handle, pathname, arguments, data, false);
> +}
>  EXPORT_SYMBOL(acpi_evaluate_integer);
>  
> +acpi_status acpi_evaluate_integer_exclusive(acpi_handle handle,
> +					    acpi_string pathname,
> +					    struct acpi_object_list *arguments,
> +					    unsigned long long *data)
> +{
> +	return __acpi_evaluate_integer(handle, pathname, arguments, data, true);
> +}
> +EXPORT_SYMBOL_GPL(acpi_evaluate_integer_exclusive);
> +
>  int acpi_get_local_u64_address(acpi_handle handle, u64 *addr)
>  {
>  	acpi_status status;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index a10a591c18b2..e7a671dba624 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -32,6 +32,14 @@ acpi_status
>  acpi_evaluate_integer(acpi_handle handle,
>  		      acpi_string pathname,
>  		      struct acpi_object_list *arguments, unsigned long long *data);
> +acpi_status acpi_evaluate_object_exclusive(acpi_handle handle,
> +					   acpi_string pathname,
> +					   struct acpi_object_list *arguments,
> +					   struct acpi_buffer *buffer);
> +acpi_status acpi_evaluate_integer_exclusive(acpi_handle handle,
> +					    acpi_string pathname,
> +					    struct acpi_object_list *arguments,
> +					    unsigned long long *data);
>  bool acpi_evaluate_reference(acpi_handle handle, acpi_string pathname,
>  			     struct acpi_object_list *arguments,
>  			     struct acpi_handle_list *list);
> 
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907

Re: [PATCH 2/3] ACPI: battery: Evaluate battery methods exclusively

[Hans de Goede]

Hi,

On 14-Oct-26 15:30, Ali Ahmet Memiş wrote:
> On ThinkPad P1 Gen2 and P53, INT3403 _TMP switches the EC page selector
> without taking the AML mutex the battery methods use, so _BIX and _BST
> running at the same time can read the wrong page. Since commit d33e89d12295
> ("thermal: core: Suspend thermal zones later and resume them earlier")
> this happens on most resumes.
> 
> Evaluate the battery methods with acpi_evaluate_object_exclusive().
> 
> 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>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans


> ---
>  drivers/acpi/battery.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 670853ec3a4d..61de3f5be51f 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -583,9 +583,9 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
>  		struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  		acpi_status status = AE_ERROR;
>  
> -		status = acpi_evaluate_object(battery->device->handle,
> -					      use_bix ? "_BIX":"_BIF",
> -					      NULL, &buffer);
> +		status = acpi_evaluate_object_exclusive(battery->device->handle,
> +							use_bix ? "_BIX":"_BIF",
> +							NULL, &buffer);
>  
>  		if (ACPI_FAILURE(status)) {
>  			acpi_handle_info(battery->device->handle,
> @@ -624,8 +624,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
>  			msecs_to_jiffies(cache_time)))
>  		return 0;
>  
> -	status = acpi_evaluate_object(battery->device->handle, "_BST",
> -				      NULL, &buffer);
> +	status = acpi_evaluate_object_exclusive(battery->device->handle, "_BST",
> +						NULL, &buffer);
>  	if (ACPI_FAILURE(status)) {
>  		acpi_handle_info(battery->device->handle,
>  				 "_BST evaluation failed: %s",
> @@ -667,6 +667,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
>  
>  static int acpi_battery_set_alarm(struct acpi_battery *battery)
>  {
> +	union acpi_object arg = { .type = ACPI_TYPE_INTEGER };
> +	struct acpi_object_list args = { .count = 1, .pointer = &arg };
>  	acpi_status status = 0;
>  
>  	lockdep_assert_held(&battery->property_lock);
> @@ -675,8 +677,9 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
>  	    !test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags))
>  		return -ENODEV;
>  
> -	status = acpi_execute_simple_method(battery->device->handle, "_BTP",
> -					    battery->alarm);
> +	arg.integer.value = battery->alarm;
> +	status = acpi_evaluate_object_exclusive(battery->device->handle, "_BTP",
> +						&args, NULL);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>  

Re: [PATCH 3/3] thermal: intel: int340x: Evaluate _TMP exclusively

[Hans de Goede]

Hi,

On 14-Oct-26 15:30, Ali Ahmet Memiş wrote:
> On ThinkPad P1 Gen2 and P53, INT3403 _TMP switches the EC page selector
> without taking the mutex the battery methods use, so it must not run
> interleaved with them.
> 
> Evaluate _TMP with acpi_evaluate_integer_exclusive().
> 
> 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>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans




> ---
>  .../thermal/intel/int340x_thermal/int340x_thermal_zone.c   | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> index 965700ee6962..de0a1fc7b817 100644
> --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -18,7 +18,12 @@ static int int340x_thermal_get_zone_temp(struct thermal_zone_device *zone,
>  	unsigned long long tmp;
>  	acpi_status status;
>  
> -	status = acpi_evaluate_integer(d->adev->handle, "_TMP", NULL, &tmp);
> +	/*
> +	 * On some ThinkPads _TMP switches the EC page selector without
> +	 * taking the mutex the battery methods use.
> +	 */
> +	status = acpi_evaluate_integer_exclusive(d->adev->handle, "_TMP", NULL,
> +						 &tmp);
>  	if (ACPI_FAILURE(status))
>  		return -EIO;
>  

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

[Hans de Goede]

Hi,

On 14-Oct-26 15:26, 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.

Thank you for your work on this!

> UCSI (bko221065) is not converted.  There the contention is on a
> 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.

Hmm, I wonder if we should export the lock added in patch 1/3 and
then also take that in the ACPI EC interrupt handler ?

The ACPI code already uses a threaded IRQ handler for both EC GPIO
IRQs and GPEs so sleeping in the handler should be ok.

Note this is something which can be done on top of this set. I'm just
thinking that if we're going to do this it would be nice to cover
the bko221065 UCSI case too. Rong's notification changes do not fix
the race, they just make it a bit harder to hit, but users are still
hitting it just less frequent.

Regards,

Hans





> 
> [1] https://lore.kernel.org/all/CAJZ5v0gS4agVUkCO6M=VW4Wi2y1-s+k23bpzmX9aAZ0Rsv4z9Q@mail.gmail.com/
> 
> Ali Ahmet Memiş (3):
>   ACPI: utils: Add exclusive AML evaluation helpers
>   ACPI: battery: Evaluate battery methods exclusively
>   thermal: intel: int340x: Evaluate _TMP exclusively
> 
>  drivers/acpi/battery.c                        | 17 +++---
>  drivers/acpi/utils.c                          | 56 +++++++++++++++++--
>  .../int340x_thermal/int340x_thermal_zone.c    |  3 +-
>  include/acpi/acpi_bus.h                       |  8 +++
>  4 files changed, 71 insertions(+), 13 deletions(-)
> 
> 
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907

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

[Ali Ahmet Memiş]

On Wed, Oct 14, 2026 at 03:45:10PM +0200, Hans de Goede wrote:
> Hmm, I wonder if we should export the lock added in patch 1/3 and
> then also take that in the ACPI EC interrupt handler ?
>
> The ACPI code already uses a threaded IRQ handler for both EC GPIO
> IRQs and GPEs so sleeping in the handler should be ok.

Sleeping is fine, but the interrupt handler completes the EC
transactions, so taking the lock there would deadlock an exclusive
_BST.  Doing it in acpi_ec_event_processor() where the _Qxx methods
are evaluated, works though.  With the brightness key queries also
running the \GTST copy, 283 of 891 _BST reads were broken without it
and 0 of 804 with it.

> I'm just thinking that if we're going to do this it would be nice to
> cover the bko221065 UCSI case too.

UCSI and _Qxx alone aren't enough there.  I emulated the ECMT users
from the 83DR tables and the EC helpers kept waiting up to ~680 ms of
their 1 s timeout behind GBMD/SBMC until the ideapad methods were
exclusive too.

> Note this is something which can be done on top of this set.

I can send a series on top of this one for the EC queries, UCSI and
ideapad if that sounds good.

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

[Hans de Goede]

Hi,

On 14-Oct-26 17:05, Ali Ahmet Memiş wrote:
> On Wed, Oct 14, 2026 at 03:45:10PM +0200, Hans de Goede wrote:
>> Hmm, I wonder if we should export the lock added in patch 1/3 and
>> then also take that in the ACPI EC interrupt handler ?
>>
>> The ACPI code already uses a threaded IRQ handler for both EC GPIO
>> IRQs and GPEs so sleeping in the handler should be ok.
> 
> Sleeping is fine, but the interrupt handler completes the EC
> transactions, so taking the lock there would deadlock an exclusive
> _BST.  Doing it in acpi_ec_event_processor() where the _Qxx methods
> are evaluated, works though.

Ack, doing it there sounds good to me.

> With the brightness key queries also
> running the \GTST copy, 283 of 891 _BST reads were broken without it
> and 0 of 804 with it.

Nice :)
>> I'm just thinking that if we're going to do this it would be nice to
>> cover the bko221065 UCSI case too.
> 
> UCSI and _Qxx alone aren't enough there.  I emulated the ECMT users
> from the 83DR tables and the EC helpers kept waiting up to ~680 ms of
> their 1 s timeout behind GBMD/SBMC until the ideapad methods were
> exclusive too.
> 
>> Note this is something which can be done on top of this set.
> 
> I can send a series on top of this one for the EC queries, UCSI and
> ideapad if that sounds good.

That sounds great, thank you so much for looking into this.

We should probably ask the reporters of bko221065 to test
the 2 series combined and see if this finally closes that issue
for real.

Regards,

Hans

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

[Rafael J. Wysocki (Intel)]

On Wed, Oct 14, 2026 at 5:30 PM Hans de Goede
<johannes.goede@oss.qualcomm.com> wrote:
>
> Hi,
>
> On 14-Oct-26 17:05, Ali Ahmet Memiş wrote:
> > On Wed, Oct 14, 2026 at 03:45:10PM +0200, Hans de Goede wrote:
> >> Hmm, I wonder if we should export the lock added in patch 1/3 and
> >> then also take that in the ACPI EC interrupt handler ?
> >>
> >> The ACPI code already uses a threaded IRQ handler for both EC GPIO
> >> IRQs and GPEs so sleeping in the handler should be ok.
> >
> > Sleeping is fine, but the interrupt handler completes the EC
> > transactions, so taking the lock there would deadlock an exclusive
> > _BST.  Doing it in acpi_ec_event_processor() where the _Qxx methods
> > are evaluated, works though.
>
> Ack, doing it there sounds good to me.

+1

> > With the brightness key queries also
> > running the \GTST copy, 283 of 891 _BST reads were broken without it
> > and 0 of 804 with it.
>
> Nice :)
> >> I'm just thinking that if we're going to do this it would be nice to
> >> cover the bko221065 UCSI case too.
> >
> > UCSI and _Qxx alone aren't enough there.  I emulated the ECMT users
> > from the 83DR tables and the EC helpers kept waiting up to ~680 ms of
> > their 1 s timeout behind GBMD/SBMC until the ideapad methods were
> > exclusive too.
> >
> >> Note this is something which can be done on top of this set.
> >
> > I can send a series on top of this one for the EC queries, UCSI and
> > ideapad if that sounds good.
>
> That sounds great, thank you so much for looking into this.

+1

> We should probably ask the reporters of bko221065 to test
> the 2 series combined and see if this finally closes that issue
> for real.

Yes, this sounds like a good idea.

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

[Rong Zhang]

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.

Please rephrase the cover letter accordingly.

> 
> [1] https://lore.kernel.org/all/CAJZ5v0gS4agVUkCO6M=VW4Wi2y1-s+k23bpzmX9aAZ0Rsv4z9Q@mail.gmail.com/

You can use Link: tags in the cover letter, e.g.:

Link: https://lore.kernel.org/all/CAJZ5v0gS4agVUkCO6M=VW4Wi2y1-s+k23bpzmX9aAZ0Rsv4z9Q@mail.gmail.com/ [1]
Link: https://bugzilla.kernel.org/show_bug.cgi?id=221065

Thanks,
Rong

> 
> Ali Ahmet Memiş (3):
>   ACPI: utils: Add exclusive AML evaluation helpers
>   ACPI: battery: Evaluate battery methods exclusively
>   thermal: intel: int340x: Evaluate _TMP exclusively
> 
>  drivers/acpi/battery.c                        | 17 +++---
>  drivers/acpi/utils.c                          | 56 +++++++++++++++++--
>  .../int340x_thermal/int340x_thermal_zone.c    |  3 +-
>  include/acpi/acpi_bus.h                       |  8 +++
>  4 files changed, 71 insertions(+), 13 deletions(-)
> 
> 
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907

Re: [PATCH 1/3] ACPI: utils: Add exclusive AML evaluation helpers

[Rong Zhang]

Hi Ali,

On Wed, 2026-10-14 at 16:30 +0300, Ali Ahmet Memiş wrote:
> ACPICA drops the interpreter lock while AML waits, for example around EC
> accesses, so two methods can run interleaved. Some firmware relies on
> methods not interleaving, for instance by switching an EC page selector
> without taking the AML mutex that the other users of that page hold.
> 
> Add acpi_evaluate_object_exclusive() and acpi_evaluate_integer_exclusive()
> which serialize the evaluation under a global mutex, for callers that are
> known to be affected.
> 
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>

There should a Link: following the Suggested-by: tag to provide some
context:

Link: https://lore.kernel.org/all/CAJZ5v0gS4agVUkCO6M=VW4Wi2y1-s+k23bpzmX9aAZ0Rsv4z9Q@mail.gmail.com/

> Cc: stable@vger.kernel.org
> Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>
> ---
>  drivers/acpi/utils.c    | 56 +++++++++++++++++++++++++++++++++++++----
>  include/acpi/acpi_bus.h |  8 ++++++
>  2 files changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index d499b72574ab..f28680424f14 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/types.h>
> @@ -243,10 +244,34 @@ acpi_extract_package(union acpi_object *package,
>  
>  EXPORT_SYMBOL(acpi_extract_package);
>  
> -acpi_status
> -acpi_evaluate_integer(acpi_handle handle,
> -		      acpi_string pathname,
> -		      struct acpi_object_list *arguments, unsigned long long *data)
> +static DEFINE_MUTEX(acpi_exclusive_eval_lock);
> +
> +/**
> + * acpi_evaluate_object_exclusive - Evaluate an object without interleaving
> + * @handle: ACPI handle of the object or its scope.
> + * @pathname: Path of the object relative to @handle.
> + * @arguments: Arguments to pass, or NULL.
> + * @buffer: Buffer for the return value, or NULL.
> + *
> + * Same as acpi_evaluate_object(), but serialized with other exclusive
> + * evaluations.  Use it only for methods known to break when interleaved.
> + */
> +acpi_status acpi_evaluate_object_exclusive(acpi_handle handle,
> +					   acpi_string pathname,
> +					   struct acpi_object_list *arguments,
> +					   struct acpi_buffer *buffer)

A single line can go up to 100 columns when it improves readability.
Reducing line breaks caused by alignment to open parenthesis falls into
the category IMO.

> +{
> +	guard(mutex)(&acpi_exclusive_eval_lock);
> +
> +	return acpi_evaluate_object(handle, pathname, arguments, buffer);
> +}
> +EXPORT_SYMBOL_GPL(acpi_evaluate_object_exclusive);
> +
> +static acpi_status __acpi_evaluate_integer(acpi_handle handle,
> +					   acpi_string pathname,
> +					   struct acpi_object_list *arguments,
> +					   unsigned long long *data,
> +					   bool exclusive)

Ditto.

>  {
>  	acpi_status status = AE_OK;
>  	union acpi_object element;
> @@ -257,7 +282,12 @@ acpi_evaluate_integer(acpi_handle handle,
>  
>  	buffer.length = sizeof(union acpi_object);
>  	buffer.pointer = &element;
> -	status = acpi_evaluate_object(handle, pathname, arguments, &buffer);
> +	if (exclusive)
> +		status = acpi_evaluate_object_exclusive(handle, pathname,
> +							arguments, &buffer);

Ditto.

> +	else
> +		status = acpi_evaluate_object(handle, pathname, arguments,
> +					      &buffer);

Ditto.

>  	if (ACPI_FAILURE(status)) {
>  		acpi_util_eval_error(handle, pathname, status);
>  		return status;
> @@ -275,8 +305,24 @@ acpi_evaluate_integer(acpi_handle handle,
>  	return AE_OK;
>  }
>  
> +acpi_status
> +acpi_evaluate_integer(acpi_handle handle,
> +		      acpi_string pathname,
> +		      struct acpi_object_list *arguments, unsigned long long *data)

Ditto.

Since you are anyway moving the existing function, it'd be better to
improve its coding style as well instead of preserving it as is.

> +{
> +	return __acpi_evaluate_integer(handle, pathname, arguments, data, false);
> +}
>  EXPORT_SYMBOL(acpi_evaluate_integer);
>  
> +acpi_status acpi_evaluate_integer_exclusive(acpi_handle handle,
> +					    acpi_string pathname,
> +					    struct acpi_object_list *arguments,
> +					    unsigned long long *data)

Ditto.

> +{
> +	return __acpi_evaluate_integer(handle, pathname, arguments, data, true);
> +}
> +EXPORT_SYMBOL_GPL(acpi_evaluate_integer_exclusive);
> +
>  int acpi_get_local_u64_address(acpi_handle handle, u64 *addr)
>  {
>  	acpi_status status;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index a10a591c18b2..e7a671dba624 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -32,6 +32,14 @@ acpi_status
>  acpi_evaluate_integer(acpi_handle handle,
>  		      acpi_string pathname,
>  		      struct acpi_object_list *arguments, unsigned long long *data);
> +acpi_status acpi_evaluate_object_exclusive(acpi_handle handle,
> +					   acpi_string pathname,
> +					   struct acpi_object_list *arguments,
> +					   struct acpi_buffer *buffer);
> +acpi_status acpi_evaluate_integer_exclusive(acpi_handle handle,
> +					    acpi_string pathname,
> +					    struct acpi_object_list *arguments,
> +					    unsigned long long *data);

Ditto.

With these comments resolved:

   Reviewed-by: Rong Zhang <i@rong.moe>

Thanks,
Rong

>  bool acpi_evaluate_reference(acpi_handle handle, acpi_string pathname,
>  			     struct acpi_object_list *arguments,
>  			     struct acpi_handle_list *list);
> 
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907

Re: [PATCH 2/3] ACPI: battery: Evaluate battery methods exclusively

[Rong Zhang]

Hi Ali,

On Wed, 2026-10-14 at 16:30 +0300, Ali Ahmet Memiş wrote:
> On ThinkPad P1 Gen2 and P53, INT3403 _TMP switches the EC page selector
> without taking the AML mutex the battery methods use, so _BIX and _BST
> running at the same time can read the wrong page. Since commit d33e89d12295
> ("thermal: core: Suspend thermal zones later and resume them earlier")
> this happens on most resumes.
> 
> Evaluate the battery methods with acpi_evaluate_object_exclusive().
> 
> 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>
> ---
>  drivers/acpi/battery.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 670853ec3a4d..61de3f5be51f 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -583,9 +583,9 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
>  		struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  		acpi_status status = AE_ERROR;
>  
> -		status = acpi_evaluate_object(battery->device->handle,
> -					      use_bix ? "_BIX":"_BIF",
> -					      NULL, &buffer);
> +		status = acpi_evaluate_object_exclusive(battery->device->handle,
> +							use_bix ? "_BIX":"_BIF",

Since you are anyway moving the alignment, let's add spaces around `:' as
well.

> +							NULL, &buffer);

Please merge it into the previous line for readability.

>  
>  		if (ACPI_FAILURE(status)) {
>  			acpi_handle_info(battery->device->handle,
> @@ -624,8 +624,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
>  			msecs_to_jiffies(cache_time)))
>  		return 0;
>  
> -	status = acpi_evaluate_object(battery->device->handle, "_BST",
> -				      NULL, &buffer);
> +	status = acpi_evaluate_object_exclusive(battery->device->handle, "_BST",
> +						NULL, &buffer);

Ditto.

>  	if (ACPI_FAILURE(status)) {
>  		acpi_handle_info(battery->device->handle,
>  				 "_BST evaluation failed: %s",
> @@ -667,6 +667,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
>  
>  static int acpi_battery_set_alarm(struct acpi_battery *battery)
>  {
> +	union acpi_object arg = { .type = ACPI_TYPE_INTEGER };
> +	struct acpi_object_list args = { .count = 1, .pointer = &arg };

You did the same in the ideapad-laptop patch.

Please don't open code when the same pattern is repeatedly used. Add an
acpi_execute_simple_method_exclusive() helper in PATCH 1 and use it here
and there.

With these comments resolved:

   Reviewed-by: Rong Zhang <i@rong.moe>

Thanks,
Rong

>  	acpi_status status = 0;
>  
>  	lockdep_assert_held(&battery->property_lock);
> @@ -675,8 +677,9 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
>  	    !test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags))
>  		return -ENODEV;
>  
> -	status = acpi_execute_simple_method(battery->device->handle, "_BTP",
> -					    battery->alarm);
> +	arg.integer.value = battery->alarm;
> +	status = acpi_evaluate_object_exclusive(battery->device->handle, "_BTP",
> +						&args, NULL);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>  

Re: [PATCH 3/3] thermal: intel: int340x: Evaluate _TMP exclusively

[Rong Zhang]

Hi Ali,

On Wed, 2026-10-14 at 16:30 +0300, Ali Ahmet Memiş wrote:
> On ThinkPad P1 Gen2 and P53, INT3403 _TMP switches the EC page selector
> without taking the mutex the battery methods use, so it must not run
> interleaved with them.
> 
> Evaluate _TMP with acpi_evaluate_integer_exclusive().
> 
> 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>
> ---
>  .../thermal/intel/int340x_thermal/int340x_thermal_zone.c   | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> index 965700ee6962..de0a1fc7b817 100644
> --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -18,7 +18,12 @@ static int int340x_thermal_get_zone_temp(struct thermal_zone_device *zone,
>  	unsigned long long tmp;
>  	acpi_status status;
>  
> -	status = acpi_evaluate_integer(d->adev->handle, "_TMP", NULL, &tmp);
> +	/*
> +	 * On some ThinkPads _TMP switches the EC page selector without
> +	 * taking the mutex the battery methods use.
> +	 */
> +	status = acpi_evaluate_integer_exclusive(d->adev->handle, "_TMP", NULL,
> +						 &tmp);

Please merge it into the previous line.

With it resolved:

   Reviewed-by: Rong Zhang <i@rong.moe>

Thanks,
Rong

>  	if (ACPI_FAILURE(status))
>  		return -EIO;
>  

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

[Rong Zhang]

Hi Ali,

On Wed, 2026-10-14 at 18:05 +0300, Ali Ahmet Memiş wrote:
> On Wed, Oct 14, 2026 at 03:45:10PM +0200, Hans de Goede wrote:
> > Hmm, I wonder if we should export the lock added in patch 1/3 and
> > then also take that in the ACPI EC interrupt handler ?
> > 
> > The ACPI code already uses a threaded IRQ handler for both EC GPIO
> > IRQs and GPEs so sleeping in the handler should be ok.
> 
> Sleeping is fine, but the interrupt handler completes the EC
> transactions, so taking the lock there would deadlock an exclusive
> _BST.  Doing it in acpi_ec_event_processor() where the _Qxx methods
> are evaluated, works though.  With the brightness key queries also
> running the \GTST copy, 283 of 891 _BST reads were broken without it
> and 0 of 804 with it.
> 
> > I'm just thinking that if we're going to do this it would be nice to
> > cover the bko221065 UCSI case too.
> 
> UCSI and _Qxx alone aren't enough there.  I emulated the ECMT users
> from the 83DR tables and the EC helpers kept waiting up to ~680 ms of
> their 1 s timeout behind GBMD/SBMC until the ideapad methods were
> exclusive too.

Nah. Emulation doesn't count at all.

Without a real EC, any call to ECCC or RDER (called by GBMD and SBMC)
times out because CMDB is never reset to 0:

        CMDB = Arg0
        Local0 = 0x0100
        While ((Local0 && CMDB))
        {
            Sleep (0x02)
            Local0--
        }

Thus, any call to ECCC or RDER consumes at least 200ms in emulation, but
in reality it doesn't.

If the EC is really stuck for such a long time in reality, the platform
will shut down or reset itself.

Thanks,
Rong

> 
> > Note this is something which can be done on top of this set.
> 
> I can send a series on top of this one for the EC queries, UCSI and
> ideapad if that sounds good.

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

[Ali Ahmet Memiş]

On Thu, Oct 15, 2026 at 07:51:22PM +0800, Rong Zhang wrote:
> Thus, any call to ECCC or RDER consumes at least 200ms in emulation, but
> in reality it doesn't.

Thanks for finding the actual deadlock!  The 680 ms came from assuming
"slow" EC commands.

I'll wait a few days for Hans, Rafael or anyone else to comment on
dropping the EC query and UCSI patches, then send a v2 that merges
both series.

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

[Hans de Goede]

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), 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 ?

Regards,

Hans


1) I guess ACPI mutexes may be taken nested from the same thread

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

[Rafael J. Wysocki (Intel)]

On Thu, Oct 15, 2026 at 2:09 PM Ali Ahmet Memiş <aliamemis@disroot.org> wrote:
>
> On Thu, Oct 15, 2026 at 07:51:22PM +0800, Rong Zhang wrote:
> > Thus, any call to ECCC or RDER consumes at least 200ms in emulation, but
> > in reality it doesn't.
>
> Thanks for finding the actual deadlock!  The 680 ms came from assuming
> "slow" EC commands.
>
> I'll wait a few days for Hans, Rafael or anyone else to comment on
> dropping the EC query and UCSI patches, then send a v2 that merges
> both series.

I would prefer to drop the EC query and UCSI patches for the time
being until it is confirmed that they actually help.

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

[Rong Zhang]

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.

> 
> Regards,
> 
> Hans
> 
> 
> 1) I guess ACPI mutexes may be taken nested from the same thread

Right. ACPI mutexes are re-entrant. IMO it's a broken design by
encouraging firmware authors to write broken ASL code.

Thanks,
Rong

> 
> 

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