[PATCH v2 1/4] platform/x86: wmi: Mark simple WMI drivers as legacy-free

[PATCH v2 1/4] platform/x86: wmi: Mark simple WMI drivers as legacy-free

[Armin Wolf]

The inspur_platform_profile driver and the xiaomi-wmi driver both
meet the requirements for modern WMI drivers, as they both do not
use the legacy GUID-based interface and can be safely instantiated
multiple times.

Mark them both as legacy-free using the no_singleton flag.

Compile-tested only.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Changes since v1:
- add Reviewed-by tag
---
 drivers/platform/x86/inspur_platform_profile.c | 1 +
 drivers/platform/x86/xiaomi-wmi.c              | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
index 743705bddda3..8440defa6788 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -207,6 +207,7 @@ static struct wmi_driver inspur_wmi_driver = {
 	.id_table = inspur_wmi_id_table,
 	.probe = inspur_wmi_probe,
 	.remove = inspur_wmi_remove,
+	.no_singleton = true,
 };

 module_wmi_driver(inspur_wmi_driver);
diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c
index 54a2546bb93b..1f5f108d87c0 100644
--- a/drivers/platform/x86/xiaomi-wmi.c
+++ b/drivers/platform/x86/xiaomi-wmi.c
@@ -83,6 +83,7 @@ static struct wmi_driver xiaomi_wmi_driver = {
 	.id_table = xiaomi_wmi_id_table,
 	.probe = xiaomi_wmi_probe,
 	.notify = xiaomi_wmi_notify,
+	.no_singleton = true,
 };
 module_wmi_driver(xiaomi_wmi_driver);

--
2.39.2

[PATCH v2 2/4] platform/x86: xiaomi-wmi: Fix race condition when reporting key events

[Armin Wolf]

Multiple WMI events can be received concurrently, so multiple instances
of xiaomi_wmi_notify() can be active at the same time. Since the input
device is shared between those handlers, the key input sequence can be
disturbed.

Fix this by protecting the key input sequence with a mutex.

Compile-tested only.

Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/xiaomi-wmi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c
index 1f5f108d87c0..7efbdc111803 100644
--- a/drivers/platform/x86/xiaomi-wmi.c
+++ b/drivers/platform/x86/xiaomi-wmi.c
@@ -2,8 +2,10 @@
 /* WMI driver for Xiaomi Laptops */

 #include <linux/acpi.h>
+#include <linux/device.h>
 #include <linux/input.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/wmi.h>

 #include <uapi/linux/input-event-codes.h>
@@ -20,12 +22,21 @@

 struct xiaomi_wmi {
 	struct input_dev *input_dev;
+	struct mutex key_lock;	/* Protects the key event sequence */
 	unsigned int key_code;
 };

+static void xiaomi_mutex_destroy(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_destroy(lock);
+}
+
 static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)
 {
 	struct xiaomi_wmi *data;
+	int ret;

 	if (wdev == NULL || context == NULL)
 		return -EINVAL;
@@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)
 		return -ENOMEM;
 	dev_set_drvdata(&wdev->dev, data);

+	mutex_init(&data->key_lock);
+	ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock);
+	if (ret < 0)
+		return ret;
+
 	data->input_dev = devm_input_allocate_device(&wdev->dev);
 	if (data->input_dev == NULL)
 		return -ENOMEM;
@@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy)
 	if (data == NULL)
 		return;

+	mutex_lock(&data->key_lock);
 	input_report_key(data->input_dev, data->key_code, 1);
 	input_sync(data->input_dev);
 	input_report_key(data->input_dev, data->key_code, 0);
 	input_sync(data->input_dev);
+	mutex_unlock(&data->key_lock);
 }

 static const struct wmi_device_id xiaomi_wmi_id_table[] = {
--
2.39.2

Re: [PATCH v2 2/4] platform/x86: xiaomi-wmi: Fix race condition when reporting key events

[Kuppuswamy Sathyanarayanan]

On 4/2/24 7:30 AM, Armin Wolf wrote:
> Multiple WMI events can be received concurrently, so multiple instances
> of xiaomi_wmi_notify() can be active at the same time. Since the input
> device is shared between those handlers, the key input sequence can be
> disturbed.
>
> Fix this by protecting the key input sequence with a mutex.
>
> Compile-tested only.
>
> Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---

LGTM

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/platform/x86/xiaomi-wmi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c
> index 1f5f108d87c0..7efbdc111803 100644
> --- a/drivers/platform/x86/xiaomi-wmi.c
> +++ b/drivers/platform/x86/xiaomi-wmi.c
> @@ -2,8 +2,10 @@
>  /* WMI driver for Xiaomi Laptops */
>
>  #include <linux/acpi.h>
> +#include <linux/device.h>
>  #include <linux/input.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/wmi.h>
>
>  #include <uapi/linux/input-event-codes.h>
> @@ -20,12 +22,21 @@
>
>  struct xiaomi_wmi {
>  	struct input_dev *input_dev;
> +	struct mutex key_lock;	/* Protects the key event sequence */
>  	unsigned int key_code;
>  };
>
> +static void xiaomi_mutex_destroy(void *data)
> +{
> +	struct mutex *lock = data;
> +
> +	mutex_destroy(lock);
> +}
> +
>  static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)
>  {
>  	struct xiaomi_wmi *data;
> +	int ret;
>
>  	if (wdev == NULL || context == NULL)
>  		return -EINVAL;
> @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)
>  		return -ENOMEM;
>  	dev_set_drvdata(&wdev->dev, data);
>
> +	mutex_init(&data->key_lock);
> +	ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock);
> +	if (ret < 0)
> +		return ret;
> +
>  	data->input_dev = devm_input_allocate_device(&wdev->dev);
>  	if (data->input_dev == NULL)
>  		return -ENOMEM;
> @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy)
>  	if (data == NULL)
>  		return;
>
> +	mutex_lock(&data->key_lock);
>  	input_report_key(data->input_dev, data->key_code, 1);
>  	input_sync(data->input_dev);
>  	input_report_key(data->input_dev, data->key_code, 0);
>  	input_sync(data->input_dev);
> +	mutex_unlock(&data->key_lock);
>  }
>
>  static const struct wmi_device_id xiaomi_wmi_id_table[] = {
> --
> 2.39.2
>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

[PATCH v2 3/4] platform/x86: xiaomi-wmi: Drop unnecessary NULL checks

[Armin Wolf]

The WMI driver core already makes sure that:

- a valid WMI device is passed to each callback
- the notify() callback runs after the probe() callback succeeds

Remove the unnecessary NULL checks.

Compile-tested only.

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

diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c
index 7efbdc111803..cbed29ca502a 100644
--- a/drivers/platform/x86/xiaomi-wmi.c
+++ b/drivers/platform/x86/xiaomi-wmi.c
@@ -38,7 +38,7 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)
 	struct xiaomi_wmi *data;
 	int ret;

-	if (wdev == NULL || context == NULL)
+	if (!context)
 		return -EINVAL;

 	data = devm_kzalloc(&wdev->dev, sizeof(struct xiaomi_wmi), GFP_KERNEL);
@@ -66,14 +66,7 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)

 static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy)
 {
-	struct xiaomi_wmi *data;
-
-	if (wdev == NULL)
-		return;
-
-	data = dev_get_drvdata(&wdev->dev);
-	if (data == NULL)
-		return;
+	struct xiaomi_wmi *data = dev_get_drvdata(&wdev->dev);

 	mutex_lock(&data->key_lock);
 	input_report_key(data->input_dev, data->key_code, 1);
--
2.39.2

Re: [PATCH v2 3/4] platform/x86: xiaomi-wmi: Drop unnecessary NULL checks

[Kuppuswamy Sathyanarayanan]

On 4/2/24 7:30 AM, Armin Wolf wrote:
> The WMI driver core already makes sure that:
>
> - a valid WMI device is passed to each callback
> - the notify() callback runs after the probe() callback succeeds
>
> Remove the unnecessary NULL checks.
>
> Compile-tested only.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---

LGTM

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/platform/x86/xiaomi-wmi.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c
> index 7efbdc111803..cbed29ca502a 100644
> --- a/drivers/platform/x86/xiaomi-wmi.c
> +++ b/drivers/platform/x86/xiaomi-wmi.c
> @@ -38,7 +38,7 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)
>  	struct xiaomi_wmi *data;
>  	int ret;
>
> -	if (wdev == NULL || context == NULL)
> +	if (!context)
>  		return -EINVAL;
>
>  	data = devm_kzalloc(&wdev->dev, sizeof(struct xiaomi_wmi), GFP_KERNEL);
> @@ -66,14 +66,7 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context)
>
>  static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy)
>  {
> -	struct xiaomi_wmi *data;
> -
> -	if (wdev == NULL)
> -		return;
> -
> -	data = dev_get_drvdata(&wdev->dev);
> -	if (data == NULL)
> -		return;
> +	struct xiaomi_wmi *data = dev_get_drvdata(&wdev->dev);
>
>  	mutex_lock(&data->key_lock);
>  	input_report_key(data->input_dev, data->key_code, 1);
> --
> 2.39.2
>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

[PATCH v2 4/4] 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 (bmfdec, lswmi, ...), 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 guide on how to develop WMI drivers
using the modern bus-based interface.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Changes since v1:
- use footnote for lwn article link
- wording fixes
---
 .../wmi/driver-development-guide.rst          | 178 ++++++++++++++++++
 Documentation/wmi/index.rst                   |   1 +
 2 files changed, 179 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..429137b2f632
--- /dev/null
+++ b/Documentation/wmi/driver-development-guide.rst
@@ -0,0 +1,178 @@
+.. 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
+to be a successor to the original LWN article [1]_ which deals with WMI drivers
+using the deprecated GUID-based WMI interface.
+
+Obtaining WMI device information
+--------------------------------
+
+Before developing an WMI driver, information about the WMI device in question
+must be obtained. The `lswmi <https://pypi.org/project/lswmi>`_ utility can be
+used to extract detailed WMI device information using the following command:
+
+::
+
+  lswmi -V
+
+The resulting output will contain information about all WMI devices available on
+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 (Managed Object Format) information used to describe WMI devices.
+The ``wmi-bmof`` driver exposes this information to userspace, see
+Documentation/wmi/devices/wmi-bmof.rst.
+
+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. The path of the ACPI
+method associated with a given WMI device can be retrieved using the ``lswmi`` utility
+as mentioned above.
+
+Basic WMI driver structure
+--------------------------
+
+The basic WMI driver is build around the struct wmi_driver, which is then bound
+to matching WMI devices using a struct wmi_device_id table:
+
+::
+
+  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,        /* recommended */
+                .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 */
+        .no_notify_data = true,       /* optional, enables events containing no additional data */
+        .no_singleton = true,         /* required for new WMI drivers */
+  };
+  module_wmi_driver(foo_driver);
+
+The probe() callback is called when the WMI driver is bound to a matching WMI device. Allocating
+driver-specific data structures and initialising interfaces to other kernel subsystems should
+normally be done in this function.
+
+The remove() callback is then called when the WMI driver is unbound from a WMI device. In order
+to unregister interfaces to other kernel subsystems and release resources, devres should be used.
+This simplifies error handling during probe and often allows to omit this callback entirely, see
+Documentation/driver-api/driver-model/devres.rst for details.
+
+Please note that new WMI drivers are required to be able to be instantiated multiple times,
+and are forbidden from using any deprecated GUID-based WMI functions. This means that the
+WMI driver should be prepared for the scenario that multiple matching WMI devices are present
+on a given machine.
+
+Because of this, WMI drivers should use the state container design pattern as described in
+Documentation/driver-api/driver-model/design-patterns.rst.
+
+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 events via the notify() callback inside the struct wmi_driver.
+The WMI subsystem will then take care of setting up the WMI event accordingly. Please note that
+the structure of the ACPI object passed to this callback is device-specific, and freeing the
+ACPI object is being done by the WMI subsystem, not the driver.
+
+The WMI driver core will take care that the notify() callback will only be called after
+the probe() callback has been called, and that no events are being received by the driver
+right before and after calling its remove() callback.
+
+However WMI driver developers should be aware that multiple WMI events can be received concurrently,
+so any locking (if necessary) needs to be provided by the WMI driver itself.
+
+In order to be able to receive WMI events containing no additional event data,
+the ``no_notify_data`` flag inside struct wmi_driver should be set to ``true``.
+
+Take a look at drivers/platform/x86/xiaomi-wmi.c for an example WMI event driver.
+
+Handling multiple WMI devices at once
+-------------------------------------
+
+There are many cases of firmware vendors using multiple WMI devices to control different aspects
+of a single physical device. This can make developing WMI drivers complicated, as those drivers
+might need to communicate with each other to present a unified interface to userspace.
+
+On such case involves a WMI event device which needs to talk to a WMI data block device or WMI
+method device upon receiving an WMI event. In such a case, two WMI drivers should be developed,
+one for the WMI event device and one for the other WMI device.
+
+The WMI event device driver has only one purpose: to receive WMI events, validate any additional
+event data and invoke a notifier chain. The other WMI driver adds itself to this notifier chain
+during probing and thus gets notified every time a WMI event is received. This WMI driver might
+then process the event further for example by using an input device.
+
+For other WMI device constellations, similar mechanisms can be used.
+
+Things to avoid
+---------------
+
+When developing WMI drivers, there are a couple of things which should be avoided:
+
+- usage of the deprecated GUID-based WMI interface which uses GUIDs instead of WMI device structs
+- 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.
+
+New WMI drivers are also required to conform to the linux kernel coding style as specified in
+Documentation/process/coding-style.rst. The checkpatch utility can catch many common coding style
+violations, you can invoke it with the following command:
+
+::
+
+  ./scripts/checkpatch.pl --strict <path to driver file>
+
+References
+==========
+
+.. [1] https://lwn.net/Articles/391230/
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 v2 4/4] platform/x86: wmi: Add driver development guide

[Kuppuswamy Sathyanarayanan]

On 4/2/24 7:30 AM, 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 (bmfdec, lswmi, ...), 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 guide on how to develop WMI drivers
> using the modern bus-based interface.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---

LGTM

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> Changes since v1:
> - use footnote for lwn article link
> - wording fixes
> ---
>  .../wmi/driver-development-guide.rst          | 178 ++++++++++++++++++
>  Documentation/wmi/index.rst                   |   1 +
>  2 files changed, 179 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..429137b2f632
> --- /dev/null
> +++ b/Documentation/wmi/driver-development-guide.rst
> @@ -0,0 +1,178 @@
> +.. 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
> +to be a successor to the original LWN article [1]_ which deals with WMI drivers
> +using the deprecated GUID-based WMI interface.
> +
> +Obtaining WMI device information
> +--------------------------------
> +
> +Before developing an WMI driver, information about the WMI device in question
> +must be obtained. The `lswmi <https://pypi.org/project/lswmi>`_ utility can be
> +used to extract detailed WMI device information using the following command:
> +
> +::
> +
> +  lswmi -V
> +
> +The resulting output will contain information about all WMI devices available on
> +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 (Managed Object Format) information used to describe WMI devices.
> +The ``wmi-bmof`` driver exposes this information to userspace, see
> +Documentation/wmi/devices/wmi-bmof.rst.
> +
> +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. The path of the ACPI
> +method associated with a given WMI device can be retrieved using the ``lswmi`` utility
> +as mentioned above.
> +
> +Basic WMI driver structure
> +--------------------------
> +
> +The basic WMI driver is build around the struct wmi_driver, which is then bound
> +to matching WMI devices using a struct wmi_device_id table:
> +
> +::
> +
> +  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,        /* recommended */
> +                .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 */
> +        .no_notify_data = true,       /* optional, enables events containing no additional data */
> +        .no_singleton = true,         /* required for new WMI drivers */
> +  };
> +  module_wmi_driver(foo_driver);
> +
> +The probe() callback is called when the WMI driver is bound to a matching WMI device. Allocating
> +driver-specific data structures and initialising interfaces to other kernel subsystems should
> +normally be done in this function.
> +
> +The remove() callback is then called when the WMI driver is unbound from a WMI device. In order
> +to unregister interfaces to other kernel subsystems and release resources, devres should be used.
> +This simplifies error handling during probe and often allows to omit this callback entirely, see
> +Documentation/driver-api/driver-model/devres.rst for details.
> +
> +Please note that new WMI drivers are required to be able to be instantiated multiple times,
> +and are forbidden from using any deprecated GUID-based WMI functions. This means that the
> +WMI driver should be prepared for the scenario that multiple matching WMI devices are present
> +on a given machine.
> +
> +Because of this, WMI drivers should use the state container design pattern as described in
> +Documentation/driver-api/driver-model/design-patterns.rst.
> +
> +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 events via the notify() callback inside the struct wmi_driver.
> +The WMI subsystem will then take care of setting up the WMI event accordingly. Please note that
> +the structure of the ACPI object passed to this callback is device-specific, and freeing the
> +ACPI object is being done by the WMI subsystem, not the driver.
> +
> +The WMI driver core will take care that the notify() callback will only be called after
> +the probe() callback has been called, and that no events are being received by the driver
> +right before and after calling its remove() callback.
> +
> +However WMI driver developers should be aware that multiple WMI events can be received concurrently,
> +so any locking (if necessary) needs to be provided by the WMI driver itself.
> +
> +In order to be able to receive WMI events containing no additional event data,
> +the ``no_notify_data`` flag inside struct wmi_driver should be set to ``true``.
> +
> +Take a look at drivers/platform/x86/xiaomi-wmi.c for an example WMI event driver.
> +
> +Handling multiple WMI devices at once
> +-------------------------------------
> +
> +There are many cases of firmware vendors using multiple WMI devices to control different aspects
> +of a single physical device. This can make developing WMI drivers complicated, as those drivers
> +might need to communicate with each other to present a unified interface to userspace.
> +
> +On such case involves a WMI event device which needs to talk to a WMI data block device or WMI
> +method device upon receiving an WMI event. In such a case, two WMI drivers should be developed,
> +one for the WMI event device and one for the other WMI device.
> +
> +The WMI event device driver has only one purpose: to receive WMI events, validate any additional
> +event data and invoke a notifier chain. The other WMI driver adds itself to this notifier chain
> +during probing and thus gets notified every time a WMI event is received. This WMI driver might
> +then process the event further for example by using an input device.
> +
> +For other WMI device constellations, similar mechanisms can be used.
> +
> +Things to avoid
> +---------------
> +
> +When developing WMI drivers, there are a couple of things which should be avoided:
> +
> +- usage of the deprecated GUID-based WMI interface which uses GUIDs instead of WMI device structs
> +- 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.
> +
> +New WMI drivers are also required to conform to the linux kernel coding style as specified in
> +Documentation/process/coding-style.rst. The checkpatch utility can catch many common coding style
> +violations, you can invoke it with the following command:
> +
> +::
> +
> +  ./scripts/checkpatch.pl --strict <path to driver file>
> +
> +References
> +==========
> +
> +.. [1] https://lwn.net/Articles/391230/
> 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
>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 1/4] platform/x86: wmi: Mark simple WMI drivers as legacy-free

[Hans de Goede]

Hi,

On 4/2/24 4:30 PM, Armin Wolf wrote:
> The inspur_platform_profile driver and the xiaomi-wmi driver both
> meet the requirements for modern WMI drivers, as they both do not
> use the legacy GUID-based interface and can be safely instantiated
> multiple times.
> 
> Mark them both as legacy-free using the no_singleton flag.
> 
> Compile-tested only.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
> Changes since v1:
> - add Reviewed-by tag
> ---
>  drivers/platform/x86/inspur_platform_profile.c | 1 +
>  drivers/platform/x86/xiaomi-wmi.c              | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
> index 743705bddda3..8440defa6788 100644
> --- a/drivers/platform/x86/inspur_platform_profile.c
> +++ b/drivers/platform/x86/inspur_platform_profile.c
> @@ -207,6 +207,7 @@ static struct wmi_driver inspur_wmi_driver = {
>  	.id_table = inspur_wmi_id_table,
>  	.probe = inspur_wmi_probe,
>  	.remove = inspur_wmi_remove,
> +	.no_singleton = true,
>  };
> 
>  module_wmi_driver(inspur_wmi_driver);
> diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c
> index 54a2546bb93b..1f5f108d87c0 100644
> --- a/drivers/platform/x86/xiaomi-wmi.c
> +++ b/drivers/platform/x86/xiaomi-wmi.c
> @@ -83,6 +83,7 @@ static struct wmi_driver xiaomi_wmi_driver = {
>  	.id_table = xiaomi_wmi_id_table,
>  	.probe = xiaomi_wmi_probe,
>  	.notify = xiaomi_wmi_notify,
> +	.no_singleton = true,
>  };
>  module_wmi_driver(xiaomi_wmi_driver);
> 
> --
> 2.39.2
>