The handling of the WExx ACPI methods used for enabling and disabling
WMI events has multiple flaws:
- the ACPI methods are called even when the WMI device has not been
marked as expensive.
- WExx ACPI methods might be called for inappropriate WMI devices.
- the error code AE_NOT_FOUND is treated as success.
The handling of the WCxx ACPI methods used for enabling and disabling
WMI data blocks is also flawed:
- WMI data blocks are enabled and disabled for every single "query"
operation. This is racy and inefficient.
Unify the handling of both ACPI methods by introducing a common
helper function for enabling and disabling WMI devices.
Also enable/disable WMI data blocks during probe/remove and shutdown
to match the handling of WMI events.
Legacy GUID-based functions still have to enable/disable the WMI
device manually and thus still suffer from a potential race condition.
Since those functions are deprecated and suffer from various other
flaws this issue is purposefully not fixed.
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
drivers/platform/x86/wmi.c | 108 +++++++++++++++++--------------------
1 file changed, 50 insertions(+), 58 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 646370bd6b03..01d4ac480930 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -123,24 +123,6 @@ static const void *find_guid_context(struct wmi_block *wblock,
return NULL;
}
-static acpi_status wmi_method_enable(struct wmi_block *wblock, bool enable)
-{
- struct guid_block *block;
- char method[5];
- acpi_status status;
- acpi_handle handle;
-
- block = &wblock->gblock;
- handle = wblock->acpi_device->handle;
-
- snprintf(method, 5, "WE%02X", block->notify_id);
- status = acpi_execute_simple_method(handle, method, enable);
- if (status == AE_NOT_FOUND)
- return AE_OK;
-
- return status;
-}
-
#define WMI_ACPI_METHOD_NAME_SIZE 5
static inline void get_acpi_method_name(const struct wmi_block *wblock,
@@ -184,6 +166,42 @@ static int wmidev_match_guid(struct device *dev, const void *data)
static const struct bus_type wmi_bus_type;
+static const struct device_type wmi_type_event;
+
+static const struct device_type wmi_type_method;
+
+static int wmi_device_enable(struct wmi_device *wdev, bool enable)
+{
+ struct wmi_block *wblock = container_of(wdev, struct wmi_block, dev);
+ char method[WMI_ACPI_METHOD_NAME_SIZE];
+ acpi_handle handle;
+ acpi_status status;
+
+ if (!(wblock->gblock.flags & ACPI_WMI_EXPENSIVE))
+ return 0;
+
+ if (wblock->dev.dev.type == &wmi_type_method)
+ return 0;
+
+ if (wblock->dev.dev.type == &wmi_type_event)
+ snprintf(method, sizeof(method), "WE%02X", wblock->gblock.notify_id);
+ else
+ get_acpi_method_name(wblock, 'C', method);
+
+ /* Not all WMI devices marked as expensive actually implement the necessary ACPI method.
+ * Ignore this missing ACPI method to match the behaviour of the Windows driver.
+ */
+ status = acpi_get_handle(wblock->acpi_device->handle, method, &handle);
+ if (ACPI_FAILURE(status))
+ return 0;
+
+ status = acpi_execute_simple_method(handle, NULL, enable);
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ return 0;
+}
+
static struct wmi_device *wmi_find_device_by_guid(const char *guid_string)
{
struct device *dev;
@@ -337,10 +355,8 @@ static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
{
struct guid_block *block;
acpi_handle handle;
- acpi_status status, wc_status = AE_ERROR;
struct acpi_object_list input;
union acpi_object wq_params[1];
- char wc_method[WMI_ACPI_METHOD_NAME_SIZE];
char method[WMI_ACPI_METHOD_NAME_SIZE];
if (!out)
@@ -364,40 +380,9 @@ static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
if (instance == 0 && test_bit(WMI_READ_TAKES_NO_ARGS, &wblock->flags))
input.count = 0;
- /*
- * If ACPI_WMI_EXPENSIVE, call the relevant WCxx method first to
- * enable collection.
- */
- if (block->flags & ACPI_WMI_EXPENSIVE) {
- get_acpi_method_name(wblock, 'C', wc_method);
-
- /*
- * Some GUIDs break the specification by declaring themselves
- * expensive, but have no corresponding WCxx method. So we
- * should not fail if this happens.
- */
- wc_status = acpi_execute_simple_method(handle, wc_method, 1);
- }
-
get_acpi_method_name(wblock, 'Q', method);
- status = acpi_evaluate_object(handle, method, &input, out);
- /*
- * If ACPI_WMI_EXPENSIVE, call the relevant WCxx method, even if
- * the WQxx method failed - we should disable collection anyway.
- */
- if ((block->flags & ACPI_WMI_EXPENSIVE) && ACPI_SUCCESS(wc_status)) {
- /*
- * Ignore whether this WCxx call succeeds or not since
- * the previously executed WQxx method call might have
- * succeeded, and returning the failing status code
- * of this call would throw away the result of the WQxx
- * call, potentially leaking memory.
- */
- acpi_execute_simple_method(handle, wc_method, 0);
- }
-
- return status;
+ return acpi_evaluate_object(handle, method, &input, out);
}
/**
@@ -421,9 +406,15 @@ acpi_status wmi_query_block(const char *guid_string, u8 instance,
if (IS_ERR(wdev))
return AE_ERROR;
+ if (wmi_device_enable(wdev, true) < 0)
+ dev_warn(&wdev->dev, "Failed to enable device\n");
+
wblock = container_of(wdev, struct wmi_block, dev);
status = __query_block(wblock, instance, out);
+ if (wmi_device_enable(wdev, false) < 0)
+ dev_warn(&wdev->dev, "Failed to disable device\n");
+
wmi_device_put(wdev);
return status;
@@ -471,6 +462,7 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance, const struct acp
return AE_ERROR;
status = wmidev_block_set(wdev, instance, in);
+
wmi_device_put(wdev);
return status;
@@ -551,7 +543,7 @@ acpi_status wmi_install_notify_handler(const char *guid,
wblock->handler = handler;
wblock->handler_data = data;
- if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
+ if (wmi_device_enable(wdev, true) < 0)
dev_warn(&wblock->dev.dev, "Failed to enable device\n");
status = AE_OK;
@@ -588,7 +580,7 @@ acpi_status wmi_remove_notify_handler(const char *guid)
if (!wblock->handler) {
status = AE_NULL_ENTRY;
} else {
- if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+ if (wmi_device_enable(wdev, false) < 0)
dev_warn(&wblock->dev.dev, "Failed to disable device\n");
wblock->handler = NULL;
@@ -844,14 +836,14 @@ static int wmi_dev_probe(struct device *dev)
return -ENODEV;
}
- if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
+ if (wmi_device_enable(to_wmi_device(dev), true) < 0)
dev_warn(dev, "failed to enable device -- probing anyway\n");
if (wdriver->probe) {
ret = wdriver->probe(to_wmi_device(dev),
find_guid_context(wblock, wdriver));
if (ret) {
- if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+ if (wmi_device_enable(to_wmi_device(dev), false) < 0)
dev_warn(dev, "Failed to disable device\n");
return ret;
@@ -877,7 +869,7 @@ static void wmi_dev_remove(struct device *dev)
if (wdriver->remove)
wdriver->remove(to_wmi_device(dev));
- if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+ if (wmi_device_enable(to_wmi_device(dev), false) < 0)
dev_warn(dev, "failed to disable device\n");
}
@@ -902,7 +894,7 @@ static void wmi_dev_shutdown(struct device *dev)
if (wdriver->shutdown)
wdriver->shutdown(to_wmi_device(dev));
- if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+ if (wmi_device_enable(to_wmi_device(dev), false) < 0)
dev_warn(dev, "Failed to disable device\n");
}
}
--
2.39.5
