Re: [PATCH v3 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API
From: Armin Wolf
Date: Tue Mar 31 2026 - 15:54:41 EST
Am 31.03.26 um 11:00 schrieb Ilpo Järvinen:
On Sat, 14 Mar 2026, Armin Wolf wrote:
Use the new buffer-based WMI API to also support ACPI firmwareThis word order sounds odd to me. I'd say:
implementations that do not use ACPI buffers for the descriptor.
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
.../platform/x86/dell/dell-wmi-descriptor.c | 94 +++++++++----------
1 file changed, 47 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..fe42eb8bbd79 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 {
+ char vendor_signature[4];
+ char object_signature[4];
+ __le32 interface_version;
+ __le32 buffer_length;
+ __le32 hotfix_number;
+} __packed;
+
struct descriptor_priv {
struct list_head list;
u32 interface_version;
@@ -88,76 +106,58 @@ 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",
Dell descriptor buffer does not contain ...
Ok.
+ buffer.length);I think these should be moving towards using __free() so we could remove
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;
those gotos to kfree(). This applies to many changes in this series. Some
may need a local variable to be introduced (here we've 'desc' already so
it's very easy in this case).
I'm fine if you want to do __free() changes in a patch separate from
this wmi conversion but we should aim to make these follow best practices
so that others pick the preferred patterns more easily.
Ok, i will resend the series with the necessary changes.
Thanks,
Armin Wolf
+ if (strncmp(desc->vendor_signature, "DELL", sizeof(desc->vendor_signature))) {
+ dev_err(&wdev->dev, "Dell descriptor buffer has invalid vendor signature (%4ph)\n",
+ 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;
+ if (strncmp(desc->object_signature, " WMI", sizeof(desc->vendor_signature))) {
+ dev_err(&wdev->dev, "Dell descriptor buffer has invalid object signature (%4ph)\n",
+ 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 +170,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device *wdev,
(unsigned long) priv->hotfix);
out:
- kfree(obj);
+ kfree(buffer.data);
return ret;
}