public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] platform/x86: wmi: Event handling fixes
@ 2024-01-03 19:27 Armin Wolf
  2024-01-03 19:27 ` [PATCH 1/4] platform/x86: wmi: Fix error handling in legacy WMI notify handler functions Armin Wolf
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Armin Wolf @ 2024-01-03 19:27 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel

This patch series contains various fixes for the handling of
WMI events. The first patch fixes an issue where the WMI core
would return an error even when a legacy WMI notify handler
was installed successfully.

The second patch cleans up the last remains of
commit 58f6425eb92f ("WMI: Cater for multiple events with same GUID"),
which is useless now that legacy WMI handlers cannot see duplicated
GUIDS. This however might highlight a potential problem:

In the email thread concerning aforementioned commit, a (now legacy)
WMI driver was mentioned which depended on this functionality.
I believe this driver is the dell-wmi-aio driver, so this driver is
likely already broken for some time and should be converted to the
new bus-based interface.

The next patch decouples the legacy WMI notify handlers from the
wmi_block_list, while the last patch fixes a race condition when a
notify callback is called by the WMI core.

All patches have been tested on a Dell Inspiron 3505 and a
Acer Aspire E1-731.

Armin Wolf (4):
  platform/x86: wmi: Fix error handling in legacy WMI notify handler
    functions
  platform/x86: wmi: Return immediately if an suitable WMI event is
    found
  platform/x86: wmi: Decouple legacy WMI notify handlers from
    wmi_block_list
  platform/x86: wmi: Fix notify callback locking

 drivers/platform/x86/wmi.c | 179 +++++++++++++++++++++++--------------
 1 file changed, 111 insertions(+), 68 deletions(-)

--
2.39.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/4] platform/x86: wmi: Fix error handling in legacy WMI notify handler functions
  2024-01-03 19:27 [PATCH 0/4] platform/x86: wmi: Event handling fixes Armin Wolf
@ 2024-01-03 19:27 ` Armin Wolf
  2024-01-04 18:10   ` Ilpo Järvinen
  2024-01-03 19:27 ` [PATCH 2/4] platform/x86: wmi: Return immediately if an suitable WMI event is found Armin Wolf
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2024-01-03 19:27 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel

When wmi_install_notify_handler()/wmi_remove_notify_handler() are
unable to enable/disable the WMI device, they unconditionally return
an error to the caller.
When registering legacy WMI notify handlers, this means that the
callback remains registered despite wmi_install_notify_handler()
having returned an error.
When removing legacy WMI notify handlers, this means that the
callback is removed despite wmi_remove_notify_handler() having
returned an error.

Fix this by only warning when the WMI device could not be enabled.
This behaviour matches the bus-based WMI interface.

Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.

Fixes: 58f6425eb92f ("WMI: Cater for multiple events with same GUID")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index a7cfcbf92432..3899a5e3fca7 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -592,9 +592,10 @@ acpi_status wmi_install_notify_handler(const char *guid,
 			block->handler_data = data;

 			wmi_status = wmi_method_enable(block, true);
-			if ((wmi_status != AE_OK) ||
-			    ((wmi_status == AE_OK) && (status == AE_NOT_EXIST)))
-				status = wmi_status;
+			if (ACPI_FAILURE(wmi_status))
+				dev_warn(&block->dev.dev, "Failed to enable device\n");
+
+			status = AE_OK;
 		}
 	}

@@ -630,10 +631,13 @@ acpi_status wmi_remove_notify_handler(const char *guid)
 				return AE_NULL_ENTRY;

 			wmi_status = wmi_method_enable(block, false);
+			if (ACPI_FAILURE(wmi_status))
+				dev_warn(&block->dev.dev, "Failed to disable device\n");
+
 			block->handler = NULL;
 			block->handler_data = NULL;
-			if (wmi_status != AE_OK || (wmi_status == AE_OK && status == AE_NOT_EXIST))
-				status = wmi_status;
+
+			status = AE_OK;
 		}
 	}

--
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] platform/x86: wmi: Return immediately if an suitable WMI event is found
  2024-01-03 19:27 [PATCH 0/4] platform/x86: wmi: Event handling fixes Armin Wolf
  2024-01-03 19:27 ` [PATCH 1/4] platform/x86: wmi: Fix error handling in legacy WMI notify handler functions Armin Wolf
@ 2024-01-03 19:27 ` Armin Wolf
  2024-01-04 18:13   ` Ilpo Järvinen
  2024-01-03 19:27 ` [PATCH 3/4] platform/x86: wmi: Decouple legacy WMI notify handlers from wmi_block_list Armin Wolf
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2024-01-03 19:27 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel

Commit 58f6425eb92f ("WMI: Cater for multiple events with same GUID")
allowed legacy WMI notify handlers to be installed for multiple WMI
devices with the same GUID.
However this is useless since the legacy GUID-based interface is
blacklisted from seeing WMI devices with duplicated GUIDs.

Return immediately if a suitable WMI event is found in
wmi_install/remove_notify_handler() since searching for other suitable
events is pointless.

Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.

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

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 3899a5e3fca7..7470a149b254 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -572,7 +572,6 @@ acpi_status wmi_install_notify_handler(const char *guid,
 				       void *data)
 {
 	struct wmi_block *block;
-	acpi_status status = AE_NOT_EXIST;
 	guid_t guid_input;

 	if (!guid || !handler)
@@ -595,11 +594,11 @@ acpi_status wmi_install_notify_handler(const char *guid,
 			if (ACPI_FAILURE(wmi_status))
 				dev_warn(&block->dev.dev, "Failed to enable device\n");

-			status = AE_OK;
+			return AE_OK;
 		}
 	}

-	return status;
+	return AE_NOT_EXIST;
 }
 EXPORT_SYMBOL_GPL(wmi_install_notify_handler);

@@ -614,7 +613,6 @@ EXPORT_SYMBOL_GPL(wmi_install_notify_handler);
 acpi_status wmi_remove_notify_handler(const char *guid)
 {
 	struct wmi_block *block;
-	acpi_status status = AE_NOT_EXIST;
 	guid_t guid_input;

 	if (!guid)
@@ -637,11 +635,11 @@ acpi_status wmi_remove_notify_handler(const char *guid)
 			block->handler = NULL;
 			block->handler_data = NULL;

-			status = AE_OK;
+			return AE_OK;
 		}
 	}

-	return status;
+	return AE_NOT_EXIST;
 }
 EXPORT_SYMBOL_GPL(wmi_remove_notify_handler);

--
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] platform/x86: wmi: Decouple legacy WMI notify handlers from wmi_block_list
  2024-01-03 19:27 [PATCH 0/4] platform/x86: wmi: Event handling fixes Armin Wolf
  2024-01-03 19:27 ` [PATCH 1/4] platform/x86: wmi: Fix error handling in legacy WMI notify handler functions Armin Wolf
  2024-01-03 19:27 ` [PATCH 2/4] platform/x86: wmi: Return immediately if an suitable WMI event is found Armin Wolf
@ 2024-01-03 19:27 ` Armin Wolf
  2024-01-04 18:14   ` Ilpo Järvinen
  2024-01-03 19:27 ` [PATCH 4/4] platform/x86: wmi: Fix notify callback locking Armin Wolf
  2024-01-08 14:00 ` [PATCH 0/4] platform/x86: wmi: Event handling fixes Hans de Goede
  4 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2024-01-03 19:27 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel

Until now, legacy WMI notify handler functions where using the
wmi_block_list, which did no refcounting on the returned WMI device.
This meant that the WMI device could disappear at any moment,
potentially leading to various errors.
Fix this by using bus_find_device() which returns an actual
reference to the found WMI device.

Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.

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

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7470a149b254..6a886635689a 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -218,6 +218,17 @@ static int wmidev_match_guid(struct device *dev, const void *data)
 	return 0;
 }

+static int wmidev_match_notify_id(struct device *dev, const void *data)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+	const u32 *notify_id = data;
+
+	if (wblock->gblock.flags & ACPI_WMI_EVENT && wblock->gblock.notify_id == *notify_id)
+		return 1;
+
+	return 0;
+}
+
 static struct bus_type wmi_bus_type;

 static struct wmi_device *wmi_find_device_by_guid(const char *guid_string)
@@ -237,6 +248,17 @@ static struct wmi_device *wmi_find_device_by_guid(const char *guid_string)
 	return dev_to_wdev(dev);
 }

+static struct wmi_device *wmi_find_event_by_notify_id(const u32 notify_id)
+{
+	struct device *dev;
+
+	dev = bus_find_device(&wmi_bus_type, NULL, &notify_id, wmidev_match_notify_id);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	return to_wmi_device(dev);
+}
+
 static void wmi_device_put(struct wmi_device *wdev)
 {
 	put_device(&wdev->dev);
@@ -571,34 +593,30 @@ acpi_status wmi_install_notify_handler(const char *guid,
 				       wmi_notify_handler handler,
 				       void *data)
 {
-	struct wmi_block *block;
-	guid_t guid_input;
-
-	if (!guid || !handler)
-		return AE_BAD_PARAMETER;
-
-	if (guid_parse(guid, &guid_input))
-		return AE_BAD_PARAMETER;
-
-	list_for_each_entry(block, &wmi_block_list, list) {
-		acpi_status wmi_status;
+	struct wmi_block *wblock;
+	struct wmi_device *wdev;
+	acpi_status status;

-		if (guid_equal(&block->gblock.guid, &guid_input)) {
-			if (block->handler)
-				return AE_ALREADY_ACQUIRED;
+	wdev = wmi_find_device_by_guid(guid);
+	if (IS_ERR(wdev))
+		return AE_ERROR;

-			block->handler = handler;
-			block->handler_data = data;
+	wblock = container_of(wdev, struct wmi_block, dev);
+	if (wblock->handler) {
+		status = AE_ALREADY_ACQUIRED;
+	} else {
+		wblock->handler = handler;
+		wblock->handler_data = data;

-			wmi_status = wmi_method_enable(block, true);
-			if (ACPI_FAILURE(wmi_status))
-				dev_warn(&block->dev.dev, "Failed to enable device\n");
+		if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
+			dev_warn(&wblock->dev.dev, "Failed to enable device\n");

-			return AE_OK;
-		}
+		status = AE_OK;
 	}

-	return AE_NOT_EXIST;
+	wmi_device_put(wdev);
+
+	return status;
 }
 EXPORT_SYMBOL_GPL(wmi_install_notify_handler);

@@ -612,34 +630,30 @@ EXPORT_SYMBOL_GPL(wmi_install_notify_handler);
  */
 acpi_status wmi_remove_notify_handler(const char *guid)
 {
-	struct wmi_block *block;
-	guid_t guid_input;
-
-	if (!guid)
-		return AE_BAD_PARAMETER;
-
-	if (guid_parse(guid, &guid_input))
-		return AE_BAD_PARAMETER;
-
-	list_for_each_entry(block, &wmi_block_list, list) {
-		acpi_status wmi_status;
+	struct wmi_block *wblock;
+	struct wmi_device *wdev;
+	acpi_status status;

-		if (guid_equal(&block->gblock.guid, &guid_input)) {
-			if (!block->handler)
-				return AE_NULL_ENTRY;
+	wdev = wmi_find_device_by_guid(guid);
+	if (IS_ERR(wdev))
+		return AE_ERROR;

-			wmi_status = wmi_method_enable(block, false);
-			if (ACPI_FAILURE(wmi_status))
-				dev_warn(&block->dev.dev, "Failed to disable device\n");
+	wblock = container_of(wdev, struct wmi_block, dev);
+	if (!wblock->handler) {
+		status = AE_NULL_ENTRY;
+	} else {
+		if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+			dev_warn(&wblock->dev.dev, "Failed to disable device\n");

-			block->handler = NULL;
-			block->handler_data = NULL;
+		wblock->handler = NULL;
+		wblock->handler_data = NULL;

-			return AE_OK;
-		}
+		status = AE_OK;
 	}

-	return AE_NOT_EXIST;
+	wmi_device_put(wdev);
+
+	return status;
 }
 EXPORT_SYMBOL_GPL(wmi_remove_notify_handler);

@@ -656,15 +670,19 @@ EXPORT_SYMBOL_GPL(wmi_remove_notify_handler);
 acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out)
 {
 	struct wmi_block *wblock;
+	struct wmi_device *wdev;
+	acpi_status status;

-	list_for_each_entry(wblock, &wmi_block_list, list) {
-		struct guid_block *gblock = &wblock->gblock;
+	wdev = wmi_find_event_by_notify_id(event);
+	if (IS_ERR(wdev))
+		return AE_NOT_FOUND;

-		if ((gblock->flags & ACPI_WMI_EVENT) && gblock->notify_id == event)
-			return get_event_data(wblock, out);
-	}
+	wblock = container_of(wdev, struct wmi_block, dev);
+	status = get_event_data(wblock, out);

-	return AE_NOT_FOUND;
+	wmi_device_put(wdev);
+
+	return status;
 }
 EXPORT_SYMBOL_GPL(wmi_get_event_data);

--
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] platform/x86: wmi: Fix notify callback locking
  2024-01-03 19:27 [PATCH 0/4] platform/x86: wmi: Event handling fixes Armin Wolf
                   ` (2 preceding siblings ...)
  2024-01-03 19:27 ` [PATCH 3/4] platform/x86: wmi: Decouple legacy WMI notify handlers from wmi_block_list Armin Wolf
@ 2024-01-03 19:27 ` Armin Wolf
  2024-01-04 18:15   ` Ilpo Järvinen
  2024-01-08 14:00 ` [PATCH 0/4] platform/x86: wmi: Event handling fixes Hans de Goede
  4 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2024-01-03 19:27 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel

When an legacy WMI event handler is removed, an WMI event could
have called the handler just before it was removed, meaning the
handler could still be running after wmi_remove_notify_handler()
returns.
Something similar could also happens when using the WMI bus, as
the WMI core might still call the notify() callback from an WMI
driver even if its remove() callback was just called.

Fix this by introducing a rw semaphore which ensures that the
event state of a WMI device does not change while the WMI core
is handling an event for it.

Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.

Fixes: 1686f5444546 ("platform/x86: wmi: Incorporate acpi_install_notify_handler")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 71 +++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 6a886635689a..1aa097d34690 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -25,6 +25,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/rwsem.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
@@ -56,7 +57,6 @@ static_assert(__alignof__(struct guid_block) == 1);

 enum {	/* wmi_block flags */
 	WMI_READ_TAKES_NO_ARGS,
-	WMI_PROBED,
 };

 struct wmi_block {
@@ -64,8 +64,10 @@ struct wmi_block {
 	struct list_head list;
 	struct guid_block gblock;
 	struct acpi_device *acpi_device;
+	struct rw_semaphore notify_lock;	/* Protects notify callback add/remove */
 	wmi_notify_handler handler;
 	void *handler_data;
+	bool driver_ready;
 	unsigned long flags;
 };

@@ -602,6 +604,8 @@ acpi_status wmi_install_notify_handler(const char *guid,
 		return AE_ERROR;

 	wblock = container_of(wdev, struct wmi_block, dev);
+
+	down_write(&wblock->notify_lock);
 	if (wblock->handler) {
 		status = AE_ALREADY_ACQUIRED;
 	} else {
@@ -613,6 +617,7 @@ acpi_status wmi_install_notify_handler(const char *guid,

 		status = AE_OK;
 	}
+	up_write(&wblock->notify_lock);

 	wmi_device_put(wdev);

@@ -639,6 +644,8 @@ acpi_status wmi_remove_notify_handler(const char *guid)
 		return AE_ERROR;

 	wblock = container_of(wdev, struct wmi_block, dev);
+
+	down_write(&wblock->notify_lock);
 	if (!wblock->handler) {
 		status = AE_NULL_ENTRY;
 	} else {
@@ -650,6 +657,7 @@ acpi_status wmi_remove_notify_handler(const char *guid)

 		status = AE_OK;
 	}
+	up_write(&wblock->notify_lock);

 	wmi_device_put(wdev);

@@ -895,7 +903,9 @@ static int wmi_dev_probe(struct device *dev)
 		}
 	}

-	set_bit(WMI_PROBED, &wblock->flags);
+	down_write(&wblock->notify_lock);
+	wblock->driver_ready = true;
+	up_write(&wblock->notify_lock);

 	return 0;
 }
@@ -905,7 +915,9 @@ static void wmi_dev_remove(struct device *dev)
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);

-	clear_bit(WMI_PROBED, &wblock->flags);
+	down_write(&wblock->notify_lock);
+	wblock->driver_ready = false;
+	up_write(&wblock->notify_lock);

 	if (wdriver->remove)
 		wdriver->remove(dev_to_wdev(dev));
@@ -1018,6 +1030,8 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 		wblock->dev.setable = true;

  out_init:
+	init_rwsem(&wblock->notify_lock);
+	wblock->driver_ready = false;
 	wblock->dev.dev.bus = &wmi_bus_type;
 	wblock->dev.dev.parent = wmi_bus_dev;

@@ -1190,6 +1204,26 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
 	}
 }

+static void wmi_notify_driver(struct wmi_block *wblock)
+{
+	struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
+	struct acpi_buffer data = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+
+	if (!driver->no_notify_data) {
+		status = get_event_data(wblock, &data);
+		if (ACPI_FAILURE(status)) {
+			dev_warn(&wblock->dev.dev, "Failed to get event data\n");
+			return;
+		}
+	}
+
+	if (driver->notify)
+		driver->notify(&wblock->dev, data.pointer);
+
+	kfree(data.pointer);
+}
+
 static int wmi_notify_device(struct device *dev, void *data)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
@@ -1198,28 +1232,17 @@ static int wmi_notify_device(struct device *dev, void *data)
 	if (!(wblock->gblock.flags & ACPI_WMI_EVENT && wblock->gblock.notify_id == *event))
 		return 0;

-	/* If a driver is bound, then notify the driver. */
-	if (test_bit(WMI_PROBED, &wblock->flags) && wblock->dev.dev.driver) {
-		struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
-		struct acpi_buffer evdata = { ACPI_ALLOCATE_BUFFER, NULL };
-		acpi_status status;
-
-		if (!driver->no_notify_data) {
-			status = get_event_data(wblock, &evdata);
-			if (ACPI_FAILURE(status)) {
-				dev_warn(&wblock->dev.dev, "failed to get event data\n");
-				return -EIO;
-			}
-		}
-
-		if (driver->notify)
-			driver->notify(&wblock->dev, evdata.pointer);
-
-		kfree(evdata.pointer);
-	} else if (wblock->handler) {
-		/* Legacy handler */
-		wblock->handler(*event, wblock->handler_data);
+	down_read(&wblock->notify_lock);
+	/* The WMI driver notify handler conflicts with the legacy WMI handler.
+	 * Because of this the WMI driver notify handler takes precedence.
+	 */
+	if (wblock->dev.dev.driver && wblock->driver_ready) {
+		wmi_notify_driver(wblock);
+	} else {
+		if (wblock->handler)
+			wblock->handler(*event, wblock->handler_data);
 	}
+	up_read(&wblock->notify_lock);

 	acpi_bus_generate_netlink_event(wblock->acpi_device->pnp.device_class,
 					dev_name(&wblock->dev.dev), *event, 0);
--
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] platform/x86: wmi: Fix error handling in legacy WMI notify handler functions
  2024-01-03 19:27 ` [PATCH 1/4] platform/x86: wmi: Fix error handling in legacy WMI notify handler functions Armin Wolf
@ 2024-01-04 18:10   ` Ilpo Järvinen
  0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2024-01-04 18:10 UTC (permalink / raw)
  To: Armin Wolf; +Cc: Hans de Goede, platform-driver-x86, LKML

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

On Wed, 3 Jan 2024, Armin Wolf wrote:

> When wmi_install_notify_handler()/wmi_remove_notify_handler() are
> unable to enable/disable the WMI device, they unconditionally return
> an error to the caller.
> When registering legacy WMI notify handlers, this means that the
> callback remains registered despite wmi_install_notify_handler()
> having returned an error.
> When removing legacy WMI notify handlers, this means that the
> callback is removed despite wmi_remove_notify_handler() having
> returned an error.
> 
> Fix this by only warning when the WMI device could not be enabled.
> This behaviour matches the bus-based WMI interface.
> 
> Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.
> 
> Fixes: 58f6425eb92f ("WMI: Cater for multiple events with same GUID")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

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

-- 
 i.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/4] platform/x86: wmi: Return immediately if an suitable WMI event is found
  2024-01-03 19:27 ` [PATCH 2/4] platform/x86: wmi: Return immediately if an suitable WMI event is found Armin Wolf
@ 2024-01-04 18:13   ` Ilpo Järvinen
  0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2024-01-04 18:13 UTC (permalink / raw)
  To: Armin Wolf; +Cc: Hans de Goede, platform-driver-x86, LKML

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

On Wed, 3 Jan 2024, Armin Wolf wrote:

> Commit 58f6425eb92f ("WMI: Cater for multiple events with same GUID")
> allowed legacy WMI notify handlers to be installed for multiple WMI
> devices with the same GUID.
> However this is useless since the legacy GUID-based interface is
> blacklisted from seeing WMI devices with duplicated GUIDs.
> 
> Return immediately if a suitable WMI event is found in
> wmi_install/remove_notify_handler() since searching for other suitable
> events is pointless.
> 
> Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

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

-- 
 i.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/4] platform/x86: wmi: Decouple legacy WMI notify handlers from wmi_block_list
  2024-01-03 19:27 ` [PATCH 3/4] platform/x86: wmi: Decouple legacy WMI notify handlers from wmi_block_list Armin Wolf
@ 2024-01-04 18:14   ` Ilpo Järvinen
  0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2024-01-04 18:14 UTC (permalink / raw)
  To: Armin Wolf; +Cc: Hans de Goede, platform-driver-x86, LKML

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

On Wed, 3 Jan 2024, Armin Wolf wrote:

> Until now, legacy WMI notify handler functions where using the
> wmi_block_list, which did no refcounting on the returned WMI device.
> This meant that the WMI device could disappear at any moment,
> potentially leading to various errors.
> Fix this by using bus_find_device() which returns an actual
> reference to the found WMI device.
> 
> Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

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

-- 
 i.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] platform/x86: wmi: Fix notify callback locking
  2024-01-03 19:27 ` [PATCH 4/4] platform/x86: wmi: Fix notify callback locking Armin Wolf
@ 2024-01-04 18:15   ` Ilpo Järvinen
  0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2024-01-04 18:15 UTC (permalink / raw)
  To: Armin Wolf; +Cc: Hans de Goede, platform-driver-x86, LKML

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

On Wed, 3 Jan 2024, Armin Wolf wrote:

> When an legacy WMI event handler is removed, an WMI event could
> have called the handler just before it was removed, meaning the
> handler could still be running after wmi_remove_notify_handler()
> returns.
> Something similar could also happens when using the WMI bus, as
> the WMI core might still call the notify() callback from an WMI
> driver even if its remove() callback was just called.
> 
> Fix this by introducing a rw semaphore which ensures that the
> event state of a WMI device does not change while the WMI core
> is handling an event for it.
> 
> Tested on a Dell Inspiron 3505 and a Acer Aspire E1-731.
> 
> Fixes: 1686f5444546 ("platform/x86: wmi: Incorporate acpi_install_notify_handler")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

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

-- 
 i.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/4] platform/x86: wmi: Event handling fixes
  2024-01-03 19:27 [PATCH 0/4] platform/x86: wmi: Event handling fixes Armin Wolf
                   ` (3 preceding siblings ...)
  2024-01-03 19:27 ` [PATCH 4/4] platform/x86: wmi: Fix notify callback locking Armin Wolf
@ 2024-01-08 14:00 ` Hans de Goede
  4 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2024-01-08 14:00 UTC (permalink / raw)
  To: Armin Wolf, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel

Hi Armin,

On 1/3/24 20:27, Armin Wolf wrote:
> This patch series contains various fixes for the handling of
> WMI events. The first patch fixes an issue where the WMI core
> would return an error even when a legacy WMI notify handler
> was installed successfully.
> 
> The second patch cleans up the last remains of
> commit 58f6425eb92f ("WMI: Cater for multiple events with same GUID"),
> which is useless now that legacy WMI handlers cannot see duplicated
> GUIDS. This however might highlight a potential problem:
> 
> In the email thread concerning aforementioned commit, a (now legacy)
> WMI driver was mentioned which depended on this functionality.
> I believe this driver is the dell-wmi-aio driver, so this driver is
> likely already broken for some time and should be converted to the
> new bus-based interface.
> 
> The next patch decouples the legacy WMI notify handlers from the
> wmi_block_list, while the last patch fixes a race condition when a
> notify callback is called by the WMI core.
> 
> All patches have been tested on a Dell Inspiron 3505 and a
> Acer Aspire E1-731.

Thank you for your patch series, I've applied this 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.

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

Regards,

Hans



> Armin Wolf (4):
>   platform/x86: wmi: Fix error handling in legacy WMI notify handler
>     functions
>   platform/x86: wmi: Return immediately if an suitable WMI event is
>     found
>   platform/x86: wmi: Decouple legacy WMI notify handlers from
>     wmi_block_list
>   platform/x86: wmi: Fix notify callback locking
> 
>  drivers/platform/x86/wmi.c | 179 +++++++++++++++++++++++--------------
>  1 file changed, 111 insertions(+), 68 deletions(-)
> 
> --
> 2.39.2
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-01-08 14:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-03 19:27 [PATCH 0/4] platform/x86: wmi: Event handling fixes Armin Wolf
2024-01-03 19:27 ` [PATCH 1/4] platform/x86: wmi: Fix error handling in legacy WMI notify handler functions Armin Wolf
2024-01-04 18:10   ` Ilpo Järvinen
2024-01-03 19:27 ` [PATCH 2/4] platform/x86: wmi: Return immediately if an suitable WMI event is found Armin Wolf
2024-01-04 18:13   ` Ilpo Järvinen
2024-01-03 19:27 ` [PATCH 3/4] platform/x86: wmi: Decouple legacy WMI notify handlers from wmi_block_list Armin Wolf
2024-01-04 18:14   ` Ilpo Järvinen
2024-01-03 19:27 ` [PATCH 4/4] platform/x86: wmi: Fix notify callback locking Armin Wolf
2024-01-04 18:15   ` Ilpo Järvinen
2024-01-08 14:00 ` [PATCH 0/4] platform/x86: wmi: Event handling fixes Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox