[PATCH 1/2] platform/x86: wmi: Update WMI driver API documentation

[PATCH 1/2] platform/x86: wmi: Update WMI driver API documentation

[Armin Wolf]

The WMI driver core now passes the WMI event data to legacy notify
handlers, so WMI devices sharing notification IDs are now being
handled properly.

Fixes: e04e2b760ddb ("platform/x86: wmi: Pass event data directly to legacy notify handlers")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/driver-api/wmi.rst | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/wmi.rst b/Documentation/driver-api/wmi.rst
index 6ca58c8249e5..4e8dbdb1fc67 100644
--- a/Documentation/driver-api/wmi.rst
+++ b/Documentation/driver-api/wmi.rst
@@ -7,12 +7,11 @@ WMI Driver API
 The WMI driver core supports a more modern bus-based interface for interacting
 with WMI devices, and an older GUID-based interface. The latter interface is
 considered to be deprecated, so new WMI drivers should generally avoid it since
-it has some issues with multiple WMI devices and events sharing the same GUIDs
-and/or notification IDs. The modern bus-based interface instead maps each
-WMI device to a :c:type:`struct wmi_device <wmi_device>`, so it supports
-WMI devices sharing GUIDs and/or notification IDs. Drivers can then register
-a :c:type:`struct wmi_driver <wmi_driver>`, which will be bound to compatible
-WMI devices by the driver core.
+it has some issues with multiple WMI devices sharing the same GUID.
+The modern bus-based interface instead maps each WMI device to a
+:c:type:`struct wmi_device <wmi_device>`, so it supports WMI devices sharing the
+same GUID. Drivers can then register a :c:type:`struct wmi_driver <wmi_driver>`
+which will be bound to compatible WMI devices by the driver core.

 .. kernel-doc:: include/linux/wmi.h
    :internal:
--
2.39.5

[PATCH 2/2] platform/x86: wmi: Implement proper shutdown handling

[Armin Wolf]

When performing a system shutdown under Windows, all WMI clients are
terminated. This means that the ACPI BIOS might expect all WMI devices
to be disabled when shutting down.

Emulate this behaviour by disabling all active WMI devices during
shutdown. Also introduce a new WMI driver callback to allow WMI drivers
to perform any device-specific actions before disabling the WMI device.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 .../wmi/driver-development-guide.rst          |  7 ++++-
 drivers/platform/x86/wmi.c                    | 27 +++++++++++++++++++
 include/linux/wmi.h                           |  2 ++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
index 429137b2f632..676873c98680 100644
--- a/Documentation/wmi/driver-development-guide.rst
+++ b/Documentation/wmi/driver-development-guide.rst
@@ -64,6 +64,7 @@ to matching WMI devices using a struct wmi_device_id table:
         .id_table = foo_id_table,
         .probe = foo_probe,
         .remove = foo_remove,         /* optional, devres is preferred */
+        .shutdown = foo_shutdown,     /* optional, called during shutdown */
         .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 */
@@ -79,6 +80,10 @@ to unregister interfaces to other kernel subsystems and release resources, devre
 This simplifies error handling during probe and often allows to omit this callback entirely, see
 Documentation/driver-api/driver-model/devres.rst for details.

+The shutdown() callback is called during shutdown, reboot or kexec. Its sole purpose is to disable
+the WMI device and put it in a well-known state for the WMI driver to pick up later after reboot
+or kexec. Most WMI drivers need no special shutdown handling and can thus omit this callback.
+
 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
@@ -123,7 +128,7 @@ 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.
+right before and after calling its remove() or shutdown() 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.
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 3cbe180c3fc0..a01223c23d10 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -884,6 +884,32 @@ static void wmi_dev_remove(struct device *dev)
 		dev_warn(dev, "failed to disable device\n");
 }

+static void wmi_dev_shutdown(struct device *dev)
+{
+	struct wmi_driver *wdriver;
+	struct wmi_block *wblock;
+
+	if (dev->driver) {
+		wdriver = drv_to_wdrv(dev->driver);
+		wblock = dev_to_wblock(dev);
+
+		/*
+		 * Some machines return bogus WMI event data when disabling
+		 * the WMI event. Because of this we must prevent the associated
+		 * WMI driver from receiving new WMI events before disabling it.
+		 */
+		down_write(&wblock->notify_lock);
+		wblock->driver_ready = false;
+		up_write(&wblock->notify_lock);
+
+		if (wdriver->shutdown)
+			wdriver->shutdown(to_wmi_device(dev));
+
+		if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+			dev_warn(dev, "Failed to disable device\n");
+	}
+}
+
 static struct class wmi_bus_class = {
 	.name = "wmi_bus",
 };
@@ -895,6 +921,7 @@ static const struct bus_type wmi_bus_type = {
 	.uevent = wmi_dev_uevent,
 	.probe = wmi_dev_probe,
 	.remove = wmi_dev_remove,
+	.shutdown = wmi_dev_shutdown,
 };

 static const struct device_type wmi_type_event = {
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 3275470b5531..120019677fc6 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -56,6 +56,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
  * @no_singleton: Driver can be instantiated multiple times
  * @probe: Callback for device binding
  * @remove: Callback for device unbinding
+ * @shutdown: Callback for device shutdown
  * @notify: Callback for receiving WMI events
  *
  * This represents WMI drivers which handle WMI devices.
@@ -68,6 +69,7 @@ struct wmi_driver {

 	int (*probe)(struct wmi_device *wdev, const void *context);
 	void (*remove)(struct wmi_device *wdev);
+	void (*shutdown)(struct wmi_device *wdev);
 	void (*notify)(struct wmi_device *device, union acpi_object *data);
 };

--
2.39.5

Re: [PATCH 1/2] platform/x86: wmi: Update WMI driver API documentation

[Hans de Goede]

Hi Armin,

On 5-Oct-24 11:38 PM, Armin Wolf wrote:
> The WMI driver core now passes the WMI event data to legacy notify
> handlers, so WMI devices sharing notification IDs are now being
> handled properly.
> 
> Fixes: e04e2b760ddb ("platform/x86: wmi: Pass event data directly to legacy notify handlers")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

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

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Since patch 2/2 makes non pure bugfix functional changes
I expect Ilpo to pick that one up for pdx86/for-next.

Regards,

Hans




> ---
>  Documentation/driver-api/wmi.rst | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/driver-api/wmi.rst b/Documentation/driver-api/wmi.rst
> index 6ca58c8249e5..4e8dbdb1fc67 100644
> --- a/Documentation/driver-api/wmi.rst
> +++ b/Documentation/driver-api/wmi.rst
> @@ -7,12 +7,11 @@ WMI Driver API
>  The WMI driver core supports a more modern bus-based interface for interacting
>  with WMI devices, and an older GUID-based interface. The latter interface is
>  considered to be deprecated, so new WMI drivers should generally avoid it since
> -it has some issues with multiple WMI devices and events sharing the same GUIDs
> -and/or notification IDs. The modern bus-based interface instead maps each
> -WMI device to a :c:type:`struct wmi_device <wmi_device>`, so it supports
> -WMI devices sharing GUIDs and/or notification IDs. Drivers can then register
> -a :c:type:`struct wmi_driver <wmi_driver>`, which will be bound to compatible
> -WMI devices by the driver core.
> +it has some issues with multiple WMI devices sharing the same GUID.
> +The modern bus-based interface instead maps each WMI device to a
> +:c:type:`struct wmi_device <wmi_device>`, so it supports WMI devices sharing the
> +same GUID. Drivers can then register a :c:type:`struct wmi_driver <wmi_driver>`
> +which will be bound to compatible WMI devices by the driver core.
> 
>  .. kernel-doc:: include/linux/wmi.h
>     :internal:
> --
> 2.39.5
> 

Re: [PATCH 1/2] platform/x86: wmi: Update WMI driver API documentation

[Ilpo Järvinen]

On Sun, 6 Oct 2024, Hans de Goede wrote:

> Hi Armin,
> 
> On 5-Oct-24 11:38 PM, Armin Wolf wrote:
> > The WMI driver core now passes the WMI event data to legacy notify
> > handlers, so WMI devices sharing notification IDs are now being
> > handled properly.
> > 
> > Fixes: e04e2b760ddb ("platform/x86: wmi: Pass event data directly to legacy notify handlers")
> > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> 
> Thank you for your patch/series, I've applied this patch
> (series) to my review-hans branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> I will include this patch in my next fixes pull-req to Linus
> for the current kernel development cycle.
> 
> Since patch 2/2 makes non pure bugfix functional changes
> I expect Ilpo to pick that one up for pdx86/for-next.

Patch 2/2 is now in review-ilpo branch.

-- 
 i.