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));
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,
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;
--
2.49.0On 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;
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
© 2016 - 2025 Red Hat, Inc.