[PATCH 0/9] Convert most Dell WMI drivers to use the new buffer-based API

[PATCH 0/9] Convert most Dell WMI drivers to use the new buffer-based API

[Armin Wolf]

The new buffer-based WMI API improves the compatibility between
different ACPI firmware implementations by performing marshalling/
unmarshalling of WMI buffers like the original Windows driver.

Convert most Dell WMI drivers to use this new API. This also
removes all ACPI-related code from most drivers because the
new buffer-based WMI API uses abstract WMI buffer objects instead
of ACPI objects.

All drivers have been tested on a Dell Inspiron 3505 and appear
to work normally.

The last three patches contain some misc. cleanups for the WMI
driver core itself. The most important change is a fix for modprobe
to verify any WMI GUID strings from WMI drivers and convert them
to uppercase if necessary. This should fix autoloading for WMI
drivers that use WMI GUID strings with lowercase letters.

Armin Wolf (9):
  platform/x86: dell-descriptor: Use new buffer-based WMI API
  platform/x86: dell-privacy: Use new buffer-based WMI API
  platform/x86: dell-smbios-wmi: Use new buffer-based WMI API
  platform/x86: dell-wmi-base: Use new buffer-based WMI API
  platform/x86: dell-ddv: Use new buffer-based WMI API
  hwmon: (dell-smm) Use new buffer-based WMI API
  platform/wmi: Make wmi_bus_class const
  platform/wmi: Make sysfs attributes const
  modpost: Handle malformed WMI GUID strings

 .../wmi/driver-development-guide.rst          |   2 +-
 drivers/hwmon/dell-smm-hwmon.c                |  47 ++---
 drivers/platform/wmi/core.c                   |  34 +--
 drivers/platform/x86/dell/dell-smbios-wmi.c   |  46 +++--
 drivers/platform/x86/dell/dell-wmi-base.c     |  68 +++---
 drivers/platform/x86/dell/dell-wmi-ddv.c      | 194 ++++++++++--------
 .../platform/x86/dell/dell-wmi-descriptor.c   |  96 ++++-----
 drivers/platform/x86/dell/dell-wmi-privacy.c  |  78 ++++---
 scripts/mod/file2alias.c                      |  28 ++-
 9 files changed, 317 insertions(+), 276 deletions(-)

-- 
2.39.5

[PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Armin Wolf]

Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for the descriptor.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 .../platform/x86/dell/dell-wmi-descriptor.c   | 96 ++++++++++---------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-descriptor.c b/drivers/platform/x86/dell/dell-wmi-descriptor.c
index c2a180202719..621502368895 100644
--- a/drivers/platform/x86/dell/dell-wmi-descriptor.c
+++ b/drivers/platform/x86/dell/dell-wmi-descriptor.c
@@ -7,7 +7,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/acpi.h>
+#include <linux/compiler_attributes.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/wmi.h>
@@ -15,6 +15,24 @@
 
 #define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
+/*
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ *       Name             Offset  Length  Value
+ * Vendor Signature          0       4    "DELL"
+ * Object Signature          4       4    " WMI"
+ * WMI Interface Version     8       4    <version>
+ * WMI buffer length        12       4    <length>
+ * WMI hotfix number        16       4    <hotfix>
+ */
+struct descriptor {
+	__le32 vendor_signature;
+	__le32 object_signature;
+	__le32 interface_version;
+	__le32 buffer_length;
+	__le32 hotfix_number;
+} __packed;
+
 struct descriptor_priv {
 	struct list_head list;
 	u32 interface_version;
@@ -88,76 +106,60 @@ bool dell_wmi_get_hotfix(u32 *hotfix)
 }
 EXPORT_SYMBOL_GPL(dell_wmi_get_hotfix);
 
-/*
- * Descriptor buffer is 128 byte long and contains:
- *
- *       Name             Offset  Length  Value
- * Vendor Signature          0       4    "DELL"
- * Object Signature          4       4    " WMI"
- * WMI Interface Version     8       4    <version>
- * WMI buffer length        12       4    <length>
- * WMI hotfix number        16       4    <hotfix>
- */
-static int dell_wmi_descriptor_probe(struct wmi_device *wdev,
-				     const void *context)
+static int dell_wmi_descriptor_probe(struct wmi_device *wdev, const void *context)
 {
-	union acpi_object *obj = NULL;
 	struct descriptor_priv *priv;
-	u32 *buffer;
+	struct wmi_buffer buffer;
+	struct descriptor *desc;
 	int ret;
 
-	obj = wmidev_block_query(wdev, 0);
-	if (!obj) {
-		dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
-		ret = -EIO;
-		goto out;
-	}
+	ret = wmidev_query_block(wdev, 0, &buffer);
+	if (ret < 0)
+		return ret;
 
-	if (obj->type != ACPI_TYPE_BUFFER) {
-		dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
+	if (buffer.length < sizeof(*desc)) {
+		dev_err(&wdev->dev,
+			"Dell descriptor buffer contains not enough data (%zu)\n",
+			buffer.length);
 		ret = -EINVAL;
 		descriptor_valid = ret;
 		goto out;
 	}
 
-	/* Although it's not technically a failure, this would lead to
-	 * unexpected behavior
-	 */
-	if (obj->buffer.length != 128) {
-		dev_err(&wdev->dev,
-			"Dell descriptor buffer has unexpected length (%d)\n",
-			obj->buffer.length);
-		ret = -EINVAL;
+	desc = buffer.data;
+
+	/* "DELL" */
+	if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {
+		dev_err(&wdev->dev, "Dell descriptor buffer has invalid vendor signature (%u)\n",
+			le32_to_cpu(desc->vendor_signature));
+		ret = -ENOMSG;
 		descriptor_valid = ret;
 		goto out;
 	}
 
-	buffer = (u32 *)obj->buffer.pointer;
-
-	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
-		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
-			buffer);
-		ret = -EINVAL;
+	/* " WMI" */
+	if (le32_to_cpu(desc->object_signature) != 0x494D5720) {
+		dev_err(&wdev->dev, "Dell descriptor buffer has invalid object signature (%u)\n",
+			le32_to_cpu(desc->object_signature));
+		ret = -ENOMSG;
 		descriptor_valid = ret;
 		goto out;
 	}
 	descriptor_valid = 0;
 
-	if (buffer[2] != 0 && buffer[2] != 1)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%lu)\n",
-			(unsigned long) buffer[2]);
-
-	priv = devm_kzalloc(&wdev->dev, sizeof(struct descriptor_priv),
-	GFP_KERNEL);
+	if (le32_to_cpu(desc->interface_version) > 2)
+		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
+			 le32_to_cpu(desc->interface_version));
 
+	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
-	priv->interface_version = buffer[2];
-	priv->size = buffer[3];
-	priv->hotfix = buffer[4];
+	priv->interface_version = le32_to_cpu(desc->interface_version);
+	priv->size = le32_to_cpu(desc->buffer_length);
+	priv->hotfix = le32_to_cpu(desc->hotfix_number);
 	ret = 0;
 	dev_set_drvdata(&wdev->dev, priv);
 	mutex_lock(&list_mutex);
@@ -170,7 +172,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device *wdev,
 		(unsigned long) priv->hotfix);
 
 out:
-	kfree(obj);
+	kfree(buffer.data);
 	return ret;
 }
 
-- 
2.39.5

[PATCH 2/9] platform/x86: dell-privacy: Use new buffer-based WMI API

[Armin Wolf]

Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for the device state.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-privacy.c | 78 ++++++++++----------
 1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-privacy.c b/drivers/platform/x86/dell/dell-wmi-privacy.c
index ed099a431ea4..470273cc2fd2 100644
--- a/drivers/platform/x86/dell/dell-wmi-privacy.c
+++ b/drivers/platform/x86/dell/dell-wmi-privacy.c
@@ -9,6 +9,7 @@
 
 #include <linux/acpi.h>
 #include <linux/bitops.h>
+#include <linux/compiler_attributes.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/list.h>
@@ -25,6 +26,26 @@
 #define DELL_PRIVACY_CAMERA_EVENT 0x2
 #define led_to_priv(c)       container_of(c, struct privacy_wmi_data, cdev)
 
+/*
+ * Describes the Device State class exposed by BIOS which can be consumed by
+ * various applications interested in knowing the Privacy feature capabilities.
+ * class DeviceState
+ * {
+ *  [key, read] string InstanceName;
+ *  [read] boolean ReadOnly;
+ *
+ *  [WmiDataId(1), read] uint32 DevicesSupported;
+ *   0 - None; 0x1 - Microphone; 0x2 - Camera; 0x4 - ePrivacy  Screen
+ *
+ *  [WmiDataId(2), read] uint32 CurrentState;
+ *   0 - Off; 1 - On; Bit0 - Microphone; Bit1 - Camera; Bit2 - ePrivacyScreen
+ * };
+ */
+struct device_state {
+	__le32 devices_supported;
+	__le32 current_state;
+} __packed;
+
 /*
  * The wmi_list is used to store the privacy_priv struct with mutex protecting
  */
@@ -185,59 +206,36 @@ static struct attribute *privacy_attrs[] = {
 };
 ATTRIBUTE_GROUPS(privacy);
 
-/*
- * Describes the Device State class exposed by BIOS which can be consumed by
- * various applications interested in knowing the Privacy feature capabilities.
- * class DeviceState
- * {
- *  [key, read] string InstanceName;
- *  [read] boolean ReadOnly;
- *
- *  [WmiDataId(1), read] uint32 DevicesSupported;
- *   0 - None; 0x1 - Microphone; 0x2 - Camera; 0x4 - ePrivacy  Screen
- *
- *  [WmiDataId(2), read] uint32 CurrentState;
- *   0 - Off; 1 - On; Bit0 - Microphone; Bit1 - Camera; Bit2 - ePrivacyScreen
- * };
- */
 static int get_current_status(struct wmi_device *wdev)
 {
 	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
-	union acpi_object *obj_present;
-	u32 *buffer;
-	int ret = 0;
+	struct device_state *state;
+	struct wmi_buffer buffer;
+	int ret;
 
 	if (!priv) {
 		dev_err(&wdev->dev, "dell privacy priv is NULL\n");
 		return -EINVAL;
 	}
+
 	/* check privacy support features and device states */
-	obj_present = wmidev_block_query(wdev, 0);
-	if (!obj_present) {
-		dev_err(&wdev->dev, "failed to read Binary MOF\n");
-		return -EIO;
-	}
+	ret = wmidev_query_block(wdev, 0, &buffer);
+	if (ret < 0)
+		return ret;
 
-	if (obj_present->type != ACPI_TYPE_BUFFER) {
-		dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
-		ret = -EIO;
-		goto obj_free;
-	}
-	/*  Although it's not technically a failure, this would lead to
-	 *  unexpected behavior
-	 */
-	if (obj_present->buffer.length != 8) {
-		dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
-				obj_present->buffer.length);
+	if (buffer.length < sizeof(*state)) {
+		dev_err(&wdev->dev, "Dell privacy buffer contains not enough data (%zu)!\n",
+			buffer.length);
 		ret = -EINVAL;
-		goto obj_free;
+		goto buffer_free;
 	}
-	buffer = (u32 *)obj_present->buffer.pointer;
-	priv->features_present = buffer[0];
-	priv->last_status = buffer[1];
 
-obj_free:
-	kfree(obj_present);
+	state = buffer.data;
+	priv->features_present = le32_to_cpu(state->devices_supported);
+	priv->last_status = le32_to_cpu(state->current_state);
+
+buffer_free:
+	kfree(buffer.data);
 	return ret;
 }
 
-- 
2.39.5

[PATCH 3/9] platform/x86: dell-smbios-wmi: Use new buffer-based WMI API

[Armin Wolf]

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 SMBIOS call.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-smbios-wmi.c | 46 +++++++++++----------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
index a7dca8c59d60..3c05b48354b3 100644
--- a/drivers/platform/x86/dell/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell/dell-smbios-wmi.c
@@ -50,38 +50,42 @@ static inline struct wmi_smbios_priv *get_first_smbios_priv(void)
 
 static int run_smbios_call(struct wmi_device *wdev)
 {
-	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
-	struct wmi_smbios_priv *priv;
-	struct acpi_buffer input;
-	union acpi_object *obj;
-	acpi_status status;
-
-	priv = dev_get_drvdata(&wdev->dev);
-	input.length = priv->req_buf_size - sizeof(u64);
-	input.pointer = &priv->buf->std;
+	struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
+	const struct wmi_buffer input = {
+		.length = priv->req_buf_size - sizeof(u64),
+		.data = &priv->buf->std,
+	};
+	struct wmi_buffer output;
+	int ret;
 
 	dev_dbg(&wdev->dev, "evaluating: %u/%u [%x,%x,%x,%x]\n",
 		priv->buf->std.cmd_class, priv->buf->std.cmd_select,
 		priv->buf->std.input[0], priv->buf->std.input[1],
 		priv->buf->std.input[2], priv->buf->std.input[3]);
 
-	status = wmidev_evaluate_method(wdev, 0, 1, &input, &output);
-	if (ACPI_FAILURE(status))
-		return -EIO;
-	obj = (union acpi_object *)output.pointer;
-	if (obj->type != ACPI_TYPE_BUFFER) {
-		dev_dbg(&wdev->dev, "received type: %d\n", obj->type);
-		if (obj->type == ACPI_TYPE_INTEGER)
-			dev_dbg(&wdev->dev, "SMBIOS call failed: %llu\n",
-				obj->integer.value);
-		kfree(output.pointer);
+	ret = wmidev_invoke_method(wdev, 0, 1, &input, &output);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * The output buffer returned by the WMI method should have at least the size
+	 * of the input buffer. Because the Windows WMI implementation ignores any surplus
+	 * data returned by a WMI method call we emulate this behavior here.
+	 *
+	 * Additionally the ACPI firmware might return buffers with not enough data to
+	 * signal an error, so we only print a debug message here.
+	 */
+	if (output.length < input.length) {
+		dev_dbg(&wdev->dev, "SMBIOS call returned not enough data (%zu)\n", output.length);
+		kfree(output.data);
 		return -EIO;
 	}
-	memcpy(input.pointer, obj->buffer.pointer, obj->buffer.length);
+
+	memcpy(input.data, output.data, input.length);
 	dev_dbg(&wdev->dev, "result: [%08x,%08x,%08x,%08x]\n",
 		priv->buf->std.output[0], priv->buf->std.output[1],
 		priv->buf->std.output[2], priv->buf->std.output[3]);
-	kfree(output.pointer);
+	kfree(output.data);
 
 	return 0;
 }
-- 
2.39.5

[PATCH 4/9] platform/x86: dell-wmi-base: Use new buffer-based WMI API

[Armin Wolf]

Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for the event data.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-base.c | 68 ++++++++++++-----------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-base.c b/drivers/platform/x86/dell/dell-wmi-base.c
index 4eefbade2f5e..4a7ab9fb3f81 100644
--- a/drivers/platform/x86/dell/dell-wmi-base.c
+++ b/drivers/platform/x86/dell/dell-wmi-base.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/compiler_attributes.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -408,7 +409,8 @@ static void dell_wmi_switch_event(struct input_dev **subdev,
 	input_sync(*subdev);
 }
 
-static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, u16 *buffer, int remaining)
+static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, __le16 *buffer,
+				int remaining)
 {
 	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
 	const struct key_entry *key;
@@ -440,15 +442,15 @@ static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, u16
 	} else if (type == 0x0011 && code == 0xe070 && remaining > 0) {
 		dell_wmi_switch_event(&priv->tabletswitch_dev,
 				      "Dell tablet mode switch",
-				      SW_TABLET_MODE, !buffer[0]);
+				      SW_TABLET_MODE, !le16_to_cpu(buffer[0]));
 		return 1;
 	} else if (type == 0x0012 && code == 0x000c && remaining > 0) {
 		/* Eprivacy toggle, switch to "on" key entry for on events */
-		if (buffer[0] == 2)
+		if (le16_to_cpu(buffer[0]) == 2)
 			key++;
 		used = 1;
 	} else if (type == 0x0012 && code == 0x000d && remaining > 0) {
-		value = (buffer[2] == 2);
+		value = (le16_to_cpu(buffer[2]) == 2);
 		used = 1;
 	}
 
@@ -457,24 +459,17 @@ static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, u16
 	return used;
 }
 
-static void dell_wmi_notify(struct wmi_device *wdev,
-			    union acpi_object *obj)
+static void dell_wmi_notify(struct wmi_device *wdev, const struct wmi_buffer *buffer)
 {
 	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
-	u16 *buffer_entry, *buffer_end;
-	acpi_size buffer_size;
+	__le16 *buffer_entry, *buffer_end;
+	size_t buffer_size;
 	int len, i;
 
-	if (obj->type != ACPI_TYPE_BUFFER) {
-		pr_warn("bad response type %x\n", obj->type);
-		return;
-	}
+	pr_debug("Received WMI event (%*ph)\n", (int)buffer->length, buffer->data);
 
-	pr_debug("Received WMI event (%*ph)\n",
-		obj->buffer.length, obj->buffer.pointer);
-
-	buffer_entry = (u16 *)obj->buffer.pointer;
-	buffer_size = obj->buffer.length/2;
+	buffer_entry = buffer->data;
+	buffer_size = buffer->length / 2;
 	buffer_end = buffer_entry + buffer_size;
 
 	/*
@@ -490,12 +485,12 @@ static void dell_wmi_notify(struct wmi_device *wdev,
 	 * one event on devices with WMI interface version 0.
 	 */
 	if (priv->interface_version == 0 && buffer_entry < buffer_end)
-		if (buffer_end > buffer_entry + buffer_entry[0] + 1)
-			buffer_end = buffer_entry + buffer_entry[0] + 1;
+		if (buffer_end > buffer_entry + le16_to_cpu(buffer_entry[0]) + 1)
+			buffer_end = buffer_entry + le16_to_cpu(buffer_entry[0]) + 1;
 
 	while (buffer_entry < buffer_end) {
 
-		len = buffer_entry[0];
+		len = le16_to_cpu(buffer_entry[0]);
 		if (len == 0)
 			break;
 
@@ -508,11 +503,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,
 
 		pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
 
-		switch (buffer_entry[1]) {
+		switch (le16_to_cpu(buffer_entry[1])) {
 		case 0x0000: /* One key pressed or event occurred */
 			if (len > 2)
-				dell_wmi_process_key(wdev, buffer_entry[1],
-						     buffer_entry[2],
+				dell_wmi_process_key(wdev, le16_to_cpu(buffer_entry[1]),
+						     le16_to_cpu(buffer_entry[2]),
 						     buffer_entry + 3,
 						     len - 3);
 			/* Extended data is currently ignored */
@@ -520,22 +515,29 @@ static void dell_wmi_notify(struct wmi_device *wdev,
 		case 0x0010: /* Sequence of keys pressed */
 		case 0x0011: /* Sequence of events occurred */
 			for (i = 2; i < len; ++i)
-				i += dell_wmi_process_key(wdev, buffer_entry[1],
-							  buffer_entry[i],
+				i += dell_wmi_process_key(wdev, le16_to_cpu(buffer_entry[1]),
+							  le16_to_cpu(buffer_entry[i]),
 							  buffer_entry + i,
 							  len - i - 1);
 			break;
 		case 0x0012:
-			if ((len > 4) && dell_privacy_process_event(buffer_entry[1], buffer_entry[3],
-								    buffer_entry[4]))
-				/* dell_privacy_process_event has handled the event */;
-			else if (len > 2)
-				dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2],
+			if (len > 4) {
+				if (dell_privacy_process_event(le16_to_cpu(buffer_entry[1]),
+							       le16_to_cpu(buffer_entry[3]),
+							       le16_to_cpu(buffer_entry[4])))
+					break;
+			}
+
+			/* dell_privacy_process_event has not handled the event */
+
+			if (len > 2)
+				dell_wmi_process_key(wdev, le16_to_cpu(buffer_entry[1]),
+						     le16_to_cpu(buffer_entry[2]),
 						     buffer_entry + 3, len - 3);
+
 			break;
 		default: /* Unknown event */
-			pr_info("Unknown WMI event type 0x%x\n",
-				(int)buffer_entry[1]);
+			pr_info("Unknown WMI event type 0x%x\n", le16_to_cpu(buffer_entry[1]));
 			break;
 		}
 
@@ -821,7 +823,7 @@ static struct wmi_driver dell_wmi_driver = {
 	.id_table = dell_wmi_id_table,
 	.probe = dell_wmi_probe,
 	.remove = dell_wmi_remove,
-	.notify = dell_wmi_notify,
+	.notify_new = dell_wmi_notify,
 };
 
 static int __init dell_wmi_init(void)
-- 
2.39.5

[PATCH 5/9] platform/x86: dell-ddv: Use new buffer-based WMI API

[Armin Wolf]

Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI intergers/strings/packages
for exchanging data.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 194 ++++++++++++-----------
 1 file changed, 105 insertions(+), 89 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 62e3d060f038..a744fd21b8af 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -7,8 +7,8 @@
 
 #define pr_format(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/acpi.h>
 #include <linux/bitfield.h>
+#include <linux/compiler_attributes.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/device/driver.h>
@@ -99,6 +99,11 @@ enum dell_ddv_method {
 	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
 };
 
+struct dell_wmi_buffer {
+	__le32 raw_size;
+	u8 raw_data[];
+} __packed;
+
 struct fan_sensor_entry {
 	u8 type;
 	__le16 rpm;
@@ -126,7 +131,7 @@ struct dell_wmi_ddv_sensors {
 	bool active;
 	struct mutex lock;	/* protect caching */
 	unsigned long timestamp;
-	union acpi_object *obj;
+	struct dell_wmi_buffer *buffer;
 	u64 entries;
 };
 
@@ -158,105 +163,122 @@ static const char * const fan_dock_labels[] = {
 	"Docking Chipset Fan",
 };
 
-static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
-				   union acpi_object **result, acpi_object_type type)
+static int dell_wmi_ddv_query(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
+			      struct wmi_buffer *output)
 {
-	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
-	const struct acpi_buffer in = {
-		.length = sizeof(arg),
-		.pointer = &arg,
+	__le32 arg2 = cpu_to_le32(arg);
+	const struct wmi_buffer input = {
+		.length = sizeof(arg2),
+		.data = &arg2,
 	};
-	union acpi_object *obj;
-	acpi_status ret;
-
-	ret = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
-	if (ACPI_FAILURE(ret))
-		return -EIO;
-
-	obj = out.pointer;
-	if (!obj)
-		return -ENODATA;
 
-	if (obj->type != type) {
-		kfree(obj);
-		return -ENOMSG;
-	}
-
-	*result = obj;
-
-	return 0;
+	return wmidev_invoke_method(wdev, 0x0, method, &input, output);
 }
 
 static int dell_wmi_ddv_query_integer(struct wmi_device *wdev, enum dell_ddv_method method,
 				      u32 arg, u32 *res)
 {
-	union acpi_object *obj;
+	struct wmi_buffer output;
+	__le32 *argr;
 	int ret;
 
-	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_INTEGER);
+	ret = dell_wmi_ddv_query(wdev, method, arg, &output);
 	if (ret < 0)
 		return ret;
 
-	if (obj->integer.value <= U32_MAX)
-		*res = (u32)obj->integer.value;
-	else
-		ret = -ERANGE;
+	if (output.length >= sizeof(*argr)) {
+		argr = output.data;
+		*res = le32_to_cpu(*argr);
+	} else {
+		ret = -EIO;
+	}
 
-	kfree(obj);
+	kfree(output.data);
 
 	return ret;
 }
 
 static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_method method,
-				     u32 arg, union acpi_object **result)
+				     u32 arg, struct dell_wmi_buffer **result)
 {
-	union acpi_object *obj;
-	u64 buffer_size;
+	struct dell_wmi_buffer *buffer;
+	struct wmi_buffer output;
+	size_t buffer_size;
 	int ret;
 
-	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_PACKAGE);
+	ret = dell_wmi_ddv_query(wdev, method, arg, &output);
 	if (ret < 0)
 		return ret;
 
-	if (obj->package.count != 2 ||
-	    obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
-	    obj->package.elements[1].type != ACPI_TYPE_BUFFER) {
-		ret = -ENOMSG;
+	if (output.length < sizeof(*buffer)) {
+		ret = -EIO;
 
 		goto err_free;
 	}
 
-	buffer_size = obj->package.elements[0].integer.value;
-
-	if (!buffer_size) {
+	buffer = output.data;
+	if (!le32_to_cpu(buffer->raw_size)) {
 		ret = -ENODATA;
 
 		goto err_free;
 	}
 
-	if (buffer_size > obj->package.elements[1].buffer.length) {
+	buffer_size = struct_size(buffer, raw_data, le32_to_cpu(buffer->raw_size));
+	if (buffer_size > output.length) {
 		dev_warn(&wdev->dev,
-			 FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer size (%d)\n",
-			 buffer_size, obj->package.elements[1].buffer.length);
+			 FW_WARN "Dell WMI buffer size (%zu) exceeds WMI buffer size (%zu)\n",
+			 buffer_size, output.length);
 		ret = -EMSGSIZE;
 
 		goto err_free;
 	}
 
-	*result = obj;
+	*result = buffer;
 
 	return 0;
 
 err_free:
-	kfree(obj);
+	kfree(output.data);
 
 	return ret;
 }
 
-static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
-				     u32 arg, union acpi_object **result)
+static ssize_t dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
+					 u32 arg, char *buf, size_t length)
 {
-	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
+	struct wmi_buffer output;
+	struct wmi_string *str;
+	size_t str_size;
+	ssize_t count;
+	int ret;
+
+	ret = dell_wmi_ddv_query(wdev, method, arg, &output);
+	if (ret < 0)
+		return ret;
+
+	if (output.length < sizeof(*str)) {
+		count = -EIO;
+
+		goto err_free;
+	}
+
+	str = output.data;
+	str_size = sizeof(*str) + le16_to_cpu(str->length);
+	if (str_size > output.length) {
+		dev_warn(&wdev->dev,
+			 FW_WARN "WMI string size (%zu) exceeds WMI buffer size (%zu)\n",
+			 str_size, output.length);
+		count = -EMSGSIZE;
+
+		goto err_free;
+	}
+
+	count = wmi_string_to_utf8s(str, buf, length);
+
+err_free:
+	kfree(output.data);
+
+	return count;
 }
 
 /*
@@ -265,28 +287,26 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
 static int dell_wmi_ddv_update_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
 				       struct dell_wmi_ddv_sensors *sensors, size_t entry_size)
 {
+	struct dell_wmi_buffer *buffer;
 	u64 buffer_size, rem, entries;
-	union acpi_object *obj;
-	u8 *buffer;
 	int ret;
 
-	if (sensors->obj) {
+	if (sensors->buffer) {
 		if (time_before(jiffies, sensors->timestamp + HZ))
 			return 0;
 
-		kfree(sensors->obj);
-		sensors->obj = NULL;
+		kfree(sensors->buffer);
+		sensors->buffer = NULL;
 	}
 
-	ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
+	ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &buffer);
 	if (ret < 0)
 		return ret;
 
 	/* buffer format sanity check */
-	buffer_size = obj->package.elements[0].integer.value;
-	buffer = obj->package.elements[1].buffer.pointer;
+	buffer_size = le32_to_cpu(buffer->raw_size);
 	entries = div64_u64_rem(buffer_size, entry_size, &rem);
-	if (rem != 1 || buffer[buffer_size - 1] != 0xff) {
+	if (rem != 1 || buffer->raw_data[buffer_size - 1] != 0xff) {
 		ret = -ENOMSG;
 		goto err_free;
 	}
@@ -296,14 +316,14 @@ static int dell_wmi_ddv_update_sensors(struct wmi_device *wdev, enum dell_ddv_me
 		goto err_free;
 	}
 
-	sensors->obj = obj;
+	sensors->buffer = buffer;
 	sensors->entries = entries;
 	sensors->timestamp = jiffies;
 
 	return 0;
 
 err_free:
-	kfree(obj);
+	kfree(buffer);
 
 	return ret;
 }
@@ -328,7 +348,7 @@ static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 att
 	if (channel >= data->fans.entries)
 		return -ENXIO;
 
-	entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
+	entry = (struct fan_sensor_entry *)data->fans.buffer->raw_data;
 	switch (attr) {
 	case hwmon_fan_input:
 		*val = get_unaligned_le16(&entry[channel].rpm);
@@ -354,7 +374,7 @@ static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 at
 	if (channel >= data->temps.entries)
 		return -ENXIO;
 
-	entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
+	entry = (struct thermal_sensor_entry *)data->temps.buffer->raw_data;
 	switch (attr) {
 	case hwmon_temp_input:
 		*val = entry[channel].now * 1000;
@@ -411,7 +431,7 @@ static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int chan
 	if (channel >= data->fans.entries)
 		return -ENXIO;
 
-	entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
+	entry = (struct fan_sensor_entry *)data->fans.buffer->raw_data;
 	type = entry[channel].type;
 	switch (type) {
 	case 0x00 ... 0x07:
@@ -442,7 +462,7 @@ static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int cha
 	if (channel >= data->temps.entries)
 		return -ENXIO;
 
-	entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
+	entry = (struct thermal_sensor_entry *)data->temps.buffer->raw_data;
 	switch (entry[channel].type) {
 	case 0x00:
 		*str = "CPU";
@@ -553,8 +573,8 @@ static void dell_wmi_ddv_hwmon_cache_invalidate(struct dell_wmi_ddv_sensors *sen
 		return;
 
 	mutex_lock(&sensors->lock);
-	kfree(sensors->obj);
-	sensors->obj = NULL;
+	kfree(sensors->buffer);
+	sensors->buffer = NULL;
 	mutex_unlock(&sensors->lock);
 }
 
@@ -564,7 +584,7 @@ static void dell_wmi_ddv_hwmon_cache_destroy(void *data)
 
 	sensors->active = false;
 	mutex_destroy(&sensors->lock);
-	kfree(sensors->obj);
+	kfree(sensors->buffer);
 }
 
 static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
@@ -750,7 +770,7 @@ static void dell_wmi_battery_invalidate(struct dell_wmi_ddv_data *data,
 static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, eppid_attr);
-	union acpi_object *obj;
+	ssize_t count;
 	u32 index;
 	int ret;
 
@@ -758,19 +778,19 @@ static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, cha
 	if (ret < 0)
 		return ret;
 
-	ret = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, &obj);
-	if (ret < 0)
-		return ret;
-
-	if (obj->string.length != DELL_EPPID_LENGTH && obj->string.length != DELL_EPPID_EXT_LENGTH)
-		dev_info_once(&data->wdev->dev, FW_INFO "Suspicious ePPID length (%d)\n",
-			      obj->string.length);
+	count = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, buf,
+					  PAGE_SIZE);
+	if (count < 0)
+		return count;
 
-	ret = sysfs_emit(buf, "%s\n", obj->string.pointer);
+	if (count != DELL_EPPID_LENGTH && count != DELL_EPPID_EXT_LENGTH)
+		dev_info_once(&data->wdev->dev, FW_INFO "Suspicious ePPID length (%zd)\n", count);
 
-	kfree(obj);
+	ret = sysfs_emit_at(buf, count, "\n");
+	if (ret < 0)
+		return ret;
 
-	return ret;
+	return count + ret;
 }
 
 static int dell_wmi_ddv_get_health(struct dell_wmi_ddv_data *data, u32 index,
@@ -994,19 +1014,15 @@ static int dell_wmi_ddv_buffer_read(struct seq_file *seq, enum dell_ddv_method m
 {
 	struct device *dev = seq->private;
 	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
-	union acpi_object *obj;
-	u64 size;
-	u8 *buf;
+	struct dell_wmi_buffer *buffer;
 	int ret;
 
-	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
+	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &buffer);
 	if (ret < 0)
 		return ret;
 
-	size = obj->package.elements[0].integer.value;
-	buf = obj->package.elements[1].buffer.pointer;
-	ret = seq_write(seq, buf, size);
-	kfree(obj);
+	ret = seq_write(seq, buffer->raw_data, le32_to_cpu(buffer->raw_size));
+	kfree(buffer);
 
 	return ret;
 }
-- 
2.39.5

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

[Armin Wolf]

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

[PATCH 7/9] platform/wmi: Make wmi_bus_class const

[Armin Wolf]

The functions class_register()/_unregister() and device_create()
both support taking a const pointer to the class struct. Use this
to mark wmi_bus_class as const so that it can be placed into
read-only memory for better security.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/wmi/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
index b8e6b9a421c6..082c85625878 100644
--- a/drivers/platform/wmi/core.c
+++ b/drivers/platform/wmi/core.c
@@ -1069,7 +1069,7 @@ static void wmi_dev_shutdown(struct device *dev)
 	}
 }
 
-static struct class wmi_bus_class = {
+static const struct class wmi_bus_class = {
 	.name = "wmi_bus",
 };
 
-- 
2.39.5

[PATCH 8/9] platform/wmi: Make sysfs attributes const

[Armin Wolf]

The sysfs core supports const attributes. Use this to mark all
sysfs attributes as const so that they can be placed into read-only
memory for better security.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/wmi/core.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
index 082c85625878..1b5bb3410252 100644
--- a/drivers/platform/wmi/core.c
+++ b/drivers/platform/wmi/core.c
@@ -812,7 +812,8 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 
 	return sysfs_emit(buf, "wmi:%pUL\n", &wblock->gblock.guid);
 }
-static DEVICE_ATTR_RO(modalias);
+
+static const DEVICE_ATTR_RO(modalias);
 
 static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
@@ -821,7 +822,8 @@ static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
 
 	return sysfs_emit(buf, "%pUL\n", &wblock->gblock.guid);
 }
-static DEVICE_ATTR_RO(guid);
+
+static const DEVICE_ATTR_RO(guid);
 
 static ssize_t instance_count_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
@@ -830,7 +832,8 @@ static ssize_t instance_count_show(struct device *dev,
 
 	return sysfs_emit(buf, "%d\n", (int)wblock->gblock.instance_count);
 }
-static DEVICE_ATTR_RO(instance_count);
+
+static const DEVICE_ATTR_RO(instance_count);
 
 static ssize_t expensive_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
@@ -840,7 +843,8 @@ static ssize_t expensive_show(struct device *dev,
 	return sysfs_emit(buf, "%d\n",
 			  (wblock->gblock.flags & ACPI_WMI_EXPENSIVE) != 0);
 }
-static DEVICE_ATTR_RO(expensive);
+
+static const DEVICE_ATTR_RO(expensive);
 
 static ssize_t driver_override_show(struct device *dev, struct device_attribute *attr,
 				    char *buf)
@@ -867,9 +871,10 @@ static ssize_t driver_override_store(struct device *dev, struct device_attribute
 
 	return count;
 }
-static DEVICE_ATTR_RW(driver_override);
 
-static struct attribute *wmi_attrs[] = {
+static const DEVICE_ATTR_RW(driver_override);
+
+static const struct attribute * const wmi_attrs[] = {
 	&dev_attr_modalias.attr,
 	&dev_attr_guid.attr,
 	&dev_attr_instance_count.attr,
@@ -886,9 +891,10 @@ static ssize_t notify_id_show(struct device *dev, struct device_attribute *attr,
 
 	return sysfs_emit(buf, "%02X\n", (unsigned int)wblock->gblock.notify_id);
 }
-static DEVICE_ATTR_RO(notify_id);
 
-static struct attribute *wmi_event_attrs[] = {
+static const DEVICE_ATTR_RO(notify_id);
+
+static const struct attribute * const wmi_event_attrs[] = {
 	&dev_attr_notify_id.attr,
 	NULL
 };
@@ -902,7 +908,8 @@ static ssize_t object_id_show(struct device *dev, struct device_attribute *attr,
 	return sysfs_emit(buf, "%c%c\n", wblock->gblock.object_id[0],
 			  wblock->gblock.object_id[1]);
 }
-static DEVICE_ATTR_RO(object_id);
+
+static const DEVICE_ATTR_RO(object_id);
 
 static ssize_t setable_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
@@ -911,16 +918,17 @@ static ssize_t setable_show(struct device *dev, struct device_attribute *attr,
 
 	return sysfs_emit(buf, "%d\n", (int)wdev->setable);
 }
-static DEVICE_ATTR_RO(setable);
 
-static struct attribute *wmi_data_attrs[] = {
+static const DEVICE_ATTR_RO(setable);
+
+static const struct attribute * const wmi_data_attrs[] = {
 	&dev_attr_object_id.attr,
 	&dev_attr_setable.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(wmi_data);
 
-static struct attribute *wmi_method_attrs[] = {
+static const struct attribute * const wmi_method_attrs[] = {
 	&dev_attr_object_id.attr,
 	NULL
 };
-- 
2.39.5

[PATCH 9/9] modpost: Handle malformed WMI GUID strings

[Armin Wolf]

Some WMI GUIDs found inside binary MOF files contain both
uppercase and lowercase characters. Blindly copying such
GUIDs will prevent the associated WMI driver from loading
automatically because the WMI GUID found inside WMI device ids
always contains uppercase characters.

Avoid this issue by always converting WMI GUID strings to
uppercase. Also verify that the WMI GUID string actually looks
like a valid GUID.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 .../wmi/driver-development-guide.rst          |  2 +-
 scripts/mod/file2alias.c                      | 28 ++++++++++++++++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
index fbc2d9b12fe9..74bb156ad9cc 100644
--- a/Documentation/wmi/driver-development-guide.rst
+++ b/Documentation/wmi/driver-development-guide.rst
@@ -54,7 +54,7 @@ to matching WMI devices using a struct wmi_device_id table:
 ::
 
   static const struct wmi_device_id foo_id_table[] = {
-         /* Only use uppercase letters! */
+         /* Using only uppercase letters is recommended */
          { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
          { }
   };
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 4e99393a35f1..20e542a888c4 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1253,6 +1253,8 @@ static void do_tee_entry(struct module *mod, void *symval)
 static void do_wmi_entry(struct module *mod, void *symval)
 {
 	DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
+	char result[sizeof(*guid_string)];
+	int i;
 
 	if (strlen(*guid_string) != UUID_STRING_LEN) {
 		warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
@@ -1260,7 +1262,31 @@ static void do_wmi_entry(struct module *mod, void *symval)
 		return;
 	}
 
-	module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", *guid_string);
+	for (i = 0; i < UUID_STRING_LEN; i++) {
+		char value = (*guid_string)[i];
+		bool valid = false;
+
+		if (i == 8 || i == 13 || i == 18 || i == 23) {
+			if (value == '-')
+				valid = true;
+		} else {
+			if (isxdigit(value))
+				valid = true;
+		}
+
+		if (!valid) {
+			warn("Invalid character %c inside WMI GUID string '%s' in '%s'\n",
+			     value, *guid_string, mod->name);
+			return;
+		}
+
+		/* Some GUIDs from BMOF definitions contain lowercase characters */
+		result[i] = toupper(value);
+	}
+
+	result[i] = '\0';
+
+	module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", result);
 }
 
 /* Looks like: mhi:S */
-- 
2.39.5

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

[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.

>   {
>   	__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

[Armin Wolf]

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;
>>   }
>
>

Re: [PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Mario Limonciello]

On 3/7/2026 6:25 PM, Armin Wolf wrote:
> Use the new buffer-based WMI API to also support ACPI firmware
> implementations that do not use ACPI buffers for the descriptor.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   .../platform/x86/dell/dell-wmi-descriptor.c   | 96 ++++++++++---------
>   1 file changed, 49 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-descriptor.c b/drivers/platform/x86/dell/dell-wmi-descriptor.c
> index c2a180202719..621502368895 100644
> --- a/drivers/platform/x86/dell/dell-wmi-descriptor.c
> +++ b/drivers/platform/x86/dell/dell-wmi-descriptor.c
> @@ -7,7 +7,7 @@
>   
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
> -#include <linux/acpi.h>
> +#include <linux/compiler_attributes.h>
>   #include <linux/list.h>
>   #include <linux/module.h>
>   #include <linux/wmi.h>
> @@ -15,6 +15,24 @@
>   
>   #define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>   
> +/*
> + * Descriptor buffer is 128 byte long and contains:
> + *
> + *       Name             Offset  Length  Value
> + * Vendor Signature          0       4    "DELL"
> + * Object Signature          4       4    " WMI"
> + * WMI Interface Version     8       4    <version>
> + * WMI buffer length        12       4    <length>
> + * WMI hotfix number        16       4    <hotfix>
> + */
> +struct descriptor {
> +	__le32 vendor_signature;
> +	__le32 object_signature;
> +	__le32 interface_version;
> +	__le32 buffer_length;
> +	__le32 hotfix_number;
> +} __packed;
> +
>   struct descriptor_priv {
>   	struct list_head list;
>   	u32 interface_version;
> @@ -88,76 +106,60 @@ bool dell_wmi_get_hotfix(u32 *hotfix)
>   }
>   EXPORT_SYMBOL_GPL(dell_wmi_get_hotfix);
>   
> -/*
> - * Descriptor buffer is 128 byte long and contains:
> - *
> - *       Name             Offset  Length  Value
> - * Vendor Signature          0       4    "DELL"
> - * Object Signature          4       4    " WMI"
> - * WMI Interface Version     8       4    <version>
> - * WMI buffer length        12       4    <length>
> - * WMI hotfix number        16       4    <hotfix>
> - */
> -static int dell_wmi_descriptor_probe(struct wmi_device *wdev,
> -				     const void *context)
> +static int dell_wmi_descriptor_probe(struct wmi_device *wdev, const void *context)
>   {
> -	union acpi_object *obj = NULL;
>   	struct descriptor_priv *priv;
> -	u32 *buffer;
> +	struct wmi_buffer buffer;
> +	struct descriptor *desc;
>   	int ret;
>   
> -	obj = wmidev_block_query(wdev, 0);
> -	if (!obj) {
> -		dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
> -		ret = -EIO;
> -		goto out;
> -	}
> +	ret = wmidev_query_block(wdev, 0, &buffer);
> +	if (ret < 0)
> +		return ret;
>   
> -	if (obj->type != ACPI_TYPE_BUFFER) {
> -		dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
> +	if (buffer.length < sizeof(*desc)) {
> +		dev_err(&wdev->dev,
> +			"Dell descriptor buffer contains not enough data (%zu)\n",
> +			buffer.length);
>   		ret = -EINVAL;
>   		descriptor_valid = ret;
>   		goto out;
>   	}
>   
> -	/* Although it's not technically a failure, this would lead to
> -	 * unexpected behavior
> -	 */
> -	if (obj->buffer.length != 128) {
> -		dev_err(&wdev->dev,
> -			"Dell descriptor buffer has unexpected length (%d)\n",
> -			obj->buffer.length);
> -		ret = -EINVAL;
> +	desc = buffer.data;
> +
> +	/* "DELL" */
> +	if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {

Do you think you could come up with a helper for matching an expected 
"string" like this?  I /suspect/ it's going to be a common pattern that 
vendors use and it will increase code readability going forward if a 
helper is possible.  I see it at least twice in this patch alone.

Something like this prototype:

bool wmi_valid_signature(u32 field, const char* expected_str);


> +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid vendor signature (%u)\n",
> +			le32_to_cpu(desc->vendor_signature));
> +		ret = -ENOMSG;
>   		descriptor_valid = ret;
>   		goto out;
>   	}
>   
> -	buffer = (u32 *)obj->buffer.pointer;
> -
> -	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
> -		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
> -			buffer);
> -		ret = -EINVAL;
> +	/* " WMI" */
> +	if (le32_to_cpu(desc->object_signature) != 0x494D5720) {
> +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid object signature (%u)\n",
> +			le32_to_cpu(desc->object_signature));
> +		ret = -ENOMSG;
>   		descriptor_valid = ret;
>   		goto out;
>   	}
>   	descriptor_valid = 0;
>   
> -	if (buffer[2] != 0 && buffer[2] != 1)
> -		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%lu)\n",
> -			(unsigned long) buffer[2]);
> -
> -	priv = devm_kzalloc(&wdev->dev, sizeof(struct descriptor_priv),
> -	GFP_KERNEL);
> +	if (le32_to_cpu(desc->interface_version) > 2)
> +		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
> +			 le32_to_cpu(desc->interface_version));
>   
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv) {
>   		ret = -ENOMEM;
>   		goto out;
>   	}
>   
> -	priv->interface_version = buffer[2];
> -	priv->size = buffer[3];
> -	priv->hotfix = buffer[4];
> +	priv->interface_version = le32_to_cpu(desc->interface_version);
> +	priv->size = le32_to_cpu(desc->buffer_length);
> +	priv->hotfix = le32_to_cpu(desc->hotfix_number);
>   	ret = 0;
>   	dev_set_drvdata(&wdev->dev, priv);
>   	mutex_lock(&list_mutex);
> @@ -170,7 +172,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device *wdev,
>   		(unsigned long) priv->hotfix);
>   
>   out:
> -	kfree(obj);
> +	kfree(buffer.data);
>   	return ret;
>   }
>   

Re: [PATCH 9/9] modpost: Handle malformed WMI GUID strings

[Mario Limonciello]

On 3/7/2026 6:25 PM, Armin Wolf wrote:
> Some WMI GUIDs found inside binary MOF files contain both
> uppercase and lowercase characters. Blindly copying such
> GUIDs will prevent the associated WMI driver from loading
> automatically because the WMI GUID found inside WMI device ids
> always contains uppercase characters.
> 
> Avoid this issue by always converting WMI GUID strings to
> uppercase. Also verify that the WMI GUID string actually looks
> like a valid GUID.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   .../wmi/driver-development-guide.rst          |  2 +-
>   scripts/mod/file2alias.c                      | 28 ++++++++++++++++++-
>   2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
> index fbc2d9b12fe9..74bb156ad9cc 100644
> --- a/Documentation/wmi/driver-development-guide.rst
> +++ b/Documentation/wmi/driver-development-guide.rst
> @@ -54,7 +54,7 @@ to matching WMI devices using a struct wmi_device_id table:
>   ::
>   
>     static const struct wmi_device_id foo_id_table[] = {
> -         /* Only use uppercase letters! */
> +         /* Using only uppercase letters is recommended */
>            { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
>            { }
>     };
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 4e99393a35f1..20e542a888c4 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1253,6 +1253,8 @@ static void do_tee_entry(struct module *mod, void *symval)
>   static void do_wmi_entry(struct module *mod, void *symval)
>   {
>   	DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> +	char result[sizeof(*guid_string)];
> +	int i;
>   
>   	if (strlen(*guid_string) != UUID_STRING_LEN) {
>   		warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> @@ -1260,7 +1262,31 @@ static void do_wmi_entry(struct module *mod, void *symval)
>   		return;
>   	}
>   
> -	module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", *guid_string);
> +	for (i = 0; i < UUID_STRING_LEN; i++) {
> +		char value = (*guid_string)[i];
> +		bool valid = false;
> +
> +		if (i == 8 || i == 13 || i == 18 || i == 23) {
> +			if (value == '-')
> +				valid = true;
> +		} else {
> +			if (isxdigit(value))
> +				valid = true;
> +		}
> +
> +		if (!valid) {
> +			warn("Invalid character %c inside WMI GUID string '%s' in '%s'\n",
> +			     value, *guid_string, mod->name);
> +			return;
> +		}
> +
> +		/* Some GUIDs from BMOF definitions contain lowercase characters */
> +		result[i] = toupper(value);
> +	}

Minor logic change that could drop the boolean variable in the for loop:

for (i = 0; i < UUID_STRING_LEN; i++) {
	char value = (*guid_string)[i];

	if (isxdigit(value)) {
		result[i] = toupper(value);
		continue;
	}

	if (value == '-' && (i == 8 || i == 13 || i == 18 || i == 23)) {
		result[i] = value;
		continue;
	}

	warn("Invalid character %c inside WMI GUID string '%s' in '%s'\n",
	     value, *guid_string, mod->name);
	return;
}

> +
> +	result[i] = '\0';
> +
> +	module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", result);
>   }
>   
>   /* Looks like: mhi:S */

Re: [PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Pali Rohár]

On Monday 09 March 2026 10:41:20 Mario Limonciello wrote:
> On 3/7/2026 6:25 PM, Armin Wolf wrote:
> > +	/* "DELL" */
> > +	if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {
> 
> Do you think you could come up with a helper for matching an expected
> "string" like this?  I /suspect/ it's going to be a common pattern that
> vendors use and it will increase code readability going forward if a helper
> is possible.  I see it at least twice in this patch alone.
> 
> Something like this prototype:
> 
> bool wmi_valid_signature(u32 field, const char* expected_str);

When I read your comment, I come to another idea. What about introducing
a macro which will convert 4-byte string to u32 number? For example:

  #define str_to_u32(str) ({ _Static_assert(__builtin_types_compatible_p(__typeof__(str), char[5]), "wrong type"); (u32)(u8)(str)[0] | ((u32)((u8)(str)[1]) << 8) | ((u32)((u8)(str)[2]) << 16) | ((u32)((u8)(str)[3]) << 24); })

The whole conversion would be done in compile time (with -O2) so should
not cause any possible performance issues.

With it, the condition could be written as:

  if (le32_to_cpu(desc->vendor_signature) != str_to_u32("DELL")) {

and no need to use magic number 0x4C4C4544 and write comment that this
number in ASCII is the "DELL" string.

> 
> > +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid vendor signature (%u)\n",
> > +			le32_to_cpu(desc->vendor_signature));
> > +		ret = -ENOMSG;
> >   		descriptor_valid = ret;
> >   		goto out;
> >   	}
> > -	buffer = (u32 *)obj->buffer.pointer;
> > -
> > -	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
> > -		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
> > -			buffer);
> > -		ret = -EINVAL;
> > +	/* " WMI" */
> > +	if (le32_to_cpu(desc->object_signature) != 0x494D5720) {
> > +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid object signature (%u)\n",
> > +			le32_to_cpu(desc->object_signature));
> > +		ret = -ENOMSG;
> >   		descriptor_valid = ret;
> >   		goto out;
> >   	}

Re: [PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Armin Wolf]

Am 09.03.26 um 18:23 schrieb Pali Rohár:

> On Monday 09 March 2026 10:41:20 Mario Limonciello wrote:
>> On 3/7/2026 6:25 PM, Armin Wolf wrote:
>>> +	/* "DELL" */
>>> +	if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {
>> Do you think you could come up with a helper for matching an expected
>> "string" like this?  I /suspect/ it's going to be a common pattern that
>> vendors use and it will increase code readability going forward if a helper
>> is possible.  I see it at least twice in this patch alone.
>>
>> Something like this prototype:
>>
>> bool wmi_valid_signature(u32 field, const char* expected_str);
> When I read your comment, I come to another idea. What about introducing
> a macro which will convert 4-byte string to u32 number? For example:
>
>    #define str_to_u32(str) ({ _Static_assert(__builtin_types_compatible_p(__typeof__(str), char[5]), "wrong type"); (u32)(u8)(str)[0] | ((u32)((u8)(str)[1]) << 8) | ((u32)((u8)(str)[2]) << 16) | ((u32)((u8)(str)[3]) << 24); })
>
> The whole conversion would be done in compile time (with -O2) so should
> not cause any possible performance issues.
>
> With it, the condition could be written as:
>
>    if (le32_to_cpu(desc->vendor_signature) != str_to_u32("DELL")) {
>
> and no need to use magic number 0x4C4C4544 and write comment that this
> number in ASCII is the "DELL" string.

To be honest i do nothing think that having a helper function for this inside the WMI driver core
is useful, especially since most vendors other than Dell do not use such magic numbers.

 From my perspective assembling the two constants ourself is fine here.

Thanks,
Armin Wolf

>
>>> +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid vendor signature (%u)\n",
>>> +			le32_to_cpu(desc->vendor_signature));
>>> +		ret = -ENOMSG;
>>>    		descriptor_valid = ret;
>>>    		goto out;
>>>    	}
>>> -	buffer = (u32 *)obj->buffer.pointer;
>>> -
>>> -	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
>>> -		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
>>> -			buffer);
>>> -		ret = -EINVAL;
>>> +	/* " WMI" */
>>> +	if (le32_to_cpu(desc->object_signature) != 0x494D5720) {
>>> +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid object signature (%u)\n",
>>> +			le32_to_cpu(desc->object_signature));
>>> +		ret = -ENOMSG;
>>>    		descriptor_valid = ret;
>>>    		goto out;
>>>    	}

Re: [PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Mario Limonciello]

On 3/9/2026 2:45 PM, Armin Wolf wrote:
> Am 09.03.26 um 18:23 schrieb Pali Rohár:
> 
>> On Monday 09 March 2026 10:41:20 Mario Limonciello wrote:
>>> On 3/7/2026 6:25 PM, Armin Wolf wrote:
>>>> +    /* "DELL" */
>>>> +    if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {
>>> Do you think you could come up with a helper for matching an expected
>>> "string" like this?  I /suspect/ it's going to be a common pattern that
>>> vendors use and it will increase code readability going forward if a 
>>> helper
>>> is possible.  I see it at least twice in this patch alone.
>>>
>>> Something like this prototype:
>>>
>>> bool wmi_valid_signature(u32 field, const char* expected_str);
>> When I read your comment, I come to another idea. What about introducing
>> a macro which will convert 4-byte string to u32 number? For example:
>>
>>    #define str_to_u32(str) 
>> ({ _Static_assert(__builtin_types_compatible_p(__typeof__(str), 
>> char[5]), "wrong type"); (u32)(u8)(str)[0] | ((u32)((u8)(str)[1]) << 
>> 8) | ((u32)((u8)(str)[2]) << 16) | ((u32)((u8)(str)[3]) << 24); })
>>
>> The whole conversion would be done in compile time (with -O2) so should
>> not cause any possible performance issues.
>>
>> With it, the condition could be written as:
>>
>>    if (le32_to_cpu(desc->vendor_signature) != str_to_u32("DELL")) {
>>
>> and no need to use magic number 0x4C4C4544 and write comment that this
>> number in ASCII is the "DELL" string.
> 
> To be honest i do nothing think that having a helper function for this 
> inside the WMI driver core
> is useful, especially since most vendors other than Dell do not use such 
> magic numbers.
> 
>  From my perspective assembling the two constants ourself is fine here.
> 
> Thanks,
> Armin Wolf

Oh I didn't realize no one else is doing this.  Yeah if it's Dell only, 
agree - meh.


> 
>>
>>>> +        dev_err(&wdev->dev, "Dell descriptor buffer has invalid 
>>>> vendor signature (%u)\n",
>>>> +            le32_to_cpu(desc->vendor_signature));
>>>> +        ret = -ENOMSG;
>>>>            descriptor_valid = ret;
>>>>            goto out;
>>>>        }
>>>> -    buffer = (u32 *)obj->buffer.pointer;
>>>> -
>>>> -    if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
>>>> -        dev_err(&wdev->dev, "Dell descriptor buffer has invalid 
>>>> signature (%8ph)\n",
>>>> -            buffer);
>>>> -        ret = -EINVAL;
>>>> +    /* " WMI" */
>>>> +    if (le32_to_cpu(desc->object_signature) != 0x494D5720) {
>>>> +        dev_err(&wdev->dev, "Dell descriptor buffer has invalid 
>>>> object signature (%u)\n",
>>>> +            le32_to_cpu(desc->object_signature));
>>>> +        ret = -ENOMSG;
>>>>            descriptor_valid = ret;
>>>>            goto out;
>>>>        }
> 

Re: [PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Gergo Koteles]

Hi Armin,

On Mon, 2026-03-09 at 20:45 +0100, Armin Wolf wrote:
> Am 09.03.26 um 18:23 schrieb Pali Rohár:
> 
> > On Monday 09 March 2026 10:41:20 Mario Limonciello wrote:
> > > On 3/7/2026 6:25 PM, Armin Wolf wrote:
> > > > +	/* "DELL" */
> > > > +	if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {
> > > Do you think you could come up with a helper for matching an expected
> > > "string" like this?  I /suspect/ it's going to be a common pattern that
> > > vendors use and it will increase code readability going forward if a helper
> > > is possible.  I see it at least twice in this patch alone.
> > > 
> > > Something like this prototype:
> > > 
> > > bool wmi_valid_signature(u32 field, const char* expected_str);
> > When I read your comment, I come to another idea. What about introducing
> > a macro which will convert 4-byte string to u32 number? For example:
> > 
> >    #define str_to_u32(str) ({ _Static_assert(__builtin_types_compatible_p(__typeof__(str), char[5]), "wrong type"); (u32)(u8)(str)[0] | ((u32)((u8)(str)[1]) << 8) | ((u32)((u8)(str)[2]) << 16) | ((u32)((u8)(str)[3]) << 24); })
> > 
> > The whole conversion would be done in compile time (with -O2) so should
> > not cause any possible performance issues.
> > 
> > With it, the condition could be written as:
> > 
> >    if (le32_to_cpu(desc->vendor_signature) != str_to_u32("DELL")) {
> > 
> > and no need to use magic number 0x4C4C4544 and write comment that this
> > number in ASCII is the "DELL" string.
> 
> To be honest i do nothing think that having a helper function for this inside the WMI driver core
> is useful, especially since most vendors other than Dell do not use such magic numbers.
> 
>  From my perspective assembling the two constants ourself is fine here.
> 

But what about the other readers? :)

Why don't you use a char array for the descriptors?

struct descriptor {
	char vendor_signature[4];
	char object_signature[4];
	__le32 interface_version;
	__le32 buffer_length;
	__le32 hotfix_number;
} __packed;

Best regards,
Gergo Koteles

Re: [PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Armin Wolf]

Am 10.03.26 um 11:46 schrieb Gergo Koteles:

> Hi Armin,
>
> On Mon, 2026-03-09 at 20:45 +0100, Armin Wolf wrote:
>> Am 09.03.26 um 18:23 schrieb Pali Rohár:
>>
>>> On Monday 09 March 2026 10:41:20 Mario Limonciello wrote:
>>>> On 3/7/2026 6:25 PM, Armin Wolf wrote:
>>>>> +	/* "DELL" */
>>>>> +	if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {
>>>> Do you think you could come up with a helper for matching an expected
>>>> "string" like this?  I /suspect/ it's going to be a common pattern that
>>>> vendors use and it will increase code readability going forward if a helper
>>>> is possible.  I see it at least twice in this patch alone.
>>>>
>>>> Something like this prototype:
>>>>
>>>> bool wmi_valid_signature(u32 field, const char* expected_str);
>>> When I read your comment, I come to another idea. What about introducing
>>> a macro which will convert 4-byte string to u32 number? For example:
>>>
>>>     #define str_to_u32(str) ({ _Static_assert(__builtin_types_compatible_p(__typeof__(str), char[5]), "wrong type"); (u32)(u8)(str)[0] | ((u32)((u8)(str)[1]) << 8) | ((u32)((u8)(str)[2]) << 16) | ((u32)((u8)(str)[3]) << 24); })
>>>
>>> The whole conversion would be done in compile time (with -O2) so should
>>> not cause any possible performance issues.
>>>
>>> With it, the condition could be written as:
>>>
>>>     if (le32_to_cpu(desc->vendor_signature) != str_to_u32("DELL")) {
>>>
>>> and no need to use magic number 0x4C4C4544 and write comment that this
>>> number in ASCII is the "DELL" string.
>> To be honest i do nothing think that having a helper function for this inside the WMI driver core
>> is useful, especially since most vendors other than Dell do not use such magic numbers.
>>
>>   From my perspective assembling the two constants ourself is fine here.
>>
> But what about the other readers? :)
>
> Why don't you use a char array for the descriptors?
>
> struct descriptor {
> 	char vendor_signature[4];
> 	char object_signature[4];
> 	__le32 interface_version;
> 	__le32 buffer_length;
> 	__le32 hotfix_number;
> } __packed;
>
> Best regards,
> Gergo Koteles
>
Fine, i will model the vendor and object signatures using char arrays.

Thanks,
Armin Wolf

Re: [PATCH 9/9] modpost: Handle malformed WMI GUID strings

[Armin Wolf]

Am 09.03.26 um 17:07 schrieb Mario Limonciello:

>
>
> On 3/7/2026 6:25 PM, Armin Wolf wrote:
>> Some WMI GUIDs found inside binary MOF files contain both
>> uppercase and lowercase characters. Blindly copying such
>> GUIDs will prevent the associated WMI driver from loading
>> automatically because the WMI GUID found inside WMI device ids
>> always contains uppercase characters.
>>
>> Avoid this issue by always converting WMI GUID strings to
>> uppercase. Also verify that the WMI GUID string actually looks
>> like a valid GUID.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   .../wmi/driver-development-guide.rst          |  2 +-
>>   scripts/mod/file2alias.c                      | 28 ++++++++++++++++++-
>>   2 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/wmi/driver-development-guide.rst 
>> b/Documentation/wmi/driver-development-guide.rst
>> index fbc2d9b12fe9..74bb156ad9cc 100644
>> --- a/Documentation/wmi/driver-development-guide.rst
>> +++ b/Documentation/wmi/driver-development-guide.rst
>> @@ -54,7 +54,7 @@ to matching WMI devices using a struct 
>> wmi_device_id table:
>>   ::
>>       static const struct wmi_device_id foo_id_table[] = {
>> -         /* Only use uppercase letters! */
>> +         /* Using only uppercase letters is recommended */
>>            { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
>>            { }
>>     };
>> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
>> index 4e99393a35f1..20e542a888c4 100644
>> --- a/scripts/mod/file2alias.c
>> +++ b/scripts/mod/file2alias.c
>> @@ -1253,6 +1253,8 @@ static void do_tee_entry(struct module *mod, 
>> void *symval)
>>   static void do_wmi_entry(struct module *mod, void *symval)
>>   {
>>       DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
>> +    char result[sizeof(*guid_string)];
>> +    int i;
>>         if (strlen(*guid_string) != UUID_STRING_LEN) {
>>           warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
>> @@ -1260,7 +1262,31 @@ static void do_wmi_entry(struct module *mod, 
>> void *symval)
>>           return;
>>       }
>>   -    module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", 
>> *guid_string);
>> +    for (i = 0; i < UUID_STRING_LEN; i++) {
>> +        char value = (*guid_string)[i];
>> +        bool valid = false;
>> +
>> +        if (i == 8 || i == 13 || i == 18 || i == 23) {
>> +            if (value == '-')
>> +                valid = true;
>> +        } else {
>> +            if (isxdigit(value))
>> +                valid = true;
>> +        }
>> +
>> +        if (!valid) {
>> +            warn("Invalid character %c inside WMI GUID string '%s' 
>> in '%s'\n",
>> +                 value, *guid_string, mod->name);
>> +            return;
>> +        }
>> +
>> +        /* Some GUIDs from BMOF definitions contain lowercase 
>> characters */
>> +        result[i] = toupper(value);
>> +    }
>
> Minor logic change that could drop the boolean variable in the for loop:
>
> for (i = 0; i < UUID_STRING_LEN; i++) {
>     char value = (*guid_string)[i];
>
>     if (isxdigit(value)) {
>         result[i] = toupper(value);
>         continue;
>     }

This would not catch invalid GUID strings containing only digits.

Thanks,
Armin Wolf

>
>     if (value == '-' && (i == 8 || i == 13 || i == 18 || i == 23)) {
>         result[i] = value;
>         continue;
>     }
>
>     warn("Invalid character %c inside WMI GUID string '%s' in '%s'\n",
>          value, *guid_string, mod->name);
>     return;
> }
>
>> +
>> +    result[i] = '\0';
>> +
>> +    module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", result);
>>   }
>>     /* Looks like: mhi:S */
>
>