Re: [PATCH v1 01/10] platform/x86: msi-wmi-platform: Use input buffer for returning result

[Armin Wolf <W_Armin@gmx.de>]

Am 12.05.25 um 01:31 schrieb Kurt Borja:

> On Sun May 11, 2025 at 5:44 PM -03, Antheas Kapenekakis wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> Modify msi_wmi_platform_query() to reuse the input buffer for
>> returning the result of a WMI method call. Using a separate output
>> buffer to return the result is unnecessary because the WMI interface
>> requires both buffers to have the same length anyway.
>>
>> Co-developed-by: Antheas Kapenekakis <lkml@antheas.dev>
>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/msi-wmi-platform.c | 53 ++++++++++++-------------
>>   1 file changed, 26 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/platform/x86/msi-wmi-platform.c b/drivers/platform/x86/msi-wmi-platform.c
>> index dc5e9878cb682..41218a9d6e35d 100644
>> --- a/drivers/platform/x86/msi-wmi-platform.c
>> +++ b/drivers/platform/x86/msi-wmi-platform.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/mutex.h>
>>   #include <linux/printk.h>
>>   #include <linux/rwsem.h>
>> +#include <linux/string.h>
>>   #include <linux/types.h>
>>   #include <linux/wmi.h>
>>   
>> @@ -140,19 +141,19 @@ static int msi_wmi_platform_parse_buffer(union acpi_object *obj, u8 *output, siz
>>   }
>>   
>>   static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
>> -				  enum msi_wmi_platform_method method, u8 *input,
>> -				  size_t input_length, u8 *output, size_t output_length)
>> +				  enum msi_wmi_platform_method method, u8 *buffer,
>> +				  size_t length)
>>   {
>>   	struct acpi_buffer out = ACPI_ALLOCATE_BUFFER, NULL };
>>   	struct acpi_buffer in =
>> -		.length = nput_length,
>> -		.pointer =nput
>> +		.length =ength,
>> +		.pointer =uffer
>>   	};
>>   	union acpi_object *obj;
>>   	acpi_status status;
>>   	int ret;
>>   
>> -	if (!input_length || !output_length)
>> +	if (!length)
>>   		return -EINVAL;
>>   
>>   	/*
>> @@ -169,7 +170,7 @@ static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
>>   	if (!obj)
>>   		return -ENODATA;
>>   
>> -	ret =si_wmi_platform_parse_buffer(obj, output, output_length);
>> +	ret =si_wmi_platform_parse_buffer(obj, buffer, length);
>>   	kfree(obj);
>>   
>>   	return ret;
>> @@ -185,17 +186,15 @@ static int msi_wmi_platform_read(struct device *dev, enum hwmon_sensor_types typ
>>   				 int channel, long *val)
>>   {
>>   	struct msi_wmi_platform_data *data =ev_get_drvdata(dev);
>> -	u8 input[32] = 0 };
>> -	u8 output[32];
>> +	u8 buffer[32] = 0 };
>>   	u16 value;
>>   	int ret;
>>   
>> -	ret =si_wmi_platform_query(data, MSI_PLATFORM_GET_FAN, input, sizeof(input), output,
>> -				     sizeof(output));
>> +	ret =si_wmi_platform_query(data, MSI_PLATFORM_GET_FAN, buf, sizeof(buf));
> s/buf/buffer/
>
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	value =et_unaligned_be16(&output[channel * 2 + 1]);
>> +	value =et_unaligned_be16(&buffer[channel * 2 + 1]);
>>   	if (!value)
>>   		*val =;
>>   	else
>> @@ -245,13 +244,17 @@ static ssize_t msi_wmi_platform_write(struct file *fp, const char __user *input,
>>   		return ret;
>>   
>>   	down_write(&data->buffer_lock);
>> -	ret =si_wmi_platform_query(data->data, data->method, payload, data->length, data->buffer,
>> +	ret =si_wmi_platform_query(data->data, data->method, data->buffer,
> Is this logic right? Shouldn't we pass payload instead of data->buffer?
>
> Better yet, I think we should write the payload directly to
> data->buffer and drop the memcpy hunk bellow
>
You are right that we indeed pass the wrong buffer here, but we should only update data->buffer
if msi_wmi_platform_query() was successful. That why we have the call to memcpy().

Thanks,
Armin Wolf