[PATCH v2 0/8] platform/x86: wmi: Rework WMI device enabling

[PATCH v2 0/8] platform/x86: wmi: Rework WMI device enabling

[Armin Wolf]

This patch series reworks how WMI devices are enabled and disabled
to improve the compatibility with various firmware implementations.

The first three patches make sure that no WMI driver using the WMI bus
infrastructure is using the deprecated GUID-based API to access the
underlying WMI device.

The fourth patch is a unrelated cleanup patch.

The last four patches finally rework the WMI device enabling inside
the WMI core and update the documentation.

The WMI core patches have been tested on a Dell Inspiron 3505, but
the remaining patches are mostly compile-tested only.

Changes since v2:
- add Reviewed-by, Acked-by and Tested-by tags
- use devres to disable the WMI device

Armin Wolf (8):
  hwmon: (hp-wmi-sensors) Use the WMI bus API when accessing sensors
  platform/x86: think-lmi: Use ACPI object when extracting strings
  platform/x86: think-lmi: Use WMI bus API when accessing BIOS settings
  platform/x86: hp-bioscfg: Use wmi_instance_count()
  platform/x86: wmi: Use devres to disable the WMI device
  platform/x86: wmi: Rework WCxx/WExx ACPI method handling
  platform/x86: wmi: Call WCxx methods when setting data blocks
  platform/x86: wmi: Update documentation regarding the GUID-based API

 Documentation/wmi/acpi-interface.rst          |   3 +
 .../wmi/driver-development-guide.rst          |   4 +
 drivers/hwmon/hp-wmi-sensors.c                |   4 +-
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c  |  13 +-
 drivers/platform/x86/think-lmi.c              |  51 +++----
 drivers/platform/x86/think-lmi.h              |   2 +
 drivers/platform/x86/wmi.c                    | 141 ++++++++++--------
 7 files changed, 116 insertions(+), 102 deletions(-)

--
2.39.5

[PATCH v2 1/8] hwmon: (hp-wmi-sensors) Use the WMI bus API when accessing sensors

[Armin Wolf]

Since the driver already binds to HP_WMI_NUMERIC_SENSOR_GUID, using
wmidev_block_query() allows for faster sensor access.

Acked-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: James Seo <james@equiv.tech>
Reviewed-by: James Seo <james@equiv.tech>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/hp-wmi-sensors.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
index d6bdad26feb1..03c684ba83bd 100644
--- a/drivers/hwmon/hp-wmi-sensors.c
+++ b/drivers/hwmon/hp-wmi-sensors.c
@@ -1197,7 +1197,7 @@ static int hp_wmi_update_info(struct hp_wmi_sensors *state,
 	if (time_after(jiffies, info->last_updated + HZ)) {
 		mutex_lock(&state->lock);

-		wobj = hp_wmi_get_wobj(HP_WMI_NUMERIC_SENSOR_GUID, instance);
+		wobj = wmidev_block_query(state->wdev, instance);
 		if (!wobj) {
 			ret = -EIO;
 			goto out_unlock;
@@ -1745,7 +1745,7 @@ static int init_numeric_sensors(struct hp_wmi_sensors *state,
 		return -ENOMEM;

 	for (i = 0, info = info_arr; i < icount; i++, info++) {
-		wobj = hp_wmi_get_wobj(HP_WMI_NUMERIC_SENSOR_GUID, i);
+		wobj = wmidev_block_query(state->wdev, i);
 		if (!wobj)
 			return -EIO;

--
2.39.5

[PATCH v2 2/8] platform/x86: think-lmi: Use ACPI object when extracting strings

[Armin Wolf]

Move the ACPI buffer handling out of tlmi_extract_output_string()
and instead pass the unpacked ACPI object to prepare for future
changes.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/think-lmi.c | 38 +++++++++++++++++---------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 323316ac6783..2c94a4af9a1d 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -262,16 +262,11 @@ static int tlmi_simple_call(const char *guid, const char *arg)
 	return 0;
 }

-/* Extract output string from WMI return buffer */
-static int tlmi_extract_output_string(const struct acpi_buffer *output,
-				      char **string)
+/* Extract output string from WMI return value */
+static int tlmi_extract_output_string(union acpi_object *obj, char **string)
 {
-	const union acpi_object *obj;
 	char *s;

-	obj = output->pointer;
-	if (!obj)
-		return -ENOMEM;
 	if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer)
 		return -EIO;

@@ -352,17 +347,21 @@ static int tlmi_opcode_setting(char *setting, const char *value)
 static int tlmi_setting(int item, char **value, const char *guid_string)
 {
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
 	acpi_status status;
 	int ret;

 	status = wmi_query_block(guid_string, item, &output);
-	if (ACPI_FAILURE(status)) {
-		kfree(output.pointer);
+	if (ACPI_FAILURE(status))
 		return -EIO;
-	}

-	ret = tlmi_extract_output_string(&output, value);
-	kfree(output.pointer);
+	obj = output.pointer;
+	if (!obj)
+		return -ENODATA;
+
+	ret = tlmi_extract_output_string(obj, value);
+	kfree(obj);
+
 	return ret;
 }

@@ -370,19 +369,22 @@ static int tlmi_get_bios_selections(const char *item, char **value)
 {
 	const struct acpi_buffer input = { strlen(item), (char *)item };
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
 	acpi_status status;
 	int ret;

 	status = wmi_evaluate_method(LENOVO_GET_BIOS_SELECTIONS_GUID,
 				     0, 0, &input, &output);
-
-	if (ACPI_FAILURE(status)) {
-		kfree(output.pointer);
+	if (ACPI_FAILURE(status))
 		return -EIO;
-	}

-	ret = tlmi_extract_output_string(&output, value);
-	kfree(output.pointer);
+	obj = output.pointer;
+	if (!obj)
+		return -ENODATA;
+
+	ret = tlmi_extract_output_string(obj, value);
+	kfree(obj);
+
 	return ret;
 }

--
2.39.5

[PATCH v2 3/8] platform/x86: think-lmi: Use WMI bus API when accessing BIOS settings

[Armin Wolf]

Since the driver already binds to LENOVO_BIOS_SETTING_GUID, using
wmidev_block_query() inside tlmi_setting() allows for faster
access to BIOS settings.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/think-lmi.c | 23 +++++++++--------------
 drivers/platform/x86/think-lmi.h |  2 ++
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 2c94a4af9a1d..0fc275e461be 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -344,20 +344,14 @@ static int tlmi_opcode_setting(char *setting, const char *value)
 	return ret;
 }

-static int tlmi_setting(int item, char **value, const char *guid_string)
+static int tlmi_setting(struct wmi_device *wdev, int item, char **value)
 {
-	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
-	acpi_status status;
 	int ret;

-	status = wmi_query_block(guid_string, item, &output);
-	if (ACPI_FAILURE(status))
-		return -EIO;
-
-	obj = output.pointer;
+	obj = wmidev_block_query(wdev, item);
 	if (!obj)
-		return -ENODATA;
+		return -EIO;

 	ret = tlmi_extract_output_string(obj, value);
 	kfree(obj);
@@ -995,7 +989,7 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
 	char *item, *value;
 	int ret;

-	ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
+	ret = tlmi_setting(setting->wdev, setting->index, &item);
 	if (ret)
 		return ret;

@@ -1588,7 +1582,7 @@ static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
 	return new_pwd;
 }

-static int tlmi_analyze(void)
+static int tlmi_analyze(struct wmi_device *wdev)
 {
 	int i, ret;

@@ -1625,7 +1619,7 @@ static int tlmi_analyze(void)
 		char *item = NULL;

 		tlmi_priv.setting[i] = NULL;
-		ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
+		ret = tlmi_setting(wdev, i, &item);
 		if (ret)
 			break;
 		if (!item)
@@ -1648,6 +1642,7 @@ static int tlmi_analyze(void)
 			kfree(item);
 			goto fail_clear_attr;
 		}
+		setting->wdev = wdev;
 		setting->index = i;
 		strscpy(setting->display_name, item);
 		/* If BIOS selections supported, load those */
@@ -1666,7 +1661,7 @@ static int tlmi_analyze(void)
 			 */
 			char *optitem, *optstart, *optend;

-			if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
+			if (!tlmi_setting(setting->wdev, setting->index, &optitem)) {
 				optstart = strstr(optitem, "[Optional:");
 				if (optstart) {
 					optstart += strlen("[Optional:");
@@ -1791,7 +1786,7 @@ static int tlmi_probe(struct wmi_device *wdev, const void *context)
 {
 	int ret;

-	ret = tlmi_analyze();
+	ret = tlmi_analyze(wdev);
 	if (ret)
 		return ret;

diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index f267d8b46957..a80452482227 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -4,6 +4,7 @@
 #define _THINK_LMI_H_

 #include <linux/types.h>
+#include <linux/wmi.h>

 #define TLMI_SETTINGS_COUNT  256
 #define TLMI_SETTINGS_MAXLEN 512
@@ -87,6 +88,7 @@ struct tlmi_pwd_setting {
 /* Attribute setting details */
 struct tlmi_attr_setting {
 	struct kobject kobj;
+	struct wmi_device *wdev;
 	int index;
 	char display_name[TLMI_SETTINGS_MAXLEN];
 	char *possible_values;
--
2.39.5

[PATCH v2 4/8] platform/x86: hp-bioscfg: Use wmi_instance_count()

[Armin Wolf]

The WMI core already knows the instance count of a WMI guid.
Use this information instead of querying all possible instances
which is slow and might be unreliable.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 0b277b7e37dd..63c78b4d8258 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -388,16 +388,13 @@ union acpi_object *hp_get_wmiobj_pointer(int instance_id, const char *guid_strin
  */
 int hp_get_instance_count(const char *guid_string)
 {
-	union acpi_object *wmi_obj = NULL;
-	int i = 0;
+	int ret;

-	do {
-		kfree(wmi_obj);
-		wmi_obj = hp_get_wmiobj_pointer(i, guid_string);
-		i++;
-	} while (wmi_obj);
+	ret = wmi_instance_count(guid_string);
+	if (ret < 0)
+		return 0;

-	return i - 1;
+	return ret;
 }

 /**
--
2.39.5

[PATCH v2 5/8] platform/x86: wmi: Use devres to disable the WMI device

[Armin Wolf]

Use devm_add_action_or_reset() to disable the WMI device instead of
manually calling wmi_method_enable() to prepare for future changes
inside the WMI data block handlign code.

The reason for this is that we have to make sure that all
devres-managed resources are released first because some might still
want to access the underlying WMI device.

Because devres-managed resources are not released during shutdown
we still have to manually disable the WMI device in this case.

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

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 646370bd6b03..7cb36a71e9ea 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -821,11 +821,19 @@ static int wmi_dev_match(struct device *dev, const struct device_driver *driver)
 	return 0;
 }

+static void wmi_dev_disable(void *data)
+{
+	struct wmi_block *wblock = data;
+
+	if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+		dev_warn(&wblock->dev.dev, "Failed to disable device\n");
+}
+
 static int wmi_dev_probe(struct device *dev)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	struct wmi_driver *wdriver = to_wmi_driver(dev->driver);
-	int ret = 0;
+	int ret;

 	/* Some older WMI drivers will break if instantiated multiple times,
 	 * so they are blocked from probing WMI devices with a duplicated GUID.
@@ -847,15 +855,19 @@ static int wmi_dev_probe(struct device *dev)
 	if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
 		dev_warn(dev, "failed to enable device -- probing anyway\n");

+	/*
+	 * We have to make sure that all devres-managed resources are released first because
+	 * some might still want to access the underlying WMI device.
+	 */
+	ret = devm_add_action_or_reset(dev, wmi_dev_disable, wblock);
+	if (ret < 0)
+		return ret;
+
 	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)))
-				dev_warn(dev, "Failed to disable device\n");
-
+		if (ret)
 			return ret;
-		}
 	}

 	down_write(&wblock->notify_lock);
@@ -876,9 +888,6 @@ 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)))
-		dev_warn(dev, "failed to disable device\n");
 }

 static void wmi_dev_shutdown(struct device *dev)
@@ -902,6 +911,10 @@ static void wmi_dev_shutdown(struct device *dev)
 		if (wdriver->shutdown)
 			wdriver->shutdown(to_wmi_device(dev));

+		/*
+		 * We still need to disable the WMI device here since devres-managed resources
+		 * like wmi_dev_disable() will not be release during shutdown.
+		 */
 		if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
 			dev_warn(dev, "Failed to disable device\n");
 	}
--
2.39.5

[PATCH v2 6/8] platform/x86: wmi: Rework WCxx/WExx ACPI method handling

[Armin Wolf]

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>
---
 drivers/platform/x86/wmi.c | 112 +++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 60 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7cb36a71e9ea..97c4a161799e 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;
@@ -823,10 +815,10 @@ static int wmi_dev_match(struct device *dev, const struct device_driver *driver)

 static void wmi_dev_disable(void *data)
 {
-	struct wmi_block *wblock = data;
+	struct device *dev = data;

-	if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
-		dev_warn(&wblock->dev.dev, "Failed to disable device\n");
+	if (wmi_device_enable(to_wmi_device(dev), false) < 0)
+		dev_warn(dev, "Failed to disable device\n");
 }

 static int wmi_dev_probe(struct device *dev)
@@ -852,14 +844,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");

 	/*
 	 * We have to make sure that all devres-managed resources are released first because
 	 * some might still want to access the underlying WMI device.
 	 */
-	ret = devm_add_action_or_reset(dev, wmi_dev_disable, wblock);
+	ret = devm_add_action_or_reset(dev, wmi_dev_disable, dev);
 	if (ret < 0)
 		return ret;

@@ -915,7 +907,7 @@ static void wmi_dev_shutdown(struct device *dev)
 		 * We still need to disable the WMI device here since devres-managed resources
 		 * like wmi_dev_disable() will not be release during shutdown.
 		 */
-		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

[PATCH v2 7/8] platform/x86: wmi: Call WCxx methods when setting data blocks

[Armin Wolf]

After performing some tests with a custom SSDT table available at
https://github.com/Wer-Wolf/acpi-wmi-ssdt i found out that Windows
also enables data block collection even when the data block is
being set.

Emulate this behaviour to avoid confusing the ACPI firmware.
The bus-based API already implements this behaviour, so only the
legacy GUID-based API needs to be changed.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/wmi/acpi-interface.rst | 3 +++
 drivers/platform/x86/wmi.c           | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/wmi/acpi-interface.rst b/Documentation/wmi/acpi-interface.rst
index 06fb7fcf4413..f1b28835d23c 100644
--- a/Documentation/wmi/acpi-interface.rst
+++ b/Documentation/wmi/acpi-interface.rst
@@ -89,6 +89,9 @@ Similar to the ``WExx`` ACPI methods, except that it controls data collection
 instead of events and thus the last two characters of the ACPI method name are
 the method ID of the data block to enable/disable.

+Those ACPI methods are also called before setting data blocks to match the
+behaviour of the Windows driver.
+
 _WED ACPI method
 ----------------

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 97c4a161799e..db76c5bc6e12 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -461,8 +461,14 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance, const struct acp
 	if (IS_ERR(wdev))
 		return AE_ERROR;

+	if (wmi_device_enable(wdev, true) < 0)
+		dev_warn(&wdev->dev, "Failed to enable device\n");
+
 	status =  wmidev_block_set(wdev, instance, in);

+	if (wmi_device_enable(wdev, false) < 0)
+		dev_warn(&wdev->dev, "Failed to disable device\n");
+
 	wmi_device_put(wdev);

 	return status;
--
2.39.5

[PATCH v2 8/8] platform/x86: wmi: Update documentation regarding the GUID-based API

[Armin Wolf]

Warn WMI driver developers to not use GUID-based and non-GUID-based
functions for querying WMI data blocks and handling WMI events
simultaneously on the same device, as this will corrupt the WMI device
state and might thus lead to erratic behaviour.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/wmi/driver-development-guide.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
index f7e1089a0559..99ef21fc1c1e 100644
--- a/Documentation/wmi/driver-development-guide.rst
+++ b/Documentation/wmi/driver-development-guide.rst
@@ -96,6 +96,10 @@ on a given machine.
 Because of this, WMI drivers should use the state container design pattern as described in
 Documentation/driver-api/driver-model/design-patterns.rst.

+.. warning:: Using both GUID-based and non-GUID-based functions for querying WMI data blocks and
+             handling WMI events simultaneously on the same device is guaranteed to corrupt the
+             WMI device state and might lead to erratic behaviour.
+
 WMI method drivers
 ------------------

--
2.39.5

Re: [PATCH v2 0/8] platform/x86: wmi: Rework WMI device enabling

[Ilpo Järvinen]

On Sun, 16 Feb 2025 20:32:43 +0100, Armin Wolf wrote:

> This patch series reworks how WMI devices are enabled and disabled
> to improve the compatibility with various firmware implementations.
> 
> The first three patches make sure that no WMI driver using the WMI bus
> infrastructure is using the deprecated GUID-based API to access the
> underlying WMI device.
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/8] hwmon: (hp-wmi-sensors) Use the WMI bus API when accessing sensors
      commit: 27cc2914e495d314d19c7c1f74687bb28bc292e8
[2/8] platform/x86: think-lmi: Use ACPI object when extracting strings
      commit: 82d3af6b30ac2c6031428dd506eab9fd416cb825
[3/8] platform/x86: think-lmi: Use WMI bus API when accessing BIOS settings
      commit: 126a53a94a3fb3171d35f3c87820aafbafa08430
[4/8] platform/x86: hp-bioscfg: Use wmi_instance_count()
      commit: 0fcc3162e3fbe4ca9347b2f42313dbf077207bb1
[5/8] platform/x86: wmi: Use devres to disable the WMI device
      commit: b6b566908cd75bd99eb0e4b396c89704588261b3
[6/8] platform/x86: wmi: Rework WCxx/WExx ACPI method handling
      commit: 656f0961d126f5d1f0bcc62fa8bb7598d8f89207
[7/8] platform/x86: wmi: Call WCxx methods when setting data blocks
      commit: 08c9f4029007325e0bbd4fdda05650b03e43f071
[8/8] platform/x86: wmi: Update documentation regarding the GUID-based API
      commit: 8d5316c6c6633c18c4ebeb2dd038933549d404de

--
 i.