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

[PATCH 0/7] 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 three 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 compile-tested only.

Armin Wolf (7):
  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: 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                    | 114 +++++++++---------
 7 files changed, 96 insertions(+), 95 deletions(-)

--
2.39.5

[PATCH 1/7] 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.

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 2/7] 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.

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 3/7] 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.

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 4/7] 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 5/7] 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 | 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

[PATCH 6/7] 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 01d4ac480930..2b2e405955cd 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 7/7] 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 1/7] hwmon: (hp-wmi-sensors) Use the WMI bus API when accessing sensors

[James Seo]

On Mon, Feb 03, 2025 at 07:23:16PM +0100, Armin Wolf wrote:
> Since the driver already binds to HP_WMI_NUMERIC_SENSOR_GUID, using
> wmidev_block_query() allows for faster sensor access.
> 
> 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
>
Works on a HP Z420.

Reviewed-by: James Seo <james@equiv.tech>

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

[Guenter Roeck]

On 2/3/25 10:23, Armin Wolf wrote:
> Since the driver already binds to HP_WMI_NUMERIC_SENSOR_GUID, using
> wmidev_block_query() allows for faster sensor access.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Acked-by: Guenter Roeck <linux@roeck-us.net>

... assuming the series will be applied together. Please let me know
if this patch should be applied through hwmon.

Guenter

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

[Armin Wolf]

Am 04.02.25 um 02:18 schrieb Guenter Roeck:

> On 2/3/25 10:23, Armin Wolf wrote:
>> Since the driver already binds to HP_WMI_NUMERIC_SENSOR_GUID, using
>> wmidev_block_query() allows for faster sensor access.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
>
> ... assuming the series will be applied together. Please let me know
> if this patch should be applied through hwmon.
>
> Guenter

Hi,

i would like to have this patch merged through the pdx86 tree.

Thanks,
Armin Wolf

Re: [PATCH 4/7] platform/x86: hp-bioscfg: Use wmi_instance_count()

[Ilpo Järvinen]

On Mon, 3 Feb 2025, Armin Wolf wrote:

> 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;
>  }

Hi Armin,

While it is the existing way of doing things, I don't like how the error 
is not properly passed on here. And if the error handling is pushed 
upwards to the calling sites, then this entire function becomes useless 
and wmi_instance_count() could be used directly in the callers.

-- 
 i.

Re: [PATCH 4/7] platform/x86: hp-bioscfg: Use wmi_instance_count()

[Armin Wolf]

Am 04.02.25 um 11:37 schrieb Ilpo Järvinen:

> On Mon, 3 Feb 2025, Armin Wolf wrote:
>
>> 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;
>>   }
> Hi Armin,
>
> While it is the existing way of doing things, I don't like how the error
> is not properly passed on here. And if the error handling is pushed
> upwards to the calling sites, then this entire function becomes useless
> and wmi_instance_count() could be used directly in the callers.
>
The thing is that for the hp-bioscfg driver, a missing WMI GUID is not an error.
In this case 0 instances are available.

I would keep this function for now.

Thanks,
Armin Wolf

Re: [PATCH 4/7] platform/x86: hp-bioscfg: Use wmi_instance_count()

[Ilpo Järvinen]

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

On Tue, 4 Feb 2025, Armin Wolf wrote:

> Am 04.02.25 um 11:37 schrieb Ilpo Järvinen:
> 
> > On Mon, 3 Feb 2025, Armin Wolf wrote:
> > 
> > > 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;
> > >   }
> > Hi Armin,
> > 
> > While it is the existing way of doing things, I don't like how the error
> > is not properly passed on here. And if the error handling is pushed
> > upwards to the calling sites, then this entire function becomes useless
> > and wmi_instance_count() could be used directly in the callers.
>
> The thing is that for the hp-bioscfg driver, a missing WMI GUID is not an
> error.
> In this case 0 instances are available.
> 
> I would keep this function for now.

Okay, fine.

-- 
 i.

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

[Armin Wolf]

Am 03.02.25 um 19:23 schrieb 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.

Hi,

i was hoping that maybe the driver maintainer could take a look at this patch
and give some feedback.

Thanks,
Armin Wolf

> 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
>
>

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

[Mark Pearson]

Hi Armin

On Sun, Feb 9, 2025, at 7:31 PM, Armin Wolf wrote:
> Am 03.02.25 um 19:23 schrieb 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.
>
> Hi,
>
> i was hoping that maybe the driver maintainer could take a look at this patch
> and give some feedback.
>
My apologies - because of this patch (and a couple of others) I've just realised I never updated the MAINTAINERS file.
I have been mothballing the markpearson@lenovo.com address as it's a nightmare to use and switched to using my personal email domain instead. It seems my email filters aren't flagging these messages the way they are supposed to be - I have to figure that out :(

> Thanks,
> Armin Wolf
>
>> 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
>>
>>
Changes look good to me. If you can hold on a bit I'll see if I can test them on a few platforms to make sure no surprises.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

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

[Ilpo Järvinen]

On Mon, 3 Feb 2025, Armin Wolf wrote:

> Since the driver already binds to LENOVO_BIOS_SETTING_GUID, using
> wmidev_block_query() inside tlmi_setting() allows for faster
> access to BIOS settings.
> 
> 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;

Hi Armin,

I'm trying to understand why there are these back and forth changes in the 
error code.

It almost looks to me like wmidev_block_query() would want to return the 
error code itself because after you abstracted this code using 
wmidev_block_query(), you had to change the error code because you no 
longer have access to the key detail to decide which error code should be 
returned. That is, use ERR_PTR() inside wmidev_block_query() and the 
callers should just pass that error code on with IS_ERR & friends?

-- 
 i.

>  	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
> 

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

[Armin Wolf]

Am 13.02.25 um 14:17 schrieb Ilpo Järvinen:

> On Mon, 3 Feb 2025, Armin Wolf wrote:
>
>> Since the driver already binds to LENOVO_BIOS_SETTING_GUID, using
>> wmidev_block_query() inside tlmi_setting() allows for faster
>> access to BIOS settings.
>>
>> 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;
> Hi Armin,
>
> I'm trying to understand why there are these back and forth changes in the
> error code.
>
> It almost looks to me like wmidev_block_query() would want to return the
> error code itself because after you abstracted this code using
> wmidev_block_query(), you had to change the error code because you no
> longer have access to the key detail to decide which error code should be
> returned. That is, use ERR_PTR() inside wmidev_block_query() and the
> callers should just pass that error code on with IS_ERR & friends?
>
Hi,

the reason why wmidev_block_query() only returns NULL in case of an error is that
according to the WMI-ACPI specification, querying a WMI data block should return data.

So we have two error scenarios:

- ACPI error => firmware error => EIO
- no data returned => violation of firmware spec => EIO

Because of this always returning EIO is the correct approach in my opinion.

Thanks,
Armin Wolf