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: <[email protected]>
>> Signed-off-by: Armin Wolf <[email protected]>
>> ---
>> 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[] = {
>> + ®s->eax,
>> + ®s->ebx,
>> + ®s->ecx,
>> + ®s->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
>>