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

["Kurt Borja" <kuurtb@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6113 bytes --]

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 = input_length,
> -		.pointer = input
> +		.length = length,
> +		.pointer = buffer
>  	};
>  	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 = msi_wmi_platform_parse_buffer(obj, output, output_length);
> +	ret = msi_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 = dev_get_drvdata(dev);
> -	u8 input[32] = { 0 };
> -	u8 output[32];
> +	u8 buffer[32] = { 0 };
>  	u16 value;
>  	int ret;
>  
> -	ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_FAN, input, sizeof(input), output,
> -				     sizeof(output));
> +	ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_FAN, buf, sizeof(buf));

s/buf/buffer/

>  	if (ret < 0)
>  		return ret;
>  
> -	value = get_unaligned_be16(&output[channel * 2 + 1]);
> +	value = get_unaligned_be16(&buffer[channel * 2 + 1]);
>  	if (!value)
>  		*val = 0;
>  	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 = msi_wmi_platform_query(data->data, data->method, payload, data->length, data->buffer,
> +	ret = msi_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

-- 
 ~ Kurt

>  				     data->length);
>  	up_write(&data->buffer_lock);
>  
>  	if (ret < 0)
>  		return ret;
>  
> +	down_write(&data->buffer_lock);
> +	memcpy(data->buffer, payload, data->length);
> +	up_write(&data->buffer_lock);
> +
>  	return length;
>  }
>  
> @@ -348,23 +351,21 @@ static int msi_wmi_platform_hwmon_init(struct msi_wmi_platform_data *data)
>  
>  static int msi_wmi_platform_ec_init(struct msi_wmi_platform_data *data)
>  {
> -	u8 input[32] = { 0 };
> -	u8 output[32];
> +	u8 buffer[32] = { 0 };
>  	u8 flags;
>  	int ret;
>  
> -	ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_EC, input, sizeof(input), output,
> -				     sizeof(output));
> +	ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_EC, buffer, sizeof(buffer));
>  	if (ret < 0)
>  		return ret;
>  
> -	flags = output[MSI_PLATFORM_EC_FLAGS_OFFSET];
> +	flags = buffer[MSI_PLATFORM_EC_FLAGS_OFFSET];
>  
>  	dev_dbg(&data->wdev->dev, "EC RAM version %lu.%lu\n",
>  		FIELD_GET(MSI_PLATFORM_EC_MAJOR_MASK, flags),
>  		FIELD_GET(MSI_PLATFORM_EC_MINOR_MASK, flags));
>  	dev_dbg(&data->wdev->dev, "EC firmware version %.28s\n",
> -		&output[MSI_PLATFORM_EC_VERSION_OFFSET]);
> +		&buffer[MSI_PLATFORM_EC_VERSION_OFFSET]);
>  
>  	if (!(flags & MSI_PLATFORM_EC_IS_TIGERLAKE)) {
>  		if (!force)
> @@ -378,27 +379,25 @@ static int msi_wmi_platform_ec_init(struct msi_wmi_platform_data *data)
>  
>  static int msi_wmi_platform_init(struct msi_wmi_platform_data *data)
>  {
> -	u8 input[32] = { 0 };
> -	u8 output[32];
> +	u8 buffer[32] = { 0 };
>  	int ret;
>  
> -	ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_WMI, input, sizeof(input), output,
> -				     sizeof(output));
> +	ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_WMI, buffer, sizeof(buffer));
>  	if (ret < 0)
>  		return ret;
>  
>  	dev_dbg(&data->wdev->dev, "WMI interface version %u.%u\n",
> -		output[MSI_PLATFORM_WMI_MAJOR_OFFSET],
> -		output[MSI_PLATFORM_WMI_MINOR_OFFSET]);
> +		buffer[MSI_PLATFORM_WMI_MAJOR_OFFSET],
> +		buffer[MSI_PLATFORM_WMI_MINOR_OFFSET]);
>  
> -	if (output[MSI_PLATFORM_WMI_MAJOR_OFFSET] != MSI_WMI_PLATFORM_INTERFACE_VERSION) {
> +	if (buffer[MSI_PLATFORM_WMI_MAJOR_OFFSET] != MSI_WMI_PLATFORM_INTERFACE_VERSION) {
>  		if (!force)
>  			return -ENODEV;
>  
>  		dev_warn(&data->wdev->dev,
>  			 "Loading despite unsupported WMI interface version (%u.%u)\n",
> -			 output[MSI_PLATFORM_WMI_MAJOR_OFFSET],
> -			 output[MSI_PLATFORM_WMI_MINOR_OFFSET]);
> +			 buffer[MSI_PLATFORM_WMI_MAJOR_OFFSET],
> +			 buffer[MSI_PLATFORM_WMI_MINOR_OFFSET]);
>  	}
>  
>  	return 0;


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]