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

[Armin Wolf <W_Armin@xxxxxx>]

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@xxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> > Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> > ---
> >   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