1. Patchew
  2. linux
  3. [PATCH 0/9] Convert most Dell WMI drivers to use the new buffer-based API
  4. View patch

[PATCH 6/9] hwmon: (dell-smm) Use new buffer-based WMI API

Armin Wolf posted 9 patches 1 month ago
There is a newer version of this series
[PATCH 6/9] hwmon: (dell-smm) Use new buffer-based WMI API
Posted by Armin Wolf 1 month ago 
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@gmx.de>
---
 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)
 {
 	__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)
 {
 	unsigned int *registers[] = {
 		&regs->eax,
@@ -261,7 +262,7 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
 		&regs->ecx,
 		&regs->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;
 }
-- 
2.39.5
Re: [PATCH 6/9] hwmon: (dell-smm) Use new buffer-based WMI API
Posted by Guenter Roeck 1 month ago 
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@gmx.de>
> ---
>   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.

>   {
>   	__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[] = {
>   		&regs->eax,
> @@ -261,7 +262,7 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
>   		&regs->ecx,
>   		&regs->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;
>   }
Re: [PATCH 6/9] hwmon: (dell-smm) Use new buffer-based WMI API
Posted by Armin Wolf 1 month ago 
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@gmx.de>
>> ---
>>   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[] = {
>>           &regs->eax,
>> @@ -261,7 +262,7 @@ static int wmi_parse_response(u8 *buffer, u32 
>> length, struct smm_regs *regs)
>>           &regs->ecx,
>>           &regs->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;
>>   }
>
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.