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

Ilpo Järvinen
2026-03-31T09:00:19Z

On Sat, 14 Mar 2026, 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 | 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",

This word order sounds odd to me. I'd say:

Dell descriptor buffer does not contain ...

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

I think these should be moving towards using __free() so we could remove
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.

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

--
i.