[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

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

[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

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;
>  

[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 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.