From 6ba7843b59b77360812617d071313c7f35f3757a Mon Sep 17 00:00:00 2001 From: Armin Wolf Date: Wed, 3 Jan 2024 20:27:04 +0100 Subject: platform/x86: wmi: Fix error handling in legacy WMI notify handler functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ilpo Järvinen Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20240103192707.115512-2-W_Armin@gmx.de Signed-off-by: Hans de Goede --- drivers/platform/x86/wmi.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'drivers/platform/x86/wmi.c') diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index bd271a5730aa..23b4043c0fe7 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -593,9 +593,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; } } @@ -631,10 +632,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; } } -- cgit v1.2.3 From 3d8a29fec2cb96b3aa75a595f20c4b73ff294a97 Mon Sep 17 00:00:00 2001 From: Armin Wolf Date: Wed, 3 Jan 2024 20:27:05 +0100 Subject: platform/x86: wmi: Return immediately if an suitable WMI event is found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ilpo Järvinen Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20240103192707.115512-3-W_Armin@gmx.de Signed-off-by: Hans de Goede --- drivers/platform/x86/wmi.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'drivers/platform/x86/wmi.c') diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index 23b4043c0fe7..4e2d0e15b155 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -573,7 +573,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) @@ -596,11 +595,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); @@ -615,7 +614,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) @@ -638,11 +636,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); -- cgit v1.2.3 From 3ea7f59af8ffa17ce5f5173d6f4bfbc73334187d Mon Sep 17 00:00:00 2001 From: Armin Wolf Date: Wed, 3 Jan 2024 20:27:06 +0100 Subject: platform/x86: wmi: Decouple legacy WMI notify handlers from wmi_block_list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ilpo Järvinen Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20240103192707.115512-4-W_Armin@gmx.de Signed-off-by: Hans de Goede --- drivers/platform/x86/wmi.c | 118 ++++++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 50 deletions(-) (limited to 'drivers/platform/x86/wmi.c') diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index 4e2d0e15b155..699157ce347c 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -219,6 +219,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) @@ -238,6 +249,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, ¬ify_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); @@ -572,34 +594,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); @@ -613,34 +631,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); @@ -657,15 +671,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); -- cgit v1.2.3 From 29e473f4b51ee56b5808323e274a8369b4d181cb Mon Sep 17 00:00:00 2001 From: Armin Wolf Date: Wed, 3 Jan 2024 20:27:07 +0100 Subject: platform/x86: wmi: Fix notify callback locking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ilpo Järvinen Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20240103192707.115512-5-W_Armin@gmx.de Signed-off-by: Hans de Goede --- drivers/platform/x86/wmi.c | 71 ++++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 24 deletions(-) (limited to 'drivers/platform/x86/wmi.c') diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index 699157ce347c..d1df1a31de00 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -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; }; @@ -603,6 +605,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 { @@ -614,6 +618,7 @@ acpi_status wmi_install_notify_handler(const char *guid, status = AE_OK; } + up_write(&wblock->notify_lock); wmi_device_put(wdev); @@ -640,6 +645,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 { @@ -651,6 +658,7 @@ acpi_status wmi_remove_notify_handler(const char *guid) status = AE_OK; } + up_write(&wblock->notify_lock); wmi_device_put(wdev); @@ -896,7 +904,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; } @@ -906,7 +916,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)); @@ -1019,6 +1031,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; @@ -1191,6 +1205,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); @@ -1199,28 +1233,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); -- cgit v1.2.3 From 8446f9d11678bc268897882e86733d73204f83c4 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Fri, 5 Jan 2024 16:47:54 +0300 Subject: platform/x86: wmi: Fix wmi_dev_probe() This has a reversed if statement so it accidentally disables the wmi method before returning. Fixes: 704af3a40747 ("platform/x86: wmi: Remove chardev interface") Signed-off-by: Dan Carpenter Reviewed-by: Armin Wolf Link: https://lore.kernel.org/r/9c81251b-bc87-4ca3-bb86-843dc85e5145@moroto.mountain Reviewed-by: Hans de Goede Signed-off-by: Hans de Goede --- drivers/platform/x86/wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/platform/x86/wmi.c') diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index d1df1a31de00..3c288e8f404b 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -896,7 +896,7 @@ static int wmi_dev_probe(struct device *dev) if (wdriver->probe) { ret = wdriver->probe(dev_to_wdev(dev), find_guid_context(wblock, wdriver)); - if (!ret) { + if (ret) { if (ACPI_FAILURE(wmi_method_enable(wblock, false))) dev_warn(dev, "Failed to disable device\n"); -- cgit v1.2.3