platform/x86: wmi: Rework WCxx/WExx ACPI method handling

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@gmx.de>
Link: https://lore.kernel.org/r/20250216193251.866125-7-W_Armin@gmx.de
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

authored by

Armin Wolf and committed by

Ilpo Järvinen 1 year ago 656f0961 b6b56690

+53 -60

1 changed file

expand all

drivers

platform

x86

wmi.c

+53 -60

drivers/platform/x86/wmi.c

··· 123 123 return NULL; 124 124 } 125 125 126 - static acpi_status wmi_method_enable(struct wmi_block *wblock, bool enable) 127 - { 128 - struct guid_block *block; 129 - char method[5]; 130 - acpi_status status; 131 - acpi_handle handle; 132 - 133 - block = &wblock->gblock; 134 - handle = wblock->acpi_device->handle; 135 - 136 - snprintf(method, 5, "WE%02X", block->notify_id); 137 - status = acpi_execute_simple_method(handle, method, enable); 138 - if (status == AE_NOT_FOUND) 139 - return AE_OK; 140 - 141 - return status; 142 - } 143 - 144 126 #define WMI_ACPI_METHOD_NAME_SIZE 5 145 127 146 128 static inline void get_acpi_method_name(const struct wmi_block *wblock, ··· 165 183 } 166 184 167 185 static const struct bus_type wmi_bus_type; 186 + 187 + static const struct device_type wmi_type_event; 188 + 189 + static const struct device_type wmi_type_method; 190 + 191 + static int wmi_device_enable(struct wmi_device *wdev, bool enable) 192 + { 193 + struct wmi_block *wblock = container_of(wdev, struct wmi_block, dev); 194 + char method[WMI_ACPI_METHOD_NAME_SIZE]; 195 + acpi_handle handle; 196 + acpi_status status; 197 + 198 + if (!(wblock->gblock.flags & ACPI_WMI_EXPENSIVE)) 199 + return 0; 200 + 201 + if (wblock->dev.dev.type == &wmi_type_method) 202 + return 0; 203 + 204 + if (wblock->dev.dev.type == &wmi_type_event) 205 + snprintf(method, sizeof(method), "WE%02X", wblock->gblock.notify_id); 206 + else 207 + get_acpi_method_name(wblock, 'C', method); 208 + 209 + /* 210 + * Not all WMI devices marked as expensive actually implement the 211 + * necessary ACPI method. Ignore this missing ACPI method to match 212 + * the behaviour of the Windows driver. 213 + */ 214 + status = acpi_get_handle(wblock->acpi_device->handle, method, &handle); 215 + if (ACPI_FAILURE(status)) 216 + return 0; 217 + 218 + status = acpi_execute_simple_method(handle, NULL, enable); 219 + if (ACPI_FAILURE(status)) 220 + return -EIO; 221 + 222 + return 0; 223 + } 168 224 169 225 static struct wmi_device *wmi_find_device_by_guid(const char *guid_string) 170 226 { ··· 357 337 { 358 338 struct guid_block *block; 359 339 acpi_handle handle; 360 - acpi_status status, wc_status = AE_ERROR; 361 340 struct acpi_object_list input; 362 341 union acpi_object wq_params[1]; 363 - char wc_method[WMI_ACPI_METHOD_NAME_SIZE]; 364 342 char method[WMI_ACPI_METHOD_NAME_SIZE]; 365 343 366 344 if (!out) ··· 382 364 if (instance == 0 && test_bit(WMI_READ_TAKES_NO_ARGS, &wblock->flags)) 383 365 input.count = 0; 384 366 385 - /* 386 - * If ACPI_WMI_EXPENSIVE, call the relevant WCxx method first to 387 - * enable collection. 388 - */ 389 - if (block->flags & ACPI_WMI_EXPENSIVE) { 390 - get_acpi_method_name(wblock, 'C', wc_method); 391 - 392 - /* 393 - * Some GUIDs break the specification by declaring themselves 394 - * expensive, but have no corresponding WCxx method. So we 395 - * should not fail if this happens. 396 - */ 397 - wc_status = acpi_execute_simple_method(handle, wc_method, 1); 398 - } 399 - 400 367 get_acpi_method_name(wblock, 'Q', method); 401 - status = acpi_evaluate_object(handle, method, &input, out); 402 368 403 - /* 404 - * If ACPI_WMI_EXPENSIVE, call the relevant WCxx method, even if 405 - * the WQxx method failed - we should disable collection anyway. 406 - */ 407 - if ((block->flags & ACPI_WMI_EXPENSIVE) && ACPI_SUCCESS(wc_status)) { 408 - /* 409 - * Ignore whether this WCxx call succeeds or not since 410 - * the previously executed WQxx method call might have 411 - * succeeded, and returning the failing status code 412 - * of this call would throw away the result of the WQxx 413 - * call, potentially leaking memory. 414 - */ 415 - acpi_execute_simple_method(handle, wc_method, 0); 416 - } 417 - 418 - return status; 369 + return acpi_evaluate_object(handle, method, &input, out); 419 370 } 420 371 421 372 /** ··· 408 421 if (IS_ERR(wdev)) 409 422 return AE_ERROR; 410 423 424 + if (wmi_device_enable(wdev, true) < 0) 425 + dev_warn(&wdev->dev, "Failed to enable device\n"); 426 + 411 427 wblock = container_of(wdev, struct wmi_block, dev); 412 428 status = __query_block(wblock, instance, out); 429 + 430 + if (wmi_device_enable(wdev, false) < 0) 431 + dev_warn(&wdev->dev, "Failed to disable device\n"); 413 432 414 433 wmi_device_put(wdev); 415 434 ··· 544 551 wblock->handler = handler; 545 552 wblock->handler_data = data; 546 553 547 - if (ACPI_FAILURE(wmi_method_enable(wblock, true))) 554 + if (wmi_device_enable(wdev, true) < 0) 548 555 dev_warn(&wblock->dev.dev, "Failed to enable device\n"); 549 556 550 557 status = AE_OK; ··· 581 588 if (!wblock->handler) { 582 589 status = AE_NULL_ENTRY; 583 590 } else { 584 - if (ACPI_FAILURE(wmi_method_enable(wblock, false))) 591 + if (wmi_device_enable(wdev, false) < 0) 585 592 dev_warn(&wblock->dev.dev, "Failed to disable device\n"); 586 593 587 594 wblock->handler = NULL; ··· 816 823 817 824 static void wmi_dev_disable(void *data) 818 825 { 819 - struct wmi_block *wblock = data; 826 + struct device *dev = data; 820 827 821 - if (ACPI_FAILURE(wmi_method_enable(wblock, false))) 822 - dev_warn(&wblock->dev.dev, "Failed to disable device\n"); 828 + if (wmi_device_enable(to_wmi_device(dev), false) < 0) 829 + dev_warn(dev, "Failed to disable device\n"); 823 830 } 824 831 825 832 static int wmi_dev_probe(struct device *dev) ··· 845 852 return -ENODEV; 846 853 } 847 854 848 - if (ACPI_FAILURE(wmi_method_enable(wblock, true))) 855 + if (wmi_device_enable(to_wmi_device(dev), true) < 0) 849 856 dev_warn(dev, "failed to enable device -- probing anyway\n"); 850 857 851 858 /* 852 859 * We have to make sure that all devres-managed resources are released first because 853 860 * some might still want to access the underlying WMI device. 854 861 */ 855 - ret = devm_add_action_or_reset(dev, wmi_dev_disable, wblock); 862 + ret = devm_add_action_or_reset(dev, wmi_dev_disable, dev); 856 863 if (ret < 0) 857 864 return ret; 858 865 ··· 908 915 * We still need to disable the WMI device here since devres-managed resources 909 916 * like wmi_dev_disable() will not be release during shutdown. 910 917 */ 911 - if (ACPI_FAILURE(wmi_method_enable(wblock, false))) 918 + if (wmi_device_enable(to_wmi_device(dev), false) < 0) 912 919 dev_warn(dev, "Failed to disable device\n"); 913 920 } 914 921 }

Configure Feed

Configure Feed