This driver requires to be able to handle transactions that perform
multiple WMI actions at a time. Therefore, it needs to be able to
lock the wmi_lock mutex for multiple operations.

Add msi_wmi_platform_query_unlocked() to allow the caller to
perform the WMI query without locking the wmi_lock mutex, by
renaming the existing function and adding a new one that only
locks the mutex.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/msi-wmi-platform.c | 27 ++++++++++++++++---------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi-platform.c b/drivers/platform/x86/msi-wmi-platform.c
index 41218a9d6e35d..f0d1b8e1a2fec 100644
--- a/drivers/platform/x86/msi-wmi-platform.c
+++ b/drivers/platform/x86/msi-wmi-platform.c
@@ -140,7 +140,7 @@ static int msi_wmi_platform_parse_buffer(union acpi_object *obj, u8 *output, siz
 	return 0;
 }
 
-static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
+static int msi_wmi_platform_query_unlocked(struct msi_wmi_platform_data *data,
 				  enum msi_wmi_platform_method method, u8 *buffer,
 				  size_t length)
 {
@@ -156,15 +156,9 @@ static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
 	if (!length)
 		return -EINVAL;
 
-	/*
-	 * The ACPI control method responsible for handling the WMI method calls
-	 * is not thread-safe. Because of this we have to do the locking ourself.
-	 */
-	scoped_guard(mutex, &data->wmi_lock) {
-		status = wmidev_evaluate_method(data->wdev, 0x0, method, &in, &out);
-		if (ACPI_FAILURE(status))
-			return -EIO;
-	}
+	status = wmidev_evaluate_method(data->wdev, 0x0, method, &in, &out);
+	if (ACPI_FAILURE(status))
+		return -EIO;
 
 	obj = out.pointer;
 	if (!obj)
@@ -176,6 +170,19 @@ static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
 	return ret;
 }
 
+static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
+				  enum msi_wmi_platform_method method, u8 *buffer,
+				  size_t length)
+{
+	/*
+	 * The ACPI control method responsible for handling the WMI method calls
+	 * is not thread-safe. Because of this we have to do the locking ourself.
+	 */
+	scoped_guard(mutex, &data->wmi_lock) {
+		return msi_wmi_platform_query_unlocked(data, method, buffer, length);
+	}
+}
+
 static umode_t msi_wmi_platform_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 					   u32 attr, int channel)
 {
-- 
2.49.0

Am 11.05.25 um 22:44 schrieb Antheas Kapenekakis:

> This driver requires to be able to handle transactions that perform
> multiple WMI actions at a time. Therefore, it needs to be able to
> lock the wmi_lock mutex for multiple operations.
>
> Add msi_wmi_platform_query_unlocked() to allow the caller to
> perform the WMI query without locking the wmi_lock mutex, by
> renaming the existing function and adding a new one that only
> locks the mutex.
>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   drivers/platform/x86/msi-wmi-platform.c | 27 ++++++++++++++++---------
>   1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/msi-wmi-platform.c b/drivers/platform/x86/msi-wmi-platform.c
> index 41218a9d6e35d..f0d1b8e1a2fec 100644
> --- a/drivers/platform/x86/msi-wmi-platform.c
> +++ b/drivers/platform/x86/msi-wmi-platform.c
> @@ -140,7 +140,7 @@ static int msi_wmi_platform_parse_buffer(union acpi_object *obj, u8 *output, siz
>   	return 0;
>   }
>   
> -static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
> +static int msi_wmi_platform_query_unlocked(struct msi_wmi_platform_data *data,
>   				  enum msi_wmi_platform_method method, u8 *buffer,
>   				  size_t length)
>   {
> @@ -156,15 +156,9 @@ static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
>   	if (!length)
>   		return -EINVAL;
>   
> -	/*
> -	 * The ACPI control method responsible for handling the WMI method calls
> -	 * is not thread-safe. Because of this we have to do the locking ourself.
> -	 */
> -	scoped_guard(mutex, &data->wmi_lock) {
> -		status = wmidev_evaluate_method(data->wdev, 0x0, method, &in, &out);
> -		if (ACPI_FAILURE(status))
> -			return -EIO;
> -	}
> +	status = wmidev_evaluate_method(data->wdev, 0x0, method, &in, &out);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
>   
>   	obj = out.pointer;
>   	if (!obj)
> @@ -176,6 +170,19 @@ static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
>   	return ret;
>   }
>   
> +static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
> +				  enum msi_wmi_platform_method method, u8 *buffer,
> +				  size_t length)
> +{
> +	/*
> +	 * The ACPI control method responsible for handling the WMI method calls
> +	 * is not thread-safe. Because of this we have to do the locking ourself.
> +	 */
> +	scoped_guard(mutex, &data->wmi_lock) {
> +		return msi_wmi_platform_query_unlocked(data, method, buffer, length);
> +	}

Please just use guard() here.

Thanks,
Armin Wolf

> +}
> +
>   static umode_t msi_wmi_platform_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>   					   u32 attr, int channel)
>   {

On Sun May 11, 2025 at 5:44 PM -03, Antheas Kapenekakis wrote:
> This driver requires to be able to handle transactions that perform
> multiple WMI actions at a time. Therefore, it needs to be able to
> lock the wmi_lock mutex for multiple operations.
>
> Add msi_wmi_platform_query_unlocked() to allow the caller to
> perform the WMI query without locking the wmi_lock mutex, by
> renaming the existing function and adding a new one that only
> locks the mutex.
>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>

You only use msi_wmi_platform_query_unlocked() to protect the
fan_curve/AP state right?

If that's the case I think we don't need it. AFAIK sysfs reads/writes
are already synchronized/locked, and as I mentioned in Patch 10, I don't
think you need this variant in probe/remove either.

I'd like to hear more opinions on this though.

-- 
 ~ Kurt

Am 12.05.25 um 21:21 schrieb Kurt Borja:

> On Sun May 11, 2025 at 5:44 PM -03, Antheas Kapenekakis wrote:
>> This driver requires to be able to handle transactions that perform
>> multiple WMI actions at a time. Therefore, it needs to be able to
>> lock the wmi_lock mutex for multiple operations.
>>
>> Add msi_wmi_platform_query_unlocked() to allow the caller to
>> perform the WMI query without locking the wmi_lock mutex, by
>> renaming the existing function and adding a new one that only
>> locks the mutex.
>>
>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> You only use msi_wmi_platform_query_unlocked() to protect the
> fan_curve/AP state right?
>
> If that's the case I think we don't need it. AFAIK sysfs reads/writes
> are already synchronized/locked, and as I mentioned in Patch 10, I don't
> think you need this variant in probe/remove either.
>
> I'd like to hear more opinions on this though.
>
The reason why we want such a function is that sometimes we need to perform
read-modify-write operations over the WMI interface, forcing us to prevent
other clients from calling a WMI method.

Thanks,
Armin Wolf