[PATCH 00/16] Convert WMI to a proper bus

[PATCH 00/16] Convert WMI to a proper bus

[Darren Hart]

From: "Darren Hart (VMware)" <dvhart@infradead.org>

This series is based on the original work of
Andy Lutomirski <luto@amacapital.net> [1]. I have made minor edits, and in
one instance, squashed two patches in which the latter undid the former.

This series converts WMI [2] into a proper bus, adds some useful information via
sysfs, and exposes the embedded MOF [3] binary. It converts dell-wmi to use the
new WMI bus architecture.

This is the first part of an ongoing effort to enhance the WMI infrastructure
within the kernel, and eventually expose WMI to userspace for the consumption of
management utilities as it was intended.

1. https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/log/?h=platform/wmi
2. https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx
3. https://msdn.microsoft.com/en-us/library/aa823192(v=vs.85).aspx

Andy Lutomirski (15):
  platform/x86: wmi: Drop "Mapper (un)loaded" messages
  platform/x86: wmi: Pass the acpi_device through to parse_wdg
  platform/x86: wmi: Clean up acpi_wmi_add
  platform/x86: wmi: Track wmi devices per ACPI device
  platform/x86: wmi: Turn WMI into a bus driver
  platform/x86: wmi: Fix error handling when creating devices
  platform/x86: wmi: Split devices into types and add basic sysfs attributes
  platform/x86: wmi: Probe data objects for read and write capabilities
  platform/x86: wmi: Instantiate all devices before adding them
  platform/x86: wmi: Incorporate acpi_install_notify_handler
  platform/x86: wmi: Add a new interface to read block data
  platform/x86: wmi: Bind the platform device, not the ACPI node
  platform/x86: wmi: Add an interface for subdrivers to access sibling devices
  platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata
  platform/x86: dell-wmi: Convert to the WMI bus infrastructure

Darren Hart (VMware) (1):
  platform/x86: wmi: Require query for data blocks, rename writable to setable

 drivers/platform/x86/Kconfig    |  12 +
 drivers/platform/x86/Makefile   |   1 +
 drivers/platform/x86/dell-wmi.c | 136 ++++----
 drivers/platform/x86/wmi-mof.c  | 125 ++++++++
 drivers/platform/x86/wmi.c      | 677 ++++++++++++++++++++++++++++++++--------
 include/linux/wmi.h             |  59 ++++
 6 files changed, 815 insertions(+), 195 deletions(-)
 create mode 100644 drivers/platform/x86/wmi-mof.c
 create mode 100644 include/linux/wmi.h

-- 
2.9.4

[PATCH 01/16] platform/x86: wmi: Drop "Mapper (un)loaded" messages

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

WMI is just a driver. There is no need to announce when it is loaded.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index ceeb8c1..0043581 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -836,7 +836,6 @@ static int __init acpi_wmi_init(void)
 		return error;
 	}
 
-	pr_info("Mapper loaded\n");
 	return 0;
 }
 
@@ -844,8 +843,6 @@ static void __exit acpi_wmi_exit(void)
 {
 	acpi_bus_unregister_driver(&acpi_wmi_driver);
 	class_unregister(&wmi_class);
-
-	pr_info("Mapper unloaded\n");
 }
 
 subsys_initcall(acpi_wmi_init);
-- 
2.9.4

[PATCH 02/16] platform/x86: wmi: Pass the acpi_device through to parse_wdg

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

We will need the device to convert to a bus architecture and bind WMI to
the platform device.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 0043581..c6e11b5 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -606,7 +606,8 @@ static struct class wmi_class = {
 };
 
 static int wmi_create_device(const struct guid_block *gblock,
-			     struct wmi_block *wblock, acpi_handle handle)
+			     struct wmi_block *wblock,
+			     struct acpi_device *device)
 {
 	wblock->dev.class = &wmi_class;
 
@@ -645,7 +646,7 @@ static bool guid_already_parsed(const char *guid_string)
 /*
  * Parse the _WDG method for the GUID data blocks
  */
-static int parse_wdg(acpi_handle handle)
+static int parse_wdg(struct acpi_device *device)
 {
 	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
 	union acpi_object *obj;
@@ -655,7 +656,7 @@ static int parse_wdg(acpi_handle handle)
 	int retval;
 	u32 i, total;
 
-	status = acpi_evaluate_object(handle, "_WDG", NULL, &out);
+	status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
 	if (ACPI_FAILURE(status))
 		return -ENXIO;
 
@@ -679,7 +680,7 @@ static int parse_wdg(acpi_handle handle)
 		if (!wblock)
 			return -ENOMEM;
 
-		wblock->handle = handle;
+		wblock->handle = device->handle;
 		wblock->gblock = gblock[i];
 
 		/*
@@ -689,7 +690,7 @@ static int parse_wdg(acpi_handle handle)
 		  for device creation.
 		*/
 		if (!guid_already_parsed(gblock[i].guid)) {
-			retval = wmi_create_device(&gblock[i], wblock, handle);
+			retval = wmi_create_device(&gblock[i], wblock, device);
 			if (retval) {
 				wmi_free_devices();
 				goto out_free_pointer;
@@ -806,7 +807,7 @@ static int acpi_wmi_add(struct acpi_device *device)
 		return -ENODEV;
 	}
 
-	error = parse_wdg(device->handle);
+	error = parse_wdg(device);
 	if (error) {
 		acpi_remove_address_space_handler(device->handle,
 						  ACPI_ADR_SPACE_EC,
-- 
2.9.4

[PATCH 03/16] platform/x86: wmi: Clean up acpi_wmi_add

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

Rearrange acpi_wmi_add to use Linux's error handling conventions.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index c6e11b5..ac60a51 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -803,20 +803,24 @@ static int acpi_wmi_add(struct acpi_device *device)
 						    &acpi_wmi_ec_space_handler,
 						    NULL, NULL);
 	if (ACPI_FAILURE(status)) {
-		pr_err("Error installing EC region handler\n");
+		dev_err(&device->dev, "Error installing EC region handler\n");
 		return -ENODEV;
 	}
 
 	error = parse_wdg(device);
 	if (error) {
-		acpi_remove_address_space_handler(device->handle,
-						  ACPI_ADR_SPACE_EC,
-						  &acpi_wmi_ec_space_handler);
 		pr_err("Failed to parse WDG method\n");
-		return error;
+		goto err_remove_handler;
 	}
 
 	return 0;
+
+err_remove_handler:
+	acpi_remove_address_space_handler(device->handle,
+					  ACPI_ADR_SPACE_EC,
+					  &acpi_wmi_ec_space_handler);
+
+	return error;
 }
 
 static int __init acpi_wmi_init(void)
-- 
2.9.4

[PATCH 04/16] platform/x86: wmi: Track wmi devices per ACPI device

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

Currently we free all devices when we detach from any ACPI node.
Instead, keep track of which node WMI devices are attached to and
free them only as needed. While we are at it, match up notifications
with the device they came from correctly.

This will make our behavior more straightforward on systems with
more than one WMI node in the ACPI tables (e.g. the Dell XPS 13
9350).

This also adds a warning when GUIDs are not unique.

NB: The guid_string parameter in guid_already_parsed was a
little-endian binary GUID, not a string.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 58 ++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index ac60a51..faaa9a7 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -64,7 +64,7 @@ struct guid_block {
 struct wmi_block {
 	struct list_head list;
 	struct guid_block gblock;
-	acpi_handle handle;
+	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
 	struct device dev;
@@ -147,7 +147,7 @@ static acpi_status wmi_method_enable(struct wmi_block *wblock, int enable)
 	acpi_handle handle;
 
 	block = &wblock->gblock;
-	handle = wblock->handle;
+	handle = wblock->acpi_device->handle;
 
 	snprintf(method, 5, "WE%02X", block->notify_id);
 	status = acpi_execute_simple_method(handle, method, enable);
@@ -186,7 +186,7 @@ u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
 		return AE_ERROR;
 
 	block = &wblock->gblock;
-	handle = wblock->handle;
+	handle = wblock->acpi_device->handle;
 
 	if (!(block->flags & ACPI_WMI_METHOD))
 		return AE_BAD_DATA;
@@ -248,7 +248,7 @@ struct acpi_buffer *out)
 		return AE_ERROR;
 
 	block = &wblock->gblock;
-	handle = wblock->handle;
+	handle = wblock->acpi_device->handle;
 
 	if (block->instance_count < instance)
 		return AE_BAD_PARAMETER;
@@ -321,7 +321,7 @@ const struct acpi_buffer *in)
 		return AE_ERROR;
 
 	block = &wblock->gblock;
-	handle = wblock->handle;
+	handle = wblock->acpi_device->handle;
 
 	if (block->instance_count < instance)
 		return AE_BAD_PARAMETER;
@@ -525,8 +525,8 @@ acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out)
 
 		if ((gblock->flags & ACPI_WMI_EVENT) &&
 			(gblock->notify_id == event))
-			return acpi_evaluate_object(wblock->handle, "_WED",
-				&input, out);
+			return acpi_evaluate_object(wblock->acpi_device->handle,
+				"_WED", &input, out);
 	}
 
 	return AE_NOT_FOUND;
@@ -618,27 +618,40 @@ static int wmi_create_device(const struct guid_block *gblock,
 	return device_register(&wblock->dev);
 }
 
-static void wmi_free_devices(void)
+static void wmi_free_devices(struct acpi_device *device)
 {
 	struct wmi_block *wblock, *next;
 
 	/* Delete devices for all the GUIDs */
 	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
-		list_del(&wblock->list);
-		if (wblock->dev.class)
-			device_unregister(&wblock->dev);
-		else
-			kfree(wblock);
+		if (wblock->acpi_device == device) {
+			list_del(&wblock->list);
+			if (wblock->dev.class)
+				device_unregister(&wblock->dev);
+			else
+				kfree(wblock);
+		}
 	}
 }
 
-static bool guid_already_parsed(const char *guid_string)
+static bool guid_already_parsed(struct acpi_device *device,
+				const u8 *guid)
 {
 	struct wmi_block *wblock;
 
-	list_for_each_entry(wblock, &wmi_block_list, list)
-		if (memcmp(wblock->gblock.guid, guid_string, 16) == 0)
+	list_for_each_entry(wblock, &wmi_block_list, list) {
+		if (memcmp(wblock->gblock.guid, guid, 16) == 0) {
+			/*
+			 * Because we historically didn't track the relationship
+			 * between GUIDs and ACPI nodes, we don't know whether
+			 * we need to suppress GUIDs that are unique on a
+			 * given node but duplicated across nodes.
+			 */
+			dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
+				 guid, dev_name(&wblock->acpi_device->dev));
 			return true;
+		}
+	}
 
 	return false;
 }
@@ -680,7 +693,7 @@ static int parse_wdg(struct acpi_device *device)
 		if (!wblock)
 			return -ENOMEM;
 
-		wblock->handle = device->handle;
+		wblock->acpi_device = device;
 		wblock->gblock = gblock[i];
 
 		/*
@@ -689,10 +702,10 @@ static int parse_wdg(struct acpi_device *device)
 		  case yet, so for now, we'll just ignore the duplicate
 		  for device creation.
 		*/
-		if (!guid_already_parsed(gblock[i].guid)) {
+		if (!guid_already_parsed(device, gblock[i].guid)) {
 			retval = wmi_create_device(&gblock[i], wblock, device);
 			if (retval) {
-				wmi_free_devices();
+				wmi_free_devices(device);
 				goto out_free_pointer;
 			}
 		}
@@ -767,8 +780,9 @@ static void acpi_wmi_notify(struct acpi_device *device, u32 event)
 		wblock = list_entry(p, struct wmi_block, list);
 		block = &wblock->gblock;
 
-		if ((block->flags & ACPI_WMI_EVENT) &&
-			(block->notify_id == event)) {
+		if (wblock->acpi_device == device &&
+		    (block->flags & ACPI_WMI_EVENT) &&
+		    (block->notify_id == event)) {
 			if (wblock->handler)
 				wblock->handler(event, wblock->handler_data);
 			if (debug_event) {
@@ -788,7 +802,7 @@ static int acpi_wmi_remove(struct acpi_device *device)
 {
 	acpi_remove_address_space_handler(device->handle,
 				ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
-	wmi_free_devices();
+	wmi_free_devices(device);
 
 	return 0;
 }
-- 
2.9.4

[PATCH 05/16] platform/x86: wmi: Turn WMI into a bus driver

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

WMI is logically a bus: the WMI driver binds to an ACPI node (or
more than one), and each instance of the WMI driver enumerates its
children and hopes that drivers will attach to the children that are
useful.

This patch gives WMI a driver model bus type and the ability to
match to drivers. The bus itself is a device in the new "wmi_bus"
class, and all of the individual WMI devices are slotted into the
device hierarchy correctly.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 197 +++++++++++++++++++++++++++++++++++----------
 include/linux/wmi.h        |  47 +++++++++++
 2 files changed, 201 insertions(+), 43 deletions(-)
 create mode 100644 include/linux/wmi.h

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index faaa9a7..f06b7c0 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -37,6 +37,7 @@
 #include <linux/acpi.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/wmi.h>
 #include <linux/uuid.h>
 
 ACPI_MODULE_NAME("wmi");
@@ -44,8 +45,6 @@ MODULE_AUTHOR("Carlos Corbacho");
 MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
 MODULE_LICENSE("GPL");
 
-#define ACPI_WMI_CLASS "wmi"
-
 static LIST_HEAD(wmi_block_list);
 
 struct guid_block {
@@ -62,12 +61,12 @@ struct guid_block {
 };
 
 struct wmi_block {
+	struct wmi_device dev;
 	struct list_head list;
 	struct guid_block gblock;
 	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
-	struct device dev;
 };
 
 
@@ -102,8 +101,8 @@ static const struct acpi_device_id wmi_device_ids[] = {
 MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
 
 static struct acpi_driver acpi_wmi_driver = {
-	.name = "wmi",
-	.class = ACPI_WMI_CLASS,
+	.name = "acpi-wmi",
+	.owner = THIS_MODULE,
 	.ids = wmi_device_ids,
 	.ops = {
 		.add = acpi_wmi_add,
@@ -545,77 +544,146 @@ bool wmi_has_guid(const char *guid_string)
 }
 EXPORT_SYMBOL_GPL(wmi_has_guid);
 
+static struct wmi_block *dev_to_wblock(struct device *dev)
+{
+	return container_of(dev, struct wmi_block, dev.dev);
+}
+
+static struct wmi_device *dev_to_wdev(struct device *dev)
+{
+	return container_of(dev, struct wmi_device, dev);
+}
+
 /*
  * sysfs interface
  */
 static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
-	struct wmi_block *wblock;
-
-	wblock = dev_get_drvdata(dev);
-	if (!wblock) {
-		strcat(buf, "\n");
-		return strlen(buf);
-	}
+	struct wmi_block *wblock = dev_to_wblock(dev);
 
 	return sprintf(buf, "wmi:%pUL\n", wblock->gblock.guid);
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	return sprintf(buf, "%pUL\n", wblock->gblock.guid);
+}
+static DEVICE_ATTR_RO(guid);
+
 static struct attribute *wmi_attrs[] = {
 	&dev_attr_modalias.attr,
+	&dev_attr_guid.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(wmi);
 
 static int wmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	char guid_string[37];
-
-	struct wmi_block *wblock;
+	struct wmi_block *wblock = dev_to_wblock(dev);
 
-	if (add_uevent_var(env, "MODALIAS="))
+	if (add_uevent_var(env, "MODALIAS=wmi:%pUL", wblock->gblock.guid))
 		return -ENOMEM;
 
-	wblock = dev_get_drvdata(dev);
-	if (!wblock)
+	if (add_uevent_var(env, "WMI_GUID=%pUL", wblock->gblock.guid))
 		return -ENOMEM;
 
-	sprintf(guid_string, "%pUL", wblock->gblock.guid);
+	return 0;
+}
+
+static void wmi_dev_release(struct device *dev)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	kfree(wblock);
+}
+
+static int wmi_dev_match(struct device *dev, struct device_driver *driver)
+{
+	struct wmi_driver *wmi_driver =
+		container_of(driver, struct wmi_driver, driver);
+	struct wmi_block *wblock = dev_to_wblock(dev);
+	const struct wmi_device_id *id = wmi_driver->id_table;
 
-	strcpy(&env->buf[env->buflen - 1], "wmi:");
-	memcpy(&env->buf[env->buflen - 1 + 4], guid_string, 36);
-	env->buflen += 40;
+	while (id->guid_string) {
+		uuid_le driver_guid;
+
+		if (WARN_ON(uuid_le_to_bin(id->guid_string, &driver_guid)))
+			continue;
+		if (!memcmp(&driver_guid, wblock->gblock.guid, 16))
+			return 1;
+
+		id++;
+	}
 
 	return 0;
 }
 
-static void wmi_dev_free(struct device *dev)
+static int wmi_dev_probe(struct device *dev)
 {
-	struct wmi_block *wmi_block = container_of(dev, struct wmi_block, dev);
+	struct wmi_block *wblock = dev_to_wblock(dev);
+	struct wmi_driver *wdriver =
+		container_of(dev->driver, struct wmi_driver, driver);
+	int ret = 0;
+
+	if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
+		dev_warn(dev, "failed to enable device -- probing anyway\n");
+
+	if (wdriver->probe) {
+		ret = wdriver->probe(dev_to_wdev(dev));
+		if (ret != 0 && ACPI_FAILURE(wmi_method_enable(wblock, 0)))
+			dev_warn(dev, "failed to disable device\n");
+	}
+
+	return ret;
+}
+
+static int wmi_dev_remove(struct device *dev)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+	struct wmi_driver *wdriver =
+		container_of(dev->driver, struct wmi_driver, driver);
+	int ret = 0;
+
+	if (wdriver->remove)
+		ret = wdriver->remove(dev_to_wdev(dev));
+
+	if (ACPI_FAILURE(wmi_method_enable(wblock, 0)))
+		dev_warn(dev, "failed to disable device\n");
 
-	kfree(wmi_block);
+	return ret;
 }
 
-static struct class wmi_class = {
+static struct class wmi_bus_class = {
+	.name = "wmi_bus",
+};
+
+static struct bus_type wmi_bus_type = {
 	.name = "wmi",
-	.dev_release = wmi_dev_free,
-	.dev_uevent = wmi_dev_uevent,
 	.dev_groups = wmi_groups,
+	.match = wmi_dev_match,
+	.uevent = wmi_dev_uevent,
+	.probe = wmi_dev_probe,
+	.remove = wmi_dev_remove,
 };
 
-static int wmi_create_device(const struct guid_block *gblock,
+static int wmi_create_device(struct device *wmi_bus_dev,
+			     const struct guid_block *gblock,
 			     struct wmi_block *wblock,
 			     struct acpi_device *device)
 {
-	wblock->dev.class = &wmi_class;
+	wblock->dev.dev.bus = &wmi_bus_type;
+	wblock->dev.dev.parent = wmi_bus_dev;
 
-	dev_set_name(&wblock->dev, "%pUL", gblock->guid);
+	dev_set_name(&wblock->dev.dev, "%pUL", gblock->guid);
 
-	dev_set_drvdata(&wblock->dev, wblock);
+	wblock->dev.dev.release = wmi_dev_release;
 
-	return device_register(&wblock->dev);
+	return device_register(&wblock->dev.dev);
 }
 
 static void wmi_free_devices(struct acpi_device *device)
@@ -626,8 +694,8 @@ static void wmi_free_devices(struct acpi_device *device)
 	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
 		if (wblock->acpi_device == device) {
 			list_del(&wblock->list);
-			if (wblock->dev.class)
-				device_unregister(&wblock->dev);
+			if (wblock->dev.dev.bus)
+				device_unregister(&wblock->dev.dev);
 			else
 				kfree(wblock);
 		}
@@ -659,7 +727,7 @@ static bool guid_already_parsed(struct acpi_device *device,
 /*
  * Parse the _WDG method for the GUID data blocks
  */
-static int parse_wdg(struct acpi_device *device)
+static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 {
 	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
 	union acpi_object *obj;
@@ -703,7 +771,8 @@ static int parse_wdg(struct acpi_device *device)
 		  for device creation.
 		*/
 		if (!guid_already_parsed(device, gblock[i].guid)) {
-			retval = wmi_create_device(&gblock[i], wblock, device);
+			retval = wmi_create_device(wmi_bus_dev, &gblock[i],
+						   wblock, device);
 			if (retval) {
 				wmi_free_devices(device);
 				goto out_free_pointer;
@@ -803,12 +872,15 @@ static int acpi_wmi_remove(struct acpi_device *device)
 	acpi_remove_address_space_handler(device->handle,
 				ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
 	wmi_free_devices(device);
+	device_unregister((struct device *)acpi_driver_data(device));
+	device->driver_data = NULL;
 
 	return 0;
 }
 
 static int acpi_wmi_add(struct acpi_device *device)
 {
+	struct device *wmi_bus_dev;
 	acpi_status status;
 	int error;
 
@@ -821,14 +893,25 @@ static int acpi_wmi_add(struct acpi_device *device)
 		return -ENODEV;
 	}
 
-	error = parse_wdg(device);
+	wmi_bus_dev = device_create(&wmi_bus_class, &device->dev, MKDEV(0, 0),
+				    NULL, "wmi_bus-%s", dev_name(&device->dev));
+	if (IS_ERR(wmi_bus_dev)) {
+		error = PTR_ERR(wmi_bus_dev);
+		goto err_remove_handler;
+	}
+	device->driver_data = wmi_bus_dev;
+
+	error = parse_wdg(wmi_bus_dev, device);
 	if (error) {
 		pr_err("Failed to parse WDG method\n");
-		goto err_remove_handler;
+		goto err_remove_busdev;
 	}
 
 	return 0;
 
+err_remove_busdev:
+	device_unregister(wmi_bus_dev);
+
 err_remove_handler:
 	acpi_remove_address_space_handler(device->handle,
 					  ACPI_ADR_SPACE_EC,
@@ -837,6 +920,22 @@ static int acpi_wmi_add(struct acpi_device *device)
 	return error;
 }
 
+int __must_check __wmi_driver_register(struct wmi_driver *driver,
+				       struct module *owner)
+{
+	driver->driver.owner = owner;
+	driver->driver.bus = &wmi_bus_type;
+
+	return driver_register(&driver->driver);
+}
+EXPORT_SYMBOL(__wmi_driver_register);
+
+void wmi_driver_unregister(struct wmi_driver *driver)
+{
+	driver_unregister(&driver->driver);
+}
+EXPORT_SYMBOL(wmi_driver_unregister);
+
 static int __init acpi_wmi_init(void)
 {
 	int error;
@@ -844,24 +943,36 @@ static int __init acpi_wmi_init(void)
 	if (acpi_disabled)
 		return -ENODEV;
 
-	error = class_register(&wmi_class);
+	error = class_register(&wmi_bus_class);
 	if (error)
 		return error;
 
+	error = bus_register(&wmi_bus_type);
+	if (error)
+		goto err_unreg_class;
+
 	error = acpi_bus_register_driver(&acpi_wmi_driver);
 	if (error) {
 		pr_err("Error loading mapper\n");
-		class_unregister(&wmi_class);
-		return error;
+		goto err_unreg_bus;
 	}
 
 	return 0;
+
+err_unreg_class:
+	class_unregister(&wmi_bus_class);
+
+err_unreg_bus:
+	bus_unregister(&wmi_bus_type);
+
+	return error;
 }
 
 static void __exit acpi_wmi_exit(void)
 {
 	acpi_bus_unregister_driver(&acpi_wmi_driver);
-	class_unregister(&wmi_class);
+	class_unregister(&wmi_bus_class);
+	bus_unregister(&wmi_bus_type);
 }
 
 subsys_initcall(acpi_wmi_init);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
new file mode 100644
index 0000000..29ed34b
--- /dev/null
+++ b/include/linux/wmi.h
@@ -0,0 +1,47 @@
+/*
+ * wmi.h - ACPI WMI interface
+ *
+ * Copyright (c) 2015 Andrew Lutomirski
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef _LINUX_WMI_H
+#define _LINUX_WMI_H
+
+#include <linux/device.h>
+#include <linux/acpi.h>
+
+struct wmi_device {
+	struct device dev;
+};
+
+struct wmi_device_id {
+	const char *guid_string;
+};
+
+struct wmi_driver {
+	struct device_driver driver;
+	const struct wmi_device_id *id_table;
+
+	int (*probe)(struct wmi_device *wdev);
+	int (*remove)(struct wmi_device *wdev);
+};
+
+extern int __must_check __wmi_driver_register(struct wmi_driver *driver,
+					      struct module *owner);
+extern void wmi_driver_unregister(struct wmi_driver *driver);
+#define wmi_driver_register(driver) __wmi_driver_register((driver), THIS_MODULE)
+
+#define module_wmi_driver(__wmi_driver) \
+	module_driver(__wmi_driver, wmi_driver_register, \
+		      wmi_driver_unregister)
+
+#endif
-- 
2.9.4

[PATCH 06/16] platform/x86: wmi: Fix error handling when creating devices

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

We have two memory leaks. If guid_already_parsed returned true, we leak
the wmi_block. If wmi_create_device failed, we leak the device.

Simplify the logic and fix both of them.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index f06b7c0..31c317f 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -757,6 +757,15 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 		if (debug_dump_wdg)
 			wmi_dump_wdg(&gblock[i]);
 
+		/*
+		 * Some WMI devices, like those for nVidia hooks, have a
+		 * duplicate GUID. It's not clear what we should do in this
+		 * case yet, so for now, we'll just ignore the duplicate
+		 * for device creation.
+		 */
+		if (guid_already_parsed(device, gblock[i].guid))
+			continue;
+
 		wblock = kzalloc(sizeof(struct wmi_block), GFP_KERNEL);
 		if (!wblock)
 			return -ENOMEM;
@@ -764,19 +773,12 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 		wblock->acpi_device = device;
 		wblock->gblock = gblock[i];
 
-		/*
-		  Some WMI devices, like those for nVidia hooks, have a
-		  duplicate GUID. It's not clear what we should do in this
-		  case yet, so for now, we'll just ignore the duplicate
-		  for device creation.
-		*/
-		if (!guid_already_parsed(device, gblock[i].guid)) {
-			retval = wmi_create_device(wmi_bus_dev, &gblock[i],
-						   wblock, device);
-			if (retval) {
-				wmi_free_devices(device);
-				goto out_free_pointer;
-			}
+		retval = wmi_create_device(wmi_bus_dev, &gblock[i],
+					   wblock, device);
+		if (retval) {
+			put_device(&wblock->dev.dev);
+			wmi_free_devices(device);
+			goto out_free_pointer;
 		}
 
 		list_add_tail(&wblock->list, &wmi_block_list);
-- 
2.9.4

[PATCH 07/16] platform/x86: wmi: Split devices into types and add basic sysfs attributes

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

Divide the "data", "method" and "event" types. All devices get
"instance_count" and "expensive" attributes, data and method devices get
"object_id" attributes, and event devices get "notify_id" attributes.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 78 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 31c317f..33a3609 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -575,13 +575,65 @@ static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(guid);
 
+static ssize_t instance_count_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	return sprintf(buf, "%d\n", (int)wblock->gblock.instance_count);
+}
+static DEVICE_ATTR_RO(instance_count);
+
+static ssize_t expensive_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	return sprintf(buf, "%d\n",
+		       (wblock->gblock.flags & ACPI_WMI_EXPENSIVE) != 0);
+}
+static DEVICE_ATTR_RO(expensive);
+
 static struct attribute *wmi_attrs[] = {
 	&dev_attr_modalias.attr,
 	&dev_attr_guid.attr,
+	&dev_attr_instance_count.attr,
+	&dev_attr_expensive.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(wmi);
 
+static ssize_t notify_id_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	return sprintf(buf, "%02X\n", (unsigned int)wblock->gblock.notify_id);
+}
+static DEVICE_ATTR_RO(notify_id);
+
+static struct attribute *wmi_event_attrs[] = {
+	&dev_attr_notify_id.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wmi_event);
+
+static ssize_t object_id_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	return sprintf(buf, "%c%c\n", wblock->gblock.object_id[0],
+		       wblock->gblock.object_id[1]);
+}
+static DEVICE_ATTR_RO(object_id);
+
+static struct attribute *wmi_data_or_method_attrs[] = {
+	&dev_attr_object_id.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wmi_data_or_method);
+
 static int wmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
@@ -671,6 +723,24 @@ static struct bus_type wmi_bus_type = {
 	.remove = wmi_dev_remove,
 };
 
+static struct device_type wmi_type_event = {
+	.name = "event",
+	.groups = wmi_event_groups,
+	.release = wmi_dev_release,
+};
+
+static struct device_type wmi_type_method = {
+	.name = "method",
+	.groups = wmi_data_or_method_groups,
+	.release = wmi_dev_release,
+};
+
+static struct device_type wmi_type_data = {
+	.name = "data",
+	.groups = wmi_data_or_method_groups,
+	.release = wmi_dev_release,
+};
+
 static int wmi_create_device(struct device *wmi_bus_dev,
 			     const struct guid_block *gblock,
 			     struct wmi_block *wblock,
@@ -681,7 +751,13 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 
 	dev_set_name(&wblock->dev.dev, "%pUL", gblock->guid);
 
-	wblock->dev.dev.release = wmi_dev_release;
+	if (gblock->flags & ACPI_WMI_EVENT) {
+		wblock->dev.dev.type = &wmi_type_event;
+	} else if (gblock->flags & ACPI_WMI_METHOD) {
+		wblock->dev.dev.type = &wmi_type_method;
+	} else {
+		wblock->dev.dev.type = &wmi_type_data;
+	}
 
 	return device_register(&wblock->dev.dev);
 }
-- 
2.9.4

[PATCH 08/16] platform/x86: wmi: Probe data objects for read and write capabilities

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

The Dell XPS 13 9350 has one RW data object, one RO data object, and one
totally inaccessible data object. Check for the existence of the
accessor methods and report in sysfs.

The docs also permit WQxx getters for single-instance objects to
take no parameters. Probe for that as well to avoid ACPICA warnings
about mismatched signatures.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 101 +++++++++++++++++++++++++++++++++++++++++++--
 include/linux/wmi.h        |   6 +++
 2 files changed, 103 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 33a3609..651693a 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -67,6 +67,8 @@ struct wmi_block {
 	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
+
+	bool read_takes_no_args;	/* only defined if readable */
 };
 
 
@@ -138,6 +140,30 @@ static bool find_guid(const char *guid_string, struct wmi_block **out)
 	return false;
 }
 
+static int get_subobj_info(acpi_handle handle, const char *pathname,
+			   struct acpi_device_info **info)
+{
+	struct acpi_device_info *dummy_info, **info_ptr;
+	acpi_handle subobj_handle;
+	acpi_status status;
+
+	status = acpi_get_handle(handle, (char *)pathname, &subobj_handle);
+	if (status == AE_NOT_FOUND)
+		return -ENOENT;
+	else if (ACPI_FAILURE(status))
+		return -EIO;
+
+	info_ptr = info ? info : &dummy_info;
+	status = acpi_get_object_info(subobj_handle, info_ptr);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	if (!info)
+		kfree(dummy_info);
+
+	return 0;
+}
+
 static acpi_status wmi_method_enable(struct wmi_block *wblock, int enable)
 {
 	struct guid_block *block = NULL;
@@ -261,6 +287,9 @@ struct acpi_buffer *out)
 	wq_params[0].type = ACPI_TYPE_INTEGER;
 	wq_params[0].integer.value = instance;
 
+	if (instance == 0 && wblock->read_takes_no_args)
+		input.count = 0;
+
 	/*
 	 * If ACPI_WMI_EXPENSIVE, call the relevant WCxx method first to
 	 * enable collection.
@@ -628,11 +657,37 @@ static ssize_t object_id_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(object_id);
 
-static struct attribute *wmi_data_or_method_attrs[] = {
+static ssize_t readable_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct wmi_device *wdev = dev_to_wdev(dev);
+
+	return sprintf(buf, "%d\n", (int)wdev->readable);
+}
+static DEVICE_ATTR_RO(readable);
+
+static ssize_t writeable_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct wmi_device *wdev = dev_to_wdev(dev);
+
+	return sprintf(buf, "%d\n", (int)wdev->writeable);
+}
+static DEVICE_ATTR_RO(writeable);
+
+static struct attribute *wmi_data_attrs[] = {
+	&dev_attr_object_id.attr,
+	&dev_attr_readable.attr,
+	&dev_attr_writeable.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wmi_data);
+
+static struct attribute *wmi_method_attrs[] = {
 	&dev_attr_object_id.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(wmi_data_or_method);
+ATTRIBUTE_GROUPS(wmi_method);
 
 static int wmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
@@ -731,13 +786,13 @@ static struct device_type wmi_type_event = {
 
 static struct device_type wmi_type_method = {
 	.name = "method",
-	.groups = wmi_data_or_method_groups,
+	.groups = wmi_method_groups,
 	.release = wmi_dev_release,
 };
 
 static struct device_type wmi_type_data = {
 	.name = "data",
-	.groups = wmi_data_or_method_groups,
+	.groups = wmi_data_groups,
 	.release = wmi_dev_release,
 };
 
@@ -756,7 +811,45 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 	} else if (gblock->flags & ACPI_WMI_METHOD) {
 		wblock->dev.dev.type = &wmi_type_method;
 	} else {
+		struct acpi_device_info *info;
+		char method[5];
+		int result;
+
 		wblock->dev.dev.type = &wmi_type_data;
+
+		strcpy(method, "WQ");
+		strncat(method, wblock->gblock.object_id, 2);
+		result = get_subobj_info(device->handle, method, &info);
+
+		if (result == 0) {
+			wblock->dev.readable = true;
+
+			/*
+			 * The Microsoft documentation specifically states:
+			 *
+			 *   Data blocks registered with only a single instance
+			 *   can ignore the parameter.
+			 *
+			 * ACPICA will get mad at us if we call the method
+			 * with the wrong number of arguments, so check what
+			 * our method expects.  (On some Dell laptops, WQxx
+			 * may not be a method at all.)
+			 */
+			if (info->type != ACPI_TYPE_METHOD ||
+			    info->param_count == 0)
+				wblock->read_takes_no_args = true;
+
+			kfree(info);
+		}
+
+		strcpy(method, "WS");
+		strncat(method, wblock->gblock.object_id, 2);
+		result = get_subobj_info(device->handle, method, NULL);
+
+		if (result == 0) {
+			wblock->dev.writeable = true;
+		}
+
 	}
 
 	return device_register(&wblock->dev.dev);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 29ed34b..5309500 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -21,6 +21,12 @@
 
 struct wmi_device {
 	struct device dev;
+
+	/*
+	 * These are true for data objects that support reads and writes,
+	 * respectively.
+	 */
+	bool readable, writeable;
 };
 
 struct wmi_device_id {
-- 
2.9.4

[PATCH 09/16] platform/x86: wmi: Instantiate all devices before adding them

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

At some point, we will want sub-drivers to get references to other
devices on the same WMI bus. This change is needed to avoid races.

This ends up simplifying the setup code and fixing some leaks, too.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 49 +++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 651693a..bfc0a3f 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -796,7 +796,7 @@ static struct device_type wmi_type_data = {
 	.release = wmi_dev_release,
 };
 
-static int wmi_create_device(struct device *wmi_bus_dev,
+static void wmi_create_device(struct device *wmi_bus_dev,
 			     const struct guid_block *gblock,
 			     struct wmi_block *wblock,
 			     struct acpi_device *device)
@@ -852,7 +852,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 
 	}
 
-	return device_register(&wblock->dev.dev);
+	device_initialize(&wblock->dev.dev);
 }
 
 static void wmi_free_devices(struct acpi_device *device)
@@ -863,10 +863,14 @@ static void wmi_free_devices(struct acpi_device *device)
 	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
 		if (wblock->acpi_device == device) {
 			list_del(&wblock->list);
-			if (wblock->dev.dev.bus)
-				device_unregister(&wblock->dev.dev);
-			else
+			if (wblock->dev.dev.bus) {
+				/* Device was initialized. */
+				device_del(&wblock->dev.dev);
+				put_device(&wblock->dev.dev);
+			} else {
+				/* device_initialize was not called. */
 				kfree(wblock);
+			}
 		}
 	}
 }
@@ -901,9 +905,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
 	union acpi_object *obj;
 	const struct guid_block *gblock;
-	struct wmi_block *wblock;
+	struct wmi_block *wblock, *next;
 	acpi_status status;
-	int retval;
+	int retval = 0;
 	u32 i, total;
 
 	status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
@@ -936,19 +940,15 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 			continue;
 
 		wblock = kzalloc(sizeof(struct wmi_block), GFP_KERNEL);
-		if (!wblock)
-			return -ENOMEM;
+		if (!wblock) {
+			retval = -ENOMEM;
+			break;
+		}
 
 		wblock->acpi_device = device;
 		wblock->gblock = gblock[i];
 
-		retval = wmi_create_device(wmi_bus_dev, &gblock[i],
-					   wblock, device);
-		if (retval) {
-			put_device(&wblock->dev.dev);
-			wmi_free_devices(device);
-			goto out_free_pointer;
-		}
+		wmi_create_device(wmi_bus_dev, &gblock[i], wblock, device);
 
 		list_add_tail(&wblock->list, &wmi_block_list);
 
@@ -958,11 +958,24 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 		}
 	}
 
-	retval = 0;
-
 out_free_pointer:
 	kfree(out.pointer);
 
+	/*
+	 * Now that all of the devices are created, add them to the
+	 * device tree and probe subdrivers.
+	 */
+	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
+		if (wblock->acpi_device == device) {
+			if (device_add(&wblock->dev.dev) != 0) {
+				dev_err(wmi_bus_dev,
+					"failed to register %pULL\n",
+					wblock->gblock.guid);
+				list_del(&wblock->list);
+			}
+		}
+	}
+
 	return retval;
 }
 
-- 
2.9.4

Re: [PATCH 09/16] platform/x86: wmi: Instantiate all devices before adding them

[Michał Kępień]

I know I have probably started sounding like a broken record by now, but
I still have not seen any response (apart from the typos getting fixed)
to my comments on this patch which I posted in January 2016 [1].

None of the issues I found back then are really critical, but I did
point out a potential memory leak (granted, an unlikely one), so it
might be a good idea to at least take a second look before merging.

[1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html

-- 
Best regards,
Michał Kępień

Re: [PATCH 09/16] platform/x86: wmi: Instantiate all devices before adding them

[Darren Hart]

On Thu, Jun 01, 2017 at 10:43:39PM +0200, Michał Kępień wrote:
> I know I have probably started sounding like a broken record by now, but
> I still have not seen any response (apart from the typos getting fixed)
> to my comments on this patch which I posted in January 2016 [1].
> 
> None of the issues I found back then are really critical, but I did
> point out a potential memory leak (granted, an unlikely one), so it
> might be a good idea to at least take a second look before merging.
> 
> [1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html

Thanks for being persistent, some good points in there. I'd like to just squash
these into this patch (9/16), but I'll include them here for an ack from you and
Andy L. that this is what you meant, and consistent with his
intent/understanding:


>From 2512da1593574a66eb48d7105885e959b38db410 Mon Sep 17 00:00:00 2001
Message-Id: <2512da1593574a66eb48d7105885e959b38db410.1496717988.git.dvhart@infradead.org>
From: "Darren Hart (VMware)" <dvhart@infradead.org>
Date: Mon, 5 Jun 2017 19:54:03 -0700
Subject: [PATCH] =?UTF-8?q?platform/x86:=20wmi:=20Apply=20fixes=20per=20Mi?=
 =?UTF-8?q?cha=C5=82=20K=C4=99pie=C5=84=20(squash)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Per:
https://www.spinics.net/lists/platform-driver-x86/msg08201.html

Eliminate the kfree of a wblock with a null bus now that we ignore
duplicate GUIDs in parse_wdg.

Move the out_free_pointer: label and kfree to the end of the function,
clearly marking it as a return path.

Rework the device_add loop at the end of parse_wdg to avoid leaking the
wblock, and symmetrically call wmi_method_enable() if (debug_event).

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index bfc0a3f..fbce876 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -863,14 +863,7 @@ static void wmi_free_devices(struct acpi_device *device)
 	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
 		if (wblock->acpi_device == device) {
 			list_del(&wblock->list);
-			if (wblock->dev.dev.bus) {
-				/* Device was initialized. */
-				device_del(&wblock->dev.dev);
-				put_device(&wblock->dev.dev);
-			} else {
-				/* device_initialize was not called. */
-				kfree(wblock);
-			}
+			device_unregister(&wblock->dev.dev);
 		}
 	}
 }
@@ -903,9 +896,9 @@ static bool guid_already_parsed(struct acpi_device *device,
 static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 {
 	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
-	union acpi_object *obj;
 	const struct guid_block *gblock;
 	struct wmi_block *wblock, *next;
+	union acpi_object *obj;
 	acpi_status status;
 	int retval = 0;
 	u32 i, total;
@@ -958,24 +951,27 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 		}
 	}
 
-out_free_pointer:
-	kfree(out.pointer);
-
 	/*
 	 * Now that all of the devices are created, add them to the
 	 * device tree and probe subdrivers.
 	 */
 	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
-		if (wblock->acpi_device == device) {
-			if (device_add(&wblock->dev.dev) != 0) {
-				dev_err(wmi_bus_dev,
-					"failed to register %pULL\n",
-					wblock->gblock.guid);
-				list_del(&wblock->list);
-			}
+		if (wblock->acpi_device != device)
+			continue;
+
+		retval = device_add(&wblock->dev.dev);
+		if (retval) {
+			dev_err(wmi_bus_dev, "failed to register %pULL\n",
+				wblock->gblock.guid);
+			if (debug_event)
+				wmi_method_enable(wblock, 0);
+			list_del(&wblock->list);
+			put_device(&wblock->dev.dev);
 		}
 	}
 
+out_free_pointer:
+	kfree(out.pointer);
 	return retval;
 }
 
-- 
2.9.4


-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 09/16] platform/x86: wmi: Instantiate all devices before adding them

[Andy Lutomirski]

On Mon, Jun 5, 2017 at 8:03 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Thu, Jun 01, 2017 at 10:43:39PM +0200, Michał Kępień wrote:
>> I know I have probably started sounding like a broken record by now, but
>> I still have not seen any response (apart from the typos getting fixed)
>> to my comments on this patch which I posted in January 2016 [1].
>>
>> None of the issues I found back then are really critical, but I did
>> point out a potential memory leak (granted, an unlikely one), so it
>> might be a good idea to at least take a second look before merging.
>>
>> [1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html
>
> Thanks for being persistent, some good points in there. I'd like to just squash
> these into this patch (9/16), but I'll include them here for an ack from you and
> Andy L. that this is what you meant, and consistent with his
> intent/understanding:
>

Looks good to me.

Re: [PATCH 09/16] platform/x86: wmi: Instantiate all devices before adding them

[Michał Kępień]

> On Mon, Jun 5, 2017 at 8:03 PM, Darren Hart <dvhart@infradead.org> wrote:
> > On Thu, Jun 01, 2017 at 10:43:39PM +0200, Michał Kępień wrote:
> >> I know I have probably started sounding like a broken record by now, but
> >> I still have not seen any response (apart from the typos getting fixed)
> >> to my comments on this patch which I posted in January 2016 [1].
> >>
> >> None of the issues I found back then are really critical, but I did
> >> point out a potential memory leak (granted, an unlikely one), so it
> >> might be a good idea to at least take a second look before merging.
> >>
> >> [1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html
> >
> > Thanks for being persistent, some good points in there. I'd like to just squash
> > these into this patch (9/16), but I'll include them here for an ack from you and
> > Andy L. that this is what you meant, and consistent with his
> > intent/understanding:
> >
> 
> Looks good to me.

Same here.

-- 
Best regards,
Michał Kępień

[PATCH 10/16] platform/x86: wmi: Incorporate acpi_install_notify_handler

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

As a platform driver, acpi_driver.notify will not be available,
so use acpi_install_notify_handler as we will be converting to a
platform driver.

This gives event drivers a simple way to handle events. It
also seems closer to what the Windows docs suggest that Windows
does: it sounds like, in Windows, the mapper is responsible for
called _WED before dispatching to the subdriver.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
[dvhart: merge two development commits and update commit message]
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 89 +++++++++++++++++++++++++++++++++++++---------
 include/linux/wmi.h        |  1 +
 2 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index bfc0a3f..208e187 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -93,7 +93,6 @@ MODULE_PARM_DESC(debug_dump_wdg,
 
 static int acpi_wmi_remove(struct acpi_device *device);
 static int acpi_wmi_add(struct acpi_device *device);
-static void acpi_wmi_notify(struct acpi_device *device, u32 event);
 
 static const struct acpi_device_id wmi_device_ids[] = {
 	{"PNP0C14", 0},
@@ -109,7 +108,6 @@ static struct acpi_driver acpi_wmi_driver = {
 	.ops = {
 		.add = acpi_wmi_add,
 		.remove = acpi_wmi_remove,
-		.notify = acpi_wmi_notify,
 	},
 };
 
@@ -1023,36 +1021,80 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
 	}
 }
 
-static void acpi_wmi_notify(struct acpi_device *device, u32 event)
+static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
+				    void *context)
 {
 	struct guid_block *block;
 	struct wmi_block *wblock;
 	struct list_head *p;
+	bool found_it = false;
 
 	list_for_each(p, &wmi_block_list) {
 		wblock = list_entry(p, struct wmi_block, list);
 		block = &wblock->gblock;
 
-		if (wblock->acpi_device == device &&
+		if (wblock->acpi_device->handle == handle &&
 		    (block->flags & ACPI_WMI_EVENT) &&
-		    (block->notify_id == event)) {
-			if (wblock->handler)
-				wblock->handler(event, wblock->handler_data);
-			if (debug_event) {
-				pr_info("DEBUG Event GUID: %pUL\n",
-					wblock->gblock.guid);
-			}
-
-			acpi_bus_generate_netlink_event(
-				device->pnp.device_class, dev_name(&device->dev),
-				event, 0);
+		    (block->notify_id == event))
+		{
+			found_it = true;
 			break;
 		}
 	}
+
+	if (!found_it)
+		return;
+
+	/* If a driver is bound, then notify the driver. */
+	if (wblock->dev.dev.driver) {
+		struct wmi_driver *driver;
+		struct acpi_object_list input;
+		union acpi_object params[1];
+		struct acpi_buffer evdata = { ACPI_ALLOCATE_BUFFER, NULL };
+		acpi_status status;
+
+		driver = container_of(wblock->dev.dev.driver,
+				      struct wmi_driver, driver);
+
+		input.count = 1;
+		input.pointer = params;
+		params[0].type = ACPI_TYPE_INTEGER;
+		params[0].integer.value = event;
+
+		status = acpi_evaluate_object(wblock->acpi_device->handle,
+					      "_WED", &input, &evdata);
+		if (ACPI_FAILURE(status)) {
+			dev_warn(&wblock->dev.dev,
+				 "failed to get event data\n");
+			return;
+		}
+
+		if (driver->notify)
+			driver->notify(&wblock->dev,
+				       (union acpi_object *)evdata.pointer);
+
+		kfree(evdata.pointer);
+	} else if (wblock->handler) {
+		/* Legacy handler */
+		wblock->handler(event, wblock->handler_data);
+	}
+
+	if (debug_event) {
+		pr_info("DEBUG Event GUID: %pUL\n",
+			wblock->gblock.guid);
+	}
+
+	acpi_bus_generate_netlink_event(
+		wblock->acpi_device->pnp.device_class,
+		dev_name(&wblock->dev.dev),
+		event, 0);
+
 }
 
 static int acpi_wmi_remove(struct acpi_device *device)
 {
+	acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+				   acpi_wmi_notify_handler);
 	acpi_remove_address_space_handler(device->handle,
 				ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
 	wmi_free_devices(device);
@@ -1077,11 +1119,20 @@ static int acpi_wmi_add(struct acpi_device *device)
 		return -ENODEV;
 	}
 
+	status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+					     acpi_wmi_notify_handler,
+					     NULL);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&device->dev, "Error installing notify handler\n");
+		error = -ENODEV;
+		goto err_remove_ec_handler;
+	}
+
 	wmi_bus_dev = device_create(&wmi_bus_class, &device->dev, MKDEV(0, 0),
 				    NULL, "wmi_bus-%s", dev_name(&device->dev));
 	if (IS_ERR(wmi_bus_dev)) {
 		error = PTR_ERR(wmi_bus_dev);
-		goto err_remove_handler;
+		goto err_remove_notify_handler;
 	}
 	device->driver_data = wmi_bus_dev;
 
@@ -1096,7 +1147,11 @@ static int acpi_wmi_add(struct acpi_device *device)
 err_remove_busdev:
 	device_unregister(wmi_bus_dev);
 
-err_remove_handler:
+err_remove_notify_handler:
+	acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+				   acpi_wmi_notify_handler);
+
+err_remove_ec_handler:
 	acpi_remove_address_space_handler(device->handle,
 					  ACPI_ADR_SPACE_EC,
 					  &acpi_wmi_ec_space_handler);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 5309500..c6eedfd 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -39,6 +39,7 @@ struct wmi_driver {
 
 	int (*probe)(struct wmi_device *wdev);
 	int (*remove)(struct wmi_device *wdev);
+	void (*notify)(struct wmi_device *device, union acpi_object *data);
 };
 
 extern int __must_check __wmi_driver_register(struct wmi_driver *driver,
-- 
2.9.4

[PATCH 11/16] platform/x86: wmi: Add a new interface to read block data

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

wmi_query_block is unnecessarily indirect. Add a straightforward
method for wmi bus drivers to use to read block data.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 54 ++++++++++++++++++++++++++++++++--------------
 include/linux/wmi.h        |  4 ++++
 2 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 208e187..483e4a6 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -244,19 +244,10 @@ u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
 }
 EXPORT_SYMBOL_GPL(wmi_evaluate_method);
 
-/**
- * wmi_query_block - Return contents of a WMI block
- * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
- * @instance: Instance index
- * &out: Empty buffer to return the contents of the data block to
- *
- * Return the contents of an ACPI-WMI data block to a buffer
- */
-acpi_status wmi_query_block(const char *guid_string, u8 instance,
-struct acpi_buffer *out)
+static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
+				 struct acpi_buffer *out)
 {
 	struct guid_block *block = NULL;
-	struct wmi_block *wblock = NULL;
 	acpi_handle handle;
 	acpi_status status, wc_status = AE_ERROR;
 	struct acpi_object_list input;
@@ -264,12 +255,9 @@ struct acpi_buffer *out)
 	char method[5];
 	char wc_method[5] = "WC";
 
-	if (!guid_string || !out)
+	if (!out)
 		return AE_BAD_PARAMETER;
 
-	if (!find_guid(guid_string, &wblock))
-		return AE_ERROR;
-
 	block = &wblock->gblock;
 	handle = wblock->acpi_device->handle;
 
@@ -320,8 +308,42 @@ struct acpi_buffer *out)
 
 	return status;
 }
+
+/**
+ * wmi_query_block - Return contents of a WMI block (deprecated)
+ * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
+ * @instance: Instance index
+ * &out: Empty buffer to return the contents of the data block to
+ *
+ * Return the contents of an ACPI-WMI data block to a buffer
+ */
+acpi_status wmi_query_block(const char *guid_string, u8 instance,
+			    struct acpi_buffer *out)
+{
+	struct wmi_block *wblock;
+
+	if (!guid_string)
+		return AE_BAD_PARAMETER;
+
+	if (!find_guid(guid_string, &wblock))
+		return AE_ERROR;
+
+	return __query_block(wblock, instance, out);
+}
 EXPORT_SYMBOL_GPL(wmi_query_block);
 
+union acpi_object *wmidev_block_query(struct wmi_device *wdev, u8 instance)
+{
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct wmi_block *wblock = container_of(wdev, struct wmi_block, dev);
+
+	if (ACPI_FAILURE(__query_block(wblock, instance, &out)))
+		return NULL;
+
+	return (union acpi_object *)out.pointer;
+}
+EXPORT_SYMBOL_GPL(wmidev_block_query);
+
 /**
  * wmi_set_block - Write to a WMI block
  * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
@@ -331,7 +353,7 @@ EXPORT_SYMBOL_GPL(wmi_query_block);
  * Write the contents of the input buffer to an ACPI-WMI data block
  */
 acpi_status wmi_set_block(const char *guid_string, u8 instance,
-const struct acpi_buffer *in)
+			  const struct acpi_buffer *in)
 {
 	struct guid_block *block = NULL;
 	struct wmi_block *wblock = NULL;
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index c6eedfd..0ab2540 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -29,6 +29,10 @@ struct wmi_device {
 	bool readable, writeable;
 };
 
+/* Caller must kfree the result. */
+extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
+					     u8 instance);
+
 struct wmi_device_id {
 	const char *guid_string;
 };
-- 
2.9.4

[PATCH 12/16] platform/x86: wmi: Bind the platform device, not the ACPI node

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

We already have the PNP glue to instantiate platform devices for the
ACPI devices that WMI drives. WMI should therefore attach to the
platform device, not the ACPI node.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 57 +++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 483e4a6..f1c9464 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -37,6 +37,7 @@
 #include <linux/acpi.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
 #include <linux/wmi.h>
 #include <linux/uuid.h>
 
@@ -91,8 +92,8 @@ module_param(debug_dump_wdg, bool, 0444);
 MODULE_PARM_DESC(debug_dump_wdg,
 		 "Dump available WMI interfaces [0/1]");
 
-static int acpi_wmi_remove(struct acpi_device *device);
-static int acpi_wmi_add(struct acpi_device *device);
+static int acpi_wmi_remove(struct platform_device *device);
+static int acpi_wmi_probe(struct platform_device *device);
 
 static const struct acpi_device_id wmi_device_ids[] = {
 	{"PNP0C14", 0},
@@ -101,14 +102,13 @@ static const struct acpi_device_id wmi_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
 
-static struct acpi_driver acpi_wmi_driver = {
-	.name = "acpi-wmi",
-	.owner = THIS_MODULE,
-	.ids = wmi_device_ids,
-	.ops = {
-		.add = acpi_wmi_add,
-		.remove = acpi_wmi_remove,
+static struct platform_driver acpi_wmi_driver = {
+	.driver = {
+		.name = "acpi-wmi",
+		.acpi_match_table = wmi_device_ids,
 	},
+	.probe = acpi_wmi_probe,
+	.remove = acpi_wmi_remove,
 };
 
 /*
@@ -1113,26 +1113,34 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
 
 }
 
-static int acpi_wmi_remove(struct acpi_device *device)
+static int acpi_wmi_remove(struct platform_device *device)
 {
-	acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+	struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
+
+	acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
 				   acpi_wmi_notify_handler);
-	acpi_remove_address_space_handler(device->handle,
+	acpi_remove_address_space_handler(acpi_device->handle,
 				ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
-	wmi_free_devices(device);
-	device_unregister((struct device *)acpi_driver_data(device));
-	device->driver_data = NULL;
+	wmi_free_devices(acpi_device);
+	device_unregister((struct device *)dev_get_drvdata(&device->dev));
 
 	return 0;
 }
 
-static int acpi_wmi_add(struct acpi_device *device)
+static int acpi_wmi_probe(struct platform_device *device)
 {
+	struct acpi_device *acpi_device;
 	struct device *wmi_bus_dev;
 	acpi_status status;
 	int error;
 
-	status = acpi_install_address_space_handler(device->handle,
+	acpi_device = ACPI_COMPANION(&device->dev);
+	if (!acpi_device) {
+		dev_err(&device->dev, "ACPI companion is missing\n");
+		return -ENODEV;
+	}
+
+	status = acpi_install_address_space_handler(acpi_device->handle,
 						    ACPI_ADR_SPACE_EC,
 						    &acpi_wmi_ec_space_handler,
 						    NULL, NULL);
@@ -1141,7 +1149,8 @@ static int acpi_wmi_add(struct acpi_device *device)
 		return -ENODEV;
 	}
 
-	status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+	status = acpi_install_notify_handler(acpi_device->handle,
+					     ACPI_DEVICE_NOTIFY,
 					     acpi_wmi_notify_handler,
 					     NULL);
 	if (ACPI_FAILURE(status)) {
@@ -1156,9 +1165,9 @@ static int acpi_wmi_add(struct acpi_device *device)
 		error = PTR_ERR(wmi_bus_dev);
 		goto err_remove_notify_handler;
 	}
-	device->driver_data = wmi_bus_dev;
+	dev_set_drvdata(&device->dev, wmi_bus_dev);
 
-	error = parse_wdg(wmi_bus_dev, device);
+	error = parse_wdg(wmi_bus_dev, acpi_device);
 	if (error) {
 		pr_err("Failed to parse WDG method\n");
 		goto err_remove_busdev;
@@ -1170,11 +1179,11 @@ static int acpi_wmi_add(struct acpi_device *device)
 	device_unregister(wmi_bus_dev);
 
 err_remove_notify_handler:
-	acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+	acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
 				   acpi_wmi_notify_handler);
 
 err_remove_ec_handler:
-	acpi_remove_address_space_handler(device->handle,
+	acpi_remove_address_space_handler(acpi_device->handle,
 					  ACPI_ADR_SPACE_EC,
 					  &acpi_wmi_ec_space_handler);
 
@@ -1212,7 +1221,7 @@ static int __init acpi_wmi_init(void)
 	if (error)
 		goto err_unreg_class;
 
-	error = acpi_bus_register_driver(&acpi_wmi_driver);
+	error = platform_driver_register(&acpi_wmi_driver);
 	if (error) {
 		pr_err("Error loading mapper\n");
 		goto err_unreg_bus;
@@ -1231,7 +1240,7 @@ static int __init acpi_wmi_init(void)
 
 static void __exit acpi_wmi_exit(void)
 {
-	acpi_bus_unregister_driver(&acpi_wmi_driver);
+	platform_driver_unregister(&acpi_wmi_driver);
 	class_unregister(&wmi_bus_class);
 	bus_unregister(&wmi_bus_type);
 }
-- 
2.9.4

[PATCH 13/16] platform/x86: wmi: Add an interface for subdrivers to access sibling devices

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

Some subdrivers need to access sibling devices. This gives them a
clean way to do so.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 17 +++++++++++++++++
 include/linux/wmi.h        |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index f1c9464..8dd9988 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -344,6 +344,23 @@ union acpi_object *wmidev_block_query(struct wmi_device *wdev, u8 instance)
 }
 EXPORT_SYMBOL_GPL(wmidev_block_query);
 
+struct wmi_device *wmidev_get_other_guid(struct wmi_device *wdev,
+					 const char *guid_string)
+{
+	struct wmi_block *this_wb = container_of(wdev, struct wmi_block, dev);
+	struct wmi_block *other_wb;
+
+	if (!find_guid(guid_string, &other_wb))
+		return NULL;
+
+	if (other_wb->acpi_device != this_wb->acpi_device)
+		return NULL;
+
+	get_device(&other_wb->dev.dev);
+	return &other_wb->dev;
+}
+EXPORT_SYMBOL_GPL(wmidev_get_other_guid);
+
 /**
  * wmi_set_block - Write to a WMI block
  * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 0ab2540..a283768 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -33,6 +33,10 @@ struct wmi_device {
 extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
 					     u8 instance);
 
+/* Gets another device on the same bus.  Caller must put_device the result. */
+extern struct wmi_device *wmidev_get_other_guid(struct wmi_device *wdev,
+						const char *guid_string);
+
 struct wmi_device_id {
 	const char *guid_string;
 };
-- 
2.9.4

[PATCH 14/16] platform/x86: wmi: Require query for data blocks, rename writable to setable

[Darren Hart]

From: "Darren Hart (VMware)" <dvhart@infradead.org>

The Microsoft WMI documentation requires all data blocks to implement
the Query Control Method (WQxx). If we encounter a data block not
implementing this control method, issue a warning, and ignore the data
block. Remove the "readable" attribute as all data blocks must be
readable (query-able).

Be consistent with the language in the documentation, replace the
"writable" attribute with "setable".

Simplify (flatten) the control flow of wmi_create_device a bit while
we are updating it for the above changes.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
---
 drivers/platform/x86/wmi.c | 117 +++++++++++++++++++++++----------------------
 include/linux/wmi.h        |   7 +--
 2 files changed, 63 insertions(+), 61 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 8dd9988..cc55952 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -69,7 +69,7 @@ struct wmi_block {
 	wmi_notify_handler handler;
 	void *handler_data;
 
-	bool read_takes_no_args;	/* only defined if readable */
+	bool read_takes_no_args;
 };
 
 
@@ -694,28 +694,18 @@ static ssize_t object_id_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(object_id);
 
-static ssize_t readable_show(struct device *dev, struct device_attribute *attr,
-			     char *buf)
+static ssize_t setable_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
 {
 	struct wmi_device *wdev = dev_to_wdev(dev);
 
-	return sprintf(buf, "%d\n", (int)wdev->readable);
+	return sprintf(buf, "%d\n", (int)wdev->setable);
 }
-static DEVICE_ATTR_RO(readable);
-
-static ssize_t writeable_show(struct device *dev, struct device_attribute *attr,
-			      char *buf)
-{
-	struct wmi_device *wdev = dev_to_wdev(dev);
-
-	return sprintf(buf, "%d\n", (int)wdev->writeable);
-}
-static DEVICE_ATTR_RO(writeable);
+static DEVICE_ATTR_RO(setable);
 
 static struct attribute *wmi_data_attrs[] = {
 	&dev_attr_object_id.attr,
-	&dev_attr_readable.attr,
-	&dev_attr_writeable.attr,
+	&dev_attr_setable.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(wmi_data);
@@ -833,63 +823,74 @@ static struct device_type wmi_type_data = {
 	.release = wmi_dev_release,
 };
 
-static void wmi_create_device(struct device *wmi_bus_dev,
+static int wmi_create_device(struct device *wmi_bus_dev,
 			     const struct guid_block *gblock,
 			     struct wmi_block *wblock,
 			     struct acpi_device *device)
 {
-	wblock->dev.dev.bus = &wmi_bus_type;
-	wblock->dev.dev.parent = wmi_bus_dev;
-
-	dev_set_name(&wblock->dev.dev, "%pUL", gblock->guid);
+	struct acpi_device_info *info;
+	char method[5];
+	int result;
 
 	if (gblock->flags & ACPI_WMI_EVENT) {
 		wblock->dev.dev.type = &wmi_type_event;
-	} else if (gblock->flags & ACPI_WMI_METHOD) {
+		goto out_init;
+	}
+
+	if (gblock->flags & ACPI_WMI_METHOD) {
 		wblock->dev.dev.type = &wmi_type_method;
-	} else {
-		struct acpi_device_info *info;
-		char method[5];
-		int result;
+		goto out_init;
+	}
 
-		wblock->dev.dev.type = &wmi_type_data;
+	/*
+	 * Data Block Query Control Method (WQxx by convention) is
+	 * required per the WMI documentation. If it is not present,
+	 * we ignore this data block.
+	 */
+	strcpy(method, "WQ");
+	strncat(method, wblock->gblock.object_id, 2);
+	result = get_subobj_info(device->handle, method, &info);
+
+	if (result) {
+		dev_warn(wmi_bus_dev,
+			 "%s data block query control method not found",
+			 method);
+		return result;
+	}
 
-		strcpy(method, "WQ");
-		strncat(method, wblock->gblock.object_id, 2);
-		result = get_subobj_info(device->handle, method, &info);
+	wblock->dev.dev.type = &wmi_type_data;
 
-		if (result == 0) {
-			wblock->dev.readable = true;
+	/*
+	 * The Microsoft documentation specifically states:
+	 *
+	 *   Data blocks registered with only a single instance
+	 *   can ignore the parameter.
+	 *
+	 * ACPICA will get mad at us if we call the method with the wrong number
+	 * of arguments, so check what our method expects.  (On some Dell
+	 * laptops, WQxx may not be a method at all.)
+	 */
+	if (info->type != ACPI_TYPE_METHOD || info->param_count == 0)
+		wblock->read_takes_no_args = true;
 
-			/*
-			 * The Microsoft documentation specifically states:
-			 *
-			 *   Data blocks registered with only a single instance
-			 *   can ignore the parameter.
-			 *
-			 * ACPICA will get mad at us if we call the method
-			 * with the wrong number of arguments, so check what
-			 * our method expects.  (On some Dell laptops, WQxx
-			 * may not be a method at all.)
-			 */
-			if (info->type != ACPI_TYPE_METHOD ||
-			    info->param_count == 0)
-				wblock->read_takes_no_args = true;
+	kfree(info);
 
-			kfree(info);
-		}
+	strcpy(method, "WS");
+	strncat(method, wblock->gblock.object_id, 2);
+	result = get_subobj_info(device->handle, method, NULL);
 
-		strcpy(method, "WS");
-		strncat(method, wblock->gblock.object_id, 2);
-		result = get_subobj_info(device->handle, method, NULL);
+	if (result == 0)
+		wblock->dev.setable = true;
 
-		if (result == 0) {
-			wblock->dev.writeable = true;
-		}
+ out_init:
+	wblock->dev.dev.bus = &wmi_bus_type;
+	wblock->dev.dev.parent = wmi_bus_dev;
 
-	}
+	dev_set_name(&wblock->dev.dev, "%pUL", gblock->guid);
 
 	device_initialize(&wblock->dev.dev);
+
+	return 0;
 }
 
 static void wmi_free_devices(struct acpi_device *device)
@@ -985,7 +986,11 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 		wblock->acpi_device = device;
 		wblock->gblock = gblock[i];
 
-		wmi_create_device(wmi_bus_dev, &gblock[i], wblock, device);
+		retval = wmi_create_device(wmi_bus_dev, &gblock[i], wblock, device);
+		if (retval) {
+			kfree(wblock);
+			continue;
+		}
 
 		list_add_tail(&wblock->list, &wmi_block_list);
 
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index a283768..cd0d773 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -22,11 +22,8 @@
 struct wmi_device {
 	struct device dev;
 
-	/*
-	 * These are true for data objects that support reads and writes,
-	 * respectively.
-	 */
-	bool readable, writeable;
+	 /* True for data blocks implementing the Set Control Method */
+	bool setable;
 };
 
 /* Caller must kfree the result. */
-- 
2.9.4

[PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

Quite a few laptops (and maybe servers?) have embedded WMI MOF
metadata. I think that Samba has tools to interpret it, but there is
currently no interface to get the data in the first place.

In most cases, the MOF can be read out of the DSDT, but that is
non-compliant and messy.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
[dvhart: make sysfs mof binary read only, fixup comment block format]
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/Kconfig   |  12 ++++
 drivers/platform/x86/Makefile  |   1 +
 drivers/platform/x86/wmi-mof.c | 125 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/platform/x86/wmi-mof.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 49a1d01..1f27600 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -656,6 +656,18 @@ config ACPI_WMI
 	  It is safe to enable this driver even if your DSDT doesn't define
 	  any ACPI-WMI devices.
 
+config WMI_MOF
+	tristate "WMI embedded MOF driver"
+	depends on ACPI_WMI
+	default y
+	---help---
+	  Say Y here if you want to be able to read a firmware-embedded
+	  WMI MOF schema.  Using this requires userspace tools and may be
+	  rather tedious.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called wmi-mof.
+
 config MSI_WMI
 	tristate "MSI WMI extras"
 	depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 652d7c8..1147212 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_MSI_WMI)		+= msi-wmi.o
 obj-$(CONFIG_PEAQ_WMI)		+= peaq-wmi.o
 obj-$(CONFIG_SURFACE3_WMI)	+= surface3-wmi.o
 obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
+obj-$(CONFIG_WMI_MOF)		+= wmi-mof.o
 
 # toshiba_acpi must link after wmi to ensure that wmi devices are found
 # before toshiba_acpi initializes
diff --git a/drivers/platform/x86/wmi-mof.c b/drivers/platform/x86/wmi-mof.c
new file mode 100644
index 0000000..464ceca
--- /dev/null
+++ b/drivers/platform/x86/wmi-mof.c
@@ -0,0 +1,125 @@
+/*
+ * WMI embedded MOF driver
+ *
+ * Copyright (c) 2015 Andrew Lutomirski
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms of the GNU General Public License version 2 as published
+ *  by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/acpi.h>
+#include <linux/string.h>
+#include <linux/dmi.h>
+#include <linux/wmi.h>
+#include <acpi/video.h>
+
+#define WMI_MOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
+MODULE_ALIAS("wmi:" WMI_MOF_GUID);
+
+struct mof_priv {
+	union acpi_object *mofdata;
+	struct bin_attribute mof_bin_attr;
+};
+
+static ssize_t
+read_mof(struct file *filp, struct kobject *kobj,
+	 struct bin_attribute *attr,
+	 char *buf, loff_t off, size_t count)
+{
+	struct mof_priv *priv =
+		container_of(attr, struct mof_priv, mof_bin_attr);
+
+	if (off >= priv->mofdata->buffer.length)
+		return 0;
+
+	if (count > priv->mofdata->buffer.length - off)
+		count = priv->mofdata->buffer.length - off;
+
+	memcpy(buf, priv->mofdata->buffer.pointer + off, count);
+	return count;
+}
+
+static int wmi_mof_probe(struct wmi_device *wdev)
+{
+	int ret;
+
+	struct mof_priv *priv =
+		devm_kzalloc(&wdev->dev, sizeof(struct mof_priv), GFP_KERNEL);
+
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(&wdev->dev, priv);
+
+	priv->mofdata = wmidev_block_query(wdev, 0);
+	if (!priv->mofdata) {
+		dev_warn(&wdev->dev, "failed to read MOF\n");
+		return -EIO;
+	}
+
+	if (priv->mofdata->type != ACPI_TYPE_BUFFER) {
+		dev_warn(&wdev->dev, "MOF is not a buffer\n");
+		ret = -EIO;
+		goto err_free;
+	}
+
+	sysfs_bin_attr_init(&priv->mof_bin_attr);
+	priv->mof_bin_attr.attr.name = "mof";
+	priv->mof_bin_attr.attr.mode = 0400;
+	priv->mof_bin_attr.read = read_mof;
+	priv->mof_bin_attr.size = priv->mofdata->buffer.length;
+
+	ret = sysfs_create_bin_file(&wdev->dev.kobj, &priv->mof_bin_attr);
+	if (ret)
+		goto err_free;
+
+	return 0;
+
+err_free:
+	kfree(priv->mofdata);
+	return ret;
+}
+
+static int wmi_mof_remove(struct wmi_device *wdev)
+{
+	struct mof_priv *priv = dev_get_drvdata(&wdev->dev);
+
+	sysfs_remove_bin_file(&wdev->dev.kobj, &priv->mof_bin_attr);
+	kfree(priv->mofdata);
+	return 0;
+}
+
+static const struct wmi_device_id wmi_mof_id_table[] = {
+	{ .guid_string = WMI_MOF_GUID },
+	{ },
+};
+
+static struct wmi_driver wmi_mof_driver = {
+	.driver = {
+		.name = "wmi-mof",
+	},
+	.probe = wmi_mof_probe,
+	.remove = wmi_mof_remove,
+	.id_table = wmi_mof_id_table,
+};
+
+module_wmi_driver(wmi_mof_driver);
+
+MODULE_AUTHOR("Andrew Lutomirski <luto@kernel.org>");
+MODULE_DESCRIPTION("WMI embedded MOF driver");
+MODULE_LICENSE("GPL");
-- 
2.9.4

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1326 bytes --]

Hi! Note that in WMI is stored binary MOF (BMOF; .bmf file; compiled 
MOF), not ordinary MOF data which are plain text. So maybe it could make 
sense to include "B" into name of sysfs entry? Or not? (Just suggestion)

On Saturday 27 May 2017 07:31:29 Darren Hart wrote:
> From: Andy Lutomirski <luto@kernel.org>
> 
> Quite a few laptops (and maybe servers?) have embedded WMI MOF

Not "a few", but "lots of" :-)

> metadata. I think that Samba has tools to interpret it, but there is
> currently no interface to get the data in the first place.

No, there is no non-ms-windows tool for interpreting those binary MOF 
(BMF) data yet.

> +	priv->mofdata = wmidev_block_query(wdev, 0);
> +	if (!priv->mofdata) {
> +		dev_warn(&wdev->dev, "failed to read MOF\n");
> +		return -EIO;
> +	}
> +
> +	if (priv->mofdata->type != ACPI_TYPE_BUFFER) {
> +		dev_warn(&wdev->dev, "MOF is not a buffer\n");
> +		ret = -EIO;
> +		goto err_free;
> +	}

Are not those problems fatal for driver and therefore dev_err() better?

> +	sysfs_bin_attr_init(&priv->mof_bin_attr);
> +	priv->mof_bin_attr.attr.name = "mof";
> +	priv->mof_bin_attr.attr.mode = 0400;

0400 means to be readable only by root? Is there then reason why normal 
user should not be able to read it?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Andy Lutomirski]

On Sat, May 27, 2017 at 4:14 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> Hi! Note that in WMI is stored binary MOF (BMOF; .bmf file; compiled
> MOF), not ordinary MOF data which are plain text. So maybe it could make
> sense to include "B" into name of sysfs entry? Or not? (Just suggestion)
>
> On Saturday 27 May 2017 07:31:29 Darren Hart wrote:
>> From: Andy Lutomirski <luto@kernel.org>
>>
>> Quite a few laptops (and maybe servers?) have embedded WMI MOF
>
> Not "a few", but "lots of" :-)
>
>> metadata. I think that Samba has tools to interpret it, but there is
>> currently no interface to get the data in the first place.
>
> No, there is no non-ms-windows tool for interpreting those binary MOF
> (BMF) data yet.
>
>> +     priv->mofdata = wmidev_block_query(wdev, 0);
>> +     if (!priv->mofdata) {
>> +             dev_warn(&wdev->dev, "failed to read MOF\n");
>> +             return -EIO;
>> +     }
>> +
>> +     if (priv->mofdata->type != ACPI_TYPE_BUFFER) {
>> +             dev_warn(&wdev->dev, "MOF is not a buffer\n");
>> +             ret = -EIO;
>> +             goto err_free;
>> +     }
>
> Are not those problems fatal for driver and therefore dev_err() better?
>
>> +     sysfs_bin_attr_init(&priv->mof_bin_attr);
>> +     priv->mof_bin_attr.attr.name = "mof";
>> +     priv->mof_bin_attr.attr.mode = 0400;
>
> 0400 means to be readable only by root? Is there then reason why normal
> user should not be able to read it?
>

I have no specific objection to making it 0444, but in general I'd
rather expose less information to unprivileged users rather than more.
I'm also having trouble imagining what an unprivileged user would do
with the MOF -- it's useful for reverse engineering and it may
eventually be useful for making WMI calls from userspace, but neither
of those is particularly useful to unprivileged users.

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Andy Shevchenko]

On Sat, 2017-05-27 at 14:07 -0700, Andy Lutomirski wrote:
> On Sat, May 27, 2017 at 4:14 AM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > > Quite a few laptops (and maybe servers?) have embedded WMI MOF
> > 
> > Not "a few", but "lots of" :-)

Aren't they are synonyms ("quite a few" vs "lots of")?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Darren Hart]

On Tue, May 30, 2017 at 06:24:54PM +0300, Andy Shevchenko wrote:
> On Sat, 2017-05-27 at 14:07 -0700, Andy Lutomirski wrote:
> > On Sat, May 27, 2017 at 4:14 AM, Pali Rohár <pali.rohar@gmail.com>
> > wrote:
> > > > Quite a few laptops (and maybe servers?) have embedded WMI MOF
> > > 
> > > Not "a few", but "lots of" :-)
> 
> Aren't they are synonyms ("quite a few" vs "lots of")?

The difference is subtle enough that people we can probably get several
different answers, but generally yes. I'm happy to update it in any case.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 848 bytes --]

On Tuesday 30 May 2017 17:24:54 Andy Shevchenko wrote:
> On Sat, 2017-05-27 at 14:07 -0700, Andy Lutomirski wrote:
> > On Sat, May 27, 2017 at 4:14 AM, Pali Rohár <pali.rohar@gmail.com>
> > 
> > wrote:
> > > > Quite a few laptops (and maybe servers?) have embedded WMI MOF
> > > 
> > > Not "a few", but "lots of" :-)
> 
> Aren't they are synonyms ("quite a few" vs "lots of")?

I understand "lots of" > "quite a few" > "a few". And synonyms do not 
have exact meaning...

But now I was told that "quite a few" has emphasise meaning "more then 
expected".

For me it is normal fact that there are lot of machines with WMI. But if 
meaning in commit message should be "I'm surprise how many machines are 
with WMI", then "quite a few" better fit.

So choose what you prefer or want...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Andy Shevchenko]

On Tue, May 30, 2017 at 8:03 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 30 May 2017 17:24:54 Andy Shevchenko wrote:
>> On Sat, 2017-05-27 at 14:07 -0700, Andy Lutomirski wrote:
>> > On Sat, May 27, 2017 at 4:14 AM, Pali Rohár <pali.rohar@gmail.com>
>> > wrote:

>> > > > Quite a few laptops (and maybe servers?) have embedded WMI MOF
>> > > Not "a few", but "lots of" :-)
>> Aren't they are synonyms ("quite a few" vs "lots of")?
>
> I understand "lots of" > "quite a few" > "a few". And synonyms do not
> have exact meaning...

I figured those two:

100% quite a lot -->> 50% <<-- quite a few 0%, which means (for my
understanding)
that with lim (n→∞) both are = 50% (n represents cases of use the terms).

> So choose what you prefer or want...

Same here!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Darren Hart]

On Sat, May 27, 2017 at 01:14:15PM +0200, Pali Rohár wrote:
> Hi! Note that in WMI is stored binary MOF (BMOF; .bmf file; compiled 
> MOF), not ordinary MOF data which are plain text. So maybe it could make 
> sense to include "B" into name of sysfs entry? Or not? (Just suggestion)

Did some digging, and .... yeah, you're right.

I've replaced with MOF with Binary MOF or bmof throughout the patch. Will resend
in a v2. Thanks.

> 
> On Saturday 27 May 2017 07:31:29 Darren Hart wrote:
> > From: Andy Lutomirski <luto@kernel.org>
> > 
> > Quite a few laptops (and maybe servers?) have embedded WMI MOF
> 
> Not "a few", but "lots of" :-)

I'll just use "Many" and save us continued debate :-)

> 
> > metadata. I think that Samba has tools to interpret it, but there is
> > currently no interface to get the data in the first place.
> 
> No, there is no non-ms-windows tool for interpreting those binary MOF 
> (BMF) data yet.

Good point. Updated.

> 
> > +	priv->mofdata = wmidev_block_query(wdev, 0);
> > +	if (!priv->mofdata) {
> > +		dev_warn(&wdev->dev, "failed to read MOF\n");
> > +		return -EIO;
> > +	}
> > +
> > +	if (priv->mofdata->type != ACPI_TYPE_BUFFER) {
> > +		dev_warn(&wdev->dev, "MOF is not a buffer\n");
> > +		ret = -EIO;
> > +		goto err_free;
> > +	}
> 
> Are not those problems fatal for driver and therefore dev_err() better?
> 

Yes, agreed. Updated.

> > +	sysfs_bin_attr_init(&priv->mof_bin_attr);
> > +	priv->mof_bin_attr.attr.name = "mof";
> > +	priv->mof_bin_attr.attr.mode = 0400;
> 
> 0400 means to be readable only by root? Is there then reason why normal 
> user should not be able to read it?
> 

We can always open access up, harder to lock it down later. Let's start with
this and adjust if necessary.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 568 bytes --]

On Tuesday 06 June 2017 00:14:56 Darren Hart wrote:
> On Sat, May 27, 2017 at 01:14:15PM +0200, Pali Rohár wrote:
> > > metadata. I think that Samba has tools to interpret it, but there
> > > is currently no interface to get the data in the first place.
> > 
> > No, there is no non-ms-windows tool for interpreting those binary
> > MOF (BMF) data yet.
> 
> Good point. Updated.

You are too late :-) Now there is my at https://github.com/pali/bmfdec 
See my email "Binary MOF buffer in WMI is finally decoded!".

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Andy Lutomirski]

On Mon, Jun 5, 2017 at 3:19 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 06 June 2017 00:14:56 Darren Hart wrote:
>> On Sat, May 27, 2017 at 01:14:15PM +0200, Pali Rohár wrote:
>> > > metadata. I think that Samba has tools to interpret it, but there
>> > > is currently no interface to get the data in the first place.
>> >
>> > No, there is no non-ms-windows tool for interpreting those binary
>> > MOF (BMF) data yet.
>>
>> Good point. Updated.
>
> You are too late :-) Now there is my at https://github.com/pali/bmfdec
> See my email "Binary MOF buffer in WMI is finally decoded!".
>

It comes out like this on my laptop.  I don't know enough about MOF to
know what we're supposed to do with this, but I suspect it at least
gives us the sizes of buffers that we should be passing to the various
methods.

class WMIEvent : __ExtrinsicEvent {
};

[WMI, Locale("MS\0x409"), Description("QDATA"),
guid("{ABBC0F60-8EA1-11d1-00A0-C90629100000}")]
class QDat {
  [WmiDataId(1), read, write, Description("qdata")] uint8 Bytes[128];
};

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
Description("BIOS WMI Query"),
guid("{8D9DDCBC-A997-11DA-B012-B622A1EF5492}")]
class WMI_Query {
  [key, read] String InstanceName;
  [read] Boolean Active;
  [WmiDataId(1), read, write, Description("BIOS WMI info")] QDat QDATA;
};

[WMI, Locale("MS\0x409"), Description("Data"),
guid("{a3776ce0-1e88-11db-a98b-0800200c9a66}")]
class BDat {
  [WmiDataId(1), read, write, Description("data")] uint8 Bytes[4096];
};

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
Description("Interface"),
guid("{A80593CE-A997-11DA-B012-B622A1EF5492}")]
class BFn {
  [key, read] String InstanceName;
  [read] Boolean Active;

  [WmiMethodId(1), Implemented, read, write, Description("Do BFn")]
void DoBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
Description("Fn buf"), ID(0)] BDat Data);
};

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
Description("Event"), guid("{9DBB5994-A997-11DA-B012-B622A1EF5492}")]
class BIOSEvent : WmiEvent {
  [key, read] String InstanceName;
  [read] Boolean Active;
  [WmiDataId(1), read, write, Description("Ev buf")] QDat Data;
};

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

On Monday 05 June 2017 15:39:44 Andy Lutomirski wrote:
> On Mon, Jun 5, 2017 at 3:19 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Tuesday 06 June 2017 00:14:56 Darren Hart wrote:
> >> On Sat, May 27, 2017 at 01:14:15PM +0200, Pali Rohár wrote:
> >> > > metadata. I think that Samba has tools to interpret it, but there
> >> > > is currently no interface to get the data in the first place.
> >> >
> >> > No, there is no non-ms-windows tool for interpreting those binary
> >> > MOF (BMF) data yet.
> >>
> >> Good point. Updated.
> >
> > You are too late :-) Now there is my at https://github.com/pali/bmfdec
> > See my email "Binary MOF buffer in WMI is finally decoded!".
> >
> 
> It comes out like this on my laptop.  I don't know enough about MOF to
> know what we're supposed to do with this, but I suspect it at least
> gives us the sizes of buffers that we should be passing to the various
> methods.

There are two tools bmfparse and bmf2mof. Difference is just output
format.

Important are WmiDataId and WmiMethodId qualifiers. Those define ids are
what are passed to kernel function wmi_evaluate_method(). Ids are
processed by corresponding WMxx ACPI function.

So instead of

  wmi_evaluate_method(guid, instance, method_id, acpi_in, acpi_out);

it should be possible to call:

  wmi_evaluate_method_name(class, name, input_params, output_params);

(once somebody implement wmi_evaluate_method_name function)

It is more readable in code to use class and function names instead of
some guids and random meaningless numbers. Also it would allow to check
size of input buffer (or types).

E.g.

  BDat data_in;
  BDat data_out;
  // fill data_in.Bytes
  wmi_evaluate_method_name("BFn", "DoBFn", &data_in, &data_out);
  // output is in data_out.Bytes

could be translated to:

  wmi_evaluate_method("A80593CE-A997-11DA-B012-B622A1EF5492", 0, 1, acpi_in, acpi_out);

Sometimes method_id is random number and hard to guess it. One of
possible solution is to trace ACPI calls on Windows, another is decode
that BMOF buffer and take correct method_id.

I will probably write another one tool which prints just important WMI
functions and their mapping to WMI ids + input/output parameters.
Without all other MOF data which are not relevant to ACPI-WMI.

> class WMIEvent : __ExtrinsicEvent {
> };
> 
> [WMI, Locale("MS\0x409"), Description("QDATA"),
> guid("{ABBC0F60-8EA1-11d1-00A0-C90629100000}")]
> class QDat {
>   [WmiDataId(1), read, write, Description("qdata")] uint8 Bytes[128];
> };
> 
> [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> Description("BIOS WMI Query"),
> guid("{8D9DDCBC-A997-11DA-B012-B622A1EF5492}")]
> class WMI_Query {
>   [key, read] String InstanceName;
>   [read] Boolean Active;
>   [WmiDataId(1), read, write, Description("BIOS WMI info")] QDat QDATA;
> };
> 
> [WMI, Locale("MS\0x409"), Description("Data"),
> guid("{a3776ce0-1e88-11db-a98b-0800200c9a66}")]
> class BDat {
>   [WmiDataId(1), read, write, Description("data")] uint8 Bytes[4096];
> };
> 
> [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> Description("Interface"),
> guid("{A80593CE-A997-11DA-B012-B622A1EF5492}")]
> class BFn {
>   [key, read] String InstanceName;
>   [read] Boolean Active;
> 
>   [WmiMethodId(1), Implemented, read, write, Description("Do BFn")]
> void DoBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
> Description("Fn buf"), ID(0)] BDat Data);
> };
> 
> [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> Description("Event"), guid("{9DBB5994-A997-11DA-B012-B622A1EF5492}")]
> class BIOSEvent : WmiEvent {
>   [key, read] String InstanceName;
>   [read] Boolean Active;
>   [WmiDataId(1), read, write, Description("Ev buf")] QDat Data;
> };

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Mario.Limonciello]

Pali,

Amazing job with what you've done so far.  A few comments I wanted to share from my taking look at your bmf2mof and comparing to "source" MOF.
1) At least in source the case used for String and Boolean is lower case.  I'm unsure if that actually matters for any MOF parsing tools, but I wanted to FYI in case it does.
2) On my system when you expand the arguments for "void DoBFn" the source doesn't describe individual arguments like you do.  
Again this might not matter to MOF parsing tools but wanted to let you know in case it does.

source:
	void DoBFn([in, out, Description("Fn buf")] BDat Data);
bmf2mof:
	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out, Description("Fn buf"), ID(0)] BDat Data);

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Tuesday, June 6, 2017 6:06 AM
> To: Andy Lutomirski <luto@amacapital.net>
> Cc: Darren Hart <dvhart@infradead.org>; platform-driver-x86@vger.kernel.org;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Andy Lutomirski
> <luto@kernel.org>; Limonciello, Mario <Mario_Limonciello@Dell.com>; Rafael
> Wysocki <rjw@rjwysocki.net>; linux-kernel@vger.kernel.org; Linux ACPI <linux-
> acpi@vger.kernel.org>
> Subject: Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose
> embedded WMI MOF metadata
> 
> On Monday 05 June 2017 15:39:44 Andy Lutomirski wrote:
> > On Mon, Jun 5, 2017 at 3:19 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > On Tuesday 06 June 2017 00:14:56 Darren Hart wrote:
> > >> On Sat, May 27, 2017 at 01:14:15PM +0200, Pali Rohár wrote:
> > >> > > metadata. I think that Samba has tools to interpret it, but there
> > >> > > is currently no interface to get the data in the first place.
> > >> >
> > >> > No, there is no non-ms-windows tool for interpreting those binary
> > >> > MOF (BMF) data yet.
> > >>
> > >> Good point. Updated.
> > >
> > > You are too late :-) Now there is my at https://github.com/pali/bmfdec
> > > See my email "Binary MOF buffer in WMI is finally decoded!".
> > >
> >
> > It comes out like this on my laptop.  I don't know enough about MOF to
> > know what we're supposed to do with this, but I suspect it at least
> > gives us the sizes of buffers that we should be passing to the various
> > methods.
> 

Yes, the size of the buffers and the format of the data you can pass in them is 
what is most important bit of information that comes out of this.

> There are two tools bmfparse and bmf2mof. Difference is just output
> format.
> 
> Important are WmiDataId and WmiMethodId qualifiers. Those define ids are
> what are passed to kernel function wmi_evaluate_method(). Ids are
> processed by corresponding WMxx ACPI function.
> 
> So instead of
> 
>   wmi_evaluate_method(guid, instance, method_id, acpi_in, acpi_out);
> 
> it should be possible to call:
> 
>   wmi_evaluate_method_name(class, name, input_params, output_params);
> 
> (once somebody implement wmi_evaluate_method_name function)
> 
Now that you have managed to decode binary MOF, it might actually change
what makes sense to export to userspace.  We were previously
discussing exporting GUID's (eg  something like /dev/wmi-$GUID)
but with readable names that actually map to the GUID's, it makes more sense
to export the classes.  Userspace tools can parse the exported MOF to know
how to interact with those classes, and the classes can then map to chardevices.
For example on a Dell system today /dev/wmi-BFn would be the most important
to export (this will change in the future).

> It is more readable in code to use class and function names instead of
> some guids and random meaningless numbers. Also it would allow to check
> size of input buffer (or types).
Completely agree.  Once the parser can be brought into kernel, this is a very
good next change for existing WMI drivers.

> 
> E.g.
> 
>   BDat data_in;
>   BDat data_out;
>   // fill data_in.Bytes
>   wmi_evaluate_method_name("BFn", "DoBFn", &data_in, &data_out);
>   // output is in data_out.Bytes
> 
> could be translated to:
> 
>   wmi_evaluate_method("A80593CE-A997-11DA-B012-B622A1EF5492", 0, 1,
> acpi_in, acpi_out);
> 
> Sometimes method_id is random number and hard to guess it. One of
> possible solution is to trace ACPI calls on Windows, another is decode
> that BMOF buffer and take correct method_id.
> 
> I will probably write another one tool which prints just important WMI
> functions and their mapping to WMI ids + input/output parameters.
> Without all other MOF data which are not relevant to ACPI-WMI.

When this goes into the kernel, I think it would be ideal to export MOF
exactly like your bmf2mof does.
The userspace MOF parsing tools that already exist will be able to process 
it more effectively this way.
Perhaps internally in the kernel this mapping information will be useful to 
be able to  create wmi_evaluate_method_name.

> 
> > class WMIEvent : __ExtrinsicEvent {
> > };
> >
> > [WMI, Locale("MS\0x409"), Description("QDATA"),
> > guid("{ABBC0F60-8EA1-11d1-00A0-C90629100000}")]
> > class QDat {
> >   [WmiDataId(1), read, write, Description("qdata")] uint8 Bytes[128];
> > };
> >
> > [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> > Description("BIOS WMI Query"),
> > guid("{8D9DDCBC-A997-11DA-B012-B622A1EF5492}")]
> > class WMI_Query {
> >   [key, read] String InstanceName;
> >   [read] Boolean Active;
> >   [WmiDataId(1), read, write, Description("BIOS WMI info")] QDat QDATA;
> > };
> >
> > [WMI, Locale("MS\0x409"), Description("Data"),
> > guid("{a3776ce0-1e88-11db-a98b-0800200c9a66}")]
> > class BDat {
> >   [WmiDataId(1), read, write, Description("data")] uint8 Bytes[4096];
> > };
> >
> > [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> > Description("Interface"),
> > guid("{A80593CE-A997-11DA-B012-B622A1EF5492}")]
> > class BFn {
> >   [key, read] String InstanceName;
> >   [read] Boolean Active;
> >
> >   [WmiMethodId(1), Implemented, read, write, Description("Do BFn")]
> > void DoBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
> > Description("Fn buf"), ID(0)] BDat Data);
> > };
> >
> > [WMI, Dynamic, Provider("WmiProv"), Locale("MS\0x409"),
> > Description("Event"), guid("{9DBB5994-A997-11DA-B012-B622A1EF5492}")]
> > class BIOSEvent : WmiEvent {
> >   [key, read] String InstanceName;
> >   [read] Boolean Active;
> >   [WmiDataId(1), read, write, Description("Ev buf")] QDat Data;
> > };
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

On Tuesday 06 June 2017 13:46:16 Mario.Limonciello@dell.com wrote:
> Pali,
> 
> Amazing job with what you've done so far.  A few comments I wanted to share from my taking look at your bmf2mof and comparing to "source" MOF.
> 1) At least in source the case used for String and Boolean is lower case.  I'm unsure if that actually matters for any MOF parsing tools, but I wanted to FYI in case it does.

In MS documentation is String, Boolean and Datetime with first char
uppercase. But looks like mofcomp accept both upper case and lower case
variants.

> 2) On my system when you expand the arguments for "void DoBFn" the source doesn't describe individual arguments like you do.  
> Again this might not matter to MOF parsing tools but wanted to let you know in case it does.

I know, this part is missing. Order of arguments are only in ID
qualifier and not sorted + in/out de-duplicated.

> source:
> 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> bmf2mof:
> 	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out, Description("Fn buf"), ID(0)] BDat Data);

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 814 bytes --]

On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> On Tuesday 06 June 2017 13:46:16 Mario.Limonciello@dell.com wrote:
> > 2) On my system when you expand the arguments for "void DoBFn" the
> > source doesn't describe individual arguments like you do. Again
> > this might not matter to MOF parsing tools but wanted to let you
> > know in case it does.
> 
> I know, this part is missing. Order of arguments are only in ID
> qualifier and not sorted + in/out de-duplicated.

Implemented! Now arguments are correctly placed based on ID qualifier.

> > source:
> > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > 
> > bmf2mof:
> > 	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
> > 	Description("Fn buf"), ID(0)] BDat Data);

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, June 7, 2017 12:39 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: luto@amacapital.net; dvhart@infradead.org; platform-driver-
> x86@vger.kernel.org; andriy.shevchenko@linux.intel.com; luto@kernel.org;
> rjw@rjwysocki.net; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose
> embedded WMI MOF metadata
> 
> On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> > On Tuesday 06 June 2017 13:46:16 Mario.Limonciello@dell.com wrote:
> > > 2) On my system when you expand the arguments for "void DoBFn" the
> > > source doesn't describe individual arguments like you do. Again
> > > this might not matter to MOF parsing tools but wanted to let you
> > > know in case it does.
> >
> > I know, this part is missing. Order of arguments are only in ID
> > qualifier and not sorted + in/out de-duplicated.
> 
> Implemented! Now arguments are correctly placed based on ID qualifier.
I think it's still off a little though.

What I'm getting back now from bmf2mof is:
	void DoBFn([in, Description("Fn buf"), out] BDat Data);

Whereas source puts Description as the last argument: 
	void DoBFn([in, out, Description("Fn buf")] BDat Data);

> 
> > > source:
> > > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > >
> > > bmf2mof:
> > > 	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
> > > 	Description("Fn buf"), ID(0)] BDat Data);
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2394 bytes --]

On Wednesday 07 June 2017 22:23:08 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Wednesday, June 7, 2017 12:39 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: luto@amacapital.net; dvhart@infradead.org; platform-driver-
> > x86@vger.kernel.org; andriy.shevchenko@linux.intel.com;
> > luto@kernel.org; rjw@rjwysocki.net; linux-kernel@vger.kernel.org;
> > linux-acpi@vger.kernel.org Subject: Re: [PATCH 15/16]
> > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > metadata
> > 
> > On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> > > On Tuesday 06 June 2017 13:46:16 Mario.Limonciello@dell.com wrote:
> > > > 2) On my system when you expand the arguments for "void DoBFn"
> > > > the source doesn't describe individual arguments like you do.
> > > > Again this might not matter to MOF parsing tools but wanted to
> > > > let you know in case it does.
> > > 
> > > I know, this part is missing. Order of arguments are only in ID
> > > qualifier and not sorted + in/out de-duplicated.
> > 
> > Implemented! Now arguments are correctly placed based on ID
> > qualifier.
> 
> I think it's still off a little though.
> 
> What I'm getting back now from bmf2mof is:
> 	void DoBFn([in, Description("Fn buf"), out] BDat Data);
> 
> Whereas source puts Description as the last argument:
> 	void DoBFn([in, out, Description("Fn buf")] BDat Data);

In BMOF from my Latitude E6440 there are specified two parameters with 
index 0. One with qualifiers ("in", Description("Fn buf")) and one with 
("out", Description("Fn buf")). I think you have similar/same data in 
BMOF.

In my bmf2mof I just combined those two parameters into one (when name, 
type and index matches) and concatenate also qualifiers with removing 
duplicates.

Do not know what is correct way, but I think qualifiers are just 
unordered set. MS decompiler probably put "in" and "out" qualifiers 
before any other for better readability.

> > > > source:
> > > > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > > > 
> > > > bmf2mof:
> > > > 	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
> > > > 	Description("Fn buf"), ID(0)] BDat Data);
> > 
> > --
> > Pali Rohár
> > pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, June 7, 2017 3:50 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: luto@amacapital.net; dvhart@infradead.org; platform-driver-
> x86@vger.kernel.org; andriy.shevchenko@linux.intel.com; luto@kernel.org;
> rjw@rjwysocki.net; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose
> embedded WMI MOF metadata
> 
> On Wednesday 07 June 2017 22:23:08 Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > Sent: Wednesday, June 7, 2017 12:39 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: luto@amacapital.net; dvhart@infradead.org; platform-driver-
> > > x86@vger.kernel.org; andriy.shevchenko@linux.intel.com;
> > > luto@kernel.org; rjw@rjwysocki.net; linux-kernel@vger.kernel.org;
> > > linux-acpi@vger.kernel.org Subject: Re: [PATCH 15/16]
> > > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > > metadata
> > >
> > > On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> > > > On Tuesday 06 June 2017 13:46:16 Mario.Limonciello@dell.com wrote:
> > > > > 2) On my system when you expand the arguments for "void DoBFn"
> > > > > the source doesn't describe individual arguments like you do.
> > > > > Again this might not matter to MOF parsing tools but wanted to
> > > > > let you know in case it does.
> > > >
> > > > I know, this part is missing. Order of arguments are only in ID
> > > > qualifier and not sorted + in/out de-duplicated.
> > >
> > > Implemented! Now arguments are correctly placed based on ID
> > > qualifier.
> >
> > I think it's still off a little though.
> >
> > What I'm getting back now from bmf2mof is:
> > 	void DoBFn([in, Description("Fn buf"), out] BDat Data);
> >
> > Whereas source puts Description as the last argument:
> > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> 
> In BMOF from my Latitude E6440 there are specified two parameters with
> index 0. One with qualifiers ("in", Description("Fn buf")) and one with
> ("out", Description("Fn buf")). I think you have similar/same data in
> BMOF.
> 
> In my bmf2mof I just combined those two parameters into one (when name,
> type and index matches) and concatenate also qualifiers with removing
> duplicates.
> 
> Do not know what is correct way, but I think qualifiers are just
> unordered set. MS decompiler probably put "in" and "out" qualifiers
> before any other for better readability.

Have you tried to run it through mofcomp.exe and then decompile again
with bmf2mof?  As long as it's coming out the same you're probably right.

> 
> > > > > source:
> > > > > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > > > >
> > > > > bmf2mof:
> > > > > 	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data, [out,
> > > > > 	Description("Fn buf"), ID(0)] BDat Data);
> > >
> > > --
> > > Pali Rohár
> > > pali.rohar@gmail.com
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3510 bytes --]

On Friday 09 June 2017 17:46:12 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Wednesday, June 7, 2017 3:50 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: luto@amacapital.net; dvhart@infradead.org; platform-driver-
> > x86@vger.kernel.org; andriy.shevchenko@linux.intel.com;
> > luto@kernel.org; rjw@rjwysocki.net; linux-kernel@vger.kernel.org;
> > linux-acpi@vger.kernel.org Subject: Re: [PATCH 15/16]
> > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > metadata
> > 
> > On Wednesday 07 June 2017 22:23:08 Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > Sent: Wednesday, June 7, 2017 12:39 PM
> > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > Cc: luto@amacapital.net; dvhart@infradead.org; platform-driver-
> > > > x86@vger.kernel.org; andriy.shevchenko@linux.intel.com;
> > > > luto@kernel.org; rjw@rjwysocki.net;
> > > > linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> > > > Subject: Re: [PATCH 15/16]
> > > > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > > > metadata
> > > > 
> > > > On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> > > > > On Tuesday 06 June 2017 13:46:16 Mario.Limonciello@dell.com
> > > > > wrote:
> > > > > > 2) On my system when you expand the arguments for "void
> > > > > > DoBFn" the source doesn't describe individual arguments
> > > > > > like you do. Again this might not matter to MOF parsing
> > > > > > tools but wanted to let you know in case it does.
> > > > > 
> > > > > I know, this part is missing. Order of arguments are only in
> > > > > ID qualifier and not sorted + in/out de-duplicated.
> > > > 
> > > > Implemented! Now arguments are correctly placed based on ID
> > > > qualifier.
> > > 
> > > I think it's still off a little though.
> > > 
> > > What I'm getting back now from bmf2mof is:
> > > 	void DoBFn([in, Description("Fn buf"), out] BDat Data);
> > > 
> > > Whereas source puts Description as the last argument:
> > > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > 
> > In BMOF from my Latitude E6440 there are specified two parameters
> > with index 0. One with qualifiers ("in", Description("Fn buf"))
> > and one with ("out", Description("Fn buf")). I think you have
> > similar/same data in BMOF.
> > 
> > In my bmf2mof I just combined those two parameters into one (when
> > name, type and index matches) and concatenate also qualifiers with
> > removing duplicates.
> > 
> > Do not know what is correct way, but I think qualifiers are just
> > unordered set. MS decompiler probably put "in" and "out" qualifiers
> > before any other for better readability.
> 
> Have you tried to run it through mofcomp.exe and then decompile again
> with bmf2mof?  As long as it's coming out the same you're probably
> right.

Yes, bmf2mof+mofcomp.exe+bmf2mof gives same output as just bmf2mof.

> > > > > > source:
> > > > > > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > > > > > 
> > > > > > bmf2mof:
> > > > > > 	void doBFn([in, Description("Fn buf"), ID(0)] BDat Data,
> > > > > > 	[out, Description("Fn buf"), ID(0)] BDat Data);
> > > > 
> > > > --
> > > > Pali Rohár
> > > > pali.rohar@gmail.com
> > 
> > --
> > Pali Rohár
> > pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3910 bytes --]

On Friday 09 June 2017 23:51:32 Pali Rohár wrote:
> On Friday 09 June 2017 17:46:12 Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > Sent: Wednesday, June 7, 2017 3:50 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: luto@amacapital.net; dvhart@infradead.org; platform-driver-
> > > x86@vger.kernel.org; andriy.shevchenko@linux.intel.com;
> > > luto@kernel.org; rjw@rjwysocki.net; linux-kernel@vger.kernel.org;
> > > linux-acpi@vger.kernel.org Subject: Re: [PATCH 15/16]
> > > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > > metadata
> > > 
> > > On Wednesday 07 June 2017 22:23:08 Mario.Limonciello@dell.com
> > > wrote:
> > > > > -----Original Message-----
> > > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > > Sent: Wednesday, June 7, 2017 12:39 PM
> > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > > Cc: luto@amacapital.net; dvhart@infradead.org;
> > > > > platform-driver- x86@vger.kernel.org;
> > > > > andriy.shevchenko@linux.intel.com; luto@kernel.org;
> > > > > rjw@rjwysocki.net;
> > > > > linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> > > > > Subject: Re: [PATCH 15/16]
> > > > > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > > > > metadata
> > > > > 
> > > > > On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> > > > > > On Tuesday 06 June 2017 13:46:16 Mario.Limonciello@dell.com
> > > > > > 
> > > > > > wrote:
> > > > > > > 2) On my system when you expand the arguments for "void
> > > > > > > DoBFn" the source doesn't describe individual arguments
> > > > > > > like you do. Again this might not matter to MOF parsing
> > > > > > > tools but wanted to let you know in case it does.
> > > > > > 
> > > > > > I know, this part is missing. Order of arguments are only
> > > > > > in ID qualifier and not sorted + in/out de-duplicated.
> > > > > 
> > > > > Implemented! Now arguments are correctly placed based on ID
> > > > > qualifier.
> > > > 
> > > > I think it's still off a little though.
> > > > 
> > > > What I'm getting back now from bmf2mof is:
> > > > 	void DoBFn([in, Description("Fn buf"), out] BDat Data);
> > > > 
> > > > Whereas source puts Description as the last argument:
> > > > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > > 
> > > In BMOF from my Latitude E6440 there are specified two parameters
> > > with index 0. One with qualifiers ("in", Description("Fn buf"))
> > > and one with ("out", Description("Fn buf")). I think you have
> > > similar/same data in BMOF.
> > > 
> > > In my bmf2mof I just combined those two parameters into one (when
> > > name, type and index matches) and concatenate also qualifiers
> > > with removing duplicates.
> > > 
> > > Do not know what is correct way, but I think qualifiers are just
> > > unordered set. MS decompiler probably put "in" and "out"
> > > qualifiers before any other for better readability.
> > 
> > Have you tried to run it through mofcomp.exe and then decompile
> > again with bmf2mof?  As long as it's coming out the same you're
> > probably right.
> 
> Yes, bmf2mof+mofcomp.exe+bmf2mof gives same output as just bmf2mof.

I changed order for printing qualifiers in bmf2mof. "in" and "out" are 
now printed before all others. So you should see now same output.

> > > > > > > source:
> > > > > > > 	void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > > > > > > 
> > > > > > > bmf2mof:
> > > > > > > 	void doBFn([in, Description("Fn buf"), ID(0)] BDat
> > > > > > > 	Data,
> > > > > > > 	[out, Description("Fn buf"), ID(0)] BDat Data);
> > > > > 
> > > > > --
> > > > > Pali Rohár
> > > > > pali.rohar@gmail.com
> > > 
> > > --
> > > Pali Rohár
> > > pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Darren Hart]

On Tue, Jun 06, 2017 at 12:19:26AM +0200, Pali Rohár wrote:
> On Tuesday 06 June 2017 00:14:56 Darren Hart wrote:
> > On Sat, May 27, 2017 at 01:14:15PM +0200, Pali Rohár wrote:
> > > > metadata. I think that Samba has tools to interpret it, but there
> > > > is currently no interface to get the data in the first place.
> > > 
> > > No, there is no non-ms-windows tool for interpreting those binary
> > > MOF (BMF) data yet.
> > 
> > Good point. Updated.
> 
> You are too late :-) Now there is my at https://github.com/pali/bmfdec 
> See my email "Binary MOF buffer in WMI is finally decoded!".

I've seen it, nice work. Will spend more time on it once I get the v2 for this
out the door.

-- 
Darren Hart
VMware Open Source Technology Center

[PATCH 16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

[Darren Hart]

From: Andy Lutomirski <luto@kernel.org>

Move some initialization out of _init and into _probe.
Update signatures and logic to use the wmi bus and device structures.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
[dvhart: drop deprecated sparse_keymap_free, order declarations, add commit msg]
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/dell-wmi.c | 136 +++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 66 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 8a64c79..badc01e 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -36,6 +36,7 @@
 #include <linux/acpi.h>
 #include <linux/string.h>
 #include <linux/dmi.h>
+#include <linux/wmi.h>
 #include <acpi/video.h>
 #include "dell-smbios.h"
 
@@ -53,6 +54,10 @@ static bool wmi_requires_smbios_request;
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+struct dell_wmi_priv {
+	struct input_dev *input_dev;
+};
+
 static int __init dmi_matched(const struct dmi_system_id *dmi)
 {
 	wmi_requires_smbios_request = 1;
@@ -86,7 +91,7 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
  * notifications (rather than requests for change) or are also sent
  * via the keyboard controller so should not be sent again.
  */
-static const struct key_entry dell_wmi_keymap_type_0000[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0000[] = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
 	/* Key code is followed by brightness level */
@@ -207,7 +212,7 @@ struct dell_dmi_results {
 };
 
 /* Uninitialized entries here are KEY_RESERVED == 0. */
-static const u16 bios_to_linux_keycode[256] __initconst = {
+static const u16 bios_to_linux_keycode[256] = {
 	[0]	= KEY_MEDIA,
 	[1]	= KEY_NEXTSONG,
 	[2]	= KEY_PLAYPAUSE,
@@ -256,7 +261,7 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
  * These are applied if the 0xB2 DMI hotkey table is present and doesn't
  * override them.
  */
-static const struct key_entry dell_wmi_keymap_type_0010[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0010[] = {
 	/* Fn-lock */
 	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
 
@@ -289,7 +294,7 @@ static const struct key_entry dell_wmi_keymap_type_0010[] __initconst = {
 /*
  * Keymap for WMI events of type 0x0011
  */
-static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0011[] = {
 	/* Battery unplugged */
 	{ KE_IGNORE, 0xfff0, { KEY_RESERVED } },
 
@@ -304,13 +309,12 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
 	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
 };
 
-static struct input_dev *dell_wmi_input_dev;
-
-static void dell_wmi_process_key(int type, int code)
+static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
 {
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
 	const struct key_entry *key;
 
-	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
+	key = sparse_keymap_entry_from_scancode(priv->input_dev,
 						(type << 16) | code);
 	if (!key) {
 		pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n",
@@ -333,33 +337,18 @@ static void dell_wmi_process_key(int type, int code)
 		dell_laptop_call_notifier(
 			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
 
-	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
+	sparse_keymap_report_entry(priv->input_dev, key, 1, true);
 }
 
-static void dell_wmi_notify(u32 value, void *context)
+static void dell_wmi_notify(struct wmi_device *wdev,
+			    union acpi_object *obj)
 {
-	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-	acpi_status status;
-	acpi_size buffer_size;
 	u16 *buffer_entry, *buffer_end;
+	acpi_size buffer_size;
 	int len, i;
 
-	status = wmi_get_event_data(value, &response);
-	if (status != AE_OK) {
-		pr_warn("bad event status 0x%x\n", status);
-		return;
-	}
-
-	obj = (union acpi_object *)response.pointer;
-	if (!obj) {
-		pr_warn("no response\n");
-		return;
-	}
-
 	if (obj->type != ACPI_TYPE_BUFFER) {
 		pr_warn("bad response type %x\n", obj->type);
-		kfree(obj);
 		return;
 	}
 
@@ -404,13 +393,14 @@ static void dell_wmi_notify(u32 value, void *context)
 		switch (buffer_entry[1]) {
 		case 0x0000: /* One key pressed or event occurred */
 			if (len > 2)
-				dell_wmi_process_key(0x0000, buffer_entry[2]);
+				dell_wmi_process_key(wdev, 0x0000,
+						     buffer_entry[2]);
 			/* Other entries could contain additional information */
 			break;
 		case 0x0010: /* Sequence of keys pressed */
 		case 0x0011: /* Sequence of events occurred */
 			for (i = 2; i < len; ++i)
-				dell_wmi_process_key(buffer_entry[1],
+				dell_wmi_process_key(wdev, buffer_entry[1],
 						     buffer_entry[i]);
 			break;
 		default: /* Unknown event */
@@ -423,7 +413,6 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	}
 
-	kfree(obj);
 }
 
 static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
@@ -437,9 +426,7 @@ static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
 	return false;
 }
 
-static void __init handle_dmi_entry(const struct dmi_header *dm,
-				    void *opaque)
-
+static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
 {
 	struct dell_dmi_results *results = opaque;
 	struct dell_bios_hotkey_table *table;
@@ -509,19 +496,20 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 	results->keymap_size = pos;
 }
 
-static int __init dell_wmi_input_setup(void)
+static int dell_wmi_input_setup(struct wmi_device *wdev)
 {
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
 	struct dell_dmi_results dmi_results = {};
 	struct key_entry *keymap;
 	int err, i, pos = 0;
 
-	dell_wmi_input_dev = input_allocate_device();
-	if (!dell_wmi_input_dev)
+	priv->input_dev = input_allocate_device();
+	if (!priv->input_dev)
 		return -ENOMEM;
 
-	dell_wmi_input_dev->name = "Dell WMI hotkeys";
-	dell_wmi_input_dev->phys = "wmi/input0";
-	dell_wmi_input_dev->id.bustype = BUS_HOST;
+	priv->input_dev->name = "Dell WMI hotkeys";
+	priv->input_dev->id.bustype = BUS_HOST;
+	priv->input_dev->dev.parent = &wdev->dev;
 
 	if (dmi_walk(handle_dmi_entry, &dmi_results)) {
 		/*
@@ -596,7 +584,7 @@ static int __init dell_wmi_input_setup(void)
 
 	keymap[pos].type = KE_END;
 
-	err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL);
+	err = sparse_keymap_setup(priv->input_dev, keymap, NULL);
 	/*
 	 * Sparse keymap library makes a copy of keymap so we don't need the
 	 * original one that was allocated.
@@ -605,17 +593,24 @@ static int __init dell_wmi_input_setup(void)
 	if (err)
 		goto err_free_dev;
 
-	err = input_register_device(dell_wmi_input_dev);
+	err = input_register_device(priv->input_dev);
 	if (err)
 		goto err_free_dev;
 
 	return 0;
 
  err_free_dev:
-	input_free_device(dell_wmi_input_dev);
+	input_free_device(priv->input_dev);
 	return err;
 }
 
+static void dell_wmi_input_destroy(struct wmi_device *wdev)
+{
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
+
+	input_unregister_device(priv->input_dev);
+}
+
 /*
  * Descriptor buffer is 128 byte long and contains:
  *
@@ -714,46 +709,55 @@ static int dell_wmi_events_set_enabled(bool enable)
 	return dell_smbios_error(ret);
 }
 
+static int dell_wmi_probe(struct wmi_device *wdev)
+{
+	struct dell_wmi_priv *priv = devm_kzalloc(
+		&wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
+
+	dev_set_drvdata(&wdev->dev, priv);
+
+	return dell_wmi_input_setup(wdev);
+}
+
+static int dell_wmi_remove(struct wmi_device *wdev)
+{
+	dell_wmi_input_destroy(wdev);
+	return 0;
+}
+static const struct wmi_device_id dell_wmi_id_table[] = {
+	{ .guid_string = DELL_EVENT_GUID },
+	{ },
+};
+
+static struct wmi_driver dell_wmi_driver = {
+	.driver = {
+		.name = "dell-wmi",
+	},
+	.id_table = dell_wmi_id_table,
+	.probe = dell_wmi_probe,
+	.remove = dell_wmi_remove,
+	.notify = dell_wmi_notify,
+};
+
 static int __init dell_wmi_init(void)
 {
 	int err;
-	acpi_status status;
-
-	if (!wmi_has_guid(DELL_EVENT_GUID) ||
-	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
-		pr_warn("Dell WMI GUID were not found\n");
-		return -ENODEV;
-	}
 
 	err = dell_wmi_check_descriptor_buffer();
 	if (err)
 		return err;
 
-	err = dell_wmi_input_setup();
-	if (err)
-		return err;
-
-	status = wmi_install_notify_handler(DELL_EVENT_GUID,
-					 dell_wmi_notify, NULL);
-	if (ACPI_FAILURE(status)) {
-		input_unregister_device(dell_wmi_input_dev);
-		pr_err("Unable to register notify handler - %d\n", status);
-		return -ENODEV;
-	}
-
 	dmi_check_system(dell_wmi_smbios_list);
 
 	if (wmi_requires_smbios_request) {
 		err = dell_wmi_events_set_enabled(true);
 		if (err) {
 			pr_err("Failed to enable WMI events\n");
-			wmi_remove_notify_handler(DELL_EVENT_GUID);
-			input_unregister_device(dell_wmi_input_dev);
 			return err;
 		}
 	}
 
-	return 0;
+	return wmi_driver_register(&dell_wmi_driver);
 }
 module_init(dell_wmi_init);
 
@@ -761,7 +765,7 @@ static void __exit dell_wmi_exit(void)
 {
 	if (wmi_requires_smbios_request)
 		dell_wmi_events_set_enabled(false);
-	wmi_remove_notify_handler(DELL_EVENT_GUID);
-	input_unregister_device(dell_wmi_input_dev);
+
+	wmi_driver_unregister(&dell_wmi_driver);
 }
 module_exit(dell_wmi_exit);
-- 
2.9.4

Re: [PATCH 16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 398 bytes --]

On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
> -	dell_wmi_input_dev->name = "Dell WMI hotkeys";
> -	dell_wmi_input_dev->phys = "wmi/input0";
> -	dell_wmi_input_dev->id.bustype = BUS_HOST;
> +	priv->input_dev->name = "Dell WMI hotkeys";
> +	priv->input_dev->id.bustype = BUS_HOST;

Is not there BUS_WMI, or something like that? (Just asking)

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

[Andy Lutomirski]

On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
>> -     dell_wmi_input_dev->phys = "wmi/input0";
>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
>> +     priv->input_dev->name = "Dell WMI hotkeys";
>> +     priv->input_dev->id.bustype = BUS_HOST;
>
> Is not there BUS_WMI, or something like that? (Just asking)
>

Jiri and/or Dmitry, what is bustype for, anyway?  I suppose we could
add BUS_PLATFORM.

Re: [PATCH 16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

[Dmitry Torokhov]

On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
>wrote:
>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
>>> -     dell_wmi_input_dev->phys = "wmi/input0";
>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
>>> +     priv->input_dev->name = "Dell WMI hotkeys";
>>> +     priv->input_dev->id.bustype = BUS_HOST;
>>
>> Is not there BUS_WMI, or something like that? (Just asking)
>>
>
>Jiri and/or Dmitry, what is bustype for, anyway? 

The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.

>I suppose we could add BUS_PLATFORM.

What would be the difference from BUS_HOST?


Thanks.

-- 
Dmitry

Re: [PATCH 16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

[Andy Lutomirski]

On Sat, May 27, 2017 at 9:17 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
>>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
>>wrote:
>>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
>>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
>>>> -     dell_wmi_input_dev->phys = "wmi/input0";
>>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
>>>> +     priv->input_dev->name = "Dell WMI hotkeys";
>>>> +     priv->input_dev->id.bustype = BUS_HOST;
>>>
>>> Is not there BUS_WMI, or something like that? (Just asking)
>>>
>>
>>Jiri and/or Dmitry, what is bustype for, anyway?
>
> The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.
>
>>I suppose we could add BUS_PLATFORM.
>
> What would be the difference from BUS_HOST?
>

If BUS_HOST means that the device is part of the host as opposed to
being plugged in, then it seems entirely reasonable.

Re: [PATCH 16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

[Dmitry Torokhov]

On Sat, May 27, 2017 at 11:40:52AM -0700, Andy Lutomirski wrote:
> On Sat, May 27, 2017 at 9:17 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
> >>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
> >>wrote:
> >>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
> >>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
> >>>> -     dell_wmi_input_dev->phys = "wmi/input0";
> >>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
> >>>> +     priv->input_dev->name = "Dell WMI hotkeys";
> >>>> +     priv->input_dev->id.bustype = BUS_HOST;
> >>>
> >>> Is not there BUS_WMI, or something like that? (Just asking)
> >>>
> >>
> >>Jiri and/or Dmitry, what is bustype for, anyway?
> >
> > The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.
> >
> >>I suppose we could add BUS_PLATFORM.
> >
> > What would be the difference from BUS_HOST?
> >
> 
> If BUS_HOST means that the device is part of the host as opposed to
> being plugged in, then it seems entirely reasonable.

Yes, it basically means platform-specific interface.

-- 
Dmitry

Re: [PATCH 16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

[Darren Hart]

On Mon, May 29, 2017 at 07:45:05PM -0700, Dmitry Torokhov wrote:
> On Sat, May 27, 2017 at 11:40:52AM -0700, Andy Lutomirski wrote:
> > On Sat, May 27, 2017 at 9:17 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
> > >>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
> > >>wrote:
> > >>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
> > >>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
> > >>>> -     dell_wmi_input_dev->phys = "wmi/input0";
> > >>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
> > >>>> +     priv->input_dev->name = "Dell WMI hotkeys";
> > >>>> +     priv->input_dev->id.bustype = BUS_HOST;
> > >>>
> > >>> Is not there BUS_WMI, or something like that? (Just asking)
> > >>>
> > >>
> > >>Jiri and/or Dmitry, what is bustype for, anyway?
> > >
> > > The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.
> > >
> > >>I suppose we could add BUS_PLATFORM.
> > >
> > > What would be the difference from BUS_HOST?
> > >
> > 
> > If BUS_HOST means that the device is part of the host as opposed to
> > being plugged in, then it seems entirely reasonable.
> 
> Yes, it basically means platform-specific interface.
> 

I'm going to leave this as BUS_HOST then. Thanks everyone.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 00/16] Convert WMI to a proper bus

[Rafael J. Wysocki]

On Friday, May 26, 2017 10:31:14 PM Darren Hart wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
> 
> This series is based on the original work of
> Andy Lutomirski <luto@amacapital.net> [1]. I have made minor edits, and in
> one instance, squashed two patches in which the latter undid the former.
> 
> This series converts WMI [2] into a proper bus, adds some useful information via
> sysfs, and exposes the embedded MOF [3] binary. It converts dell-wmi to use the
> new WMI bus architecture.
> 
> This is the first part of an ongoing effort to enhance the WMI infrastructure
> within the kernel, and eventually expose WMI to userspace for the consumption of
> management utilities as it was intended.
> 
> 1. https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/log/?h=platform/wmi
> 2. https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx
> 3. https://msdn.microsoft.com/en-us/library/aa823192(v=vs.85).aspx
> 
> Andy Lutomirski (15):
>   platform/x86: wmi: Drop "Mapper (un)loaded" messages
>   platform/x86: wmi: Pass the acpi_device through to parse_wdg
>   platform/x86: wmi: Clean up acpi_wmi_add
>   platform/x86: wmi: Track wmi devices per ACPI device
>   platform/x86: wmi: Turn WMI into a bus driver
>   platform/x86: wmi: Fix error handling when creating devices
>   platform/x86: wmi: Split devices into types and add basic sysfs attributes
>   platform/x86: wmi: Probe data objects for read and write capabilities
>   platform/x86: wmi: Instantiate all devices before adding them
>   platform/x86: wmi: Incorporate acpi_install_notify_handler
>   platform/x86: wmi: Add a new interface to read block data
>   platform/x86: wmi: Bind the platform device, not the ACPI node
>   platform/x86: wmi: Add an interface for subdrivers to access sibling devices
>   platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata
>   platform/x86: dell-wmi: Convert to the WMI bus infrastructure
> 
> Darren Hart (VMware) (1):
>   platform/x86: wmi: Require query for data blocks, rename writable to setable
> 
>  drivers/platform/x86/Kconfig    |  12 +
>  drivers/platform/x86/Makefile   |   1 +
>  drivers/platform/x86/dell-wmi.c | 136 ++++----
>  drivers/platform/x86/wmi-mof.c  | 125 ++++++++
>  drivers/platform/x86/wmi.c      | 677 ++++++++++++++++++++++++++++++++--------
>  include/linux/wmi.h             |  59 ++++
>  6 files changed, 815 insertions(+), 195 deletions(-)
>  create mode 100644 drivers/platform/x86/wmi-mof.c
>  create mode 100644 include/linux/wmi.h

All of this makes sense from the ACPI core perspective, so

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the series and I'm assuming that it will go in via the platform/x86 tree.

Thanks,
Rafael

Re: [PATCH 00/16] Convert WMI to a proper bus

[Andy Shevchenko]

On Sat, May 27, 2017 at 10:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, May 26, 2017 10:31:14 PM Darren Hart wrote:
>> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>>
>> This series is based on the original work of
>> Andy Lutomirski <luto@amacapital.net> [1]. I have made minor edits, and in
>> one instance, squashed two patches in which the latter undid the former.
>>
>> This series converts WMI [2] into a proper bus, adds some useful information via
>> sysfs, and exposes the embedded MOF [3] binary. It converts dell-wmi to use the
>> new WMI bus architecture.
>>
>> This is the first part of an ongoing effort to enhance the WMI infrastructure
>> within the kernel, and eventually expose WMI to userspace for the consumption of
>> management utilities as it was intended.
>>
>> 1. https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/log/?h=platform/wmi
>> 2. https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx
>> 3. https://msdn.microsoft.com/en-us/library/aa823192(v=vs.85).aspx
>>
>> Andy Lutomirski (15):
>>   platform/x86: wmi: Drop "Mapper (un)loaded" messages
>>   platform/x86: wmi: Pass the acpi_device through to parse_wdg
>>   platform/x86: wmi: Clean up acpi_wmi_add
>>   platform/x86: wmi: Track wmi devices per ACPI device
>>   platform/x86: wmi: Turn WMI into a bus driver
>>   platform/x86: wmi: Fix error handling when creating devices
>>   platform/x86: wmi: Split devices into types and add basic sysfs attributes
>>   platform/x86: wmi: Probe data objects for read and write capabilities
>>   platform/x86: wmi: Instantiate all devices before adding them
>>   platform/x86: wmi: Incorporate acpi_install_notify_handler
>>   platform/x86: wmi: Add a new interface to read block data
>>   platform/x86: wmi: Bind the platform device, not the ACPI node
>>   platform/x86: wmi: Add an interface for subdrivers to access sibling devices
>>   platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata
>>   platform/x86: dell-wmi: Convert to the WMI bus infrastructure
>>
>> Darren Hart (VMware) (1):
>>   platform/x86: wmi: Require query for data blocks, rename writable to setable
>>
>>  drivers/platform/x86/Kconfig    |  12 +
>>  drivers/platform/x86/Makefile   |   1 +
>>  drivers/platform/x86/dell-wmi.c | 136 ++++----
>>  drivers/platform/x86/wmi-mof.c  | 125 ++++++++
>>  drivers/platform/x86/wmi.c      | 677 ++++++++++++++++++++++++++++++++--------
>>  include/linux/wmi.h             |  59 ++++
>>  6 files changed, 815 insertions(+), 195 deletions(-)
>>  create mode 100644 drivers/platform/x86/wmi-mof.c
>>  create mode 100644 include/linux/wmi.h
>
> All of this makes sense from the ACPI core perspective, so
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks, Rafael.

> for the series and I'm assuming that it will go in via the platform/x86 tree.

Correct.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/16] Convert WMI to a proper bus

[Darren Hart]

On Fri, May 26, 2017 at 10:31:14PM -0700, Darren Hart wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
> 
> This series is based on the original work of
> Andy Lutomirski <luto@amacapital.net> [1]. I have made minor edits, and in
> one instance, squashed two patches in which the latter undid the former.
> 
> This series converts WMI [2] into a proper bus, adds some useful information via
> sysfs, and exposes the embedded MOF [3] binary. It converts dell-wmi to use the
> new WMI bus architecture.
> 
> This is the first part of an ongoing effort to enhance the WMI infrastructure
> within the kernel, and eventually expose WMI to userspace for the consumption of
> management utilities as it was intended.

I have incorporated all feedback, all of it minor, and with confirmation for
those modifications, have committed the series to the testing branch:

http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/testing

We would appreciate any additional real-world testing anyone can offer.

-- 
Darren Hart
VMware Open Source Technology Center