Re: [PATCH v3 7/9] hwmon: (dell-smm) Add support for WMI SMM interface

From: Armin Wolf
Date: Fri Nov 17 2023 - 17:32:21 EST

Am 14.11.23 um 15:12 schrieb Pali Rohár:

On Monday 06 November 2023 07:43:49 Armin Wolf wrote:
Some Dell machines like the Dell Optiplex 7000 do not support
the legacy SMM interface, but instead expect all SMM calls
to be issued over a special WMI interface.
Add support for this interface so users can control the fans
on those machines.

Tested-by: <serverror@xxxxxxxxxxxxx>
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
drivers/hwmon/Kconfig | 1 +
drivers/hwmon/dell-smm-hwmon.c | 198 +++++++++++++++++++++++++++++----
drivers/platform/x86/wmi.c | 1 +
3 files changed, 181 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index cf27523eed5a..76cb05db1dcf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -512,6 +512,7 @@ config SENSORS_DS1621

config SENSORS_DELL_SMM
tristate "Dell laptop SMM BIOS hwmon driver"
+ depends on ACPI_WMI
depends on X86
imply THERMAL
help
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 2547b09929e6..d1bcfd447bb0 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -12,6 +12,7 @@

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/acpi.h>
#include <linux/capability.h>
#include <linux/cpu.h>
#include <linux/ctype.h>
@@ -34,8 +35,10 @@
#include <linux/thermal.h>
#include <linux/types.h>
#include <linux/uaccess.h>
+#include <linux/wmi.h>

#include <linux/i8k.h>
+#include <asm/unaligned.h>

#define I8K_SMM_FN_STATUS 0x0025
#define I8K_SMM_POWER_STATUS 0x0069
@@ -66,6 +69,9 @@
#define I8K_POWER_AC 0x05
#define I8K_POWER_BATTERY 0x01

+#define DELL_SMM_WMI_GUID "F1DDEE52-063C-4784-A11E-8A06684B9B01"
+#define DELL_SMM_LEGACY_EXECUTE 0x1
+
#define DELL_SMM_NO_TEMP 10
#define DELL_SMM_NO_FANS 3

@@ -219,6 +225,102 @@ static const struct dell_smm_ops i8k_smm_ops = {
.smm_call = i8k_smm_call,
};

+/*
+ * Call the System Management Mode BIOS over WMI.
+ */
+static int wmi_parse_register(u8 *buffer, u32 length, int *reg)
+{
+ __le32 value;
+ u32 reg_size;
+
+ if (length <= sizeof(reg_size))
+ return -ENODATA;
+
+ reg_size = get_unaligned_le32(buffer);
+ if (!reg_size || reg_size > sizeof(value))
+ return -ENOMSG;
+
+ if (length < sizeof(reg_size) + reg_size)
+ return -ENODATA;
+
+ memcpy_and_pad(&value, sizeof(value), buffer + sizeof(reg_size), reg_size, 0);
Hello! In one of the patches in this patch series you changed type of
register from unsigned 32 bit integer to signed 32 bit integers. I'm not
sure if this change is really intended and what is the real reason for
it (because there is no explanation for it in the commit message). But
this memcpy_and_pad would not work correctly for signed negative values
because it is the highest bit which indicates negative number.

In my opinion, numbers and registers are unsigned. But if you have
figure out that they has to be treated as signed with possible negative
values then this fact has to be somehow handled.

That change was by mistake, i will send an updated patch series once Hans
has finished his review.

+ *reg = le32_to_cpu(value);
+
+ return (int)(reg_size + sizeof(reg_size));
+}
+
+static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
+{
+ int *registers[] = {
+ &regs->eax,
+ &regs->ebx,
+ &regs->ecx,
+ &regs->edx
+ };
+ u32 offset = 0;
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(registers); i++) {
+ if (offset >= length)
+ return -ENODATA;
+
+ ret = wmi_parse_register(buffer + offset, length - offset, registers[i]);
+ if (ret < 0)
+ return ret;
+
+ offset += ret;
+ }
+
+ 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,
+ sizeof(regs->ebx),
+ regs->ebx,
+ sizeof(regs->ecx),
+ regs->ecx,
+ sizeof(regs->edx),
+ regs->edx
+ };
+ const struct acpi_buffer in = {
+ .length = sizeof(wmi_payload),
+ .pointer = &wmi_payload,
+ };
+ union acpi_object *obj;
+ acpi_status status;
+ 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);
+
+err_free:
+ kfree(obj);
+
+ return ret;
+}
+
...
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index a78ddd83cda0..0b3e63c21d26 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -106,6 +106,7 @@ MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
static const char * const allow_duplicates[] = {
"05901221-D566-11D1-B2F0-00A0C9062910", /* wmi-bmof */
"8A42EA14-4F2A-FD45-6422-0087F7A7E608", /* dell-wmi-ddv */
+ "F1DDEE52-063C-4784-A11E-8A06684B9B01", /* dell-smm-hwmon */
Here you used space instead of TAB after the comma.

You are right, i will fix this in the updated patch series.

Thanks,
Armin Wolf

NULL
};

--
2.39.2