[PATCH 00/14] Big WMI driver rework

[PATCH 00/14] Big WMI driver rework

[Andy Lutomirski]

Here's v1.  I'm sure it still needs work.  Thoughts?

Andy Lutomirski (14):
  wmi: Drop "Mapper (un)loaded" messages
  wmi: Pass the acpi_device through to parse_wdg
  wmi: Clean up acpi_wmi_add
  wmi: Track wmi devices per ACPI device
  wmi: Turn WMI into a bus driver
  wmi: Fix error handling when creating devices
  wmi: Split devices into types and add basic sysfs attributes
  wmi: Probe data objects for read and write capabilities
  wmi: Instantiate all devices before adding them
  wmi: Add a driver .notify function
  wmi: Add a new interface to read block data
  wmi: Switch from acpi_driver.notify to acpi_install_notify_handler
  wmi: Bind the platform device, not the ACPI node
  dell-wmi: Convert to the WMI bus infrastructure

 drivers/platform/x86/dell-wmi.c | 132 ++++----
 drivers/platform/x86/wmi.c      | 649 ++++++++++++++++++++++++++++++++--------
 include/linux/wmi.h             |  58 ++++
 3 files changed, 644 insertions(+), 195 deletions(-)
 create mode 100644 include/linux/wmi.h

-- 
2.5.0

[PATCH 01/14] wmi: Drop "Mapper (un)loaded" messages

[Andy Lutomirski]

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

Signed-off-by: Andy Lutomirski <luto@kernel.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 eb391a281833..a17c3d5effe4 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -914,7 +914,6 @@ static int __init acpi_wmi_init(void)
 		return error;
 	}
 
-	pr_info("Mapper loaded\n");
 	return 0;
 }
 
@@ -922,8 +921,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.5.0

[PATCH 02/14] wmi: Pass the acpi_device through to parse_wdg

[Andy Lutomirski]

Signed-off-by: Andy Lutomirski <luto@kernel.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 a17c3d5effe4..383efb8260c7 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -684,7 +684,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;
 
@@ -723,7 +724,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;
@@ -733,7 +734,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;
 
@@ -757,7 +758,7 @@ static int parse_wdg(acpi_handle handle)
 		if (!wblock)
 			return -ENOMEM;
 
-		wblock->handle = handle;
+		wblock->handle = device->handle;
 		wblock->gblock = gblock[i];
 
 		/*
@@ -767,7 +768,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;
@@ -884,7 +885,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.5.0

[PATCH 03/14] wmi: Clean up acpi_wmi_add

[Andy Lutomirski]

Rearrange acpi_wmi_add to use Linux's error handling conventions.

Signed-off-by: Andy Lutomirski <luto@kernel.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 383efb8260c7..fc1f339b7431 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -881,20 +881,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.5.0

[PATCH 04/14] wmi: Track wmi devices per ACPI device

[Andy Lutomirski]

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're 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 aren't 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>
---
 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 fc1f339b7431..dcb34a5f5669 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -63,7 +63,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;
@@ -225,7 +225,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);
@@ -264,7 +264,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;
@@ -326,7 +326,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;
@@ -399,7 +399,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;
@@ -603,8 +603,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;
@@ -696,27 +696,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;
 }
@@ -758,7 +771,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];
 
 		/*
@@ -767,10 +780,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;
 			}
 		}
@@ -845,8 +858,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) {
@@ -866,7 +880,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.5.0

[PATCH 05/14] wmi: Turn WMI into a bus driver

[Andy Lutomirski]

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>
---
 drivers/platform/x86/wmi.c | 198 +++++++++++++++++++++++++++++++++++----------
 include/linux/wmi.h        |  47 +++++++++++
 2 files changed, 202 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 dcb34a5f5669..2fa9493bd35c 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -37,14 +37,13 @@
 #include <linux/acpi.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/wmi.h>
 
 ACPI_MODULE_NAME("wmi");
 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 {
@@ -61,12 +60,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;
 };
 
 
@@ -101,8 +100,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,
@@ -623,77 +622,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) {
+		u8 tmp[16], driver_guid[16];
+
+		wmi_parse_guid(id->guid_string, tmp);
+		wmi_swap_bytes(tmp, driver_guid);
+		if (!memcmp(driver_guid, wblock->gblock.guid, 16))
+			return 1;
+
+		id++;
+	}
 
 	return 0;
 }
 
-static void wmi_dev_free(struct device *dev)
+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;
+}
+
+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)
@@ -704,8 +772,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);
 		}
@@ -737,7 +805,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;
@@ -781,7 +849,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;
@@ -881,12 +950,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;
 
@@ -899,14 +971,26 @@ 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);
+	put_device(wmi_bus_dev);
+
 err_remove_handler:
 	acpi_remove_address_space_handler(device->handle,
 					  ACPI_ADR_SPACE_EC,
@@ -915,6 +999,22 @@ err_remove_handler:
 	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;
@@ -922,24 +1022,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 000000000000..29ed34b4dae1
--- /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.5.0

Re: [PATCH 05/14] wmi: Turn WMI into a bus driver

[Michał Kępień]

> 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>
> ---
>  drivers/platform/x86/wmi.c | 198 +++++++++++++++++++++++++++++++++++----------
>  include/linux/wmi.h        |  47 +++++++++++
>  2 files changed, 202 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 dcb34a5f5669..2fa9493bd35c 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -37,14 +37,13 @@
>  #include <linux/acpi.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/wmi.h>
>  
>  ACPI_MODULE_NAME("wmi");
>  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 {
> @@ -61,12 +60,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;
>  };
>  
>  
> @@ -101,8 +100,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,
> @@ -623,77 +622,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) {
> +		u8 tmp[16], driver_guid[16];
> +
> +		wmi_parse_guid(id->guid_string, tmp);
> +		wmi_swap_bytes(tmp, driver_guid);
> +		if (!memcmp(driver_guid, wblock->gblock.guid, 16))
> +			return 1;
> +
> +		id++;
> +	}
>  
>  	return 0;
>  }
>  
> -static void wmi_dev_free(struct device *dev)
> +int wmi_dev_probe(struct device *dev)

Can be static.

>  {
> -	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;
> +}
> +
> +int wmi_dev_remove(struct device *dev)

Can be static.

> +{
> +	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)
> @@ -704,8 +772,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);
>  		}
> @@ -737,7 +805,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;
> @@ -781,7 +849,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;
> @@ -881,12 +950,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;
>  
> @@ -899,14 +971,26 @@ 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);
> +	put_device(wmi_bus_dev);

I believe this put_device() call is excessive.  device_create() calls
device_initialize() and device_add(); device_unregister() calls
device_del() and put_device(), so it should already take care of
removing all references to wmi_bus_dev.  Am I missing something?

-- 
Best regards,
Michał Kępień

Re: [PATCH 05/14] wmi: Turn WMI into a bus driver

[Andy Lutomirski]

On Sat, Jan 16, 2016 at 4:56 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> 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>
>> ---
>>  drivers/platform/x86/wmi.c | 198 +++++++++++++++++++++++++++++++++++----------
>>  include/linux/wmi.h        |  47 +++++++++++
>>  2 files changed, 202 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 dcb34a5f5669..2fa9493bd35c 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -37,14 +37,13 @@
>>  #include <linux/acpi.h>
>>  #include <linux/slab.h>
>>  #include <linux/module.h>
>> +#include <linux/wmi.h>
>>
>>  ACPI_MODULE_NAME("wmi");
>>  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 {
>> @@ -61,12 +60,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;
>>  };
>>
>>
>> @@ -101,8 +100,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,
>> @@ -623,77 +622,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) {
>> +             u8 tmp[16], driver_guid[16];
>> +
>> +             wmi_parse_guid(id->guid_string, tmp);
>> +             wmi_swap_bytes(tmp, driver_guid);
>> +             if (!memcmp(driver_guid, wblock->gblock.guid, 16))
>> +                     return 1;
>> +
>> +             id++;
>> +     }
>>
>>       return 0;
>>  }
>>
>> -static void wmi_dev_free(struct device *dev)
>> +int wmi_dev_probe(struct device *dev)
>
> Can be static.

Indeed, thanks.

>
>>  {
>> -     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;
>> +}
>> +
>> +int wmi_dev_remove(struct device *dev)
>
> Can be static.

Fixed.

>
>> +{
>> +     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)
>> @@ -704,8 +772,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);
>>               }
>> @@ -737,7 +805,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;
>> @@ -781,7 +849,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;
>> @@ -881,12 +950,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;
>>
>> @@ -899,14 +971,26 @@ 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);
>> +     put_device(wmi_bus_dev);
>
> I believe this put_device() call is excessive.  device_create() calls
> device_initialize() and device_add(); device_unregister() calls
> device_del() and put_device(), so it should already take care of
> removing all references to wmi_bus_dev.  Am I missing something?
>

I think you're right, and my acpi_wmi_remove agrees.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/14] wmi: Fix error handling when creating devices

[Andy Lutomirski]

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

Simplify the logic and fix both of them.

Signed-off-by: Andy Lutomirski <luto@kernel.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 2fa9493bd35c..c181cf5ecdc6 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -835,6 +835,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;
@@ -842,19 +851,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.5.0

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

[Andy Lutomirski]

This devices 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>
---
 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 c181cf5ecdc6..5571ae7354a1 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -653,13 +653,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);
@@ -749,6 +801,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,
@@ -759,7 +829,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.5.0

[PATCH 08/14] wmi: Probe data objects for read and write capabilities

[Andy Lutomirski]

My laptop 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>
---
 drivers/platform/x86/wmi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/wmi.h        |  6 +++
 2 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 5571ae7354a1..49be61207c4a 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -66,6 +66,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 */
 };
 
 
@@ -216,6 +218,25 @@ 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)
+{
+	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;
+
+	status = acpi_get_object_info(subobj_handle, &info);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
+}
+
 static acpi_status wmi_method_enable(struct wmi_block *wblock, int enable)
 {
 	struct guid_block *block = NULL;
@@ -339,6 +360,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.
@@ -706,11 +730,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_or_method);
+ATTRIBUTE_GROUPS(wmi_data);
+
+static struct attribute *wmi_method_attrs[] = {
+	&dev_attr_object_id.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wmi_method);
 
 static int wmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
@@ -809,13 +859,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,
 };
 
@@ -834,7 +884,43 @@ 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;
+		}
+
+		strcpy(method, "WS");
+		strncat(method, wblock->gblock.object_id, 2);
+		result = get_subobj_info(device->handle, method, &info);
+
+		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 29ed34b4dae1..53095006821e 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.5.0

Re: [PATCH 08/14] wmi: Probe data objects for read and write capabilities

[Michał Kępień]

> My laptop 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>
> ---
>  drivers/platform/x86/wmi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/wmi.h        |  6 +++
>  2 files changed, 96 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 5571ae7354a1..49be61207c4a 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -66,6 +66,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 */
>  };
>  
>  
> @@ -216,6 +218,25 @@ 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)
> +{
> +	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;
> +
> +	status = acpi_get_object_info(subobj_handle, &info);

acpi_get_object_info() allocates the returned buffer itself.  The latter
should then subsequently be freed by the caller.  One solution is to
change get_subobj_info() so that it takes a struct acpi_device_info **
as an argument, which should then be passed to acpi_get_object_info().
See also below.

> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
>  static acpi_status wmi_method_enable(struct wmi_block *wblock, int enable)
>  {
>  	struct guid_block *block = NULL;
> @@ -339,6 +360,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.
> @@ -706,11 +730,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_or_method);
> +ATTRIBUTE_GROUPS(wmi_data);
> +
> +static struct attribute *wmi_method_attrs[] = {
> +	&dev_attr_object_id.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(wmi_method);
>  
>  static int wmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
> @@ -809,13 +859,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,
>  };
>  
> @@ -834,7 +884,43 @@ 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;

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, &info);
> +
> +		if (result == 0) {
> +			wblock->dev.writeable = true;

kfree(info)

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/14] wmi: Probe data objects for read and write capabilities

[Andy Lutomirski]

On Sat, Jan 16, 2016 at 5:11 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> My laptop 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>
>> ---
>>  drivers/platform/x86/wmi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
>>  include/linux/wmi.h        |  6 +++
>>  2 files changed, 96 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index 5571ae7354a1..49be61207c4a 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -66,6 +66,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 */
>>  };
>>
>>
>> @@ -216,6 +218,25 @@ 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)
>> +{
>> +     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;
>> +
>> +     status = acpi_get_object_info(subobj_handle, &info);
>
> acpi_get_object_info() allocates the returned buffer itself.  The latter
> should then subsequently be freed by the caller.  One solution is to
> change get_subobj_info() so that it takes a struct acpi_device_info **
> as an argument, which should then be passed to acpi_get_object_info().
> See also below.

I have an updated version of this patch in my queue.  I'll try to send
it early next week.

--Andy

Re: [PATCH 08/14] wmi: Probe data objects for read and write capabilities

[Michał Kępień]

> On Sat, Jan 16, 2016 at 5:11 AM, Michał Kępień <kernel@kempniu.pl> wrote:
> >> My laptop 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>
> >> ---
> >>  drivers/platform/x86/wmi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
> >>  include/linux/wmi.h        |  6 +++
> >>  2 files changed, 96 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> >> index 5571ae7354a1..49be61207c4a 100644
> >> --- a/drivers/platform/x86/wmi.c
> >> +++ b/drivers/platform/x86/wmi.c
> >> @@ -66,6 +66,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 */
> >>  };
> >>
> >>
> >> @@ -216,6 +218,25 @@ 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)
> >> +{
> >> +     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;
> >> +
> >> +     status = acpi_get_object_info(subobj_handle, &info);
> >
> > acpi_get_object_info() allocates the returned buffer itself.  The latter
> > should then subsequently be freed by the caller.  One solution is to
> > change get_subobj_info() so that it takes a struct acpi_device_info **
> > as an argument, which should then be passed to acpi_get_object_info().
> > See also below.
> 
> I have an updated version of this patch in my queue.  I'll try to send
> it early next week.

Hi Andy,

Are you planning any further work on this patch series (originally
posted in December 2015)?  It seems to be a valuable cleanup.

-- 
Best regards,
Michał Kępień

Re: [PATCH 08/14] wmi: Probe data objects for read and write capabilities

[Andy Lutomirski]

On Mon, Jan 9, 2017 at 9:56 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>> On Sat, Jan 16, 2016 at 5:11 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> >> My laptop 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>
>> >> ---
>> >>  drivers/platform/x86/wmi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
>> >>  include/linux/wmi.h        |  6 +++
>> >>  2 files changed, 96 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> >> index 5571ae7354a1..49be61207c4a 100644
>> >> --- a/drivers/platform/x86/wmi.c
>> >> +++ b/drivers/platform/x86/wmi.c
>> >> @@ -66,6 +66,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 */
>> >>  };
>> >>
>> >>
>> >> @@ -216,6 +218,25 @@ 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)
>> >> +{
>> >> +     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;
>> >> +
>> >> +     status = acpi_get_object_info(subobj_handle, &info);
>> >
>> > acpi_get_object_info() allocates the returned buffer itself.  The latter
>> > should then subsequently be freed by the caller.  One solution is to
>> > change get_subobj_info() so that it takes a struct acpi_device_info **
>> > as an argument, which should then be passed to acpi_get_object_info().
>> > See also below.
>>
>> I have an updated version of this patch in my queue.  I'll try to send
>> it early next week.
>
> Hi Andy,
>
> Are you planning any further work on this patch series (originally
> posted in December 2015)?  It seems to be a valuable cleanup.

I have a tree in which the patches are sitting and I've kept them
rebased, but I haven't had a chance to go through all the review
comments and send them out.  Want to do it?

--Andy

Re: [PATCH 08/14] wmi: Probe data objects for read and write capabilities

[Michał Kępień]

> On Mon, Jan 9, 2017 at 9:56 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> >> On Sat, Jan 16, 2016 at 5:11 AM, Michał Kępień <kernel@kempniu.pl> wrote:
> >> >> My laptop 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>
> >> >> ---
> >> >>  drivers/platform/x86/wmi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
> >> >>  include/linux/wmi.h        |  6 +++
> >> >>  2 files changed, 96 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> >> >> index 5571ae7354a1..49be61207c4a 100644
> >> >> --- a/drivers/platform/x86/wmi.c
> >> >> +++ b/drivers/platform/x86/wmi.c
> >> >> @@ -66,6 +66,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 */
> >> >>  };
> >> >>
> >> >>
> >> >> @@ -216,6 +218,25 @@ 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)
> >> >> +{
> >> >> +     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;
> >> >> +
> >> >> +     status = acpi_get_object_info(subobj_handle, &info);
> >> >
> >> > acpi_get_object_info() allocates the returned buffer itself.  The latter
> >> > should then subsequently be freed by the caller.  One solution is to
> >> > change get_subobj_info() so that it takes a struct acpi_device_info **
> >> > as an argument, which should then be passed to acpi_get_object_info().
> >> > See also below.
> >>
> >> I have an updated version of this patch in my queue.  I'll try to send
> >> it early next week.
> >
> > Hi Andy,
> >
> > Are you planning any further work on this patch series (originally
> > posted in December 2015)?  It seems to be a valuable cleanup.
> 
> I have a tree in which the patches are sitting and I've kept them
> rebased, but I haven't had a chance to go through all the review
> comments and send them out.  Want to do it?

If you want me to take the rebased patches, fix the issues I can find
and resubmit as v2 with your name in From: then sure, I can give it a
shot once I find the time (might not be soon, though, but I guess there
is no rush).  If you had something else on your mind, please elaborate.

-- 
Best regards,
Michał Kępień

Re: [PATCH 08/14] wmi: Probe data objects for read and write capabilities

[Andy Lutomirski]

On Wed, Jan 11, 2017 at 9:01 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>> On Mon, Jan 9, 2017 at 9:56 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>> >> On Sat, Jan 16, 2016 at 5:11 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> >> >> My laptop 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>
>> >> >> ---
>> >> >>  drivers/platform/x86/wmi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
>> >> >>  include/linux/wmi.h        |  6 +++
>> >> >>  2 files changed, 96 insertions(+), 4 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> >> >> index 5571ae7354a1..49be61207c4a 100644
>> >> >> --- a/drivers/platform/x86/wmi.c
>> >> >> +++ b/drivers/platform/x86/wmi.c
>> >> >> @@ -66,6 +66,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 */
>> >> >>  };
>> >> >>
>> >> >>
>> >> >> @@ -216,6 +218,25 @@ 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)
>> >> >> +{
>> >> >> +     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;
>> >> >> +
>> >> >> +     status = acpi_get_object_info(subobj_handle, &info);
>> >> >
>> >> > acpi_get_object_info() allocates the returned buffer itself.  The latter
>> >> > should then subsequently be freed by the caller.  One solution is to
>> >> > change get_subobj_info() so that it takes a struct acpi_device_info **
>> >> > as an argument, which should then be passed to acpi_get_object_info().
>> >> > See also below.
>> >>
>> >> I have an updated version of this patch in my queue.  I'll try to send
>> >> it early next week.
>> >
>> > Hi Andy,
>> >
>> > Are you planning any further work on this patch series (originally
>> > posted in December 2015)?  It seems to be a valuable cleanup.
>>
>> I have a tree in which the patches are sitting and I've kept them
>> rebased, but I haven't had a chance to go through all the review
>> comments and send them out.  Want to do it?
>
> If you want me to take the rebased patches, fix the issues I can find
> and resubmit as v2 with your name in From: then sure, I can give it a
> shot once I find the time (might not be soon, though, but I guess there
> is no rush).  If you had something else on your mind, please elaborate.
>

That would be fantastic.  Let me know if/when you get started.  In the
mean time, the rebased and slightly updated series is here:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=platform/wmi

--Andy

[PATCH 09/14] wmi: Instantiate all devices before adding them

[Andy Lutomirski]

At some point, we'll want to subdrivers 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 fixng some leaks, too.

Signed-off-by: Andy Lutomirski <luto@kernel.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 49be61207c4a..7d8a11a45bf9 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -869,7 +869,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)
@@ -923,7 +923,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)
@@ -934,10 +934,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);
+			}
 		}
 	}
 }
@@ -972,9 +976,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);
@@ -1007,19 +1011,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);
 
@@ -1029,11 +1029,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.5.0

Re: [PATCH 09/14] wmi: Instantiate all devices before adding them

[Michał Kępień]

> At some point, we'll want to subdrivers to get references to other

Did you mean "(...) we'll want subdrivers to get references to (...)"
(without the first "to")?

> devices on the same WMI bus.  This change is needed to avoid races.
> 
> This ends up simplifying the setup code and fixng some leaks, too.

fixing

> 
> Signed-off-by: Andy Lutomirski <luto@kernel.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 49be61207c4a..7d8a11a45bf9 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -869,7 +869,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)
> @@ -923,7 +923,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)
> @@ -934,10 +934,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);

Is there any specific reason for splitting device_unregister() into two
separate calls it performs?

> +			} else {
> +				/* device_initialize was not called. */
>  				kfree(wblock);

This kfree() obviously wasn't introduced by you, but I believe that
after patch 06 from this series is applied, this branch cannot be
reached any more.  If I understand correctly, it could only be reached
(using code without this patch series applied) when duplicate GUIDs were
present, for which a struct wmi_block was allocated and added to
wmi_block_list, but for which wmi_create_device() wasn't called.  Patch
06 prevents a struct wmi_block from ever being allocated for duplicate
GUIDs and this patch ensures that even if device_add() fails for some
reason, the relevant struct wmi_block is removed from wmi_block_list and
freed anyway.  I also don't see any way for a struct wmi_block to be
inserted into wmi_block_list without both wmi_create_device() (so
device_initialize()) and device_add() being called.  So perhaps this
kfree() call (and the relevant conditional expression) could be removed
in another patch?

> +			}
>  		}
>  	}
>  }
> @@ -972,9 +976,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);
> @@ -1007,19 +1011,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);
>  
> @@ -1029,11 +1029,24 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
>  		}
>  	}
>  
> -	retval = 0;
> -
>  out_free_pointer:
>  	kfree(out.pointer);

Perhaps this label and the kfree() call should be moved closer to the
return statement below to avoid confusion?  If obj is not a buffer then
no new struct wmi_block(s) will be added to wmi_block_list, so there is
little point in iterating over the latter below.

>  
> +	/*
> +	 * 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);

A put_device(&wblock->dev.dev) is missing here.  If we don't do that,
the reference count for wblock->dev.dev will still be at 1 due to
device_initialize() being called before and we will leak wblock.

I have one more minor issue with error handling for device_add().  A bit
earlier, in the GUID-processing loop, there's this snippet:

	if (debug_event) {
		wblock->handler = wmi_notify_debug;
		wmi_method_enable(wblock, 1);
	}

So we should probably put:

	if (debug_event)
		wmi_method_enable(wblock, 0);

before the put_device() call I mentioned above.

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/14] wmi: Add a driver .notify function

[Andy Lutomirski]

This gives event drivers a very 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>
---
 drivers/platform/x86/wmi.c | 62 ++++++++++++++++++++++++++++++++++++++--------
 include/linux/wmi.h        |  1 +
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7d8a11a45bf9..24fabfef5e8e 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1099,6 +1099,7 @@ static void acpi_wmi_notify(struct acpi_device *device, u32 event)
 	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);
@@ -1106,20 +1107,59 @@ static void acpi_wmi_notify(struct acpi_device *device, u32 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) {
-				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(
+		device->pnp.device_class, dev_name(&device->dev),
+		event, 0);
+
 }
 
 static int acpi_wmi_remove(struct acpi_device *device)
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 53095006821e..c6eedfd94e7d 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.5.0

[PATCH 11/14] wmi: Add a new interface to read block data

[Andy Lutomirski]

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>
---
 drivers/platform/x86/wmi.c | 52 +++++++++++++++++++++++++++++++++-------------
 include/linux/wmi.h        |  4 ++++
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 24fabfef5e8e..29448289f660 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -319,19 +319,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;
@@ -339,12 +330,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;
 
@@ -395,8 +383,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
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index c6eedfd94e7d..0ab254019488 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.5.0

[PATCH 12/14] wmi: Switch from acpi_driver.notify to acpi_install_notify_handler

[Andy Lutomirski]

Arguably wmi should attach to the PNP platform device, not the ACPI
device.  As a platform driver, acpi_driver.notify won't be
available, so stop using it.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/platform/x86/wmi.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 29448289f660..88265b28430c 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -92,7 +92,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},
@@ -108,7 +107,6 @@ static struct acpi_driver acpi_wmi_driver = {
 	.ops = {
 		.add = acpi_wmi_add,
 		.remove = acpi_wmi_remove,
-		.notify = acpi_wmi_notify,
 	},
 };
 
@@ -1116,7 +1114,8 @@ 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;
@@ -1127,7 +1126,7 @@ static void acpi_wmi_notify(struct acpi_device *device, u32 event)
 		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))
 		{
@@ -1179,13 +1178,16 @@ static void acpi_wmi_notify(struct acpi_device *device, u32 event)
 	}
 
 	acpi_bus_generate_netlink_event(
-		device->pnp.device_class, dev_name(&device->dev),
+		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);
@@ -1210,11 +1212,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;
 
@@ -1230,7 +1241,11 @@ err_remove_busdev:
 	device_unregister(wmi_bus_dev);
 	put_device(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);
-- 
2.5.0

[PATCH 13/14] wmi: Bind the platform device, not the ACPI node

[Andy Lutomirski]

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>
---
 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 88265b28430c..100130895fe2 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>
 
 ACPI_MODULE_NAME("wmi");
@@ -90,8 +91,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},
@@ -100,14 +101,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,
 };
 
 /*
@@ -1184,26 +1184,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);
@@ -1212,7 +1220,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)) {
@@ -1227,9 +1236,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;
@@ -1242,11 +1251,11 @@ err_remove_busdev:
 	put_device(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);
 
@@ -1284,7 +1293,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;
@@ -1303,7 +1312,7 @@ err_unreg_bus:
 
 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.5.0

[PATCH 14/14] dell-wmi: Convert to the WMI bus infrastructure

[Andy Lutomirski]

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/platform/x86/dell-wmi.c | 132 ++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 66 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 7c3ebda811ca..48423e53bf94 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -35,6 +35,7 @@
 #include <linux/acpi.h>
 #include <linux/string.h>
 #include <linux/dmi.h>
+#include <linux/wmi.h>
 #include <acpi/video.h>
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
@@ -47,13 +48,17 @@ static int acpi_video;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 
+struct dell_wmi_priv {
+	struct input_dev *input_dev;
+};
+
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * 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_legacy_keymap[] __initconst = {
+static const struct key_entry dell_wmi_legacy_keymap[] = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
 	{ KE_KEY, 0xe045, { KEY_PROG1 } },
@@ -119,7 +124,7 @@ struct dell_bios_hotkey_table {
 static const struct dell_bios_hotkey_table *dell_bios_hotkey_table;
 
 /* 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,
@@ -163,7 +168,7 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
 };
 
 /* These are applied if the hk table is present and doesn't override them. */
-static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
+static const struct key_entry dell_wmi_extra_keymap[] = {
 	/* Fn-lock */
 	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
 
@@ -183,13 +188,12 @@ static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
 	{ KE_IGNORE, 0x155, { KEY_RESERVED } },
 };
 
-static struct input_dev *dell_wmi_input_dev;
-
-static void dell_wmi_process_key(int reported_key)
+static void dell_wmi_process_key(struct wmi_device *wdev, int reported_key)
 {
+	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,
 						reported_key);
 	if (!key) {
 		pr_info("Unknown key with scancode 0x%x pressed\n",
@@ -204,33 +208,18 @@ static void dell_wmi_process_key(int reported_key)
 	     key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
 		return;
 
-	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;
 	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;
 	}
 
@@ -242,12 +231,11 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	if (!dell_new_hk_type) {
 		if (buffer_size >= 3 && buffer_entry[1] == 0x0)
-			dell_wmi_process_key(buffer_entry[2]);
+			dell_wmi_process_key(wdev, buffer_entry[2]);
 		else if (buffer_size >= 2)
-			dell_wmi_process_key(buffer_entry[1]);
+			dell_wmi_process_key(wdev, buffer_entry[1]);
 		else
 			pr_info("Received unknown WMI event\n");
-		kfree(obj);
 		return;
 	}
 
@@ -293,7 +281,7 @@ static void dell_wmi_notify(u32 value, void *context)
 		case 0x10:
 			/* Keys pressed */
 			for (i = 2; i < len; ++i)
-				dell_wmi_process_key(buffer_entry[i]);
+				dell_wmi_process_key(wdev, buffer_entry[i]);
 			break;
 		case 0x11:
 			for (i = 2; i < len; ++i) {
@@ -334,7 +322,6 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	}
 
-	kfree(obj);
 }
 
 static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
@@ -407,17 +394,18 @@ skip:
 	return keymap;
 }
 
-static int __init dell_wmi_input_setup(void)
+static int dell_wmi_input_setup(struct wmi_device *wdev)
 {
 	int err;
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
 
-	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 (dell_new_hk_type) {
 		const struct key_entry *keymap = dell_wmi_prepare_new_keymap();
@@ -426,7 +414,7 @@ static int __init dell_wmi_input_setup(void)
 			goto err_free_dev;
 		}
 
-		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
@@ -434,29 +422,31 @@ static int __init dell_wmi_input_setup(void)
 		 */
 		kfree(keymap);
 	} else {
-		err = sparse_keymap_setup(dell_wmi_input_dev,
+		err = sparse_keymap_setup(priv->input_dev,
 					  dell_wmi_legacy_keymap, NULL);
 	}
 	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_keymap;
 
 	return 0;
 
  err_free_keymap:
-	sparse_keymap_free(dell_wmi_input_dev);
+	sparse_keymap_free(priv->input_dev);
  err_free_dev:
-	input_free_device(dell_wmi_input_dev);
+	input_free_device(priv->input_dev);
 	return err;
 }
 
-static void dell_wmi_input_destroy(void)
+static void dell_wmi_input_destroy(struct wmi_device *wdev)
 {
-	sparse_keymap_free(dell_wmi_input_dev);
-	input_unregister_device(dell_wmi_input_dev);
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
+
+	sparse_keymap_free(priv->input_dev);
+	input_unregister_device(priv->input_dev);
 }
 
 static void __init find_hk_type(const struct dmi_header *dm, void *dummy)
@@ -468,38 +458,48 @@ static void __init find_hk_type(const struct dmi_header *dm, void *dummy)
 	}
 }
 
-static int __init dell_wmi_init(void)
+static int dell_wmi_probe(struct wmi_device *wdev)
 {
-	int err;
-	acpi_status status;
+	struct dell_wmi_priv *priv = devm_kzalloc(
+		&wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
 
-	if (!wmi_has_guid(DELL_EVENT_GUID)) {
-		pr_warn("No known WMI GUID found\n");
-		return -ENODEV;
-	}
+	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)
+{
 	dmi_walk(find_hk_type, NULL);
 	acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
 
-	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)) {
-		dell_wmi_input_destroy();
-		pr_err("Unable to register notify handler - %d\n", status);
-		return -ENODEV;
-	}
-
-	return 0;
+	return wmi_driver_register(&dell_wmi_driver);
 }
 module_init(dell_wmi_init);
 
 static void __exit dell_wmi_exit(void)
 {
-	wmi_remove_notify_handler(DELL_EVENT_GUID);
-	dell_wmi_input_destroy();
+	wmi_driver_unregister(&dell_wmi_driver);
 }
 module_exit(dell_wmi_exit);
-- 
2.5.0