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

Armin Wolf
2026-03-31T19:54:27Z

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

Ok.

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

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