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

[Armin Wolf <W_Armin@xxxxxx>]

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