[PATCH 0/5] platform/x86: wmi: Cleanup obsolete features

[PATCH 0/5] platform/x86: wmi: Cleanup obsolete features

[Armin Wolf]

This patch series removes three features deemed obsolete:
- the debug_dump_wdg module param:
  - suffers from garbled output due to pr_cont()
  - functionality is better provided by "fwts wmi"
- the debug_event module param:
  - pr_cont() usage
  - uses the deprecated GUID-based API
  - largely replaced by the ACPI netlink interface
- ioctl interface
  - used only by a single driver, no adoption otherwise
  - numerous design issues

Since the ioctl interface is actually used by userspace programs,
the only user (the dell-smbios-wmi driver) was modified to implement
the necessary pieces itself so that no regressions are expected.

The last patch in contrast adds a short WMI driver development guide
to the WMI subsystem documentation, so that driver developers stop
submitting WMI drivers using the deprecated GUID-based interface.

The series depends on
commit cbf54f37600e ("platform/x86: wmi: Skip blocks with zero instances"),
which is currently in the "fixes" tree.

All patches where tested on a Dell Inspiron 3505 and work without
issues.

Armin Wolf (5):
  platform/x86: wmi: Remove debug_dump_wdg module param
  platform/x86: wmi: Remove debug_wmi module param
  platform/x86: dell-smbios-wmi: Stop using WMI chardev
  platform/x86: wmi: Remove chardev interface
  platform/x86: wmi: Add driver development guide

 .../wmi/driver-development-guide.rst          | 126 ++++++++
 Documentation/wmi/index.rst                   |   1 +
 drivers/platform/x86/dell/dell-smbios-wmi.c   | 163 +++++++---
 drivers/platform/x86/wmi.c                    | 285 +-----------------
 include/linux/wmi.h                           |   8 -
 5 files changed, 256 insertions(+), 327 deletions(-)
 create mode 100644 Documentation/wmi/driver-development-guide.rst

--
2.39.2

[PATCH 1/5] platform/x86: wmi: Remove debug_dump_wdg module param

[Armin Wolf]

The functionality of dumping WDG entries is better provided by
userspace tools like "fwts wmi", which also does not suffer from
garbled printk output caused by pr_cont().

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4f94e4b117f1..e8019bc19b4f 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -90,11 +90,6 @@ module_param(debug_event, bool, 0444);
 MODULE_PARM_DESC(debug_event,
 		 "Log WMI Events [0/1]");

-static bool debug_dump_wdg;
-module_param(debug_dump_wdg, bool, 0444);
-MODULE_PARM_DESC(debug_dump_wdg,
-		 "Dump available WMI interfaces [0/1]");
-
 static const struct acpi_device_id wmi_device_ids[] = {
 	{"PNP0C14", 0},
 	{"pnp0c14", 0},
@@ -597,29 +592,6 @@ acpi_status wmidev_block_set(struct wmi_device *wdev, u8 instance, const struct
 }
 EXPORT_SYMBOL_GPL(wmidev_block_set);

-static void wmi_dump_wdg(const struct guid_block *g)
-{
-	pr_info("%pUL:\n", &g->guid);
-	if (g->flags & ACPI_WMI_EVENT)
-		pr_info("\tnotify_id: 0x%02X\n", g->notify_id);
-	else
-		pr_info("\tobject_id: %2pE\n", g->object_id);
-	pr_info("\tinstance_count: %d\n", g->instance_count);
-	pr_info("\tflags: %#x", g->flags);
-	if (g->flags) {
-		if (g->flags & ACPI_WMI_EXPENSIVE)
-			pr_cont(" ACPI_WMI_EXPENSIVE");
-		if (g->flags & ACPI_WMI_METHOD)
-			pr_cont(" ACPI_WMI_METHOD");
-		if (g->flags & ACPI_WMI_STRING)
-			pr_cont(" ACPI_WMI_STRING");
-		if (g->flags & ACPI_WMI_EVENT)
-			pr_cont(" ACPI_WMI_EVENT");
-	}
-	pr_cont("\n");
-
-}
-
 static void wmi_notify_debug(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -1343,9 +1315,6 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 	total = obj->buffer.length / sizeof(struct guid_block);

 	for (i = 0; i < total; i++) {
-		if (debug_dump_wdg)
-			wmi_dump_wdg(&gblock[i]);
-
 		if (!gblock[i].instance_count) {
 			dev_info(wmi_bus_dev, FW_INFO "%pUL has zero instances\n", &gblock[i].guid);
 			continue;
--
2.39.2

Re: [PATCH 1/5] platform/x86: wmi: Remove debug_dump_wdg module param

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

On Thu, 7 Dec 2023, Armin Wolf wrote:

> The functionality of dumping WDG entries is better provided by
> userspace tools like "fwts wmi", which also does not suffer from
> garbled printk output caused by pr_cont().
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH 2/5] platform/x86: wmi: Remove debug_event module param

[Armin Wolf]

Users can already listen to ACPI WMI events through
the ACPI netlink interface. The old wmi_notify_debug()
interface also uses the deprecated GUID-based interface.
Remove it to make the event handling code more readable.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 74 ++++----------------------------------
 1 file changed, 7 insertions(+), 67 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index e8019bc19b4f..7df5b5ee7983 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -85,11 +85,6 @@ struct wmi_block {
 #define ACPI_WMI_STRING      BIT(2)	/* GUID takes & returns a string */
 #define ACPI_WMI_EVENT       BIT(3)	/* GUID is an event */

-static bool debug_event;
-module_param(debug_event, bool, 0444);
-MODULE_PARM_DESC(debug_event,
-		 "Log WMI Events [0/1]");
-
 static const struct acpi_device_id wmi_device_ids[] = {
 	{"PNP0C14", 0},
 	{"pnp0c14", 0},
@@ -592,42 +587,6 @@ acpi_status wmidev_block_set(struct wmi_device *wdev, u8 instance, const struct
 }
 EXPORT_SYMBOL_GPL(wmidev_block_set);

-static void wmi_notify_debug(u32 value, void *context)
-{
-	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-	acpi_status status;
-
-	status = wmi_get_event_data(value, &response);
-	if (status != AE_OK) {
-		pr_info("bad event status 0x%x\n", status);
-		return;
-	}
-
-	obj = response.pointer;
-	if (!obj)
-		return;
-
-	pr_info("DEBUG: event 0x%02X ", value);
-	switch (obj->type) {
-	case ACPI_TYPE_BUFFER:
-		pr_cont("BUFFER_TYPE - length %u\n", obj->buffer.length);
-		break;
-	case ACPI_TYPE_STRING:
-		pr_cont("STRING_TYPE - %s\n", obj->string.pointer);
-		break;
-	case ACPI_TYPE_INTEGER:
-		pr_cont("INTEGER_TYPE - %llu\n", obj->integer.value);
-		break;
-	case ACPI_TYPE_PACKAGE:
-		pr_cont("PACKAGE_TYPE - %u elements\n", obj->package.count);
-		break;
-	default:
-		pr_cont("object type 0x%X\n", obj->type);
-	}
-	kfree(obj);
-}
-
 /**
  * wmi_install_notify_handler - Register handler for WMI events (deprecated)
  * @guid: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
@@ -656,8 +615,7 @@ acpi_status wmi_install_notify_handler(const char *guid,
 		acpi_status wmi_status;

 		if (guid_equal(&block->gblock.guid, &guid_input)) {
-			if (block->handler &&
-			    block->handler != wmi_notify_debug)
+			if (block->handler)
 				return AE_ALREADY_ACQUIRED;

 			block->handler = handler;
@@ -698,22 +656,14 @@ acpi_status wmi_remove_notify_handler(const char *guid)
 		acpi_status wmi_status;

 		if (guid_equal(&block->gblock.guid, &guid_input)) {
-			if (!block->handler ||
-			    block->handler == wmi_notify_debug)
+			if (!block->handler)
 				return AE_NULL_ENTRY;

-			if (debug_event) {
-				block->handler = wmi_notify_debug;
-				status = AE_OK;
-			} else {
-				wmi_status = wmi_method_enable(block, false);
-				block->handler = NULL;
-				block->handler_data = NULL;
-				if ((wmi_status != AE_OK) ||
-				    ((wmi_status == AE_OK) &&
-				     (status == AE_NOT_EXIST)))
-					status = wmi_status;
-			}
+			wmi_status = wmi_method_enable(block, false);
+			block->handler = NULL;
+			block->handler_data = NULL;
+			if (wmi_status != AE_OK || (wmi_status == AE_OK && status == AE_NOT_EXIST))
+				status = wmi_status;
 		}
 	}

@@ -1340,17 +1290,10 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)

 		list_add_tail(&wblock->list, &wmi_block_list);

-		if (debug_event) {
-			wblock->handler = wmi_notify_debug;
-			wmi_method_enable(wblock, true);
-		}
-
 		retval = wmi_add_device(pdev, &wblock->dev);
 		if (retval) {
 			dev_err(wmi_bus_dev, "failed to register %pUL\n",
 				&wblock->gblock.guid);
-			if (debug_event)
-				wmi_method_enable(wblock, false);

 			list_del(&wblock->list);
 			put_device(&wblock->dev.dev);
@@ -1445,9 +1388,6 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
 		wblock->handler(event, wblock->handler_data);
 	}

-	if (debug_event)
-		pr_info("DEBUG: GUID %pUL event 0x%02X\n", &wblock->gblock.guid, event);
-
 	acpi_bus_generate_netlink_event(
 		wblock->acpi_device->pnp.device_class,
 		dev_name(&wblock->dev.dev),
--
2.39.2

Re: [PATCH 2/5] platform/x86: wmi: Remove debug_event module param

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

On Thu, 7 Dec 2023, Armin Wolf wrote:

> Users can already listen to ACPI WMI events through
> the ACPI netlink interface. The old wmi_notify_debug()
> interface also uses the deprecated GUID-based interface.
> Remove it to make the event handling code more readable.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH 3/5] platform/x86: dell-smbios-wmi: Stop using WMI chardev

[Armin Wolf]

The WMI chardev API will be removed in the near future.
Reimplement the necessary bits used by this driver so
that userspace software depending on it does no break.

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

diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
index 931cc50136de..61f40f462eca 100644
--- a/drivers/platform/x86/dell/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell/dell-smbios-wmi.c
@@ -7,11 +7,14 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

 #include <linux/dmi.h>
+#include <linux/fs.h>
 #include <linux/list.h>
+#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/wmi.h>
+#include <uapi/linux/wmi.h>
 #include "dell-smbios.h"
 #include "dell-wmi-descriptor.h"

@@ -32,7 +35,9 @@ struct wmi_smbios_priv {
 	struct list_head list;
 	struct wmi_device *wdev;
 	struct device *child;
-	u32 req_buf_size;
+	u64 req_buf_size;
+	u32 hotfix;
+	struct miscdevice char_dev;
 };
 static LIST_HEAD(wmi_list);

@@ -108,48 +113,106 @@ static int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
 	return ret;
 }

-static long dell_smbios_wmi_filter(struct wmi_device *wdev, unsigned int cmd,
-				   struct wmi_ioctl_buffer *arg)
+static int dell_smbios_wmi_open(struct inode *inode, struct file *filp)
 {
 	struct wmi_smbios_priv *priv;
-	int ret = 0;
-
-	switch (cmd) {
-	case DELL_WMI_SMBIOS_CMD:
-		mutex_lock(&call_mutex);
-		priv = dev_get_drvdata(&wdev->dev);
-		if (!priv) {
-			ret = -ENODEV;
-			goto fail_smbios_cmd;
-		}
-		memcpy(priv->buf, arg, priv->req_buf_size);
-		if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
-			dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
-				priv->buf->std.cmd_class,
-				priv->buf->std.cmd_select,
-				priv->buf->std.input[0]);
-			ret = -EFAULT;
-			goto fail_smbios_cmd;
-		}
-		ret = run_smbios_call(priv->wdev);
-		if (ret)
-			goto fail_smbios_cmd;
-		memcpy(arg, priv->buf, priv->req_buf_size);
-fail_smbios_cmd:
-		mutex_unlock(&call_mutex);
-		break;
-	default:
-		ret = -ENOIOCTLCMD;
+
+	priv = container_of(filp->private_data, struct wmi_smbios_priv, char_dev);
+	filp->private_data = priv;
+
+	return nonseekable_open(inode, filp);
+}
+
+static ssize_t dell_smbios_wmi_read(struct file *filp, char __user *buffer, size_t length,
+				    loff_t *offset)
+{
+	struct wmi_smbios_priv *priv = filp->private_data;
+
+	return simple_read_from_buffer(buffer, length, offset, &priv->req_buf_size,
+				       sizeof(priv->req_buf_size));
+}
+
+static long dell_smbios_wmi_do_ioctl(struct wmi_smbios_priv *priv,
+				     struct dell_wmi_smbios_buffer __user *arg)
+{
+	long ret;
+
+	if (get_user(priv->buf->length, &arg->length))
+		return -EFAULT;
+
+	if (priv->buf->length < priv->req_buf_size)
+		return -EINVAL;
+
+	/* if it's too big, warn, driver will only use what is needed */
+	if (priv->buf->length > priv->req_buf_size)
+		dev_err(&priv->wdev->dev, "Buffer %llu is bigger than required %llu\n",
+			priv->buf->length, priv->req_buf_size);
+
+	if (copy_from_user(priv->buf, arg, priv->req_buf_size))
+		return -EFAULT;
+
+	if (dell_smbios_call_filter(&priv->wdev->dev, &priv->buf->std)) {
+		dev_err(&priv->wdev->dev, "Invalid call %d/%d:%8x\n",
+			priv->buf->std.cmd_class,
+			priv->buf->std.cmd_select,
+			priv->buf->std.input[0]);
+
+		return -EINVAL;
 	}
+
+	ret = run_smbios_call(priv->wdev);
+	if (ret)
+		return ret;
+
+	if (copy_to_user(arg, priv->buf, priv->req_buf_size))
+		return -EFAULT;
+
+	return 0;
+}
+
+static long dell_smbios_wmi_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct dell_wmi_smbios_buffer __user *input = (struct dell_wmi_smbios_buffer __user *)arg;
+	struct wmi_smbios_priv *priv = filp->private_data;
+	long ret;
+
+	if (cmd != DELL_WMI_SMBIOS_CMD)
+		return -ENOIOCTLCMD;
+
+	mutex_lock(&call_mutex);
+	ret = dell_smbios_wmi_do_ioctl(priv, input);
+	mutex_unlock(&call_mutex);
+
 	return ret;
 }

+static const struct file_operations dell_smbios_wmi_fops = {
+	.owner		= THIS_MODULE,
+	.open		= dell_smbios_wmi_open,
+	.read		= dell_smbios_wmi_read,
+	.unlocked_ioctl	= dell_smbios_wmi_ioctl,
+	.compat_ioctl	= compat_ptr_ioctl,
+};
+
+static int dell_smbios_wmi_register_chardev(struct wmi_smbios_priv *priv)
+{
+	priv->char_dev.minor = MISC_DYNAMIC_MINOR;
+	priv->char_dev.name = "wmi/dell-smbios";
+	priv->char_dev.fops = &dell_smbios_wmi_fops;
+	priv->char_dev.mode = 0444;
+
+	return misc_register(&priv->char_dev);
+}
+
+static void dell_smbios_wmi_unregister_chardev(struct wmi_smbios_priv *priv)
+{
+	misc_deregister(&priv->char_dev);
+}
+
 static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
 {
-	struct wmi_driver *wdriver =
-		container_of(wdev->dev.driver, struct wmi_driver, driver);
 	struct wmi_smbios_priv *priv;
-	u32 hotfix;
+	u32 buffer_size;
 	int count;
 	int ret;

@@ -162,39 +225,44 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
 	if (!priv)
 		return -ENOMEM;

+	priv->wdev = wdev;
+	dev_set_drvdata(&wdev->dev, priv);
+
 	/* WMI buffer size will be either 4k or 32k depending on machine */
-	if (!dell_wmi_get_size(&priv->req_buf_size))
+	if (!dell_wmi_get_size(&buffer_size))
 		return -EPROBE_DEFER;

+	priv->req_buf_size = buffer_size;
+
 	/* some SMBIOS calls fail unless BIOS contains hotfix */
-	if (!dell_wmi_get_hotfix(&hotfix))
+	if (!dell_wmi_get_hotfix(&priv->hotfix))
 		return -EPROBE_DEFER;
-	if (!hotfix) {
+
+	if (!priv->hotfix)
 		dev_warn(&wdev->dev,
 			"WMI SMBIOS userspace interface not supported(%u), try upgrading to a newer BIOS\n",
-			hotfix);
-		wdriver->filter_callback = NULL;
-	}
+			priv->hotfix);

 	/* add in the length object we will use internally with ioctl */
 	priv->req_buf_size += sizeof(u64);
-	ret = set_required_buffer_size(wdev, priv->req_buf_size);
-	if (ret)
-		return ret;

 	count = get_order(priv->req_buf_size);
 	priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
 	if (!priv->buf)
 		return -ENOMEM;

+	if (priv->hotfix) {
+		ret = dell_smbios_wmi_register_chardev(priv);
+		if (ret)
+			goto fail_chardev;
+	}
+
 	/* ID is used by dell-smbios to set priority of drivers */
 	wdev->dev.id = 1;
 	ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
 	if (ret)
 		goto fail_register;

-	priv->wdev = wdev;
-	dev_set_drvdata(&wdev->dev, priv);
 	mutex_lock(&list_mutex);
 	list_add_tail(&priv->list, &wmi_list);
 	mutex_unlock(&list_mutex);
@@ -202,6 +270,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
 	return 0;

 fail_register:
+	if (priv->hotfix)
+		dell_smbios_wmi_unregister_chardev(priv);
+fail_chardev:
 	free_pages((unsigned long)priv->buf, count);
 	return ret;
 }
@@ -211,6 +282,7 @@ static void dell_smbios_wmi_remove(struct wmi_device *wdev)
 	struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
 	int count;

+	dell_smbios_wmi_unregister_chardev(priv);
 	mutex_lock(&call_mutex);
 	mutex_lock(&list_mutex);
 	list_del(&priv->list);
@@ -256,7 +328,6 @@ static struct wmi_driver dell_smbios_wmi_driver = {
 	.probe = dell_smbios_wmi_probe,
 	.remove = dell_smbios_wmi_remove,
 	.id_table = dell_smbios_wmi_id_table,
-	.filter_callback = dell_smbios_wmi_filter,
 };

 int init_dell_smbios_wmi(void)
--
2.39.2

Re: [PATCH 3/5] platform/x86: dell-smbios-wmi: Stop using WMI chardev

[Ilpo Järvinen]

On Thu, 7 Dec 2023, Armin Wolf wrote:

> The WMI chardev API will be removed in the near future.
> Reimplement the necessary bits used by this driver so
> that userspace software depending on it does no break.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/dell/dell-smbios-wmi.c | 163 ++++++++++++++------
>  1 file changed, 117 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
> index 931cc50136de..61f40f462eca 100644
> --- a/drivers/platform/x86/dell/dell-smbios-wmi.c
> +++ b/drivers/platform/x86/dell/dell-smbios-wmi.c

> @@ -32,7 +35,9 @@ struct wmi_smbios_priv {
>  	struct list_head list;
>  	struct wmi_device *wdev;
>  	struct device *child;
> -	u32 req_buf_size;
> +	u64 req_buf_size;
> +	u32 hotfix;
> +	struct miscdevice char_dev;
>  };
>  static LIST_HEAD(wmi_list);


>  static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
>  {
> -	struct wmi_driver *wdriver =
> -		container_of(wdev->dev.driver, struct wmi_driver, driver);
>  	struct wmi_smbios_priv *priv;
> -	u32 hotfix;
> +	u32 buffer_size;
>  	int count;
>  	int ret;
> 
> @@ -162,39 +225,44 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
>  	if (!priv)
>  		return -ENOMEM;
> 
> +	priv->wdev = wdev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +
>  	/* WMI buffer size will be either 4k or 32k depending on machine */
> -	if (!dell_wmi_get_size(&priv->req_buf_size))
> +	if (!dell_wmi_get_size(&buffer_size))
>  		return -EPROBE_DEFER;
> 
> +	priv->req_buf_size = buffer_size;
> +
>  	/* some SMBIOS calls fail unless BIOS contains hotfix */
> -	if (!dell_wmi_get_hotfix(&hotfix))
> +	if (!dell_wmi_get_hotfix(&priv->hotfix))
>  		return -EPROBE_DEFER;
> -	if (!hotfix) {
> +
> +	if (!priv->hotfix)
>  		dev_warn(&wdev->dev,
>  			"WMI SMBIOS userspace interface not supported(%u), try upgrading to a newer BIOS\n",
> -			hotfix);
> -		wdriver->filter_callback = NULL;
> -	}
> +			priv->hotfix);
> 
>  	/* add in the length object we will use internally with ioctl */
>  	priv->req_buf_size += sizeof(u64);
> -	ret = set_required_buffer_size(wdev, priv->req_buf_size);
> -	if (ret)
> -		return ret;
> 
>  	count = get_order(priv->req_buf_size);
>  	priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
>  	if (!priv->buf)
>  		return -ENOMEM;
> 
> +	if (priv->hotfix) {
> +		ret = dell_smbios_wmi_register_chardev(priv);
> +		if (ret)
> +			goto fail_chardev;
> +	}
> +
>  	/* ID is used by dell-smbios to set priority of drivers */
>  	wdev->dev.id = 1;
>  	ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
>  	if (ret)
>  		goto fail_register;
> 
> -	priv->wdev = wdev;
> -	dev_set_drvdata(&wdev->dev, priv);
>  	mutex_lock(&list_mutex);
>  	list_add_tail(&priv->list, &wmi_list);
>  	mutex_unlock(&list_mutex);
> @@ -202,6 +270,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
>  	return 0;
> 
>  fail_register:
> +	if (priv->hotfix)
> +               dell_smbios_wmi_unregister_chardev(priv);

I don't understand how hotfix -> priv->hotfix is related to this patch nor 
why it's necessary?

Or did you mean to use it also in dell_smbios_wmi_remove() but forgot to 
add the if (priv->hotfix) there?

In any case, it would be better to put that conversion into own patch 
before this one.

> @@ -211,6 +282,7 @@ static void dell_smbios_wmi_remove(struct wmi_device *wdev)
>        struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
>        int count;
>
> +      dell_smbios_wmi_unregister_chardev(priv);
>        mutex_lock(&call_mutex);
>        mutex_lock(&list_mutex);
>        list_del(&priv->list);

-- 
 i.

Re: [PATCH 3/5] platform/x86: dell-smbios-wmi: Stop using WMI chardev

[Armin Wolf]

Am 08.12.23 um 14:41 schrieb Ilpo Järvinen:

> On Thu, 7 Dec 2023, Armin Wolf wrote:
>
>> The WMI chardev API will be removed in the near future.
>> Reimplement the necessary bits used by this driver so
>> that userspace software depending on it does no break.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/dell/dell-smbios-wmi.c | 163 ++++++++++++++------
>>   1 file changed, 117 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
>> index 931cc50136de..61f40f462eca 100644
>> --- a/drivers/platform/x86/dell/dell-smbios-wmi.c
>> +++ b/drivers/platform/x86/dell/dell-smbios-wmi.c
>> @@ -32,7 +35,9 @@ struct wmi_smbios_priv {
>>   	struct list_head list;
>>   	struct wmi_device *wdev;
>>   	struct device *child;
>> -	u32 req_buf_size;
>> +	u64 req_buf_size;
>> +	u32 hotfix;
>> +	struct miscdevice char_dev;
>>   };
>>   static LIST_HEAD(wmi_list);
>
>>   static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
>>   {
>> -	struct wmi_driver *wdriver =
>> -		container_of(wdev->dev.driver, struct wmi_driver, driver);
>>   	struct wmi_smbios_priv *priv;
>> -	u32 hotfix;
>> +	u32 buffer_size;
>>   	int count;
>>   	int ret;
>>
>> @@ -162,39 +225,44 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
>>   	if (!priv)
>>   		return -ENOMEM;
>>
>> +	priv->wdev = wdev;
>> +	dev_set_drvdata(&wdev->dev, priv);
>> +
>>   	/* WMI buffer size will be either 4k or 32k depending on machine */
>> -	if (!dell_wmi_get_size(&priv->req_buf_size))
>> +	if (!dell_wmi_get_size(&buffer_size))
>>   		return -EPROBE_DEFER;
>>
>> +	priv->req_buf_size = buffer_size;
>> +
>>   	/* some SMBIOS calls fail unless BIOS contains hotfix */
>> -	if (!dell_wmi_get_hotfix(&hotfix))
>> +	if (!dell_wmi_get_hotfix(&priv->hotfix))
>>   		return -EPROBE_DEFER;
>> -	if (!hotfix) {
>> +
>> +	if (!priv->hotfix)
>>   		dev_warn(&wdev->dev,
>>   			"WMI SMBIOS userspace interface not supported(%u), try upgrading to a newer BIOS\n",
>> -			hotfix);
>> -		wdriver->filter_callback = NULL;
>> -	}
>> +			priv->hotfix);
>>
>>   	/* add in the length object we will use internally with ioctl */
>>   	priv->req_buf_size += sizeof(u64);
>> -	ret = set_required_buffer_size(wdev, priv->req_buf_size);
>> -	if (ret)
>> -		return ret;
>>
>>   	count = get_order(priv->req_buf_size);
>>   	priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
>>   	if (!priv->buf)
>>   		return -ENOMEM;
>>
>> +	if (priv->hotfix) {
>> +		ret = dell_smbios_wmi_register_chardev(priv);
>> +		if (ret)
>> +			goto fail_chardev;
>> +	}
>> +
>>   	/* ID is used by dell-smbios to set priority of drivers */
>>   	wdev->dev.id = 1;
>>   	ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
>>   	if (ret)
>>   		goto fail_register;
>>
>> -	priv->wdev = wdev;
>> -	dev_set_drvdata(&wdev->dev, priv);
>>   	mutex_lock(&list_mutex);
>>   	list_add_tail(&priv->list, &wmi_list);
>>   	mutex_unlock(&list_mutex);
>> @@ -202,6 +270,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
>>   	return 0;
>>
>>   fail_register:
>> +	if (priv->hotfix)
>> +               dell_smbios_wmi_unregister_chardev(priv);
> I don't understand how hotfix -> priv->hotfix is related to this patch nor
> why it's necessary?
>
> Or did you mean to use it also in dell_smbios_wmi_remove() but forgot to
> add the if (priv->hotfix) there?

I indeed forgot to add the "if (priv->hotfix)" here, good catch.

> In any case, it would be better to put that conversion into own patch
> before this one.

I could also drop the priv->hotfix related changes and instead modify the driver
to use devres (devm_get_free_pages() for example). This would also simplify the
error handling code.

I will send a v2 soon containing the necessary patches.

>> @@ -211,6 +282,7 @@ static void dell_smbios_wmi_remove(struct wmi_device *wdev)
>>         struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
>>         int count;
>>
>> +      dell_smbios_wmi_unregister_chardev(priv);
>>         mutex_lock(&call_mutex);
>>         mutex_lock(&list_mutex);
>>         list_del(&priv->list);

[PATCH 4/5] platform/x86: wmi: Remove chardev interface

[Armin Wolf]

The design of the WMI chardev interface is broken:
- it assumes that WMI drivers are not instantiated twice
- it offers next to no abstractions, the WMI driver gets
a raw byte buffer
- it is only used by a single driver, something which is
unlikely to change

Since the only user (dell-smbios-wmi) has been migrated
to his own ioctl interface, remove it.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 180 ++-----------------------------------
 include/linux/wmi.h        |   8 --
 2 files changed, 5 insertions(+), 183 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7df5b5ee7983..7303702290e5 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -23,17 +23,14 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
-#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
-#include <linux/uaccess.h>
 #include <linux/uuid.h>
 #include <linux/wmi.h>
 #include <linux/fs.h>
-#include <uapi/linux/wmi.h>

 MODULE_AUTHOR("Carlos Corbacho");
 MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
@@ -66,12 +63,9 @@ struct wmi_block {
 	struct wmi_device dev;
 	struct list_head list;
 	struct guid_block gblock;
-	struct miscdevice char_dev;
-	struct mutex char_mutex;
 	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
-	u64 req_buf_size;
 	unsigned long flags;
 };

@@ -256,26 +250,6 @@ static void wmi_device_put(struct wmi_device *wdev)
  * Exported WMI functions
  */

-/**
- * set_required_buffer_size - Sets the buffer size needed for performing IOCTL
- * @wdev: A wmi bus device from a driver
- * @length: Required buffer size
- *
- * Allocates memory needed for buffer, stores the buffer size in that memory.
- *
- * Return: 0 on success or a negative error code for failure.
- */
-int set_required_buffer_size(struct wmi_device *wdev, u64 length)
-{
-	struct wmi_block *wblock;
-
-	wblock = container_of(wdev, struct wmi_block, dev);
-	wblock->req_buf_size = length;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(set_required_buffer_size);
-
 /**
  * wmi_instance_count - Get number of WMI object instances
  * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
@@ -884,111 +858,12 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)

 	return 0;
 }
-static int wmi_char_open(struct inode *inode, struct file *filp)
-{
-	/*
-	 * The miscdevice already stores a pointer to itself
-	 * inside filp->private_data
-	 */
-	struct wmi_block *wblock = container_of(filp->private_data, struct wmi_block, char_dev);
-
-	filp->private_data = wblock;
-
-	return nonseekable_open(inode, filp);
-}
-
-static ssize_t wmi_char_read(struct file *filp, char __user *buffer,
-			     size_t length, loff_t *offset)
-{
-	struct wmi_block *wblock = filp->private_data;
-
-	return simple_read_from_buffer(buffer, length, offset,
-				       &wblock->req_buf_size,
-				       sizeof(wblock->req_buf_size));
-}
-
-static long wmi_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
-	struct wmi_ioctl_buffer __user *input =
-		(struct wmi_ioctl_buffer __user *) arg;
-	struct wmi_block *wblock = filp->private_data;
-	struct wmi_ioctl_buffer *buf;
-	struct wmi_driver *wdriver;
-	int ret;
-
-	if (_IOC_TYPE(cmd) != WMI_IOC)
-		return -ENOTTY;
-
-	/* make sure we're not calling a higher instance than exists*/
-	if (_IOC_NR(cmd) >= wblock->gblock.instance_count)
-		return -EINVAL;
-
-	mutex_lock(&wblock->char_mutex);
-	buf = wblock->handler_data;
-	if (get_user(buf->length, &input->length)) {
-		dev_dbg(&wblock->dev.dev, "Read length from user failed\n");
-		ret = -EFAULT;
-		goto out_ioctl;
-	}
-	/* if it's too small, abort */
-	if (buf->length < wblock->req_buf_size) {
-		dev_err(&wblock->dev.dev,
-			"Buffer %lld too small, need at least %lld\n",
-			buf->length, wblock->req_buf_size);
-		ret = -EINVAL;
-		goto out_ioctl;
-	}
-	/* if it's too big, warn, driver will only use what is needed */
-	if (buf->length > wblock->req_buf_size)
-		dev_warn(&wblock->dev.dev,
-			"Buffer %lld is bigger than required %lld\n",
-			buf->length, wblock->req_buf_size);
-
-	/* copy the structure from userspace */
-	if (copy_from_user(buf, input, wblock->req_buf_size)) {
-		dev_dbg(&wblock->dev.dev, "Copy %llu from user failed\n",
-			wblock->req_buf_size);
-		ret = -EFAULT;
-		goto out_ioctl;
-	}
-
-	/* let the driver do any filtering and do the call */
-	wdriver = drv_to_wdrv(wblock->dev.dev.driver);
-	if (!try_module_get(wdriver->driver.owner)) {
-		ret = -EBUSY;
-		goto out_ioctl;
-	}
-	ret = wdriver->filter_callback(&wblock->dev, cmd, buf);
-	module_put(wdriver->driver.owner);
-	if (ret)
-		goto out_ioctl;
-
-	/* return the result (only up to our internal buffer size) */
-	if (copy_to_user(input, buf, wblock->req_buf_size)) {
-		dev_dbg(&wblock->dev.dev, "Copy %llu to user failed\n",
-			wblock->req_buf_size);
-		ret = -EFAULT;
-	}
-
-out_ioctl:
-	mutex_unlock(&wblock->char_mutex);
-	return ret;
-}
-
-static const struct file_operations wmi_fops = {
-	.owner		= THIS_MODULE,
-	.read		= wmi_char_read,
-	.open		= wmi_char_open,
-	.unlocked_ioctl	= wmi_ioctl,
-	.compat_ioctl	= compat_ptr_ioctl,
-};

 static int wmi_dev_probe(struct device *dev)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
 	int ret = 0;
-	char *buf;

 	if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
 		dev_warn(dev, "failed to enable device -- probing anyway\n");
@@ -996,55 +871,17 @@ static int wmi_dev_probe(struct device *dev)
 	if (wdriver->probe) {
 		ret = wdriver->probe(dev_to_wdev(dev),
 				find_guid_context(wblock, wdriver));
-		if (ret != 0)
-			goto probe_failure;
-	}
-
-	/* driver wants a character device made */
-	if (wdriver->filter_callback) {
-		/* check that required buffer size declared by driver or MOF */
-		if (!wblock->req_buf_size) {
-			dev_err(&wblock->dev.dev,
-				"Required buffer size not set\n");
-			ret = -EINVAL;
-			goto probe_failure;
-		}
+		if (!ret) {
+			if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+				dev_warn(dev, "Failed to disable device\n");

-		wblock->handler_data = kmalloc(wblock->req_buf_size,
-					       GFP_KERNEL);
-		if (!wblock->handler_data) {
-			ret = -ENOMEM;
-			goto probe_failure;
-		}
-
-		buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name);
-		if (!buf) {
-			ret = -ENOMEM;
-			goto probe_string_failure;
-		}
-		wblock->char_dev.minor = MISC_DYNAMIC_MINOR;
-		wblock->char_dev.name = buf;
-		wblock->char_dev.fops = &wmi_fops;
-		wblock->char_dev.mode = 0444;
-		ret = misc_register(&wblock->char_dev);
-		if (ret) {
-			dev_warn(dev, "failed to register char dev: %d\n", ret);
-			ret = -ENOMEM;
-			goto probe_misc_failure;
+			return ret;
 		}
 	}

 	set_bit(WMI_PROBED, &wblock->flags);
-	return 0;

-probe_misc_failure:
-	kfree(buf);
-probe_string_failure:
-	kfree(wblock->handler_data);
-probe_failure:
-	if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
-		dev_warn(dev, "failed to disable device\n");
-	return ret;
+	return 0;
 }

 static void wmi_dev_remove(struct device *dev)
@@ -1054,12 +891,6 @@ static void wmi_dev_remove(struct device *dev)

 	clear_bit(WMI_PROBED, &wblock->flags);

-	if (wdriver->filter_callback) {
-		misc_deregister(&wblock->char_dev);
-		kfree(wblock->char_dev.name);
-		kfree(wblock->handler_data);
-	}
-
 	if (wdriver->remove)
 		wdriver->remove(dev_to_wdev(dev));

@@ -1131,7 +962,6 @@ static int wmi_create_device(struct device *wmi_bus_dev,

 	if (wblock->gblock.flags & ACPI_WMI_METHOD) {
 		wblock->dev.dev.type = &wmi_type_method;
-		mutex_init(&wblock->char_mutex);
 		goto out_init;
 	}

diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 8a643c39fcce..50f7f1e4fd4f 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -11,7 +11,6 @@
 #include <linux/device.h>
 #include <linux/acpi.h>
 #include <linux/mod_devicetable.h>
-#include <uapi/linux/wmi.h>

 /**
  * struct wmi_device - WMI device structure
@@ -47,8 +46,6 @@ acpi_status wmidev_block_set(struct wmi_device *wdev, u8 instance, const struct

 u8 wmidev_instance_count(struct wmi_device *wdev);

-extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
-
 /**
  * struct wmi_driver - WMI driver structure
  * @driver: Driver model structure
@@ -57,11 +54,8 @@ extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
  * @probe: Callback for device binding
  * @remove: Callback for device unbinding
  * @notify: Callback for receiving WMI events
- * @filter_callback: Callback for filtering device IOCTLs
  *
  * This represents WMI drivers which handle WMI devices.
- * @filter_callback is only necessary for drivers which
- * want to set up a WMI IOCTL interface.
  */
 struct wmi_driver {
 	struct device_driver driver;
@@ -71,8 +65,6 @@ struct wmi_driver {
 	int (*probe)(struct wmi_device *wdev, const void *context);
 	void (*remove)(struct wmi_device *wdev);
 	void (*notify)(struct wmi_device *device, union acpi_object *data);
-	long (*filter_callback)(struct wmi_device *wdev, unsigned int cmd,
-				struct wmi_ioctl_buffer *arg);
 };

 extern int __must_check __wmi_driver_register(struct wmi_driver *driver,
--
2.39.2

Re: [PATCH 4/5] platform/x86: wmi: Remove chardev interface

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Thu, 7 Dec 2023, Armin Wolf wrote:

> The design of the WMI chardev interface is broken:
> - it assumes that WMI drivers are not instantiated twice
> - it offers next to no abstractions, the WMI driver gets
> a raw byte buffer
> - it is only used by a single driver, something which is
> unlikely to change
> 
> Since the only user (dell-smbios-wmi) has been migrated
> to his own ioctl interface, remove it.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH 5/5] platform/x86: wmi: Add driver development guide

[Armin Wolf]

Since 2010, an LWN article covering WMI drivers exists:

	https://lwn.net/Articles/391230/

Since the introduction of the modern bus-based interface
and other userspace tooling (fwts wmi, bmfdec, ...), this
article is outdated and causes people to still submit new
WMI drivers using the deprecated GUID-based interface.
Fix this by adding a short guid on how to develop WMI drivers
using the modern bus-based interface.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 .../wmi/driver-development-guide.rst          | 126 ++++++++++++++++++
 Documentation/wmi/index.rst                   |   1 +
 2 files changed, 127 insertions(+)
 create mode 100644 Documentation/wmi/driver-development-guide.rst

diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
new file mode 100644
index 000000000000..a831e2728d25
--- /dev/null
+++ b/Documentation/wmi/driver-development-guide.rst
@@ -0,0 +1,126 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+============================
+WMI driver development guide
+============================
+
+The WMI subsystem provides a rich driver api for implementing WMI drivers,
+documented at Documentation/driver-api/wmi.rst. This document will serve
+as an introductory guide for WMI driver writers using this API. It is supposed
+t be an successor to the original `LWN article <https://lwn.net/Articles/391230/>`_
+which deals with WMI drivers using the deprecated GUID-based WMI interface.
+
+Optaining WMI device information
+--------------------------------
+
+Before developing an WMI driver, information about the WMI device in question
+must be optained. The `lswmi <https://pypi.org/project/lswmi>`_ utility can be
+used to display detailed WMI device information using the following command:
+
+::
+
+  lswmi -V
+
+The resulting output will contain information about all WMI devices inside a given
+machine, plus some extra information.
+
+In order to find out more about the interface used to communicate with a WMI device,
+the `bmfdec <https://github.com/pali/bmfdec>`_ utilities can be used to decode
+the Binary MOF information used to describe WMI devices. The ``wmi-bmof`` driver
+exposes this information to userspace, see Documentation/ABI/stable/sysfs-platform-wmi-bmof.
+In order to retrieve the decoded Binary MOF information, use the following command (requires root):
+
+::
+
+  ./bmf2mof /sys/bus/wmi/devices/05901221-D566-11D1-B2F0-00A0C9062910[-X]/bmof
+
+Sometimes, looking at the disassembled ACPI tables used to describe the WMI device
+helps in understanding how the WMI device is supposed to work. To find out which
+ACPI method handles which WMI device, the `fwts <https://github.com/fwts/fwts>`_
+program can be used with the following command (requires root):
+
+::
+
+  fwts wmi -
+
+Basic WMI driver structure
+--------------------------
+
+The basic WMI driver is build around the struct wmi_driver, which is then bound
+to matching WMI devices using an struct wmi_device_id table. Please note that each
+WMI driver should be able to be instantiated multiple times.
+
+::
+
+  static const struct wmi_device_id foo_id_table[] = {
+         { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
+         { }
+  };
+  MODULE_DEVICE_TABLE(wmi, foo_id_table);
+
+  static struct wmi_driver foo_driver = {
+        .driver = {
+                .name = "foo",
+                .probe_type = PROBE_PREFER_ASYNCHRONOUS,        /* optional */
+                .pm = pm_sleep_ptr(&foo_dev_pm_ops),            /* optional */
+        },
+        .id_table = foo_id_table,
+        .probe = foo_probe,
+        .remove = foo_remove,         /* optional, devres is preferred */
+        .notify = foo_notify,         /* optional, for event handling */
+  };
+  module_wmi_driver(foo_driver);
+
+If your WMI driver is not using any deprecated GUID-based WMI functions and is
+able to be instantiated multiple times, please add its GUID to ``allow_duplicates``
+inside drivers/platform/x86/wmi.c, so that the WMI subsystem does not block duplicate
+GUIDs for it.
+
+WMI method drivers
+------------------
+
+WMI drivers can call WMI device methods using wmidev_evaluate_method(), the
+structure of the ACPI buffer passed to this function is device-specific and usually
+needs some tinkering to get right. Looking at the ACPI tables containing the WMI
+device usually helps here. The method id and instance number passed to this function
+are also device-specific, looking at the decoded Binary MOF is usually enough to
+find the right values.
+The maximum instance number can be retrieved during runtime using wmidev_instance_count().
+
+Take a look at drivers/platform/x86/inspur_platform_profile.c for an example WMI method driver.
+
+WMI data block drivers
+----------------------
+
+WMI drivers can query WMI device data blocks using wmidev_block_query(), the
+structure of the returned ACPI object is again device-specific. Some WMI devices
+also allow for setting data blocks using wmidev_block_set().
+The maximum instance number can also be retrieved using wmidev_instance_count().
+
+Take a look at drivers/platform/x86/intel/wmi/sbl-fw-update.c for an example
+WMI data block driver.
+
+WMI event drivers
+-----------------
+
+WMI drivers can receive WMI event notifications by providing the notify() callback
+inside the struct wmi_driver. The WMI subsystem will then take care of setting
+up the WMI event accordingly. Plase note that the ACPI object passed to this callback
+is optional and its structure device-specific. It also does not need to be freed,
+the WMI subsystem takes care of that.
+
+Take a look at drivers/platform/x86/xiaomi-wmi.c for an example WMI event driver.
+
+Things to avoid
+---------------
+
+When developing WMI drivers, there are a couple of things which should be avoid
+if feasible:
+
+- usage of the deprecated GUID-based WMI interface
+- bypassing of the WMI subsystem when talking to WMI devices
+- WMI drivers which cannot be instantiated multiple times.
+
+Many older WMI drivers violate one or more points from this list. The reason for
+this is that the WMI subsystem evolved significantly over the last two decades,
+so there is a lot of legacy cruft inside older WMI drivers.
diff --git a/Documentation/wmi/index.rst b/Documentation/wmi/index.rst
index 537cff188e14..fec4b6ae97b3 100644
--- a/Documentation/wmi/index.rst
+++ b/Documentation/wmi/index.rst
@@ -8,6 +8,7 @@ WMI Subsystem
    :maxdepth: 1

    acpi-interface
+   driver-development-guide
    devices/index

 .. only::  subproject and html
--
2.39.2

Re: [PATCH 5/5] platform/x86: wmi: Add driver development guide

[Ilpo Järvinen]

On Thu, 7 Dec 2023, Armin Wolf wrote:

> Since 2010, an LWN article covering WMI drivers exists:
> 
> 	https://lwn.net/Articles/391230/
> 
> Since the introduction of the modern bus-based interface
> and other userspace tooling (fwts wmi, bmfdec, ...), this
> article is outdated and causes people to still submit new
> WMI drivers using the deprecated GUID-based interface.
> Fix this by adding a short guid on how to develop WMI drivers

Too used to typing guid(?), here you want "guide" instead. :-D (I know 
that feeling when my fingers type something else than I think).

> using the modern bus-based interface.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  .../wmi/driver-development-guide.rst          | 126 ++++++++++++++++++
>  Documentation/wmi/index.rst                   |   1 +
>  2 files changed, 127 insertions(+)
>  create mode 100644 Documentation/wmi/driver-development-guide.rst
> 
> diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
> new file mode 100644
> index 000000000000..a831e2728d25
> --- /dev/null
> +++ b/Documentation/wmi/driver-development-guide.rst
> @@ -0,0 +1,126 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +============================
> +WMI driver development guide
> +============================
> +
> +The WMI subsystem provides a rich driver api for implementing WMI drivers,

API

> +documented at Documentation/driver-api/wmi.rst. This document will serve
> +as an introductory guide for WMI driver writers using this API. It is supposed
> +t be an successor to the original `LWN article <https://lwn.net/Articles/391230/>`_

t -> to

> +which deals with WMI drivers using the deprecated GUID-based WMI interface.

> +
> +Optaining WMI device information

Obtaining

> +--------------------------------
> +
> +Before developing an WMI driver, information about the WMI device in question
> +must be optained. The `lswmi <https://pypi.org/project/lswmi>`_ utility can be

obtained

> +used to display detailed WMI device information using the following command:
> +
> +::
> +
> +  lswmi -V
> +
> +The resulting output will contain information about all WMI devices inside a given
> +machine, plus some extra information.
> +
> +In order to find out more about the interface used to communicate with a WMI device,
> +the `bmfdec <https://github.com/pali/bmfdec>`_ utilities can be used to decode
> +the Binary MOF information used to describe WMI devices. The ``wmi-bmof`` driver

(Managed Object Format)

> +exposes this information to userspace, see Documentation/ABI/stable/sysfs-platform-wmi-bmof.

This should use a true link to the file.

> +In order to retrieve the decoded Binary MOF information, use the following command (requires root):
> +
> +::
> +
> +  ./bmf2mof /sys/bus/wmi/devices/05901221-D566-11D1-B2F0-00A0C9062910[-X]/bmof
> +
> +Sometimes, looking at the disassembled ACPI tables used to describe the WMI device
> +helps in understanding how the WMI device is supposed to work. To find out which
> +ACPI method handles which WMI device, the `fwts <https://github.com/fwts/fwts>`_
> +program can be used with the following command (requires root):
> +
> +::
> +
> +  fwts wmi -
> +
> +Basic WMI driver structure
> +--------------------------
> +
> +The basic WMI driver is build around the struct wmi_driver, which is then bound
> +to matching WMI devices using an struct wmi_device_id table. Please note that each

an struct -> a struct

> +WMI driver should be able to be instantiated multiple times.
> +
> +::
> +
> +  static const struct wmi_device_id foo_id_table[] = {
> +         { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
> +         { }
> +  };
> +  MODULE_DEVICE_TABLE(wmi, foo_id_table);
> +
> +  static struct wmi_driver foo_driver = {
> +        .driver = {
> +                .name = "foo",
> +                .probe_type = PROBE_PREFER_ASYNCHRONOUS,        /* optional */
> +                .pm = pm_sleep_ptr(&foo_dev_pm_ops),            /* optional */
> +        },
> +        .id_table = foo_id_table,
> +        .probe = foo_probe,
> +        .remove = foo_remove,         /* optional, devres is preferred */
> +        .notify = foo_notify,         /* optional, for event handling */
> +  };
> +  module_wmi_driver(foo_driver);
> +
> +If your WMI driver is not using any deprecated GUID-based WMI functions and is
> +able to be instantiated multiple times, please add its GUID to ``allow_duplicates``
> +inside drivers/platform/x86/wmi.c, so that the WMI subsystem does not block duplicate
> +GUIDs for it.

Just voicing wouldn't it be more useful to not burden new stuff with this 
at all and construct the opposite list instead with the GUIDs that have 
a driver that don't support duplicates? It's the existing set of GUIDs we 
have in-tree minus those currently on the list, correct?

> +WMI method drivers
> +------------------
> +
> +WMI drivers can call WMI device methods using wmidev_evaluate_method(), the
> +structure of the ACPI buffer passed to this function is device-specific and usually
> +needs some tinkering to get right. Looking at the ACPI tables containing the WMI
> +device usually helps here. The method id and instance number passed to this function
> +are also device-specific, looking at the decoded Binary MOF is usually enough to
> +find the right values.
> +The maximum instance number can be retrieved during runtime using wmidev_instance_count().
> +
> +Take a look at drivers/platform/x86/inspur_platform_profile.c for an example WMI method driver.
> +
> +WMI data block drivers
> +----------------------
> +
> +WMI drivers can query WMI device data blocks using wmidev_block_query(), the
> +structure of the returned ACPI object is again device-specific. Some WMI devices
> +also allow for setting data blocks using wmidev_block_set().
> +The maximum instance number can also be retrieved using wmidev_instance_count().
> +
> +Take a look at drivers/platform/x86/intel/wmi/sbl-fw-update.c for an example
> +WMI data block driver.
> +
> +WMI event drivers
> +-----------------
> +
> +WMI drivers can receive WMI event notifications by providing the notify() callback
> +inside the struct wmi_driver. The WMI subsystem will then take care of setting
> +up the WMI event accordingly. Plase note that the ACPI object passed to this callback

Plase -> Please

> +is optional and its structure device-specific. It also does not need to be freed,

structure is device-specific.

> +the WMI subsystem takes care of that.

I'd state the freeing part more strongly:

Releasing the ACPI object is handled by the WMI subsystem, not the driver.

> +
> +Take a look at drivers/platform/x86/xiaomi-wmi.c for an example WMI event driver.
> +
> +Things to avoid
> +---------------
> +
> +When developing WMI drivers, there are a couple of things which should be avoid
> +if feasible:
> +
> +- usage of the deprecated GUID-based WMI interface

It would be nice to be more specific because it's far from obvious at this 
point how to differentiate. So perhaps adding something like this would 
help:

(avoid functions with wmi_ prefix that input GUID converting it into 
a wmi_device using wmi_find_device_by_guid()).

> +- bypassing of the WMI subsystem when talking to WMI devices
> +- WMI drivers which cannot be instantiated multiple times.
> +
> +Many older WMI drivers violate one or more points from this list. The reason for
> +this is that the WMI subsystem evolved significantly over the last two decades,
> +so there is a lot of legacy cruft inside older WMI drivers.


-- 
 i.

Re: [PATCH 5/5] platform/x86: wmi: Add driver development guide

[Armin Wolf]

Am 08.12.23 um 13:25 schrieb Ilpo Järvinen:

> On Thu, 7 Dec 2023, Armin Wolf wrote:
>
>> Since 2010, an LWN article covering WMI drivers exists:
>>
>> 	https://lwn.net/Articles/391230/
>>
>> Since the introduction of the modern bus-based interface
>> and other userspace tooling (fwts wmi, bmfdec, ...), this
>> article is outdated and causes people to still submit new
>> WMI drivers using the deprecated GUID-based interface.
>> Fix this by adding a short guid on how to develop WMI drivers
> Too used to typing guid(?), here you want "guide" instead. :-D (I know
> that feeling when my fingers type something else than I think).
>
>> using the modern bus-based interface.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   .../wmi/driver-development-guide.rst          | 126 ++++++++++++++++++
>>   Documentation/wmi/index.rst                   |   1 +
>>   2 files changed, 127 insertions(+)
>>   create mode 100644 Documentation/wmi/driver-development-guide.rst
>>
>> diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
>> new file mode 100644
>> index 000000000000..a831e2728d25
>> --- /dev/null
>> +++ b/Documentation/wmi/driver-development-guide.rst
>> @@ -0,0 +1,126 @@
>> +.. SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +============================
>> +WMI driver development guide
>> +============================
>> +
>> +The WMI subsystem provides a rich driver api for implementing WMI drivers,
> API
>
>> +documented at Documentation/driver-api/wmi.rst. This document will serve
>> +as an introductory guide for WMI driver writers using this API. It is supposed
>> +t be an successor to the original `LWN article <https://lwn.net/Articles/391230/>`_
> t -> to
>
>> +which deals with WMI drivers using the deprecated GUID-based WMI interface.
>> +
>> +Optaining WMI device information
> Obtaining
>
>> +--------------------------------
>> +
>> +Before developing an WMI driver, information about the WMI device in question
>> +must be optained. The `lswmi <https://pypi.org/project/lswmi>`_ utility can be
> obtained
>
>> +used to display detailed WMI device information using the following command:
>> +
>> +::
>> +
>> +  lswmi -V
>> +
>> +The resulting output will contain information about all WMI devices inside a given
>> +machine, plus some extra information.
>> +
>> +In order to find out more about the interface used to communicate with a WMI device,
>> +the `bmfdec <https://github.com/pali/bmfdec>`_ utilities can be used to decode
>> +the Binary MOF information used to describe WMI devices. The ``wmi-bmof`` driver
> (Managed Object Format)
>
>> +exposes this information to userspace, see Documentation/ABI/stable/sysfs-platform-wmi-bmof.
> This should use a true link to the file.
>
>> +In order to retrieve the decoded Binary MOF information, use the following command (requires root):
>> +
>> +::
>> +
>> +  ./bmf2mof /sys/bus/wmi/devices/05901221-D566-11D1-B2F0-00A0C9062910[-X]/bmof
>> +
>> +Sometimes, looking at the disassembled ACPI tables used to describe the WMI device
>> +helps in understanding how the WMI device is supposed to work. To find out which
>> +ACPI method handles which WMI device, the `fwts <https://github.com/fwts/fwts>`_
>> +program can be used with the following command (requires root):
>> +
>> +::
>> +
>> +  fwts wmi -
>> +
>> +Basic WMI driver structure
>> +--------------------------
>> +
>> +The basic WMI driver is build around the struct wmi_driver, which is then bound
>> +to matching WMI devices using an struct wmi_device_id table. Please note that each
> an struct -> a struct
>
>> +WMI driver should be able to be instantiated multiple times.
>> +
>> +::
>> +
>> +  static const struct wmi_device_id foo_id_table[] = {
>> +         { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
>> +         { }
>> +  };
>> +  MODULE_DEVICE_TABLE(wmi, foo_id_table);
>> +
>> +  static struct wmi_driver foo_driver = {
>> +        .driver = {
>> +                .name = "foo",
>> +                .probe_type = PROBE_PREFER_ASYNCHRONOUS,        /* optional */
>> +                .pm = pm_sleep_ptr(&foo_dev_pm_ops),            /* optional */
>> +        },
>> +        .id_table = foo_id_table,
>> +        .probe = foo_probe,
>> +        .remove = foo_remove,         /* optional, devres is preferred */
>> +        .notify = foo_notify,         /* optional, for event handling */
>> +  };
>> +  module_wmi_driver(foo_driver);
>> +
>> +If your WMI driver is not using any deprecated GUID-based WMI functions and is
>> +able to be instantiated multiple times, please add its GUID to ``allow_duplicates``
>> +inside drivers/platform/x86/wmi.c, so that the WMI subsystem does not block duplicate
>> +GUIDs for it.
> Just voicing wouldn't it be more useful to not burden new stuff with this
> at all and construct the opposite list instead with the GUIDs that have
> a driver that don't support duplicates? It's the existing set of GUIDs we
> have in-tree minus those currently on the list, correct?

You are right about this, i am already thinking about a different approach which
does not rely on such a whitelist.

Basically, the legacy GUID-based functions only act on WMI devices which have an
ID of zero (which means they where found first), so that legacy drivers do not see
WMI devices with a duplicate GUID.
At the same time, WMI drivers would have to set a flag inside their struct wmi_driver
to indicate that they can be safely instantiated multiple times, otherwise they would
only be allowed to bind to WMI devices with an ID of zero (which are unique).

This would replace the whitelist with a flag inside wmi_driver, which can be enabled
by the driver developer without having to touch the WMI driver core at all.

I think i will split out this patch from the next revision of the series, since getting
rid of the whitelist should be a separate series.

Thanks,
Armin Wolf

>> +WMI method drivers
>> +------------------
>> +
>> +WMI drivers can call WMI device methods using wmidev_evaluate_method(), the
>> +structure of the ACPI buffer passed to this function is device-specific and usually
>> +needs some tinkering to get right. Looking at the ACPI tables containing the WMI
>> +device usually helps here. The method id and instance number passed to this function
>> +are also device-specific, looking at the decoded Binary MOF is usually enough to
>> +find the right values.
>> +The maximum instance number can be retrieved during runtime using wmidev_instance_count().
>> +
>> +Take a look at drivers/platform/x86/inspur_platform_profile.c for an example WMI method driver.
>> +
>> +WMI data block drivers
>> +----------------------
>> +
>> +WMI drivers can query WMI device data blocks using wmidev_block_query(), the
>> +structure of the returned ACPI object is again device-specific. Some WMI devices
>> +also allow for setting data blocks using wmidev_block_set().
>> +The maximum instance number can also be retrieved using wmidev_instance_count().
>> +
>> +Take a look at drivers/platform/x86/intel/wmi/sbl-fw-update.c for an example
>> +WMI data block driver.
>> +
>> +WMI event drivers
>> +-----------------
>> +
>> +WMI drivers can receive WMI event notifications by providing the notify() callback
>> +inside the struct wmi_driver. The WMI subsystem will then take care of setting
>> +up the WMI event accordingly. Plase note that the ACPI object passed to this callback
> Plase -> Please
>
>> +is optional and its structure device-specific. It also does not need to be freed,
> structure is device-specific.
>
>> +the WMI subsystem takes care of that.
> I'd state the freeing part more strongly:
>
> Releasing the ACPI object is handled by the WMI subsystem, not the driver.
>
>> +
>> +Take a look at drivers/platform/x86/xiaomi-wmi.c for an example WMI event driver.
>> +
>> +Things to avoid
>> +---------------
>> +
>> +When developing WMI drivers, there are a couple of things which should be avoid
>> +if feasible:
>> +
>> +- usage of the deprecated GUID-based WMI interface
> It would be nice to be more specific because it's far from obvious at this
> point how to differentiate. So perhaps adding something like this would
> help:
>
> (avoid functions with wmi_ prefix that input GUID converting it into
> a wmi_device using wmi_find_device_by_guid()).
>
>> +- bypassing of the WMI subsystem when talking to WMI devices
>> +- WMI drivers which cannot be instantiated multiple times.
>> +
>> +Many older WMI drivers violate one or more points from this list. The reason for
>> +this is that the WMI subsystem evolved significantly over the last two decades,
>> +so there is a lot of legacy cruft inside older WMI drivers.
>