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 = 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:
signature.asc
Description: PGP signature
