Re: [PATCH 6/9] hwmon: (dell-smm) Use new buffer-based WMI API
From: Armin Wolf
Date: Sun Mar 08 2026 - 16:07:42 EST
Am 08.03.26 um 15:52 schrieb Guenter Roeck:
On 3/7/26 16:25, Armin Wolf wrote:
Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for returning the
results of a SMM call.
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
drivers/hwmon/dell-smm-hwmon.c | 47 ++++++++++++----------------------
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 038edffc1ac7..07c05a82dc26 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -12,8 +12,9 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/acpi.h>
+#include <linux/align.h>
#include <linux/capability.h>
+#include <linux/compiler_attributes.h>
#include <linux/cpu.h>
#include <linux/ctype.h>
#include <linux/delay.h>
@@ -36,10 +37,10 @@
#include <linux/thermal.h>
#include <linux/types.h>
#include <linux/uaccess.h>
+#include <linux/unaligned.h>
#include <linux/wmi.h>
#include <linux/i8k.h>
-#include <linux/unaligned.h>
#define I8K_SMM_FN_STATUS 0x0025
#define I8K_SMM_POWER_STATUS 0x0069
@@ -232,7 +233,7 @@ static const struct dell_smm_ops i8k_smm_ops = {
/*
* Call the System Management Mode BIOS over WMI.
*/
-static ssize_t wmi_parse_register(u8 *buffer, u32 length, unsigned int *reg)
+static ssize_t wmi_parse_register(void *buffer, size_t length, unsigned int *reg)
Later in this function:
memcpy_and_pad(&value, sizeof(value), buffer + sizeof(reg_size), reg_size, 0);
So the new code relies on pointer arithmetic on void *. This used to be invalid,
and it seems fragile.
I see, good catch. I will keep "buffer" as a u8 pointer then.
Thanks,
Armin Wolf
{
__le32 value;
u32 reg_size;
@@ -253,7 +254,7 @@ static ssize_t wmi_parse_register(u8 *buffer, u32 length, unsigned int *reg)
return reg_size + sizeof(reg_size);
}
-static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
+static int wmi_parse_response(void *buffer, size_t length, struct smm_regs *regs)
Same here.
Given that those are internal functions, I don't really see the point of changing
the parameter type.
Thanks,
Guenter
{
unsigned int *registers[] = {
®s->eax,
@@ -261,7 +262,7 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
®s->ecx,
®s->edx
};
- u32 offset = 0;
+ size_t offset = 0;
ssize_t ret;
int i;
@@ -273,19 +274,16 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
if (ret < 0)
return ret;
- offset += ret;
+ /* WMI aligns u32 integers on a 4 byte boundary */
+ offset = ALIGN(offset + ret, 4);
}
- if (offset != length)
- return -ENOMSG;
-
return 0;
}
static int wmi_smm_call(struct device *dev, struct smm_regs *regs)
{
struct wmi_device *wdev = container_of(dev, struct wmi_device, dev);
- struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
u32 wmi_payload[] = {
sizeof(regs->eax),
regs->eax,
@@ -296,32 +294,19 @@ static int wmi_smm_call(struct device *dev, struct smm_regs *regs)
sizeof(regs->edx),
regs->edx
};
- const struct acpi_buffer in = {
+ const struct wmi_buffer in = {
.length = sizeof(wmi_payload),
- .pointer = &wmi_payload,
+ .data = &wmi_payload,
};
- union acpi_object *obj;
- acpi_status status;
+ struct wmi_buffer out;
int ret;
- status = wmidev_evaluate_method(wdev, 0x0, DELL_SMM_LEGACY_EXECUTE, &in, &out);
- if (ACPI_FAILURE(status))
- return -EIO;
-
- obj = out.pointer;
- if (!obj)
- return -ENODATA;
-
- if (obj->type != ACPI_TYPE_BUFFER) {
- ret = -ENOMSG;
-
- goto err_free;
- }
-
- ret = wmi_parse_response(obj->buffer.pointer, obj->buffer.length, regs);
+ ret = wmidev_invoke_method(wdev, 0x0, DELL_SMM_LEGACY_EXECUTE, &in, &out);
+ if (ret < 0)
+ return ret;
-err_free:
- kfree(obj);
+ ret = wmi_parse_response(out.data, out.length, regs);
+ kfree(out.data);
return ret;
}