2023-12-10 20:26:17

by Armin Wolf

[permalink] [raw]
Subject: [PATCH v2 0/5] platform/x86: wmi: Cleanup obsolete features

This patch series removes three features deemed obsolete:
- the debug_dump_wdg module param:
- suffers from garbled output due to pr_cont()
- functionality is better provided by "fwts wmi"
- the debug_event module param:
- pr_cont() usage
- uses the deprecated GUID-based API
- largely replaced by the ACPI netlink interface
- ioctl interface
- used only by a single driver, no adoption otherwise
- numerous design issues

Since the ioctl interface is actually used by userspace programs,
the only user (the dell-smbios-wmi driver) was modified to implement
the necessary pieces itself so that no regressions are expected.

The series depends on
commit cbf54f37600e ("platform/x86: wmi: Skip blocks with zero instances"),
which is currently in the "fixes" tree.

All patches where tested on a Dell Inspiron 3505 and work without
issues.

Changes since v1:
- add Reviewed-by to patches 1, 2 and 5
- drop patch adding the driver development guide
- rework error handling in dell-smbios-wmi

Armin Wolf (5):
platform/x86: wmi: Remove debug_dump_wdg module param
platform/x86: wmi: Remove debug_event module param
platform/x86: dell-smbios-wmi: Use devm_get_free_pages()
platform/x86: dell-smbios-wmi: Stop using WMI chardev
platform/x86: wmi: Remove chardev interface

drivers/platform/x86/dell/dell-smbios-wmi.c | 173 ++++++++----
drivers/platform/x86/wmi.c | 285 +-------------------
include/linux/wmi.h | 8 -
3 files changed, 132 insertions(+), 334 deletions(-)

--
2.39.2


2023-12-10 20:26:24

by Armin Wolf

[permalink] [raw]
Subject: [PATCH v2 2/5] platform/x86: wmi: Remove debug_event module param

Users can already listen to ACPI WMI events through
the ACPI netlink interface. The old wmi_notify_debug()
interface also uses the deprecated GUID-based interface.
Remove it to make the event handling code more readable.

Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/platform/x86/wmi.c | 74 ++++----------------------------------
1 file changed, 7 insertions(+), 67 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index e8019bc19b4f..7df5b5ee7983 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -85,11 +85,6 @@ struct wmi_block {
#define ACPI_WMI_STRING BIT(2) /* GUID takes & returns a string */
#define ACPI_WMI_EVENT BIT(3) /* GUID is an event */

-static bool debug_event;
-module_param(debug_event, bool, 0444);
-MODULE_PARM_DESC(debug_event,
- "Log WMI Events [0/1]");
-
static const struct acpi_device_id wmi_device_ids[] = {
{"PNP0C14", 0},
{"pnp0c14", 0},
@@ -592,42 +587,6 @@ acpi_status wmidev_block_set(struct wmi_device *wdev, u8 instance, const struct
}
EXPORT_SYMBOL_GPL(wmidev_block_set);

-static void wmi_notify_debug(u32 value, void *context)
-{
- struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
- union acpi_object *obj;
- acpi_status status;
-
- status = wmi_get_event_data(value, &response);
- if (status != AE_OK) {
- pr_info("bad event status 0x%x\n", status);
- return;
- }
-
- obj = response.pointer;
- if (!obj)
- return;
-
- pr_info("DEBUG: event 0x%02X ", value);
- switch (obj->type) {
- case ACPI_TYPE_BUFFER:
- pr_cont("BUFFER_TYPE - length %u\n", obj->buffer.length);
- break;
- case ACPI_TYPE_STRING:
- pr_cont("STRING_TYPE - %s\n", obj->string.pointer);
- break;
- case ACPI_TYPE_INTEGER:
- pr_cont("INTEGER_TYPE - %llu\n", obj->integer.value);
- break;
- case ACPI_TYPE_PACKAGE:
- pr_cont("PACKAGE_TYPE - %u elements\n", obj->package.count);
- break;
- default:
- pr_cont("object type 0x%X\n", obj->type);
- }
- kfree(obj);
-}
-
/**
* wmi_install_notify_handler - Register handler for WMI events (deprecated)
* @guid: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
@@ -656,8 +615,7 @@ acpi_status wmi_install_notify_handler(const char *guid,
acpi_status wmi_status;

if (guid_equal(&block->gblock.guid, &guid_input)) {
- if (block->handler &&
- block->handler != wmi_notify_debug)
+ if (block->handler)
return AE_ALREADY_ACQUIRED;

block->handler = handler;
@@ -698,22 +656,14 @@ acpi_status wmi_remove_notify_handler(const char *guid)
acpi_status wmi_status;

if (guid_equal(&block->gblock.guid, &guid_input)) {
- if (!block->handler ||
- block->handler == wmi_notify_debug)
+ if (!block->handler)
return AE_NULL_ENTRY;

- if (debug_event) {
- block->handler = wmi_notify_debug;
- status = AE_OK;
- } else {
- wmi_status = wmi_method_enable(block, false);
- block->handler = NULL;
- block->handler_data = NULL;
- if ((wmi_status != AE_OK) ||
- ((wmi_status == AE_OK) &&
- (status == AE_NOT_EXIST)))
- status = wmi_status;
- }
+ wmi_status = wmi_method_enable(block, false);
+ block->handler = NULL;
+ block->handler_data = NULL;
+ if (wmi_status != AE_OK || (wmi_status == AE_OK && status == AE_NOT_EXIST))
+ status = wmi_status;
}
}

@@ -1340,17 +1290,10 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)

list_add_tail(&wblock->list, &wmi_block_list);

- if (debug_event) {
- wblock->handler = wmi_notify_debug;
- wmi_method_enable(wblock, true);
- }
-
retval = wmi_add_device(pdev, &wblock->dev);
if (retval) {
dev_err(wmi_bus_dev, "failed to register %pUL\n",
&wblock->gblock.guid);
- if (debug_event)
- wmi_method_enable(wblock, false);

list_del(&wblock->list);
put_device(&wblock->dev.dev);
@@ -1445,9 +1388,6 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
wblock->handler(event, wblock->handler_data);
}

- if (debug_event)
- pr_info("DEBUG: GUID %pUL event 0x%02X\n", &wblock->gblock.guid, event);
-
acpi_bus_generate_netlink_event(
wblock->acpi_device->pnp.device_class,
dev_name(&wblock->dev.dev),
--
2.39.2

2023-12-10 20:26:34

by Armin Wolf

[permalink] [raw]
Subject: [PATCH v2 4/5] platform/x86: dell-smbios-wmi: Stop using WMI chardev

The WMI chardev API will be removed in the near future.
Reimplement the necessary bits used by this driver so
that userspace software depending on it does no break.

Signed-off-by: Armin Wolf <[email protected]>
---
drivers/platform/x86/dell/dell-smbios-wmi.c | 161 ++++++++++++++------
1 file changed, 117 insertions(+), 44 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
index 7eb7c61bb27d..ae9012549560 100644
--- a/drivers/platform/x86/dell/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell/dell-smbios-wmi.c
@@ -8,11 +8,14 @@

#include <linux/device.h>
#include <linux/dmi.h>
+#include <linux/fs.h>
#include <linux/list.h>
+#include <linux/miscdevice.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/wmi.h>
+#include <uapi/linux/wmi.h>
#include "dell-smbios.h"
#include "dell-wmi-descriptor.h"

@@ -33,7 +36,8 @@ struct wmi_smbios_priv {
struct list_head list;
struct wmi_device *wdev;
struct device *child;
- u32 req_buf_size;
+ u64 req_buf_size;
+ struct miscdevice char_dev;
};
static LIST_HEAD(wmi_list);

@@ -109,48 +113,115 @@ static int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
return ret;
}

-static long dell_smbios_wmi_filter(struct wmi_device *wdev, unsigned int cmd,
- struct wmi_ioctl_buffer *arg)
+static int dell_smbios_wmi_open(struct inode *inode, struct file *filp)
{
struct wmi_smbios_priv *priv;
- int ret = 0;
-
- switch (cmd) {
- case DELL_WMI_SMBIOS_CMD:
- mutex_lock(&call_mutex);
- priv = dev_get_drvdata(&wdev->dev);
- if (!priv) {
- ret = -ENODEV;
- goto fail_smbios_cmd;
- }
- memcpy(priv->buf, arg, priv->req_buf_size);
- if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
- dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
- priv->buf->std.cmd_class,
- priv->buf->std.cmd_select,
- priv->buf->std.input[0]);
- ret = -EFAULT;
- goto fail_smbios_cmd;
- }
- ret = run_smbios_call(priv->wdev);
- if (ret)
- goto fail_smbios_cmd;
- memcpy(arg, priv->buf, priv->req_buf_size);
-fail_smbios_cmd:
- mutex_unlock(&call_mutex);
- break;
- default:
- ret = -ENOIOCTLCMD;
+
+ priv = container_of(filp->private_data, struct wmi_smbios_priv, char_dev);
+ filp->private_data = priv;
+
+ return nonseekable_open(inode, filp);
+}
+
+static ssize_t dell_smbios_wmi_read(struct file *filp, char __user *buffer, size_t length,
+ loff_t *offset)
+{
+ struct wmi_smbios_priv *priv = filp->private_data;
+
+ return simple_read_from_buffer(buffer, length, offset, &priv->req_buf_size,
+ sizeof(priv->req_buf_size));
+}
+
+static long dell_smbios_wmi_do_ioctl(struct wmi_smbios_priv *priv,
+ struct dell_wmi_smbios_buffer __user *arg)
+{
+ long ret;
+
+ if (get_user(priv->buf->length, &arg->length))
+ return -EFAULT;
+
+ if (priv->buf->length < priv->req_buf_size)
+ return -EINVAL;
+
+ /* if it's too big, warn, driver will only use what is needed */
+ if (priv->buf->length > priv->req_buf_size)
+ dev_err(&priv->wdev->dev, "Buffer %llu is bigger than required %llu\n",
+ priv->buf->length, priv->req_buf_size);
+
+ if (copy_from_user(priv->buf, arg, priv->req_buf_size))
+ return -EFAULT;
+
+ if (dell_smbios_call_filter(&priv->wdev->dev, &priv->buf->std)) {
+ dev_err(&priv->wdev->dev, "Invalid call %d/%d:%8x\n",
+ priv->buf->std.cmd_class,
+ priv->buf->std.cmd_select,
+ priv->buf->std.input[0]);
+
+ return -EINVAL;
}
+
+ ret = run_smbios_call(priv->wdev);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(arg, priv->buf, priv->req_buf_size))
+ return -EFAULT;
+
+ return 0;
+}
+
+static long dell_smbios_wmi_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ struct dell_wmi_smbios_buffer __user *input = (struct dell_wmi_smbios_buffer __user *)arg;
+ struct wmi_smbios_priv *priv = filp->private_data;
+ long ret;
+
+ if (cmd != DELL_WMI_SMBIOS_CMD)
+ return -ENOIOCTLCMD;
+
+ mutex_lock(&call_mutex);
+ ret = dell_smbios_wmi_do_ioctl(priv, input);
+ mutex_unlock(&call_mutex);
+
return ret;
}

+static const struct file_operations dell_smbios_wmi_fops = {
+ .owner = THIS_MODULE,
+ .open = dell_smbios_wmi_open,
+ .read = dell_smbios_wmi_read,
+ .unlocked_ioctl = dell_smbios_wmi_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
+};
+
+static void dell_smbios_wmi_unregister_chardev(void *data)
+{
+ struct miscdevice *char_dev = data;
+
+ misc_deregister(char_dev);
+}
+
+static int dell_smbios_wmi_register_chardev(struct wmi_smbios_priv *priv)
+{
+ int ret;
+
+ priv->char_dev.minor = MISC_DYNAMIC_MINOR;
+ priv->char_dev.name = "wmi/dell-smbios";
+ priv->char_dev.fops = &dell_smbios_wmi_fops;
+ priv->char_dev.mode = 0444;
+
+ ret = misc_register(&priv->char_dev);
+ if (ret < 0)
+ return ret;
+
+ return devm_add_action_or_reset(&priv->wdev->dev, dell_smbios_wmi_unregister_chardev,
+ &priv->char_dev);
+}
+
static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
{
- struct wmi_driver *wdriver =
- container_of(wdev->dev.driver, struct wmi_driver, driver);
struct wmi_smbios_priv *priv;
- u32 hotfix;
+ u32 buffer_size, hotfix;
int count;
int ret;

@@ -163,39 +234,42 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
if (!priv)
return -ENOMEM;

+ priv->wdev = wdev;
+ dev_set_drvdata(&wdev->dev, priv);
+
/* WMI buffer size will be either 4k or 32k depending on machine */
- if (!dell_wmi_get_size(&priv->req_buf_size))
+ if (!dell_wmi_get_size(&buffer_size))
return -EPROBE_DEFER;

+ priv->req_buf_size = buffer_size;
+
/* some SMBIOS calls fail unless BIOS contains hotfix */
if (!dell_wmi_get_hotfix(&hotfix))
return -EPROBE_DEFER;
- if (!hotfix) {
+
+ if (!hotfix)
dev_warn(&wdev->dev,
"WMI SMBIOS userspace interface not supported(%u), try upgrading to a newer BIOS\n",
hotfix);
- wdriver->filter_callback = NULL;
- }

/* add in the length object we will use internally with ioctl */
priv->req_buf_size += sizeof(u64);
- ret = set_required_buffer_size(wdev, priv->req_buf_size);
- if (ret)
- return ret;

count = get_order(priv->req_buf_size);
priv->buf = (void *)devm_get_free_pages(&wdev->dev, GFP_KERNEL, count);
if (!priv->buf)
return -ENOMEM;

+ ret = dell_smbios_wmi_register_chardev(priv);
+ if (ret)
+ return ret;
+
/* ID is used by dell-smbios to set priority of drivers */
wdev->dev.id = 1;
ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
if (ret)
return ret;

- priv->wdev = wdev;
- dev_set_drvdata(&wdev->dev, priv);
mutex_lock(&list_mutex);
list_add_tail(&priv->list, &wmi_list);
mutex_unlock(&list_mutex);
@@ -250,7 +324,6 @@ static struct wmi_driver dell_smbios_wmi_driver = {
.probe = dell_smbios_wmi_probe,
.remove = dell_smbios_wmi_remove,
.id_table = dell_smbios_wmi_id_table,
- .filter_callback = dell_smbios_wmi_filter,
};

int init_dell_smbios_wmi(void)
--
2.39.2

2023-12-10 20:26:47

by Armin Wolf

[permalink] [raw]
Subject: [PATCH v2 5/5] platform/x86: wmi: Remove chardev interface

The design of the WMI chardev interface is broken:
- it assumes that WMI drivers are not instantiated twice
- it offers next to no abstractions, the WMI driver gets
a raw byte buffer
- it is only used by a single driver, something which is
unlikely to change

Since the only user (dell-smbios-wmi) has been migrated
to his own ioctl interface, remove it.

Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/platform/x86/wmi.c | 180 ++-----------------------------------
include/linux/wmi.h | 8 --
2 files changed, 5 insertions(+), 183 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7df5b5ee7983..7303702290e5 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -23,17 +23,14 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
-#include <linux/miscdevice.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
#include <linux/types.h>
-#include <linux/uaccess.h>
#include <linux/uuid.h>
#include <linux/wmi.h>
#include <linux/fs.h>
-#include <uapi/linux/wmi.h>

MODULE_AUTHOR("Carlos Corbacho");
MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
@@ -66,12 +63,9 @@ struct wmi_block {
struct wmi_device dev;
struct list_head list;
struct guid_block gblock;
- struct miscdevice char_dev;
- struct mutex char_mutex;
struct acpi_device *acpi_device;
wmi_notify_handler handler;
void *handler_data;
- u64 req_buf_size;
unsigned long flags;
};

@@ -256,26 +250,6 @@ static void wmi_device_put(struct wmi_device *wdev)
* Exported WMI functions
*/

-/**
- * set_required_buffer_size - Sets the buffer size needed for performing IOCTL
- * @wdev: A wmi bus device from a driver
- * @length: Required buffer size
- *
- * Allocates memory needed for buffer, stores the buffer size in that memory.
- *
- * Return: 0 on success or a negative error code for failure.
- */
-int set_required_buffer_size(struct wmi_device *wdev, u64 length)
-{
- struct wmi_block *wblock;
-
- wblock = container_of(wdev, struct wmi_block, dev);
- wblock->req_buf_size = length;
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(set_required_buffer_size);
-
/**
* wmi_instance_count - Get number of WMI object instances
* @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
@@ -884,111 +858,12 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)

return 0;
}
-static int wmi_char_open(struct inode *inode, struct file *filp)
-{
- /*
- * The miscdevice already stores a pointer to itself
- * inside filp->private_data
- */
- struct wmi_block *wblock = container_of(filp->private_data, struct wmi_block, char_dev);
-
- filp->private_data = wblock;
-
- return nonseekable_open(inode, filp);
-}
-
-static ssize_t wmi_char_read(struct file *filp, char __user *buffer,
- size_t length, loff_t *offset)
-{
- struct wmi_block *wblock = filp->private_data;
-
- return simple_read_from_buffer(buffer, length, offset,
- &wblock->req_buf_size,
- sizeof(wblock->req_buf_size));
-}
-
-static long wmi_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
- struct wmi_ioctl_buffer __user *input =
- (struct wmi_ioctl_buffer __user *) arg;
- struct wmi_block *wblock = filp->private_data;
- struct wmi_ioctl_buffer *buf;
- struct wmi_driver *wdriver;
- int ret;
-
- if (_IOC_TYPE(cmd) != WMI_IOC)
- return -ENOTTY;
-
- /* make sure we're not calling a higher instance than exists*/
- if (_IOC_NR(cmd) >= wblock->gblock.instance_count)
- return -EINVAL;
-
- mutex_lock(&wblock->char_mutex);
- buf = wblock->handler_data;
- if (get_user(buf->length, &input->length)) {
- dev_dbg(&wblock->dev.dev, "Read length from user failed\n");
- ret = -EFAULT;
- goto out_ioctl;
- }
- /* if it's too small, abort */
- if (buf->length < wblock->req_buf_size) {
- dev_err(&wblock->dev.dev,
- "Buffer %lld too small, need at least %lld\n",
- buf->length, wblock->req_buf_size);
- ret = -EINVAL;
- goto out_ioctl;
- }
- /* if it's too big, warn, driver will only use what is needed */
- if (buf->length > wblock->req_buf_size)
- dev_warn(&wblock->dev.dev,
- "Buffer %lld is bigger than required %lld\n",
- buf->length, wblock->req_buf_size);
-
- /* copy the structure from userspace */
- if (copy_from_user(buf, input, wblock->req_buf_size)) {
- dev_dbg(&wblock->dev.dev, "Copy %llu from user failed\n",
- wblock->req_buf_size);
- ret = -EFAULT;
- goto out_ioctl;
- }
-
- /* let the driver do any filtering and do the call */
- wdriver = drv_to_wdrv(wblock->dev.dev.driver);
- if (!try_module_get(wdriver->driver.owner)) {
- ret = -EBUSY;
- goto out_ioctl;
- }
- ret = wdriver->filter_callback(&wblock->dev, cmd, buf);
- module_put(wdriver->driver.owner);
- if (ret)
- goto out_ioctl;
-
- /* return the result (only up to our internal buffer size) */
- if (copy_to_user(input, buf, wblock->req_buf_size)) {
- dev_dbg(&wblock->dev.dev, "Copy %llu to user failed\n",
- wblock->req_buf_size);
- ret = -EFAULT;
- }
-
-out_ioctl:
- mutex_unlock(&wblock->char_mutex);
- return ret;
-}
-
-static const struct file_operations wmi_fops = {
- .owner = THIS_MODULE,
- .read = wmi_char_read,
- .open = wmi_char_open,
- .unlocked_ioctl = wmi_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
-};

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);
int ret = 0;
- char *buf;

if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
dev_warn(dev, "failed to enable device -- probing anyway\n");
@@ -996,55 +871,17 @@ static int wmi_dev_probe(struct device *dev)
if (wdriver->probe) {
ret = wdriver->probe(dev_to_wdev(dev),
find_guid_context(wblock, wdriver));
- if (ret != 0)
- goto probe_failure;
- }
-
- /* driver wants a character device made */
- if (wdriver->filter_callback) {
- /* check that required buffer size declared by driver or MOF */
- if (!wblock->req_buf_size) {
- dev_err(&wblock->dev.dev,
- "Required buffer size not set\n");
- ret = -EINVAL;
- goto probe_failure;
- }
+ if (!ret) {
+ if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
+ dev_warn(dev, "Failed to disable device\n");

- wblock->handler_data = kmalloc(wblock->req_buf_size,
- GFP_KERNEL);
- if (!wblock->handler_data) {
- ret = -ENOMEM;
- goto probe_failure;
- }
-
- buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name);
- if (!buf) {
- ret = -ENOMEM;
- goto probe_string_failure;
- }
- wblock->char_dev.minor = MISC_DYNAMIC_MINOR;
- wblock->char_dev.name = buf;
- wblock->char_dev.fops = &wmi_fops;
- wblock->char_dev.mode = 0444;
- ret = misc_register(&wblock->char_dev);
- if (ret) {
- dev_warn(dev, "failed to register char dev: %d\n", ret);
- ret = -ENOMEM;
- goto probe_misc_failure;
+ return ret;
}
}

set_bit(WMI_PROBED, &wblock->flags);
- return 0;

-probe_misc_failure:
- kfree(buf);
-probe_string_failure:
- kfree(wblock->handler_data);
-probe_failure:
- if (ACPI_FAILURE(wmi_method_enable(wblock, false)))
- dev_warn(dev, "failed to disable device\n");
- return ret;
+ return 0;
}

static void wmi_dev_remove(struct device *dev)
@@ -1054,12 +891,6 @@ static void wmi_dev_remove(struct device *dev)

clear_bit(WMI_PROBED, &wblock->flags);

- if (wdriver->filter_callback) {
- misc_deregister(&wblock->char_dev);
- kfree(wblock->char_dev.name);
- kfree(wblock->handler_data);
- }
-
if (wdriver->remove)
wdriver->remove(dev_to_wdev(dev));

@@ -1131,7 +962,6 @@ static int wmi_create_device(struct device *wmi_bus_dev,

if (wblock->gblock.flags & ACPI_WMI_METHOD) {
wblock->dev.dev.type = &wmi_type_method;
- mutex_init(&wblock->char_mutex);
goto out_init;
}

diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 8a643c39fcce..50f7f1e4fd4f 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -11,7 +11,6 @@
#include <linux/device.h>
#include <linux/acpi.h>
#include <linux/mod_devicetable.h>
-#include <uapi/linux/wmi.h>

/**
* struct wmi_device - WMI device structure
@@ -47,8 +46,6 @@ acpi_status wmidev_block_set(struct wmi_device *wdev, u8 instance, const struct

u8 wmidev_instance_count(struct wmi_device *wdev);

-extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
-
/**
* struct wmi_driver - WMI driver structure
* @driver: Driver model structure
@@ -57,11 +54,8 @@ extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
* @probe: Callback for device binding
* @remove: Callback for device unbinding
* @notify: Callback for receiving WMI events
- * @filter_callback: Callback for filtering device IOCTLs
*
* This represents WMI drivers which handle WMI devices.
- * @filter_callback is only necessary for drivers which
- * want to set up a WMI IOCTL interface.
*/
struct wmi_driver {
struct device_driver driver;
@@ -71,8 +65,6 @@ struct wmi_driver {
int (*probe)(struct wmi_device *wdev, const void *context);
void (*remove)(struct wmi_device *wdev);
void (*notify)(struct wmi_device *device, union acpi_object *data);
- long (*filter_callback)(struct wmi_device *wdev, unsigned int cmd,
- struct wmi_ioctl_buffer *arg);
};

extern int __must_check __wmi_driver_register(struct wmi_driver *driver,
--
2.39.2

2023-12-11 10:25:21

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v2 0/5] platform/x86: wmi: Cleanup obsolete features

Hi,

On 12/10/23 21:24, Armin Wolf wrote:
> This patch series removes three features deemed obsolete:
> - the debug_dump_wdg module param:
> - suffers from garbled output due to pr_cont()
> - functionality is better provided by "fwts wmi"
> - the debug_event module param:
> - pr_cont() usage
> - uses the deprecated GUID-based API
> - largely replaced by the ACPI netlink interface
> - ioctl interface
> - used only by a single driver, no adoption otherwise
> - numerous design issues
>
> Since the ioctl interface is actually used by userspace programs,
> the only user (the dell-smbios-wmi driver) was modified to implement
> the necessary pieces itself so that no regressions are expected.
>
> The series depends on
> commit cbf54f37600e ("platform/x86: wmi: Skip blocks with zero instances"),
> which is currently in the "fixes" tree.
>
> All patches where tested on a Dell Inspiron 3505 and work without
> issues.

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans







> Changes since v1:
> - add Reviewed-by to patches 1, 2 and 5
> - drop patch adding the driver development guide
> - rework error handling in dell-smbios-wmi
>
> Armin Wolf (5):
> platform/x86: wmi: Remove debug_dump_wdg module param
> platform/x86: wmi: Remove debug_event module param
> platform/x86: dell-smbios-wmi: Use devm_get_free_pages()
> platform/x86: dell-smbios-wmi: Stop using WMI chardev
> platform/x86: wmi: Remove chardev interface
>
> drivers/platform/x86/dell/dell-smbios-wmi.c | 173 ++++++++----
> drivers/platform/x86/wmi.c | 285 +-------------------
> include/linux/wmi.h | 8 -
> 3 files changed, 132 insertions(+), 334 deletions(-)
>
> --
> 2.39.2
>