[PATCH] platform/x86: dell-wmi-aio: Convert to use the bus-based WMI API

[PATCH] platform/x86: dell-wmi-aio: Convert to use the bus-based WMI API

[Armin Wolf]

It turns out that some devices like the Dell Inspiron One 2310
contain multiple WMI event devices with a matching GUID of
284A0E6B-380E-472A-921F-E52786257FB4, each handling a separate
hotkey (volume up, volume down, ...). The dell-wmi-aio driver
however still uses the legacy GUID-based WMI API and can thus
only see the first of those WMI event devices, preventing the
remaining buttons from working.

Fix this by converting the driver to use the modern bus-based
WMI API. This also includes replacing the usage of
union acpi_object with struct wmi_buffer.

Tested using the ACPI-WMI ASL code from the Dell Inspiron One 2310
together with some changes to allow injecting events.

Link: https://linux-hardware.org/?probe=e86d77e44d
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-aio.c | 198 +++++++++--------------
 1 file changed, 78 insertions(+), 120 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-aio.c b/drivers/platform/x86/dell/dell-wmi-aio.c
index 54096495719b..8849ad93480b 100644
--- a/drivers/platform/x86/dell/dell-wmi-aio.c
+++ b/drivers/platform/x86/dell/dell-wmi-aio.c
@@ -5,14 +5,20 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/cleanup.h>
+#include <linux/compiler_attributes.h>
+#include <linux/device.h>
+#include <linux/device/driver.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/overflow.h>
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
-#include <linux/acpi.h>
 #include <linux/string.h>
+#include <linux/wmi.h>
 
 MODULE_DESCRIPTION("WMI hotkeys driver for Dell All-In-One series");
 MODULE_LICENSE("GPL");
@@ -20,22 +26,19 @@ MODULE_LICENSE("GPL");
 #define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
 #define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
 
+struct dell_wmi_aio_data {
+	struct input_dev *input_device;
+	/* Protects the input sequence */
+	struct mutex input_lock;
+};
+
 struct dell_wmi_event {
-	u16	length;
+	__le16	length;
 	/* 0x000: A hot key pressed or an event occurred
 	 * 0x00F: A sequence of hot keys are pressed */
-	u16	type;
-	u16	event[];
-};
-
-static const char *dell_wmi_aio_guids[] = {
-	EVENT_GUID1,
-	EVENT_GUID2,
-	NULL
-};
-
-MODULE_ALIAS("wmi:"EVENT_GUID1);
-MODULE_ALIAS("wmi:"EVENT_GUID2);
+	__le16	type;
+	__le16	event[];
+} __packed;
 
 static const struct key_entry dell_wmi_aio_keymap[] = {
 	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
@@ -50,137 +53,92 @@ static const struct key_entry dell_wmi_aio_keymap[] = {
 	{ KE_END, 0 }
 };
 
-static struct input_dev *dell_wmi_aio_input_dev;
-
 /*
  * The new WMI event data format will follow the dell_wmi_event structure
  * So, we will check if the buffer matches the format
  */
-static bool dell_wmi_aio_event_check(u8 *buffer, int length)
+static bool dell_wmi_aio_event_check(const struct wmi_buffer *buffer)
 {
-	struct dell_wmi_event *event = (struct dell_wmi_event *)buffer;
+	struct dell_wmi_event *event;
+	u16 length, type;
 
-	if (event == NULL || length < 6)
+	if (buffer->length < struct_size(event, event, 1))
 		return false;
 
-	if ((event->type == 0 || event->type == 0xf) &&
-			event->length >= 2)
+	event = buffer->data;
+	length = le16_to_cpu(event->length);
+	type = le16_to_cpu(event->type);
+	if ((type == 0 || type == 0xf) && length >= 2)
 		return true;
 
 	return false;
 }
 
-static void dell_wmi_aio_notify(union acpi_object *obj, void *context)
+static void dell_wmi_aio_notify(struct wmi_device *wdev, const struct wmi_buffer *data)
 {
-	struct dell_wmi_event *event;
-
-	if (obj) {
-		unsigned int scancode = 0;
-
-		switch (obj->type) {
-		case ACPI_TYPE_INTEGER:
-			/* Most All-In-One correctly return integer scancode */
-			scancode = obj->integer.value;
-			sparse_keymap_report_event(dell_wmi_aio_input_dev,
-				scancode, 1, true);
-			break;
-		case ACPI_TYPE_BUFFER:
-			if (dell_wmi_aio_event_check(obj->buffer.pointer,
-						obj->buffer.length)) {
-				event = (struct dell_wmi_event *)
-					obj->buffer.pointer;
-				scancode = event->event[0];
-			} else {
-				/* Broken machines return the scancode in a
-				   buffer */
-				if (obj->buffer.pointer &&
-						obj->buffer.length > 0)
-					scancode = obj->buffer.pointer[0];
-			}
-			if (scancode)
-				sparse_keymap_report_event(
-					dell_wmi_aio_input_dev,
-					scancode, 1, true);
-			break;
-		}
-	}
-}
-
-static int __init dell_wmi_aio_input_setup(void)
-{
-	int err;
-
-	dell_wmi_aio_input_dev = input_allocate_device();
-
-	if (!dell_wmi_aio_input_dev)
-		return -ENOMEM;
-
-	dell_wmi_aio_input_dev->name = "Dell AIO WMI hotkeys";
-	dell_wmi_aio_input_dev->phys = "wmi/input0";
-	dell_wmi_aio_input_dev->id.bustype = BUS_HOST;
-
-	err = sparse_keymap_setup(dell_wmi_aio_input_dev,
-			dell_wmi_aio_keymap, NULL);
-	if (err) {
-		pr_err("Unable to setup input device keymap\n");
-		goto err_free_dev;
-	}
-	err = input_register_device(dell_wmi_aio_input_dev);
-	if (err) {
-		pr_info("Unable to register input device\n");
-		goto err_free_dev;
+	struct dell_wmi_aio_data *drvdata = dev_get_drvdata(&wdev->dev);
+	const struct dell_wmi_event *new_event;
+	unsigned int scancode;
+	const u8 *old_event;
+
+	if (dell_wmi_aio_event_check(data)) {
+		new_event = data->data;
+		scancode = le16_to_cpu(new_event->event[0]);
+	} else {
+		old_event = data->data;
+		scancode = old_event[0];
 	}
-	return 0;
-
-err_free_dev:
-	input_free_device(dell_wmi_aio_input_dev);
-	return err;
-}
-
-static const char *dell_wmi_aio_find(void)
-{
-	int i;
 
-	for (i = 0; dell_wmi_aio_guids[i] != NULL; i++)
-		if (wmi_has_guid(dell_wmi_aio_guids[i]))
-			return dell_wmi_aio_guids[i];
+	guard(mutex)(&drvdata->input_lock);
 
-	return NULL;
+	sparse_keymap_report_event(drvdata->input_device, scancode, 1, true);
 }
 
-static int __init dell_wmi_aio_init(void)
+static int dell_wmi_aio_probe(struct wmi_device *wdev, const void *context)
 {
-	int err;
-	const char *guid;
+	struct dell_wmi_aio_data *data;
+	int ret;
 
-	guid = dell_wmi_aio_find();
-	if (!guid) {
-		pr_warn("No known WMI GUID found\n");
-		return -ENXIO;
-	}
+	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
-	err = dell_wmi_aio_input_setup();
-	if (err)
-		return err;
+	dev_set_drvdata(&wdev->dev, data);
+	ret = devm_mutex_init(&wdev->dev, &data->input_lock);
+	if (ret < 0)
+		return ret;
 
-	err = wmi_install_notify_handler(guid, dell_wmi_aio_notify, NULL);
-	if (err) {
-		pr_err("Unable to register notify handler - %d\n", err);
-		input_unregister_device(dell_wmi_aio_input_dev);
-		return err;
-	}
+	data->input_device = devm_input_allocate_device(&wdev->dev);
+	if (!data->input_device)
+		return -ENOMEM;
 
-	return 0;
-}
+	data->input_device->name = "Dell AIO WMI hotkeys";
+	data->input_device->phys = "wmi/input0";
+	data->input_device->id.bustype = BUS_HOST;
 
-static void __exit dell_wmi_aio_exit(void)
-{
-	const char *guid;
+	ret = sparse_keymap_setup(data->input_device, dell_wmi_aio_keymap, NULL);
+	if (ret < 0)
+		return ret;
 
-	guid = dell_wmi_aio_find();
-	wmi_remove_notify_handler(guid);
-	input_unregister_device(dell_wmi_aio_input_dev);
+	return input_register_device(data->input_device);
 }
 
-module_init(dell_wmi_aio_init);
-module_exit(dell_wmi_aio_exit);
+static const struct wmi_device_id dell_wmi_aio_id_table[] = {
+	{ EVENT_GUID1, NULL },
+	{ EVENT_GUID2, NULL },
+	{ }
+};
+MODULE_DEVICE_TABLE(wmi, dell_wmi_aio_id_table);
+
+static struct wmi_driver dell_wmi_aio_driver = {
+	.driver = {
+		.name = "dell-wmi-aio",
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.id_table = dell_wmi_aio_id_table,
+	.probe = dell_wmi_aio_probe,
+	.notify_new = dell_wmi_aio_notify,
+	.min_event_size = sizeof(u8),
+	.no_singleton = true,
+};
+module_wmi_driver(dell_wmi_aio_driver);
-- 
2.39.5

Re: [PATCH] platform/x86: dell-wmi-aio: Convert to use the bus-based WMI API

[Armin Wolf]

Am 02.08.26 um 14:45 schrieb Armin Wolf:

> It turns out that some devices like the Dell Inspiron One 2310
> contain multiple WMI event devices with a matching GUID of
> 284A0E6B-380E-472A-921F-E52786257FB4, each handling a separate
> hotkey (volume up, volume down, ...). The dell-wmi-aio driver
> however still uses the legacy GUID-based WMI API and can thus
> only see the first of those WMI event devices, preventing the
> remaining buttons from working.
>
> Fix this by converting the driver to use the modern bus-based
> WMI API. This also includes replacing the usage of
> union acpi_object with struct wmi_buffer.
>
> Tested using the ACPI-WMI ASL code from the Dell Inspiron One 2310
> together with some changes to allow injecting events.

Any thoughts on this?

> Link: https://linux-hardware.org/?probe=e86d77e44d
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   drivers/platform/x86/dell/dell-wmi-aio.c | 198 +++++++++--------------
>   1 file changed, 78 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-wmi-aio.c b/drivers/platform/x86/dell/dell-wmi-aio.c
> index 54096495719b..8849ad93480b 100644
> --- a/drivers/platform/x86/dell/dell-wmi-aio.c
> +++ b/drivers/platform/x86/dell/dell-wmi-aio.c
> @@ -5,14 +5,20 @@
>   
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
> +#include <linux/cleanup.h>
> +#include <linux/compiler_attributes.h>
> +#include <linux/device.h>
> +#include <linux/device/driver.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
>   #include <linux/init.h>
>   #include <linux/types.h>
>   #include <linux/input.h>
>   #include <linux/input/sparse-keymap.h>
> -#include <linux/acpi.h>
>   #include <linux/string.h>
> +#include <linux/wmi.h>
>   
>   MODULE_DESCRIPTION("WMI hotkeys driver for Dell All-In-One series");
>   MODULE_LICENSE("GPL");
> @@ -20,22 +26,19 @@ MODULE_LICENSE("GPL");
>   #define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
>   #define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
>   
> +struct dell_wmi_aio_data {
> +	struct input_dev *input_device;
> +	/* Protects the input sequence */
> +	struct mutex input_lock;
> +};
> +
>   struct dell_wmi_event {
> -	u16	length;
> +	__le16	length;
>   	/* 0x000: A hot key pressed or an event occurred
>   	 * 0x00F: A sequence of hot keys are pressed */
> -	u16	type;
> -	u16	event[];
> -};
> -
> -static const char *dell_wmi_aio_guids[] = {
> -	EVENT_GUID1,
> -	EVENT_GUID2,
> -	NULL
> -};
> -
> -MODULE_ALIAS("wmi:"EVENT_GUID1);
> -MODULE_ALIAS("wmi:"EVENT_GUID2);
> +	__le16	type;
> +	__le16	event[];
> +} __packed;
>   
>   static const struct key_entry dell_wmi_aio_keymap[] = {
>   	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
> @@ -50,137 +53,92 @@ static const struct key_entry dell_wmi_aio_keymap[] = {
>   	{ KE_END, 0 }
>   };
>   
> -static struct input_dev *dell_wmi_aio_input_dev;
> -
>   /*
>    * The new WMI event data format will follow the dell_wmi_event structure
>    * So, we will check if the buffer matches the format
>    */
> -static bool dell_wmi_aio_event_check(u8 *buffer, int length)
> +static bool dell_wmi_aio_event_check(const struct wmi_buffer *buffer)
>   {
> -	struct dell_wmi_event *event = (struct dell_wmi_event *)buffer;
> +	struct dell_wmi_event *event;
> +	u16 length, type;
>   
> -	if (event == NULL || length < 6)
> +	if (buffer->length < struct_size(event, event, 1))
>   		return false;
>   
> -	if ((event->type == 0 || event->type == 0xf) &&
> -			event->length >= 2)
> +	event = buffer->data;
> +	length = le16_to_cpu(event->length);
> +	type = le16_to_cpu(event->type);
> +	if ((type == 0 || type == 0xf) && length >= 2)
>   		return true;
>   
>   	return false;
>   }
>   
> -static void dell_wmi_aio_notify(union acpi_object *obj, void *context)
> +static void dell_wmi_aio_notify(struct wmi_device *wdev, const struct wmi_buffer *data)
>   {
> -	struct dell_wmi_event *event;
> -
> -	if (obj) {
> -		unsigned int scancode = 0;
> -
> -		switch (obj->type) {
> -		case ACPI_TYPE_INTEGER:
> -			/* Most All-In-One correctly return integer scancode */
> -			scancode = obj->integer.value;
> -			sparse_keymap_report_event(dell_wmi_aio_input_dev,
> -				scancode, 1, true);
> -			break;
> -		case ACPI_TYPE_BUFFER:
> -			if (dell_wmi_aio_event_check(obj->buffer.pointer,
> -						obj->buffer.length)) {
> -				event = (struct dell_wmi_event *)
> -					obj->buffer.pointer;
> -				scancode = event->event[0];
> -			} else {
> -				/* Broken machines return the scancode in a
> -				   buffer */
> -				if (obj->buffer.pointer &&
> -						obj->buffer.length > 0)
> -					scancode = obj->buffer.pointer[0];
> -			}
> -			if (scancode)
> -				sparse_keymap_report_event(
> -					dell_wmi_aio_input_dev,
> -					scancode, 1, true);
> -			break;
> -		}
> -	}
> -}
> -
> -static int __init dell_wmi_aio_input_setup(void)
> -{
> -	int err;
> -
> -	dell_wmi_aio_input_dev = input_allocate_device();
> -
> -	if (!dell_wmi_aio_input_dev)
> -		return -ENOMEM;
> -
> -	dell_wmi_aio_input_dev->name = "Dell AIO WMI hotkeys";
> -	dell_wmi_aio_input_dev->phys = "wmi/input0";
> -	dell_wmi_aio_input_dev->id.bustype = BUS_HOST;
> -
> -	err = sparse_keymap_setup(dell_wmi_aio_input_dev,
> -			dell_wmi_aio_keymap, NULL);
> -	if (err) {
> -		pr_err("Unable to setup input device keymap\n");
> -		goto err_free_dev;
> -	}
> -	err = input_register_device(dell_wmi_aio_input_dev);
> -	if (err) {
> -		pr_info("Unable to register input device\n");
> -		goto err_free_dev;
> +	struct dell_wmi_aio_data *drvdata = dev_get_drvdata(&wdev->dev);
> +	const struct dell_wmi_event *new_event;
> +	unsigned int scancode;
> +	const u8 *old_event;
> +
> +	if (dell_wmi_aio_event_check(data)) {
> +		new_event = data->data;
> +		scancode = le16_to_cpu(new_event->event[0]);
> +	} else {
> +		old_event = data->data;
> +		scancode = old_event[0];
>   	}
> -	return 0;
> -
> -err_free_dev:
> -	input_free_device(dell_wmi_aio_input_dev);
> -	return err;
> -}
> -
> -static const char *dell_wmi_aio_find(void)
> -{
> -	int i;
>   
> -	for (i = 0; dell_wmi_aio_guids[i] != NULL; i++)
> -		if (wmi_has_guid(dell_wmi_aio_guids[i]))
> -			return dell_wmi_aio_guids[i];
> +	guard(mutex)(&drvdata->input_lock);
>   
> -	return NULL;
> +	sparse_keymap_report_event(drvdata->input_device, scancode, 1, true);
>   }
>   
> -static int __init dell_wmi_aio_init(void)
> +static int dell_wmi_aio_probe(struct wmi_device *wdev, const void *context)
>   {
> -	int err;
> -	const char *guid;
> +	struct dell_wmi_aio_data *data;
> +	int ret;
>   
> -	guid = dell_wmi_aio_find();
> -	if (!guid) {
> -		pr_warn("No known WMI GUID found\n");
> -		return -ENXIO;
> -	}
> +	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
>   
> -	err = dell_wmi_aio_input_setup();
> -	if (err)
> -		return err;
> +	dev_set_drvdata(&wdev->dev, data);
> +	ret = devm_mutex_init(&wdev->dev, &data->input_lock);
> +	if (ret < 0)
> +		return ret;
>   
> -	err = wmi_install_notify_handler(guid, dell_wmi_aio_notify, NULL);
> -	if (err) {
> -		pr_err("Unable to register notify handler - %d\n", err);
> -		input_unregister_device(dell_wmi_aio_input_dev);
> -		return err;
> -	}
> +	data->input_device = devm_input_allocate_device(&wdev->dev);
> +	if (!data->input_device)
> +		return -ENOMEM;
>   
> -	return 0;
> -}
> +	data->input_device->name = "Dell AIO WMI hotkeys";
> +	data->input_device->phys = "wmi/input0";
> +	data->input_device->id.bustype = BUS_HOST;
>   
> -static void __exit dell_wmi_aio_exit(void)
> -{
> -	const char *guid;
> +	ret = sparse_keymap_setup(data->input_device, dell_wmi_aio_keymap, NULL);
> +	if (ret < 0)
> +		return ret;
>   
> -	guid = dell_wmi_aio_find();
> -	wmi_remove_notify_handler(guid);
> -	input_unregister_device(dell_wmi_aio_input_dev);
> +	return input_register_device(data->input_device);
>   }
>   
> -module_init(dell_wmi_aio_init);
> -module_exit(dell_wmi_aio_exit);
> +static const struct wmi_device_id dell_wmi_aio_id_table[] = {
> +	{ EVENT_GUID1, NULL },
> +	{ EVENT_GUID2, NULL },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_aio_id_table);
> +
> +static struct wmi_driver dell_wmi_aio_driver = {
> +	.driver = {
> +		.name = "dell-wmi-aio",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.id_table = dell_wmi_aio_id_table,
> +	.probe = dell_wmi_aio_probe,
> +	.notify_new = dell_wmi_aio_notify,
> +	.min_event_size = sizeof(u8),
> +	.no_singleton = true,
> +};
> +module_wmi_driver(dell_wmi_aio_driver);