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

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

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 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 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

[PATCH 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

[PATCH 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>
---
 .../wmi/driver-development-guide.rst          | 173 ++++++++++++++++++
 Documentation/wmi/index.rst                   |   1 +
 2 files changed, 174 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..9a1b0a8490bb
--- /dev/null
+++ b/Documentation/wmi/driver-development-guide.rst
@@ -0,0 +1,173 @@
+.. 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 an successor to the original `LWN article <https://lwn.net/Articles/391230/>`_
+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 display 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 by providing 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 containg 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>
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 4/4] platform/x86: wmi: Add driver development guide

[Kuppuswamy Sathyanarayanan]

On 3/27/24 6:23 PM, 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>
> ---
>  .../wmi/driver-development-guide.rst          | 173 ++++++++++++++++++
>  Documentation/wmi/index.rst                   |   1 +
>  2 files changed, 174 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..9a1b0a8490bb
> --- /dev/null
> +++ b/Documentation/wmi/driver-development-guide.rst
> @@ -0,0 +1,173 @@
> +.. 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 an successor to the original `LWN article <https://lwn.net/Articles/391230/>`_

/s/an successor/a successor

IMO, you don't have to refer to lwn article here. You can add it at the bottom
under a references section.

References
==========

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



> +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 display detailed WMI device information using the following command:

Instead of display, I think "extract" or "get the " usage will be better.

> +
> +::
> +
> +  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 by providing the notify() callback inside the struct wmi_driver.

IMO /s/providing/via the is better.

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

I think locking is needed always for notify handler right?

> +
> +In order to be able to receive WMI events containg no additional event data,

/s/containg/containing

> +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>
> 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 1/4] platform/x86: wmi: Mark simple WMI drivers as legacy-free

[Kuppuswamy Sathyanarayanan]

On 3/27/24 6:23 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.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
LGTM

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>  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
>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Kuppuswamy Sathyanarayanan]

On 3/27/24 6:23 PM, 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.

Since locking is needed for all notify related calls in all WMI drivers,
is there a generic way to add this support in WMI core driver? Like
defining some common function which will hold the lock and call
driver specific notify handler? I am just thinking aloud.. If it is not
feasible, then it is fine.

> 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
>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Armin Wolf]

Am 28.03.24 um 03:58 schrieb Kuppuswamy Sathyanarayanan:

> On 3/27/24 6:23 PM, 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.
> Since locking is needed for all notify related calls in all WMI drivers,
> is there a generic way to add this support in WMI core driver? Like
> defining some common function which will hold the lock and call
> driver specific notify handler? I am just thinking aloud.. If it is not
> feasible, then it is fine.

Hi,

actually, not all notify-related calls need locking. It just so happens that
input drivers must protect their key sequence, other WMI drivers might not need
to use any locking at all.

I would prefer if the WMI driver does the locking, so it can be optimized for
each WMI driver.

Thanks,
Armin Wolf

>> 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 2/4] platform/x86: xiaomi-wmi: Fix race condition when reporting key events

[Kuppuswamy Sathyanarayanan]

On 3/28/24 5:26 PM, Armin Wolf wrote:
> Am 28.03.24 um 03:58 schrieb Kuppuswamy Sathyanarayanan:
>
>> On 3/27/24 6:23 PM, 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.
>> Since locking is needed for all notify related calls in all WMI drivers,
>> is there a generic way to add this support in WMI core driver? Like
>> defining some common function which will hold the lock and call
>> driver specific notify handler? I am just thinking aloud.. If it is not
>> feasible, then it is fine.
>
> Hi,
>
> actually, not all notify-related calls need locking. It just so happens that
> input drivers must protect their key sequence, other WMI drivers might not need
> to use any locking at all.

Got it.

>
> I would prefer if the WMI driver does the locking, so it can be optimized for
> each WMI driver.

Why not implement this support?

>
> Thanks,
> Armin Wolf
>
>>> 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
>>>
>>>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Armin Wolf]

Am 29.03.24 um 02:37 schrieb Kuppuswamy Sathyanarayanan:

> On 3/28/24 5:26 PM, Armin Wolf wrote:
>> Am 28.03.24 um 03:58 schrieb Kuppuswamy Sathyanarayanan:
>>
>>> On 3/27/24 6:23 PM, 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.
>>> Since locking is needed for all notify related calls in all WMI drivers,
>>> is there a generic way to add this support in WMI core driver? Like
>>> defining some common function which will hold the lock and call
>>> driver specific notify handler? I am just thinking aloud.. If it is not
>>> feasible, then it is fine.
>> Hi,
>>
>> actually, not all notify-related calls need locking. It just so happens that
>> input drivers must protect their key sequence, other WMI drivers might not need
>> to use any locking at all.
> Got it.
>
>> I would prefer if the WMI driver does the locking, so it can be optimized for
>> each WMI driver.
> Why not implement this support?

Because different WMI drivers will most certainly have different need when it comes to locking.
Some might want to use a simple mutex, some might want to use a RW-lock, and others might need
something totally different.

Implementing all of this inside the WMI driver core will be difficult.

Thanks,
Armin Wolf

>> Thanks,
>> Armin Wolf
>>
>>>> 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
>>>>
>>>>