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

[Armin Wolf <W_Armin@gmx.de>]

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@serverror.com>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   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
>>