[PATCH 1/3] platform/x86: wmi: Remove wmi_block_list

[PATCH 1/3] platform/x86: wmi: Remove wmi_block_list

[Armin Wolf]

The wmi_block_list is only used by guid_count() and without proper
protection. It also duplicates some of the WMI bus functionality.

Remove the wmi_block_list and use bus_for_each_dev() instead.

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

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 2d6885c67ac0..4704e79197f6 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -22,7 +22,6 @@
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
-#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/rwsem.h>
@@ -37,8 +36,6 @@ MODULE_AUTHOR("Carlos Corbacho");
 MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
 MODULE_LICENSE("GPL");

-static LIST_HEAD(wmi_block_list);
-
 struct guid_block {
 	guid_t guid;
 	union {
@@ -63,7 +60,6 @@ enum {	/* wmi_block flags */

 struct wmi_block {
 	struct wmi_device dev;
-	struct list_head list;
 	struct guid_block gblock;
 	struct acpi_device *acpi_device;
 	struct rw_semaphore notify_lock;	/* Protects notify callback add/remove */
@@ -73,6 +69,10 @@ struct wmi_block {
 	unsigned long flags;
 };

+struct wmi_guid_count_context {
+	const guid_t *guid;
+	int count;
+};

 /*
  * If the GUID data block is marked as expensive, we must enable and
@@ -942,21 +942,30 @@ static const struct device_type wmi_type_data = {
 	.release = wmi_dev_release,
 };

-/*
- * _WDG is a static list that is only parsed at startup,
- * so it's safe to count entries without extra protection.
- */
+static int wmi_count_guids(struct device *dev, void *data)
+{
+	struct wmi_guid_count_context *context = data;
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	if (guid_equal(&wblock->gblock.guid, context->guid))
+		context->count++;
+
+	return 0;
+}
+
 static int guid_count(const guid_t *guid)
 {
-	struct wmi_block *wblock;
-	int count = 0;
+	struct wmi_guid_count_context context = {
+		.guid = guid,
+		.count = 0,
+	};
+	int ret;

-	list_for_each_entry(wblock, &wmi_block_list, list) {
-		if (guid_equal(&wblock->gblock.guid, guid))
-			count++;
-	}
+	ret = bus_for_each_dev(&wmi_bus_type, NULL, &context, wmi_count_guids);
+	if (ret < 0)
+		return ret;

-	return count;
+	return context.count;
 }

 static int wmi_create_device(struct device *wmi_bus_dev,
@@ -967,7 +976,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 	struct acpi_device_info *info;
 	acpi_handle method_handle;
 	acpi_status status;
-	uint count;
+	int count;

 	if (wblock->gblock.flags & ACPI_WMI_EVENT) {
 		wblock->dev.dev.type = &wmi_type_event;
@@ -1035,6 +1044,9 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 	wblock->dev.dev.parent = wmi_bus_dev;

 	count = guid_count(&wblock->gblock.guid);
+	if (count < 0)
+		return count;
+
 	if (count) {
 		dev_set_name(&wblock->dev.dev, "%pUL-%d", &wblock->gblock.guid, count);
 		set_bit(WMI_GUID_DUPLICATED, &wblock->flags);
@@ -1120,14 +1132,11 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 			continue;
 		}

-		list_add_tail(&wblock->list, &wmi_block_list);
-
 		retval = wmi_add_device(pdev, &wblock->dev);
 		if (retval) {
 			dev_err(wmi_bus_dev, "failed to register %pUL\n",
 				&wblock->gblock.guid);

-			list_del(&wblock->list);
 			put_device(&wblock->dev.dev);
 		}
 	}
@@ -1227,9 +1236,6 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event, void *context

 static int wmi_remove_device(struct device *dev, void *data)
 {
-	struct wmi_block *wblock = dev_to_wblock(dev);
-
-	list_del(&wblock->list);
 	device_unregister(dev);

 	return 0;
--
2.39.5

[PATCH 2/3] platform/x86: wmi: Replace dev_to_wdev() with to_wmi_device()

[Armin Wolf]

Replace dev_to_wdev() with to_wmi_device() to stop duplicating
functionality.

Also switch to_wmi_device() to use container_of_const() so const
values are handled correctly.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 9 ++++-----
 include/linux/wmi.h        | 2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4704e79197f6..b2576d5189ed 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -91,7 +91,6 @@ static const struct acpi_device_id wmi_device_ids[] = {
 MODULE_DEVICE_TABLE(acpi, wmi_device_ids);

 #define dev_to_wblock(__dev)	container_of_const(__dev, struct wmi_block, dev.dev)
-#define dev_to_wdev(__dev)	container_of_const(__dev, struct wmi_device, dev)

 /*
  * GUID parsing functions
@@ -199,7 +198,7 @@ static struct wmi_device *wmi_find_device_by_guid(const char *guid_string)
 	if (!dev)
 		return ERR_PTR(-ENODEV);

-	return dev_to_wdev(dev);
+	return to_wmi_device(dev);
 }

 static void wmi_device_put(struct wmi_device *wdev)
@@ -761,7 +760,7 @@ static DEVICE_ATTR_RO(object_id);
 static ssize_t setable_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
-	struct wmi_device *wdev = dev_to_wdev(dev);
+	struct wmi_device *wdev = to_wmi_device(dev);

 	return sysfs_emit(buf, "%d\n", (int)wdev->setable);
 }
@@ -851,7 +850,7 @@ static int wmi_dev_probe(struct device *dev)
 		dev_warn(dev, "failed to enable device -- probing anyway\n");

 	if (wdriver->probe) {
-		ret = wdriver->probe(dev_to_wdev(dev),
+		ret = wdriver->probe(to_wmi_device(dev),
 				find_guid_context(wblock, wdriver));
 		if (ret) {
 			if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
@@ -878,7 +877,7 @@ static void wmi_dev_remove(struct device *dev)
 	up_write(&wblock->notify_lock);

 	if (wdriver->remove)
-		wdriver->remove(dev_to_wdev(dev));
+		wdriver->remove(to_wmi_device(dev));

 	if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
 		dev_warn(dev, "failed to disable device\n");
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 120019677fc6..fca5fd43c707 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -34,7 +34,7 @@ struct wmi_device {
  *
  * Cast a struct device to a struct wmi_device.
  */
-#define to_wmi_device(device)	container_of(device, struct wmi_device, dev)
+#define to_wmi_device(device)	container_of_const(device, struct wmi_device, dev)

 extern acpi_status wmidev_evaluate_method(struct wmi_device *wdev,
 					  u8 instance, u32 method_id,
--
2.39.5

[PATCH 3/3] platform/x86: wmi: Introduce to_wmi_driver()

[Armin Wolf]

Introduce to_wmi_driver() as a replacement for dev_to_wdrv()
so WMI drivers can use this support macro instead of having
to duplicate its functionality.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 12 +++++-------
 include/linux/wmi.h        |  8 ++++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index b2576d5189ed..646370bd6b03 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -653,8 +653,6 @@ char *wmi_get_acpi_device_uid(const char *guid_string)
 }
 EXPORT_SYMBOL_GPL(wmi_get_acpi_device_uid);

-#define drv_to_wdrv(__drv)	container_of_const(__drv, struct wmi_driver, driver)
-
 /*
  * sysfs interface
  */
@@ -802,7 +800,7 @@ static void wmi_dev_release(struct device *dev)

 static int wmi_dev_match(struct device *dev, const struct device_driver *driver)
 {
-	const struct wmi_driver *wmi_driver = drv_to_wdrv(driver);
+	const struct wmi_driver *wmi_driver = to_wmi_driver(driver);
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	const struct wmi_device_id *id = wmi_driver->id_table;

@@ -826,7 +824,7 @@ static int wmi_dev_match(struct device *dev, const struct device_driver *driver)
 static int wmi_dev_probe(struct device *dev)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
-	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
+	struct wmi_driver *wdriver = to_wmi_driver(dev->driver);
 	int ret = 0;

 	/* Some older WMI drivers will break if instantiated multiple times,
@@ -870,7 +868,7 @@ static int wmi_dev_probe(struct device *dev)
 static void wmi_dev_remove(struct device *dev)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
-	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
+	struct wmi_driver *wdriver = to_wmi_driver(dev->driver);

 	down_write(&wblock->notify_lock);
 	wblock->driver_ready = false;
@@ -889,7 +887,7 @@ static void wmi_dev_shutdown(struct device *dev)
 	struct wmi_block *wblock;

 	if (dev->driver) {
-		wdriver = drv_to_wdrv(dev->driver);
+		wdriver = to_wmi_driver(dev->driver);
 		wblock = dev_to_wblock(dev);

 		/*
@@ -1173,7 +1171,7 @@ static int wmi_get_notify_data(struct wmi_block *wblock, union acpi_object **obj

 static void wmi_notify_driver(struct wmi_block *wblock, union acpi_object *obj)
 {
-	struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
+	struct wmi_driver *driver = to_wmi_driver(wblock->dev.dev.driver);

 	if (!obj && !driver->no_notify_data) {
 		dev_warn(&wblock->dev.dev, "Event contains no event data\n");
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index fca5fd43c707..10751c8e5e6a 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -73,6 +73,14 @@ struct wmi_driver {
 	void (*notify)(struct wmi_device *device, union acpi_object *data);
 };

+/**
+ * to_wmi_driver() - Helper macro to cast a driver to a wmi_driver
+ * @drv: driver struct
+ *
+ * Cast a struct device_driver to a struct wmi_driver.
+ */
+#define to_wmi_driver(drv)	container_of_const(drv, struct wmi_driver, driver)
+
 extern int __must_check __wmi_driver_register(struct wmi_driver *driver,
 					      struct module *owner);
 extern void wmi_driver_unregister(struct wmi_driver *driver);
--
2.39.5

Re: [PATCH 1/3] platform/x86: wmi: Remove wmi_block_list

[Ilpo Järvinen]

On Sat, 26 Oct 2024 21:38:01 +0200, Armin Wolf wrote:

> The wmi_block_list is only used by guid_count() and without proper
> protection. It also duplicates some of the WMI bus functionality.
> 
> Remove the wmi_block_list and use bus_for_each_dev() instead.
> 
> 


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

The list of commits applied:
[1/3] platform/x86: wmi: Remove wmi_block_list
      commit: 049571ce7678221767ec3ee5d522bc3de7d57ed3
[2/3] platform/x86: wmi: Replace dev_to_wdev() with to_wmi_device()
      commit: c382429b587ac49bd179d768f13e7fa5e7ed1787
[3/3] platform/x86: wmi: Introduce to_wmi_driver()
      commit: e001341a984e709e377b275123aecb5a763eaef9

--
 i.