Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character device for userspace

328 views
Skip to first unread message

Mario Limonciello

unread,
Sep 21, 2017, 10:00:09 AM9/21/17
to
This userspace character device will be used to perform SMBIOS calls
from any applications sending a properly formatted 4k calling interface
buffer.

This character device is intended to deprecate the dcdbas kernel module
and the interface that it provides to userspace.

It's important for the driver to provide a R/W ioctl to ensure that
two competing userspace processes don't race to provide or read each
others data.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
Documentation/ABI/testing/dell-wmi-smbios | 19 ++++++
drivers/platform/x86/dell-wmi-smbios.c | 108 ++++++++++++++++++++++++++----
drivers/platform/x86/dell-wmi-smbios.h | 5 ++
3 files changed, 120 insertions(+), 12 deletions(-)
create mode 100644 Documentation/ABI/testing/dell-wmi-smbios

diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
new file mode 100644
index 000000000000..54dcf73b3031
--- /dev/null
+++ b/Documentation/ABI/testing/dell-wmi-smbios
@@ -0,0 +1,19 @@
+What: /dev/wmi-dell-wmi-smbios
+Date: October 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.li...@dell.com>
+Description:
+ Perform an SMBIOS call on a supported Dell machine
+ through the Dell ACPI-WMI interface.
+
+ To make a call prepare a 4k buffer like this:
+ struct buffer {
+ u16 class;
+ u16 select;
+ u32 input[4];
+ u32 output[4];
+ u8 data[4060];
+ } __packed;
+
+ Perform this RW IOCTL to get the result:
+ _IOWR('D', 0, struct calling_interface_buffer)
diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
index 9deb851ff517..22e47fba6a59 100644
--- a/drivers/platform/x86/dell-wmi-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -21,6 +21,7 @@
#include <linux/err.h>
#include <linux/mutex.h>
#include <linux/wmi.h>
+#include <linux/uaccess.h>
#include "dell-wmi-smbios.h"

#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
@@ -34,7 +35,8 @@ struct calling_interface_structure {
struct calling_interface_token tokens[];
} __packed;

-static struct calling_interface_buffer *buffer;
+static struct calling_interface_buffer *internal_buffer;
+static struct calling_interface_buffer *sysfs_buffer;
static DEFINE_MUTEX(buffer_mutex);

static int da_command_address;
@@ -61,13 +63,13 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
{
mutex_lock(&buffer_mutex);
dell_smbios_clear_buffer();
- return buffer;
+ return internal_buffer;
}
EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);

void dell_smbios_clear_buffer(void)
{
- memset(buffer, 0, sizeof(struct calling_interface_buffer));
+ memset(internal_buffer, 0, sizeof(struct calling_interface_buffer));
}
EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);

@@ -107,9 +109,9 @@ int run_wmi_smbios_call(struct calling_interface_buffer *buffer)

void dell_smbios_send_request(int class, int select)
{
- buffer->class = class;
- buffer->select = select;
- run_wmi_smbios_call(buffer);
+ internal_buffer->class = class;
+ internal_buffer->select = select;
+ run_wmi_smbios_call(internal_buffer);
}
EXPORT_SYMBOL_GPL(dell_smbios_send_request);

@@ -218,6 +220,68 @@ static const struct attribute_group smbios_attribute_group = {
.attrs = smbios_attrs,
};

+static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
+{
+ return nonseekable_open(inode, file);
+}
+
+static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static ssize_t dell_wmi_smbios_read(struct file *file, char __user *data,
+ size_t len, loff_t *ppos)
+{
+ ssize_t size = sizeof(struct calling_interface_buffer);
+ void *src;
+
+ if (*ppos >= size)
+ return 0;
+ if (len >= size)
+ len = size;
+ if (*ppos + len > size)
+ len = size - *ppos;
+ src = (void __force *) (sysfs_buffer + *ppos);
+ if (copy_to_user(data, src, len))
+ return -EFAULT;
+
+ *ppos += len;
+ return len;
+}
+
+static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret = 0;
+ size_t size;
+
+ if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
+ return -ENOTTY;
+
+ switch (cmd) {
+ case DELL_WMI_SMBIOS_CMD:
+ size = sizeof(struct calling_interface_buffer);
+ mutex_lock(&buffer_mutex);
+ if (copy_from_user(sysfs_buffer, (void __user *) arg, size)) {
+ ret = -EFAULT;
+ goto fail_smbios_cmd;
+ }
+ ret = run_wmi_smbios_call(sysfs_buffer);
+ if (ret != 0)
+ goto fail_smbios_cmd;
+ if (copy_to_user((void __user *) arg, sysfs_buffer, size))
+ ret = -EFAULT;
+fail_smbios_cmd:
+ mutex_unlock(&buffer_mutex);
+ break;
+ default:
+ pr_err("unsupported ioctl: %d.\n", cmd);
+ ret = -ENOIOCTLCMD;
+ }
+ return ret;
+}
+
/*
* Descriptor buffer is 128 byte long and contains:
*
@@ -306,12 +370,19 @@ static int dell_wmi_smbios_probe(struct wmi_device *wdev)
if (ret)
return ret;

- buffer = (void *)__get_free_page(GFP_KERNEL);
- if (!buffer) {
+ internal_buffer = (void *)__get_free_page(GFP_KERNEL);
+ if (!internal_buffer) {
ret = -ENOMEM;
- goto fail_buffer;
+ goto fail_internal_buffer;
}

+ sysfs_buffer = (void *)__get_free_page(GFP_KERNEL);
+ if (!sysfs_buffer) {
+ ret = -ENOMEM;
+ goto fail_sysfs_buffer;
+ }
+ memset(sysfs_buffer, 0, sizeof(struct calling_interface_buffer));
+
ret = sysfs_create_group(&wdev->dev.kobj, &smbios_attribute_group);
if (ret)
goto fail_create_group;
@@ -320,9 +391,12 @@ static int dell_wmi_smbios_probe(struct wmi_device *wdev)
return 0;

fail_create_group:
- free_page((unsigned long)buffer);
+ free_page((unsigned long)sysfs_buffer);

-fail_buffer:
+fail_sysfs_buffer:
+ free_page((unsigned long)internal_buffer);
+
+fail_internal_buffer:
kfree(da_tokens);
return ret;
}
@@ -331,7 +405,8 @@ static int dell_wmi_smbios_remove(struct wmi_device *wdev)
{
sysfs_remove_group(&wdev->dev.kobj, &smbios_attribute_group);
kfree(da_tokens);
- free_page((unsigned long)buffer);
+ free_page((unsigned long)internal_buffer);
+ free_page((unsigned long)sysfs_buffer);
kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
return 0;
}
@@ -341,6 +416,14 @@ static const struct wmi_device_id dell_wmi_smbios_id_table[] = {
{ },
};

+static const struct file_operations dell_wmi_smbios_fops = {
+ .owner = THIS_MODULE,
+ .read = dell_wmi_smbios_read,
+ .unlocked_ioctl = dell_wmi_smbios_ioctl,
+ .open = dell_wmi_smbios_open,
+ .release = dell_wmi_smbios_release,
+};
+
static struct wmi_driver dell_wmi_smbios_driver = {
.driver = {
.name = "dell-wmi-smbios",
@@ -348,6 +431,7 @@ static struct wmi_driver dell_wmi_smbios_driver = {
.probe = dell_wmi_smbios_probe,
.remove = dell_wmi_smbios_remove,
.id_table = dell_wmi_smbios_id_table,
+ .file_operations = &dell_wmi_smbios_fops,
};
module_wmi_driver(dell_wmi_smbios_driver);

diff --git a/drivers/platform/x86/dell-wmi-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
index 0521ec5d437b..b8215eb879b2 100644
--- a/drivers/platform/x86/dell-wmi-smbios.h
+++ b/drivers/platform/x86/dell-wmi-smbios.h
@@ -18,6 +18,7 @@
#define _DELL_WMI_SMBIOS_H_

#include <linux/wmi.h>
+#include <linux/ioctl.h>

struct notifier_block;

@@ -40,6 +41,10 @@ struct calling_interface_token {
};
};

+#define DELL_WMI_SMBIOS_IOC 'D'
+#define DELL_WMI_SMBIOS_CMD _IOWR(DELL_WMI_SMBIOS_IOC, 0, \
+ struct calling_interface_buffer)
+
int dell_smbios_error(int value);

struct calling_interface_buffer *dell_smbios_get_buffer(void);
--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:09 AM9/21/17
to
This driver serves the purpose of responding to WMI based notifications
from the DELL_EVENT_GUID (9DBB5994-A997-11DA-B012-B622A1EF5492).
Other GUIDs will be handled by separate drivers.

Update the language used by this driver to avoid future confusion.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
MAINTAINERS | 2 +-
drivers/platform/x86/Kconfig | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2281af4b41b6..5d8ea24a8ee7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3996,7 +3996,7 @@ S: Maintained
F: Documentation/dcdbas.txt
F: drivers/firmware/dcdbas.*

-DELL WMI EXTRAS DRIVER
+DELL WMI NOTIFICATIONS DRIVER
M: Matthew Garrett <mj...@srcf.ucam.org>
M: Pali Rohár <pali....@gmail.com>
S: Maintained
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 80b87954f6dd..9e52f05daa2e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -116,7 +116,7 @@ config DELL_LAPTOP
laptops (except for some models covered by the Compal driver).

config DELL_WMI
- tristate "Dell WMI extras"
+ tristate "Dell WMI notifications"
depends on ACPI_WMI
depends on DMI
depends on INPUT
--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:09 AM9/21/17
to
The existing way that the dell-smbios helper module and associated
other drivers (dell-laptop, dell-wmi) communicate with the platform
really isn't secure. It requires creating a buffer in physical
DMA32 memory space and passing that to the platform via SMM.

Since the platform got a physical memory pointer, you've just got
to trust that the platform has only modified (and accessed) memory
within that buffer.

Dell Platform designers recognize this security risk and offer a
safer way to communicate with the platform over ACPI. This is
in turn exposed via a WMI interface to the OS.

When communicating over WMI-ACPI the communication doesn't occur
with physical memory pointers. When the ASL is invoked, the fixed
length ACPI buffer is copied to a small operating region. The ASL
will invoke the SMI, and SMM will only have access to this operating
region. When the ASL returns the buffer is copied back for the OS
to process.

This method of communication should also deprecate the usage of the
dcdbas kernel module and software dependent upon it's interface.
Instead offer a syfs interface for communicating with this ASL
method to allow userspace to use instead.

To faciliate that needs for userspace and kernel space this patch
series introduces a generic way for WMI drivers to be able to
create character devices through the WMI bus when desired.
Requiring WMI drivers to explictly ask for this functionality will
act as an effective vendor whitelist.

Mario Limonciello (12):
platform/x86: dell-wmi: label driver as handling notifications
platform/x86: dell-wmi: Don't match on descriptor GUID modalias
platform/x86: dell-smbios: Add pr_fmt definition to driver
platform/x86: dell-smbios: Switch to a WMI-ACPI interface
platform/x86: dell-smbios: rename to dell-wmi-smbios
platform/x86: dell-wmi-smbios: Add a sysfs interface for SMBIOS tokens
platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check
platform/x86: wmi: Cleanup exit routine in reverse order of init
platform/x86: wmi: create character devices when requested by drivers
platform/x86: wmi: destroy on cleanup rather than unregister
platform/x86: dell-wmi-smbios: introduce character device for
userspace
platform/x86: Kconfig: Change the default settings for dell-wmi-smbios

Documentation/ABI/testing/dell-wmi-smbios | 19 +
.../ABI/testing/sysfs-platform-dell-wmi-smbios | 16 +
MAINTAINERS | 8 +-
drivers/platform/x86/Kconfig | 13 +-
drivers/platform/x86/Makefile | 2 +-
drivers/platform/x86/dell-laptop.c | 2 +-
drivers/platform/x86/dell-smbios.c | 213 ----------
drivers/platform/x86/dell-wmi-smbios.c | 444 +++++++++++++++++++++
.../x86/{dell-smbios.h => dell-wmi-smbios.h} | 23 +-
drivers/platform/x86/dell-wmi.c | 78 +---
drivers/platform/x86/wmi.c | 104 ++++-
include/linux/wmi.h | 1 +
12 files changed, 610 insertions(+), 313 deletions(-)
create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
delete mode 100644 drivers/platform/x86/dell-smbios.c
create mode 100644 drivers/platform/x86/dell-wmi-smbios.c
rename drivers/platform/x86/{dell-smbios.h => dell-wmi-smbios.h} (75%)

--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:12 AM9/21/17
to
Currently userspace tools can access system tokens via the dcdbas
kernel module and a SMI call that will cause the platform to execute
SMM code.

With a goal in mind of deprecating the dcdbas kernel module a different
method for accessing these tokens from userspace needs to be created.

This is intentionally marked to only be readable as root as it can
contain sensitive information about the platform's configuration.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
.../ABI/testing/sysfs-platform-dell-wmi-smbios | 16 +++++++++
drivers/platform/x86/dell-wmi-smbios.c | 38 ++++++++++++++++++++++
2 files changed, 54 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios

diff --git a/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios b/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
new file mode 100644
index 000000000000..6d151f2dffba
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
@@ -0,0 +1,16 @@
+What: /sys/devices/platform/<platform>/tokens
+Date: October 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.li...@dell.com>
+Description:
+ A read-only description of Dell platform tokens
+ available on the machine.
+
+ The tokens will be displayed in the following
+ machine readable format with each token on a
+ new line:
+
+ ID @ Location : value
+
+ For example token:
+ 5 @ 5 : 3
diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
index 7f896701fb7b..c3701fdadf7b 100644
--- a/drivers/platform/x86/dell-wmi-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -189,6 +189,34 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

+static ssize_t tokens_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ size_t off = 0;
+ int i;
+
+ for (i = 0; i < da_num_tokens; i++) {
+ if (off > PAGE_SIZE)
+ break;
+ off += scnprintf(buf+off, PAGE_SIZE-off, "%x @ %x : %x\n",
+ da_tokens[i].tokenID, da_tokens[i].location,
+ da_tokens[i].value);
+ }
+
+ return off;
+}
+
+DEVICE_ATTR(tokens, 0400, tokens_show, NULL);
+
+static struct attribute *smbios_attrs[] = {
+ &dev_attr_tokens.attr,
+ NULL
+};
+
+static const struct attribute_group smbios_attribute_group = {
+ .attrs = smbios_attrs,
+};
+
static int dell_wmi_smbios_probe(struct wmi_device *wdev)
{
int ret;
@@ -206,8 +234,16 @@ static int dell_wmi_smbios_probe(struct wmi_device *wdev)
goto fail_buffer;
}

+ ret = sysfs_create_group(&wdev->dev.kobj, &smbios_attribute_group);
+ if (ret)
+ goto fail_create_group;
+ kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
+
return 0;

+fail_create_group:
+ free_page((unsigned long)buffer);
+
fail_buffer:
kfree(da_tokens);
return ret;
@@ -215,8 +251,10 @@ static int dell_wmi_smbios_probe(struct wmi_device *wdev)

static int dell_wmi_smbios_remove(struct wmi_device *wdev)
{
+ sysfs_remove_group(&wdev->dev.kobj, &smbios_attribute_group);
kfree(da_tokens);
free_page((unsigned long)buffer);
+ kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
return 0;
}

--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:12 AM9/21/17
to
The driver currently uses an SMI interface which grants direct access
to physical memory to the platform via a pointer.

Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
for a buffer of data storage when platform calls are performed.

This is a safer approach to use in kernel drivers as the platform will
only have access to that OperationRegion.

As a result, this change removes the dependency on this driver on the
dcdbas kernel module.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/Kconfig | 8 ++--
drivers/platform/x86/dell-smbios.c | 76 ++++++++++++++++++++++++++------------
drivers/platform/x86/dell-smbios.h | 11 +++---
3 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 9e52f05daa2e..81d61c0f4ef8 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -92,13 +92,13 @@ config ASUS_LAPTOP
If you have an ACPI-compatible ASUS laptop, say Y or M here.

config DELL_SMBIOS
- tristate
- select DCDBAS
+ tristate "Dell WMI SMBIOS calling interface"
+ depends on ACPI_WMI
---help---
This module provides common functions for kernel modules using
- Dell SMBIOS.
+ Dell SMBIOS over ACPI-WMI.

- If you have a Dell laptop, say Y or M here.
+ If you have a Dell computer, say Y or M here.

config DELL_LAPTOP
tristate "Dell Laptop Extras"
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index e9b1ca07c872..c06262a89169 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -4,6 +4,7 @@
* Copyright (c) Red Hat <m...@redhat.com>
* Copyright (c) 2014 Gabriele Mazzotta <gabrie...@gmail.com>
* Copyright (c) 2014 Pali Rohár <pali....@gmail.com>
+ * Copyright (c) 2017 Dell Inc.
*
* Based on documentation in the libsmbios package:
* Copyright (C) 2005-2014 Dell Inc.
@@ -18,13 +19,12 @@
#include <linux/module.h>
#include <linux/dmi.h>
#include <linux/err.h>
-#include <linux/gfp.h>
#include <linux/mutex.h>
-#include <linux/slab.h>
-#include <linux/io.h>
-#include "../../firmware/dcdbas.h"
+#include <linux/wmi.h>
#include "dell-smbios.h"

+#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+
struct calling_interface_structure {
struct dmi_header header;
u16 cmdIOAddress;
@@ -76,20 +76,39 @@ void dell_smbios_release_buffer(void)
}
EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);

-void dell_smbios_send_request(int class, int select)
+int run_wmi_smbios_call(struct calling_interface_buffer *buffer)
{
- struct smi_cmd command;
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+ struct acpi_buffer input;
+ union acpi_object *obj;
+ acpi_status status;
+
+ input.length = sizeof(struct calling_interface_buffer);
+ input.pointer = buffer;
+
+ status = wmi_evaluate_method(DELL_WMI_SMBIOS_GUID,
+ 0, 1, &input, &output);
+ if (ACPI_FAILURE(status)) {
+ pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
+ buffer->class, buffer->select, buffer->input[0],
+ buffer->input[1], buffer->input[2], buffer->input[3]);
+ return -EIO;
+ }
+ obj = (union acpi_object *)output.pointer;
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ pr_err("invalid type : %d\n", obj->type);
+ return -EIO;
+ }
+ memcpy(buffer, obj->buffer.pointer, input.length);

- command.magic = SMI_CMD_MAGIC;
- command.command_address = da_command_address;
- command.command_code = da_command_code;
- command.ebx = virt_to_phys(buffer);
- command.ecx = 0x42534931;
+ return 0;
+}

+void dell_smbios_send_request(int class, int select)
+{
buffer->class = class;
buffer->select = select;
-
- dcdbas_smi_request(&command);
+ run_wmi_smbios_call(buffer);
}
EXPORT_SYMBOL_GPL(dell_smbios_send_request);

@@ -170,7 +189,7 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

-static int __init dell_smbios_init(void)
+static int dell_smbios_probe(struct wmi_device *wdev)
{
int ret;

@@ -181,11 +200,7 @@ static int __init dell_smbios_init(void)
return -ENODEV;
}

- /*
- * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
- * is passed to SMI handler.
- */
- buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+ buffer = (void *)__get_free_page(GFP_KERNEL);
if (!buffer) {
ret = -ENOMEM;
goto fail_buffer;
@@ -198,17 +213,32 @@ static int __init dell_smbios_init(void)
return ret;
}

-static void __exit dell_smbios_exit(void)
+static int dell_smbios_remove(struct wmi_device *wdev)
{
kfree(da_tokens);
free_page((unsigned long)buffer);
+ return 0;
}

-subsys_initcall(dell_smbios_init);
-module_exit(dell_smbios_exit);
+static const struct wmi_device_id dell_smbios_id_table[] = {
+ { .guid_string = DELL_WMI_SMBIOS_GUID },
+ { },
+};
+
+static struct wmi_driver dell_smbios_driver = {
+ .driver = {
+ .name = "dell-smbios",
+ },
+ .probe = dell_smbios_probe,
+ .remove = dell_smbios_remove,
+ .id_table = dell_smbios_id_table,
+};
+module_wmi_driver(dell_smbios_driver);
+

MODULE_AUTHOR("Matthew Garrett <m...@redhat.com>");
MODULE_AUTHOR("Gabriele Mazzotta <gabrie...@gmail.com>");
MODULE_AUTHOR("Pali Rohár <pali....@gmail.com>");
-MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS");
+MODULE_AUTHOR("Mario Limonciello <mario.li...@dell.com>");
+MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS over WMI");
MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 45cbc2292cd3..e1e29697b362 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -4,6 +4,7 @@
* Copyright (c) Red Hat <m...@redhat.com>
* Copyright (c) 2014 Gabriele Mazzotta <gabrie...@gmail.com>
* Copyright (c) 2014 Pali Rohár <pali....@gmail.com>
+ * Copyright (c) 2017 Dell Inc.
*
* Based on documentation in the libsmbios package:
* Copyright (C) 2005-2014 Dell Inc.
@@ -18,14 +19,14 @@

struct notifier_block;

-/* This structure will be modified by the firmware when we enter
- * system management mode, hence the volatiles */
-
struct calling_interface_buffer {
u16 class;
u16 select;
- volatile u32 input[4];
- volatile u32 output[4];
+ u32 input[4];
+ u32 output[4];
+ u32 argattrib;
+ u32 blength;
+ u8 data[4052];
} __packed;

struct calling_interface_token {
--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:12 AM9/21/17
to
The dell-wmi-smbios driver should be enabled by default when ACPI_WMI
is enabled (like many other WMI drivers).

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index a70bcd8caa72..4f9ca51b1968 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -94,6 +94,7 @@ config ASUS_LAPTOP
config DELL_WMI_SMBIOS
tristate "Dell WMI SMBIOS calling interface"
depends on ACPI_WMI
+ default ACPI_WMI
---help---
This module provides common functions for kernel modules using
Dell SMBIOS over ACPI-WMI.
--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:15 AM9/21/17
to
device_create documentation says to cleanup using device_destroy

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index d1e128864d24..84314c0ef9c2 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1223,7 +1223,7 @@ static int acpi_wmi_remove(struct platform_device *device)
acpi_remove_address_space_handler(acpi_device->handle,
ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
wmi_free_devices(acpi_device);
- device_unregister((struct device *)dev_get_drvdata(&device->dev));
+ device_destroy(&wmi_bus_class, MKDEV(0, 0));

return 0;
}
@@ -1277,7 +1277,7 @@ static int acpi_wmi_probe(struct platform_device *device)
return 0;

err_remove_busdev:
- device_unregister(wmi_bus_dev);
+ device_destroy(&wmi_bus_class, MKDEV(0, 0));

err_remove_notify_handler:
acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:15 AM9/21/17
to
The Dell WMI descriptor check is used as an indication that WMI
calls are safe to run both when used with the notification
ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.

As some code in dell-wmi-smbios is already a prerequisite for
dell-wmi, move the code for performing the descriptor check into
dell-wmi-smbios and let both drivers use it from there.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-wmi-smbios.c | 78 ++++++++++++++++++++++++++++++++++
drivers/platform/x86/dell-wmi-smbios.h | 3 ++
drivers/platform/x86/dell-wmi.c | 75 +-------------------------------
3 files changed, 82 insertions(+), 74 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
index c3701fdadf7b..9deb851ff517 100644
--- a/drivers/platform/x86/dell-wmi-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -24,6 +24,7 @@
#include "dell-wmi-smbios.h"

#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"

struct calling_interface_structure {
struct dmi_header header;
@@ -217,8 +218,81 @@ static const struct attribute_group smbios_attribute_group = {
.attrs = smbios_attrs,
};

+/*
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ * Name Offset Length Value
+ * Vendor Signature 0 4 "DELL"
+ * Object Signature 4 4 " WMI"
+ * WMI Interface Version 8 4 <version>
+ * WMI buffer length 12 4 4096
+ */
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version)
+{
+ union acpi_object *obj = NULL;
+ struct wmi_device *desc_dev;
+ u32 *desc_buffer;
+ int ret;
+
+ desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
+ if (!desc_dev) {
+ dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
+ return -ENODEV;
+ }
+
+ obj = wmidev_block_query(desc_dev, 0);
+ if (!obj) {
+ dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
+ ret = -EIO;
+ goto out;
+ }
+
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (obj->buffer.length != 128) {
+ dev_err(&wdev->dev,
+ "Dell descriptor buffer has invalid length (%d)\n",
+ obj->buffer.length);
+ if (obj->buffer.length < 16) {
+ ret = -EINVAL;
+ goto out;
+ }
+ }
+
+ desc_buffer = (u32 *)obj->buffer.pointer;
+
+ if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
+ 8, desc_buffer);
+
+ if (desc_buffer[2] != 0 && desc_buffer[2] != 1)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
+ desc_buffer[2]);
+
+ if (desc_buffer[3] != 4096)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
+ desc_buffer[3]);
+
+ *version = desc_buffer[2];
+ ret = 0;
+
+ dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
+ *version);
+
+out:
+ kfree(obj);
+ put_device(&desc_dev->dev);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_check_descriptor_buffer);
+
static int dell_wmi_smbios_probe(struct wmi_device *wdev)
{
+ u32 interface_version;
int ret;

dmi_walk(find_tokens, NULL);
@@ -228,6 +302,10 @@ static int dell_wmi_smbios_probe(struct wmi_device *wdev)
return -ENODEV;
}

+ ret = dell_wmi_check_descriptor_buffer(wdev, &interface_version);
+ if (ret)
+ return ret;
+
buffer = (void *)__get_free_page(GFP_KERNEL);
if (!buffer) {
ret = -ENOMEM;
diff --git a/drivers/platform/x86/dell-wmi-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
index e6e9990bb2b7..0521ec5d437b 100644
--- a/drivers/platform/x86/dell-wmi-smbios.h
+++ b/drivers/platform/x86/dell-wmi-smbios.h
@@ -17,6 +17,8 @@
#ifndef _DELL_WMI_SMBIOS_H_
#define _DELL_WMI_SMBIOS_H_

+#include <linux/wmi.h>
+
struct notifier_block;

struct calling_interface_buffer {
@@ -54,5 +56,6 @@ enum dell_laptop_notifier_actions {
int dell_laptop_register_notifier(struct notifier_block *nb);
int dell_laptop_unregister_notifier(struct notifier_block *nb);
void dell_laptop_call_notifier(unsigned long action, void *data);
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version);

#endif
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index e8b4d412eabc..e7011792127f 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -46,7 +46,6 @@ MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver");
MODULE_LICENSE("GPL");

#define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
-#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"

static bool wmi_requires_smbios_request;

@@ -617,78 +616,6 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
input_unregister_device(priv->input_dev);
}

-/*
- * Descriptor buffer is 128 byte long and contains:
- *
- * Name Offset Length Value
- * Vendor Signature 0 4 "DELL"
- * Object Signature 4 4 " WMI"
- * WMI Interface Version 8 4 <version>
- * WMI buffer length 12 4 4096
- */
-static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
-{
- struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
- union acpi_object *obj = NULL;
- struct wmi_device *desc_dev;
- u32 *buffer;
- int ret;
-
- desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
- if (!desc_dev) {
- dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
- return -ENODEV;
- }
-
- obj = wmidev_block_query(desc_dev, 0);
- if (!obj) {
- dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
- ret = -EIO;
- goto out;
- }
-
- if (obj->type != ACPI_TYPE_BUFFER) {
- dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
- ret = -EINVAL;
- goto out;
- }
-
- if (obj->buffer.length != 128) {
- dev_err(&wdev->dev,
- "Dell descriptor buffer has invalid length (%d)\n",
- obj->buffer.length);
- if (obj->buffer.length < 16) {
- ret = -EINVAL;
- goto out;
- }
- }
-
- buffer = (u32 *)obj->buffer.pointer;
-
- if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
- 8, buffer);
-
- if (buffer[2] != 0 && buffer[2] != 1)
- dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
- buffer[2]);
-
- if (buffer[3] != 4096)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
- buffer[3]);
-
- priv->interface_version = buffer[2];
- ret = 0;
-
- dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
- priv->interface_version);
-
-out:
- kfree(obj);
- put_device(&desc_dev->dev);
- return ret;
-}
-
/*
* According to Dell SMBIOS documentation:
*
@@ -732,7 +659,7 @@ static int dell_wmi_probe(struct wmi_device *wdev)
return -ENOMEM;
dev_set_drvdata(&wdev->dev, priv);

- err = dell_wmi_check_descriptor_buffer(wdev);
+ err = dell_wmi_check_descriptor_buffer(wdev, &priv->interface_version);
if (err)
return err;

--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:00:15 AM9/21/17
to
For WMI operations that are only Set or Query read or write sysfs
attributes created by WMI vendor drivers make sense.

For other WMI operations that are run on Method, there needs to be a
way to guarantee to userspace that the results from the method call
belong to the data request to the method call. Sysfs attributes don't
work well in this scenario because two userspace processes may be
competing at reading/writing an attribute and step on each other's
data.

When a WMI vendor driver declares a set of functions in a
file_operations object the WMI bus driver will create a character
device that maps to those file operations.

The WMI vendor drivers will be responsible for managing access to
this character device and proper locking on it.

When a WMI vendor driver is unloaded the WMI bus driver will clean
up the character device.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
include/linux/wmi.h | 1 +
2 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 077a9b7459ef..d1e128864d24 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -44,12 +44,17 @@
#include <linux/platform_device.h>
#include <linux/wmi.h>
#include <linux/uuid.h>
+#include <linux/cdev.h>
+#include <linux/idr.h>

ACPI_MODULE_NAME("wmi");
MODULE_AUTHOR("Carlos Corbacho");
MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
MODULE_LICENSE("GPL");

+#define WMI_MAX_DEVS MINORMASK
+static DEFINE_IDR(wmi_idr);
+static DEFINE_MUTEX(wmi_minor_lock);
static LIST_HEAD(wmi_block_list);

struct guid_block {
@@ -69,6 +74,8 @@ struct wmi_block {
struct wmi_device dev;
struct list_head list;
struct guid_block gblock;
+ struct cdev *cdev;
+ int minor;
struct acpi_device *acpi_device;
wmi_notify_handler handler;
void *handler_data;
@@ -86,6 +93,7 @@ struct wmi_block {
#define ACPI_WMI_STRING 0x4 /* GUID takes & returns a string */
#define ACPI_WMI_EVENT 0x8 /* GUID is an event */

+static dev_t wmi_devt;
static bool debug_event;
module_param(debug_event, bool, 0444);
MODULE_PARM_DESC(debug_event,
@@ -762,21 +770,88 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
return 0;
}

+static struct class wmi_bus_class = {
+ .name = "wmi_bus",
+};
+
+static int wmi_minor_get(struct wmi_block *wblock)
+{
+ int ret;
+
+ mutex_lock(&wmi_minor_lock);
+ ret = idr_alloc(&wmi_idr, wblock, 0, WMI_MAX_DEVS, GFP_KERNEL);
+ if (ret >= 0)
+ wblock->minor = ret;
+ else if (ret == -ENOSPC)
+ dev_err(&wblock->dev.dev, "too many wmi devices\n");
+
+ mutex_unlock(&wmi_minor_lock);
+ return ret;
+}
+
+static void wmi_minor_free(struct wmi_block *wblock)
+{
+ mutex_lock(&wmi_minor_lock);
+ idr_remove(&wmi_idr, wblock->minor);
+ mutex_unlock(&wmi_minor_lock);
+}
+
+
static int wmi_dev_probe(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;
+ struct device *clsdev;
+ int ret = 0, devno;

if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
dev_warn(dev, "failed to enable device -- probing anyway\n");

+ /* driver wants a character device made */
+ if (wdriver->file_operations) {
+ dev->devt = wmi_devt;
+ wblock->cdev = cdev_alloc();
+ if (!wblock->cdev) {
+ dev_err(dev, "failed to allocate cdev\n");
+ return -ENOMEM;
+ }
+ cdev_init(wblock->cdev, wdriver->file_operations);
+ wblock->cdev->owner = wdriver->file_operations->owner;
+ ret = wmi_minor_get(wblock);
+ if (ret < 0)
+ return ret;
+ devno = MKDEV(MAJOR(wmi_devt), wblock->minor);
+ ret = cdev_add(wblock->cdev, devno, 1);
+ if (ret) {
+ dev_err(dev, "unable to create device %d:%d\n",
+ MAJOR(wmi_devt), wblock->minor);
+ goto err_probe_cdev;
+ }
+ clsdev = device_create(&wmi_bus_class, dev,
+ MKDEV(MAJOR(wmi_devt), wblock->minor),
+ NULL, "wmi-%s", wdriver->driver.name);
+
+ if (IS_ERR(clsdev)) {
+ dev_err(dev, "unable to create device %d:%d\n",
+ MAJOR(wmi_devt), wblock->minor);
+ ret = PTR_ERR(clsdev);
+ goto err_probe_class;
+ }
+ }
+
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;
+
+err_probe_class:
+ cdev_del(wblock->cdev);
+
+err_probe_cdev:
+ wmi_minor_free(wblock);

return ret;
}
@@ -788,6 +863,13 @@ static int wmi_dev_remove(struct device *dev)
container_of(dev->driver, struct wmi_driver, driver);
int ret = 0;

+ if (wdriver->file_operations) {
+ device_destroy(&wmi_bus_class,
+ MKDEV(MAJOR(wmi_devt), wblock->minor));
+ cdev_del(wblock->cdev);
+ wmi_minor_free(wblock);
+ }
+
if (wdriver->remove)
ret = wdriver->remove(dev_to_wdev(dev));

@@ -797,10 +879,6 @@ static int wmi_dev_remove(struct device *dev)
return ret;
}

-static struct class wmi_bus_class = {
- .name = "wmi_bus",
-};
-
static struct bus_type wmi_bus_type = {
.name = "wmi",
.dev_groups = wmi_groups,
@@ -1250,8 +1328,17 @@ static int __init acpi_wmi_init(void)
goto err_unreg_bus;
}

+ error = alloc_chrdev_region(&wmi_devt, 0, WMI_MAX_DEVS, "wmi");
+ if (error < 0) {
+ pr_err("unable to allocate char dev region\n");
+ goto err_unreg_platform;
+ }
+
return 0;

+err_unreg_platform:
+ platform_driver_unregister(&acpi_wmi_driver);
+
err_unreg_bus:
bus_unregister(&wmi_bus_type);

@@ -1263,6 +1350,7 @@ static int __init acpi_wmi_init(void)

static void __exit acpi_wmi_exit(void)
{
+ unregister_chrdev_region(wmi_devt, WMI_MAX_DEVS);
platform_driver_unregister(&acpi_wmi_driver);
bus_unregister(&wmi_bus_type);
class_unregister(&wmi_bus_class);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index cd0d7734dc49..6899ddb6cd30 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -41,6 +41,7 @@ struct wmi_device_id {
struct wmi_driver {
struct device_driver driver;
const struct wmi_device_id *id_table;
+ const struct file_operations *file_operations;

int (*probe)(struct wmi_device *wdev);
int (*remove)(struct wmi_device *wdev);
--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:10:06 AM9/21/17
to
This follows the style of the rest of the platform x86 WMI drivers.

Renaming the driver requires adjusting the other drivers using
dell-smbios to pick up the newly named includes.

While renaming, I noticed that this driver was missing from
MAINTAINERs. Add it to that and myself to the list of people maintaing
it.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
MAINTAINERS | 6 ++++++
drivers/platform/x86/Kconfig | 2 +-
drivers/platform/x86/Makefile | 2 +-
drivers/platform/x86/dell-laptop.c | 2 +-
.../x86/{dell-smbios.c => dell-wmi-smbios.c} | 20 ++++++++++----------
.../x86/{dell-smbios.h => dell-wmi-smbios.h} | 4 ++--
drivers/platform/x86/dell-wmi.c | 2 +-
7 files changed, 22 insertions(+), 16 deletions(-)
rename drivers/platform/x86/{dell-smbios.c => dell-wmi-smbios.c} (92%)
rename drivers/platform/x86/{dell-smbios.h => dell-wmi-smbios.h} (96%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d8ea24a8ee7..437daa9062e1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4002,6 +4002,12 @@ M: Pali Rohár <pali....@gmail.com>
S: Maintained
F: drivers/platform/x86/dell-wmi.c

+DELL WMI SMBIOS DRIVER
+M: Pali Rohár <pali....@gmail.com>
+M: Mario Limonciello <mario.li...@dell.com>
+S: Maintained
+F: drivers/platform/x86/dell-wmi-smbios.c
+
DELTA ST MEDIA DRIVER
M: Hugues Fruchet <hugues....@st.com>
L: linux...@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 81d61c0f4ef8..a70bcd8caa72 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -91,7 +91,7 @@ config ASUS_LAPTOP

If you have an ACPI-compatible ASUS laptop, say Y or M here.

-config DELL_SMBIOS
+config DELL_WMI_SMBIOS
tristate "Dell WMI SMBIOS calling interface"
depends on ACPI_WMI
---help---
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 91cec1751461..b127a3bc1fab 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -11,11 +11,11 @@ obj-$(CONFIG_EEEPC_WMI) += eeepc-wmi.o
obj-$(CONFIG_MSI_LAPTOP) += msi-laptop.o
obj-$(CONFIG_ACPI_CMPC) += classmate-laptop.o
obj-$(CONFIG_COMPAL_LAPTOP) += compal-laptop.o
-obj-$(CONFIG_DELL_SMBIOS) += dell-smbios.o
obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
obj-$(CONFIG_DELL_WMI) += dell-wmi.o
obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
+obj-$(CONFIG_DELL_WMI_SMBIOS) += dell-wmi-smbios.o
obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
obj-$(CONFIG_DELL_RBTN) += dell-rbtn.o
obj-$(CONFIG_ACER_WMI) += acer-wmi.o
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index f42159fd2031..bf569ea93e9d 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -33,7 +33,7 @@
#include <linux/seq_file.h>
#include <acpi/video.h>
#include "dell-rbtn.h"
-#include "dell-smbios.h"
+#include "dell-wmi-smbios.h"

#define BRIGHTNESS_TOKEN 0x7d
#define KBD_LED_OFF_TOKEN 0x01E1
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
similarity index 92%
rename from drivers/platform/x86/dell-smbios.c
rename to drivers/platform/x86/dell-wmi-smbios.c
index c06262a89169..7f896701fb7b 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -21,7 +21,7 @@
#include <linux/err.h>
#include <linux/mutex.h>
#include <linux/wmi.h>
-#include "dell-smbios.h"
+#include "dell-wmi-smbios.h"

#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"

@@ -189,7 +189,7 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

-static int dell_smbios_probe(struct wmi_device *wdev)
+static int dell_wmi_smbios_probe(struct wmi_device *wdev)
{
int ret;

@@ -213,27 +213,27 @@ static int dell_smbios_probe(struct wmi_device *wdev)
return ret;
}

-static int dell_smbios_remove(struct wmi_device *wdev)
+static int dell_wmi_smbios_remove(struct wmi_device *wdev)
{
kfree(da_tokens);
free_page((unsigned long)buffer);
return 0;
}

-static const struct wmi_device_id dell_smbios_id_table[] = {
+static const struct wmi_device_id dell_wmi_smbios_id_table[] = {
{ .guid_string = DELL_WMI_SMBIOS_GUID },
{ },
};

-static struct wmi_driver dell_smbios_driver = {
+static struct wmi_driver dell_wmi_smbios_driver = {
.driver = {
- .name = "dell-smbios",
+ .name = "dell-wmi-smbios",
},
- .probe = dell_smbios_probe,
- .remove = dell_smbios_remove,
- .id_table = dell_smbios_id_table,
+ .probe = dell_wmi_smbios_probe,
+ .remove = dell_wmi_smbios_remove,
+ .id_table = dell_wmi_smbios_id_table,
};
-module_wmi_driver(dell_smbios_driver);
+module_wmi_driver(dell_wmi_smbios_driver);


MODULE_AUTHOR("Matthew Garrett <m...@redhat.com>");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
similarity index 96%
rename from drivers/platform/x86/dell-smbios.h
rename to drivers/platform/x86/dell-wmi-smbios.h
index e1e29697b362..e6e9990bb2b7 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-wmi-smbios.h
@@ -14,8 +14,8 @@
* published by the Free Software Foundation.
*/

-#ifndef _DELL_SMBIOS_H_
-#define _DELL_SMBIOS_H_
+#ifndef _DELL_WMI_SMBIOS_H_
+#define _DELL_WMI_SMBIOS_H_

struct notifier_block;

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1fbef560ca67..e8b4d412eabc 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -38,7 +38,7 @@
#include <linux/dmi.h>
#include <linux/wmi.h>
#include <acpi/video.h>
-#include "dell-smbios.h"
+#include "dell-wmi-smbios.h"

MODULE_AUTHOR("Matthew Garrett <m...@redhat.com>");
MODULE_AUTHOR("Pali Rohár <pali....@gmail.com>");
--
2.14.1

Mario Limonciello

unread,
Sep 21, 2017, 10:10:06 AM9/21/17
to
Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-smbios.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 0a5723468bff..e9b1ca07c872 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -12,6 +12,7 @@
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/kernel.h>
#include <linux/module.h>
--
2.14.1

Andy Shevchenko

unread,
Sep 21, 2017, 12:30:09 PM9/21/17
to
On Thu, Sep 21, 2017 at 4:57 PM, Mario Limonciello
<mario.li...@dell.com> wrote:

We need a (formal) commit message even for simple patches.

> Signed-off-by: Mario Limonciello <mario.li...@dell.com>

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko

unread,
Sep 21, 2017, 12:50:10 PM9/21/17
to
On Thu, Sep 21, 2017 at 4:57 PM, Mario Limonciello
<mario.li...@dell.com> wrote:
> The Dell WMI descriptor check is used as an indication that WMI
> calls are safe to run both when used with the notification
> ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.
>
> As some code in dell-wmi-smbios is already a prerequisite for
> dell-wmi, move the code for performing the descriptor check into
> dell-wmi-smbios and let both drivers use it from there.

> + desc_buffer = (u32 *)obj->buffer.pointer;
> +

> + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)

I was thinking about strncmp() here for a full line, though decide not
to push it anyhow. Those IDs is binary data, can be anything and you
have comment of what is expected here.

But I think it would be nice to create a separate definitions and make
comments there.

> + dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
> + 8, desc_buffer);

%8ph ?

> +
> + if (desc_buffer[2] != 0 && desc_buffer[2] != 1)
> + dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
> + desc_buffer[2]);

%u ? u32 can't be negative and you basically allow it.

> +
> + if (desc_buffer[3] != 4096)
> + dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
> + desc_buffer[3]);

Ditto.


P.S. I noticed this all in old code, so, you can address my comments
in a separate patch if you find them useful.

Andy Shevchenko

unread,
Sep 21, 2017, 12:50:10 PM9/21/17
to
On Thu, Sep 21, 2017 at 4:57 PM, Mario Limonciello
<mario.li...@dell.com> wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
>
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call. Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.
>
> When a WMI vendor driver declares a set of functions in a
> file_operations object the WMI bus driver will create a character
> device that maps to those file operations.
>
> The WMI vendor drivers will be responsible for managing access to
> this character device and proper locking on it.
>
> When a WMI vendor driver is unloaded the WMI bus driver will clean
> up the character device.
>

> @@ -44,12 +44,17 @@
> #include <linux/platform_device.h>
> #include <linux/wmi.h>
> #include <linux/uuid.h>
> +#include <linux/cdev.h>
> +#include <linux/idr.h>

Keep alphabetical ordering.

Mario.Li...@dell.com

unread,
Sep 21, 2017, 3:30:06 PM9/21/17
to
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.sh...@gmail.com]
> Sent: Thursday, September 21, 2017 11:47 AM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; LKML <linux-...@vger.kernel.org>; Platform Driver
> <platform-...@vger.kernel.org>; quas...@google.com; Pali Rohár
> <pali....@gmail.com>
> Subject: Re: [PATCH 09/12] platform/x86: wmi: create character devices when
> requested by drivers
>
The existing list wasn't in alphabetical order. I'll submit a patch in v2 earlier
in the series to sort and then put them in the right place for my patch.

Mario.Li...@dell.com

unread,
Sep 21, 2017, 5:00:10 PM9/21/17
to
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.sh...@gmail.com]
> Sent: Thursday, September 21, 2017 11:44 AM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; LKML <linux-...@vger.kernel.org>; Platform Driver
> <platform-...@vger.kernel.org>; quas...@google.com; Pali Rohár
> <pali....@gmail.com>
> Subject: Re: [PATCH 07/12] platform/x86: dell-wmi-smbios: Use Dell WMI
> descriptor check
>
Yeah this is all old code. I've made some adjustments to it in v2 in a completely separate
patch that comes after the patch it's "moved". I'll submit back after other feedback
to this series.

Pali Rohár

unread,
Sep 25, 2017, 12:10:09 PM9/25/17
to
On Thursday 21 September 2017 08:57:06 Mario Limonciello wrote:
> This driver serves the purpose of responding to WMI based notifications
> from the DELL_EVENT_GUID (9DBB5994-A997-11DA-B012-B622A1EF5492).
> Other GUIDs will be handled by separate drivers.
>
> Update the language used by this driver to avoid future confusion.

Hi! I'm not sure if if "notifications" word is better then "extras".
Basically the most important part of the dell-wmi driver is to deliver
key press events via input device.

Has anybody else better word or description for this?
Pali Rohár
pali....@gmail.com

Pali Rohár

unread,
Sep 25, 2017, 12:10:09 PM9/25/17
to
After fixing Andy's comment, you can add my

Reviewed-by: Pali Rohár <pali....@gmail.com>

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

Pali Rohár

unread,
Sep 25, 2017, 12:20:07 PM9/25/17
to
On Thursday 21 September 2017 08:57:05 Mario Limonciello wrote:
> The existing way that the dell-smbios helper module and associated
> other drivers (dell-laptop, dell-wmi) communicate with the platform
> really isn't secure. It requires creating a buffer in physical
> DMA32 memory space and passing that to the platform via SMM.
>
> Since the platform got a physical memory pointer, you've just got
> to trust that the platform has only modified (and accessed) memory
> within that buffer.

And what is the problem? The whole memory management is done by kernel
itself, so you already need to trust it.

> Dell Platform designers recognize this security risk and offer a
> safer way to communicate with the platform over ACPI. This is
> in turn exposed via a WMI interface to the OS.

Hm... I cannot understand how some proprietary ACPI bytecode interpreted
by kernel can be safer as kernel code itself.

Can you describe more details about this security risk?

> When communicating over WMI-ACPI the communication doesn't occur
> with physical memory pointers. When the ASL is invoked, the fixed
> length ACPI buffer is copied to a small operating region. The ASL
> will invoke the SMI, and SMM will only have access to this operating
> region. When the ASL returns the buffer is copied back for the OS
> to process.

If problem is in current kernel implementation, then it can be fixed.

I'm not against using new WMI communication, but I cannot understand how
kernel code itself is less safer as some other code which is interpreted
by kernel. It does not make sense for me.

> This method of communication should also deprecate the usage of the
> dcdbas kernel module and software dependent upon it's interface.
> Instead offer a syfs interface for communicating with this ASL
> method to allow userspace to use instead.
>
> To faciliate that needs for userspace and kernel space this patch
> series introduces a generic way for WMI drivers to be able to
> create character devices through the WMI bus when desired.
> Requiring WMI drivers to explictly ask for this functionality will
> act as an effective vendor whitelist.

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

Pali Rohár

unread,
Sep 25, 2017, 12:30:08 PM9/25/17
to
On Thursday 21 September 2017 08:57:09 Mario Limonciello wrote:
> The driver currently uses an SMI interface which grants direct access
> to physical memory to the platform via a pointer.
>
> Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
> for a buffer of data storage when platform calls are performed.
>
> This is a safer approach to use in kernel drivers as the platform will
> only have access to that OperationRegion.

In my opinion direct access is safer then using ACPI wrapper for same
functionality.

Anyway, this change would break support for laptops without ACPI-WMI
functionality. IIRC I read in some Dell ACPI-WMI documentation that Dell
SMM via ACPI-WMI is not supported on all machines (probably older
machines) and it is needed to check some vendor bit in DMI data if Dell
SMM ACPI-WMI is really supported.

In linux kernel we do not want to remove support for older machines,
just because machines with new firmware can use also different new
communication method/protocol.
Pali Rohár
pali....@gmail.com

Pali Rohár

unread,
Sep 25, 2017, 12:30:08 PM9/25/17
to
On Thursday 21 September 2017 08:57:11 Mario Limonciello wrote:
> Currently userspace tools can access system tokens via the dcdbas
> kernel module and a SMI call that will cause the platform to execute
> SMM code.
>
> With a goal in mind of deprecating the dcdbas kernel module a different
> method for accessing these tokens from userspace needs to be created.
>
> This is intentionally marked to only be readable as root as it can
> contain sensitive information about the platform's configuration.

Darren, Andy, any comments? I'm not quite sure if such API is suitable
for long term in kernel.

Basically tokens are list of tuples <id, location, value> with
possibility to active them, right?

Does not kernel have some better API for it?

Also, keep in mind security aspect of tokens. They represent e.g. boot
order priority or enable/disable some machine peripheral.
Pali Rohár
pali....@gmail.com

Mario.Li...@dell.com

unread,
Sep 25, 2017, 12:40:08 PM9/25/17
to
Hi Pali,

> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Monday, September 25, 2017 12:14 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; LKML <linux-...@vger.kernel.org>; platform-driver-
> x...@vger.kernel.org; quas...@google.com
> Subject: Re: [PATCH 00/12] Introduce support for Dell SMBIOS over WMI
>
> On Thursday 21 September 2017 08:57:05 Mario Limonciello wrote:
> > The existing way that the dell-smbios helper module and associated
> > other drivers (dell-laptop, dell-wmi) communicate with the platform
> > really isn't secure. It requires creating a buffer in physical
> > DMA32 memory space and passing that to the platform via SMM.
> >
> > Since the platform got a physical memory pointer, you've just got
> > to trust that the platform has only modified (and accessed) memory
> > within that buffer.
>
> And what is the problem? The whole memory management is done by kernel
> itself, so you already need to trust it.

There's a lot of ifs, but it's not that crazy of a scenario.

The problem is that if a malicious payload was delivered to the platform
and exercised a vulnerability in the platform code that payload could
potentially modify memory that it wasn't intended to modify and the OS
would not be aware as operating in SMM.

>
> > Dell Platform designers recognize this security risk and offer a
> > safer way to communicate with the platform over ACPI. This is
> > in turn exposed via a WMI interface to the OS.
>
> Hm... I cannot understand how some proprietary ACPI bytecode interpreted
> by kernel can be safer as kernel code itself.
>

Inherently ACPI can only operate on operation regions and not physical memory.
Data passed into ACPI needs to be copied to an operation region for any ACPI
calls to use it.

Furthermore you can decompile the ASL and audit, you can't do this with direct
SMI/SMM.

> Can you describe more details about this security risk?
>
> > When communicating over WMI-ACPI the communication doesn't occur
> > with physical memory pointers. When the ASL is invoked, the fixed
> > length ACPI buffer is copied to a small operating region. The ASL
> > will invoke the SMI, and SMM will only have access to this operating
> > region. When the ASL returns the buffer is copied back for the OS
> > to process.
>
> If problem is in current kernel implementation, then it can be fixed.
>
>
> I'm not against using new WMI communication, but I cannot understand how
> kernel code itself is less safer as some other code which is interpreted
> by kernel. It does not make sense for me.
>

Well we're talking hypotheticals here in the way things work.
There aren't necessarily problems with the current implementation.

Also, I didn't already mention this explicitly but I've alluded it to it;
Dell is deprecating that interface. I can't say when, but it will stop working
on some new hardware at some point.

That's the other reason why I'm pushing for the new communication path
now.

Pali Rohár

unread,
Sep 25, 2017, 12:40:08 PM9/25/17
to
On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
> This userspace character device will be used to perform SMBIOS calls
> from any applications sending a properly formatted 4k calling interface
> buffer.
>
> This character device is intended to deprecate the dcdbas kernel module
> and the interface that it provides to userspace.
>
> It's important for the driver to provide a R/W ioctl to ensure that
> two competing userspace processes don't race to provide or read each
> others data.
>
> Signed-off-by: Mario Limonciello <mario.li...@dell.com>
> ---
> Documentation/ABI/testing/dell-wmi-smbios | 19 ++++++
> drivers/platform/x86/dell-wmi-smbios.c | 108 ++++++++++++++++++++++++++----
> drivers/platform/x86/dell-wmi-smbios.h | 5 ++
> 3 files changed, 120 insertions(+), 12 deletions(-)
> create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
>
> diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
> new file mode 100644
> index 000000000000..54dcf73b3031
> --- /dev/null
> +++ b/Documentation/ABI/testing/dell-wmi-smbios
> @@ -0,0 +1,19 @@
> +What: /dev/wmi-dell-wmi-smbios

What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
related and I think userspace does not have to care if it is via WMI or
direct SMM mode... Important is that it provides character device for
SMBIOS API and not how it is implemented.

Anyway, Darren, Andy, do we have some convention for naming platform
character devices?

> +Date: October 2017
> +KernelVersion: 4.15
> +Contact: "Mario Limonciello" <mario.li...@dell.com>
> +Description:
> + Perform an SMBIOS call on a supported Dell machine
> + through the Dell ACPI-WMI interface.
> +
> + To make a call prepare a 4k buffer like this:
> + struct buffer {
> + u16 class;
> + u16 select;
> + u32 input[4];
> + u32 output[4];
> + u8 data[4060];
> + } __packed;
> +
> + Perform this RW IOCTL to get the result:
> + _IOWR('D', 0, struct calling_interface_buffer)

I would suggest to provide uapi header file with all needed structures
and defines. So userspace application would have it and would not need
to implement own buffer...
Pali Rohár
pali....@gmail.com

Pali Rohár

unread,
Sep 25, 2017, 12:50:10 PM9/25/17
to
But operation regions access is implemented by ACPI interpreter, which
is again kernel code.

> Furthermore you can decompile the ASL and audit, you can't do this with direct
> SMI/SMM.
>
> > Can you describe more details about this security risk?
> >
> > > When communicating over WMI-ACPI the communication doesn't occur
> > > with physical memory pointers. When the ASL is invoked, the fixed
> > > length ACPI buffer is copied to a small operating region. The ASL
> > > will invoke the SMI, and SMM will only have access to this operating
> > > region. When the ASL returns the buffer is copied back for the OS
> > > to process.
> >
> > If problem is in current kernel implementation, then it can be fixed.
> >
> >
> > I'm not against using new WMI communication, but I cannot understand how
> > kernel code itself is less safer as some other code which is interpreted
> > by kernel. It does not make sense for me.
> >
>
> Well we're talking hypotheticals here in the way things work.
> There aren't necessarily problems with the current implementation.

Ok.

> Also, I didn't already mention this explicitly but I've alluded it to it;
> Dell is deprecating that interface. I can't say when, but it will stop working
> on some new hardware at some point.
>
> That's the other reason why I'm pushing for the new communication path
> now.

Ok, as I wrote I'm not against new communication method and specially
now, when you confirmed that in future new machines would not support
"old" method...

... but old communication method should stay there for older machines. I
do not think it would be hard to have both implementations in kernel and
choosing that which is supported on current machine.

> > > This method of communication should also deprecate the usage of the
> > > dcdbas kernel module and software dependent upon it's interface.
> > > Instead offer a syfs interface for communicating with this ASL
> > > method to allow userspace to use instead.
> > >
> > > To faciliate that needs for userspace and kernel space this patch
> > > series introduces a generic way for WMI drivers to be able to
> > > create character devices through the WMI bus when desired.
> > > Requiring WMI drivers to explictly ask for this functionality will
> > > act as an effective vendor whitelist.
> >
> > --
> > Pali Rohár
> > pali....@gmail.com

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

Andy Shevchenko

unread,
Sep 25, 2017, 1:00:08 PM9/25/17
to
On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali....@gmail.com> wrote:
> On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
>> This userspace character device will be used to perform SMBIOS calls
>> from any applications sending a properly formatted 4k calling interface
>> buffer.
>>
>> This character device is intended to deprecate the dcdbas kernel module
>> and the interface that it provides to userspace.
>>
>> It's important for the driver to provide a R/W ioctl to ensure that
>> two competing userspace processes don't race to provide or read each
>> others data.

>> +What: /dev/wmi-dell-wmi-smbios
>
> What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
> related and I think userspace does not have to care if it is via WMI or
> direct SMM mode... Important is that it provides character device for
> SMBIOS API and not how it is implemented.
>
> Anyway, Darren, Andy, do we have some convention for naming platform
> character devices?

For me, looking to this case, seems better to expose a folder like
/dev/smbios/
with actual vendor device nodes inside like
/dev/smbios/dell

Darren?

P.S. Interestingly I wasn't in Cc list for this series at all.

Andy Shevchenko

unread,
Sep 25, 2017, 1:10:06 PM9/25/17
to
On Mon, Sep 25, 2017 at 7:23 PM, Pali Rohár <pali....@gmail.com> wrote:
> On Thursday 21 September 2017 08:57:11 Mario Limonciello wrote:
>> Currently userspace tools can access system tokens via the dcdbas
>> kernel module and a SMI call that will cause the platform to execute
>> SMM code.
>>
>> With a goal in mind of deprecating the dcdbas kernel module a different
>> method for accessing these tokens from userspace needs to be created.
>>
>> This is intentionally marked to only be readable as root as it can
>> contain sensitive information about the platform's configuration.
>
> Darren, Andy, any comments? I'm not quite sure if such API is suitable
> for long term in kernel.

I would try to avoid sysfs interfaces for some particular devices.
Besides we are creating a character device. Would it be suitable there?

> Basically tokens are list of tuples <id, location, value> with
> possibility to active them, right?
>
> Does not kernel have some better API for it?

I think the best what kernel may provide is a CSV-like format with or
without title line and different delimiter (TAB/space/etc).

>
> Also, keep in mind security aspect of tokens. They represent e.g. boot
> order priority or enable/disable some machine peripheral.

For IOCTLs we may use capabilities.
In sysfs case we may zero output based on capabilities or some other factors.

Mario.Li...@dell.com

unread,
Sep 25, 2017, 1:40:12 PM9/25/17
to
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.sh...@gmail.com]
> Sent: Monday, September 25, 2017 1:04 PM
> To: Pali Rohár <pali....@gmail.com>
> Cc: Limonciello, Mario <Mario_Li...@Dell.com>; dvh...@infradead.org;
> LKML <linux-...@vger.kernel.org>; Platform Driver <platform-driver-
> x...@vger.kernel.org>; quas...@google.com
> Subject: Re: [PATCH 06/12] platform/x86: dell-wmi-smbios: Add a sysfs interface
> for SMBIOS tokens
>
> On Mon, Sep 25, 2017 at 7:23 PM, Pali Rohár <pali....@gmail.com> wrote:
> > On Thursday 21 September 2017 08:57:11 Mario Limonciello wrote:
> >> Currently userspace tools can access system tokens via the dcdbas
> >> kernel module and a SMI call that will cause the platform to execute
> >> SMM code.
> >>
> >> With a goal in mind of deprecating the dcdbas kernel module a different
> >> method for accessing these tokens from userspace needs to be created.
> >>
> >> This is intentionally marked to only be readable as root as it can
> >> contain sensitive information about the platform's configuration.
> >
> > Darren, Andy, any comments? I'm not quite sure if such API is suitable
> > for long term in kernel.
>
> I would try to avoid sysfs interfaces for some particular devices.
> Besides we are creating a character device. Would it be suitable there?

If the character device having 2 different ioctls for different needs is
acceptable I'm happy to adjust the series to do this instead.

>
> > Basically tokens are list of tuples <id, location, value> with
> > possibility to active them, right?
> >

I didn't add a way to activate them through this, it was only for
reading purpose. Activating them should be possible through the
SMBIOS calling interface though.

> > Does not kernel have some better API for it?
>
> I think the best what kernel may provide is a CSV-like format with or
> without title line and different delimiter (TAB/space/etc).
>
> >
> > Also, keep in mind security aspect of tokens. They represent e.g. boot
> > order priority or enable/disable some machine peripheral.
>
> For IOCTLs we may use capabilities.
> In sysfs case we may zero output based on capabilities or some other factors.
>

Can you recommend what capabilities you would prefer to see this based upon?

Mario.Li...@dell.com

unread,
Sep 25, 2017, 1:50:08 PM9/25/17
to
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.sh...@gmail.com]
> Sent: Monday, September 25, 2017 12:58 PM
> To: Pali Rohár <pali....@gmail.com>
> Cc: Limonciello, Mario <Mario_Li...@Dell.com>; dvh...@infradead.org;
> LKML <linux-...@vger.kernel.org>; Platform Driver <platform-driver-
> x...@vger.kernel.org>; quas...@google.com
> Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
>
I disagree with this. Dell exposes smbios calling in this kernel interface but
other vendor drivers may also expose different methods for character devices
that are not SMBIOS.

I'm envisioning that this is just the first kernel module that will use a WMI
character device to userspace. That's why I wanted to use a generic namespace.

I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you want
something more generic.
As long as it's discovereable from uevent that's fine to me.

>
> Darren?
>
> P.S. Interestingly I wasn't in Cc list for this series at all.

Sorry, my mistake in gitconfig. I'll add you in next version submission.


Mario.Li...@dell.com

unread,
Sep 25, 2017, 3:30:05 PM9/25/17
to
So isn't that making my point?
* Kernel can control operation region accessibility. SMM can't operate outside
of this region.
* Direct SMI gives platform access to everything < 4G, kernel can't control this.
The WMI interface has been around for at least 10 years. I think the number
of machines still running on the older implementation only is very small.

There is however a bit that will set the availability of this interface. I'll rework
my patches to offer both WMI and legacy SMI approach based upon that
presence of the bit.

Mario.Li...@dell.com

unread,
Sep 25, 2017, 3:30:06 PM9/25/17
to


> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Monday, September 25, 2017 12:19 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; LKML <linux-...@vger.kernel.org>; platform-driver-
> x...@vger.kernel.org; quas...@google.com
> Subject: Re: [PATCH 04/12] platform/x86: dell-smbios: Switch to a WMI-ACPI
> interface
>
> On Thursday 21 September 2017 08:57:09 Mario Limonciello wrote:
> > The driver currently uses an SMI interface which grants direct access
> > to physical memory to the platform via a pointer.
> >
> > Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
> > for a buffer of data storage when platform calls are performed.
> >
> > This is a safer approach to use in kernel drivers as the platform will
> > only have access to that OperationRegion.
>
> In my opinion direct access is safer then using ACPI wrapper for same
> functionality.

I'd like to hear how this is safer.

>
> Anyway, this change would break support for laptops without ACPI-WMI
> functionality. IIRC I read in some Dell ACPI-WMI documentation that Dell
> SMM via ACPI-WMI is not supported on all machines (probably older
> machines) and it is needed to check some vendor bit in DMI data if Dell
> SMM ACPI-WMI is really supported.
>
> In linux kernel we do not want to remove support for older machines,
> just because machines with new firmware can use also different new
> communication method/protocol.
>

As mentioned on other email, I'll rework to support both methods and
prefer WMI method.

Mario.Li...@dell.com

unread,
Sep 25, 2017, 4:20:09 PM9/25/17
to
> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Monday, September 25, 2017 12:04 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; LKML <linux-...@vger.kernel.org>; platform-driver-
> x...@vger.kernel.org; quas...@google.com
> Subject: Re: [PATCH 01/12] platform/x86: dell-wmi: label driver as handling
> notifications
>
> On Thursday 21 September 2017 08:57:06 Mario Limonciello wrote:
> > This driver serves the purpose of responding to WMI based notifications
> > from the DELL_EVENT_GUID (9DBB5994-A997-11DA-B012-B622A1EF5492).
> > Other GUIDs will be handled by separate drivers.
> >
> > Update the language used by this driver to avoid future confusion.
>
> Hi! I'm not sure if if "notifications" word is better then "extras".
> Basically the most important part of the dell-wmi driver is to deliver
> key press events via input device.
>
> Has anybody else better word or description for this?
>
I was actually tempted to rename the driver itself to dell-wmi-notifications.
Realistically it is hooking up to the notifications _WED0 AML method so yes
it is picking up notifications exclusively.

I thought about this too, but I can also envision that the notifications that
come through this driver that aren't consumed by the kernel for keypress
purposes are also useful to user space potentially. It's not part of this series
but maybe in the future providing those through a character device too may
make sense.

Mario Limonciello

unread,
Sep 26, 2017, 3:00:06 PM9/26/17
to
The existing way that the dell-smbios helper module and associated
other drivers (dell-laptop, dell-wmi) communicate with the platform
really isn't secure. It requires creating a buffer in physical
DMA32 memory space and passing that to the platform via SMM.

Since the platform got a physical memory pointer, you've just got
to trust that the platform has only modified (and accessed) memory
within that buffer.

Dell Platform designers recognize this security risk and offer a
safer way to communicate with the platform over ACPI. This is
in turn exposed via a WMI interface to the OS.

When communicating over WMI-ACPI the communication doesn't occur
with physical memory pointers. When the ASL is invoked, the fixed
length ACPI buffer is copied to a small operating region. The ASL
will invoke the SMI, and SMM will only have access to this operating
region. When the ASL returns the buffer is copied back for the OS
to process.

This method of communication should also deprecate the usage of the
dcdbas kernel module and software dependent upon it's interface.
Instead offer a character device interface for communicating with this
ASL method to allow userspace to use instead.

To faciliate that this patch series introduces a generic way for WMI
drivers to be able to create discoverable character devices through
the WMI bus when desired.
Requiring WMI drivers to explicitly ask for this functionality will
act as an effective vendor whitelist to character device creation.

changes between v1 and v2:
* Introduce another patch to sort the includes in wmi.c
* Introduce another patch to cleanup dell_wmi_check_descriptor_buffer
checks.
* Add a commit message to the pr_fmt commit
* Introduce includes to wmi.c in proper location
* Add Reviewed-by to relevant patches from Pali
* Make the WMI introduction patch fallback to legacy SMI
if compiled with CONFIG_DCDBAS
* Separate format of WMI and SMI buffers. WMI buffer supports more
arguments and data.
* Adjust the rename patch for changes to fallback
* Drop sysfs token creation patch
* Adjust WMI descriptor check patch for changes to fallback
* introduce another patch to remove needless includes in dell-smbios.c
* Add token ioctl interface to character device.
- Can query number of tokens
- Can query values in all tokens
* Expose format of all buffers and IOCTL commands to uapi header
* Drop the read interface from character device. It doesn't make
sense with multiple different ioctl methods.
* Default WMI interface to 32k (This would normally be queried via
MOF, but that's not possible yet)
* Create separate buffers for WMI and SMI. If WMI is available,
free the SMI buffer.
* Reorder patches so all fixups come first in the series.

Mario Limonciello (14):
platform/x86: dell-wmi: label driver as handling notifications
platform/x86: dell-smbios: drop needless includes
platform/x86: dell-wmi: Don't match on descriptor GUID modalias
platform/x86: dell-smbios: Add pr_fmt definition to driver
platform/x86: wmi: sort include list
platform/x86: wmi: Cleanup exit routine in reverse order of init
platform/x86: wmi: destroy on cleanup rather than unregister
platform/x86: dell-smbios: Introduce a WMI-ACPI interface
platform/x86: dell-smbios: rename to dell-wmi-smbios
platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check
platform/x86: wmi: create character devices when requested by drivers
platform/x86: dell-wmi-smbios: introduce character device for
userspace
platform/x86: Kconfig: Change the default settings for dell-wmi-smbios
platform/x86: dell-wmi-smbios: clean up wmi descriptor check

Documentation/ABI/testing/dell-wmi-smbios | 10 +
MAINTAINERS | 8 +-
drivers/platform/x86/Kconfig | 13 +-
drivers/platform/x86/Makefile | 2 +-
drivers/platform/x86/dell-laptop.c | 2 +-
drivers/platform/x86/dell-smbios.c | 213 ---------
drivers/platform/x86/dell-wmi-smbios.c | 481 +++++++++++++++++++++
.../x86/{dell-smbios.h => dell-wmi-smbios.h} | 28 +-
drivers/platform/x86/dell-wmi.c | 78 +---
drivers/platform/x86/wmi.c | 116 ++++-
include/linux/wmi.h | 1 +
include/uapi/linux/dell-wmi-smbios.h | 55 +++
12 files changed, 674 insertions(+), 333 deletions(-)
create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
delete mode 100644 drivers/platform/x86/dell-smbios.c
create mode 100644 drivers/platform/x86/dell-wmi-smbios.c
rename drivers/platform/x86/{dell-smbios.h => dell-wmi-smbios.h} (74%)
create mode 100644 include/uapi/linux/dell-wmi-smbios.h

--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:06 PM9/26/17
to
device_create documentation says to cleanup using device_destroy

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index acbc2b02db3d..7a05843aff19 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1145,7 +1145,7 @@ static int acpi_wmi_remove(struct platform_device *device)
acpi_remove_address_space_handler(acpi_device->handle,
ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
wmi_free_devices(acpi_device);
- device_unregister((struct device *)dev_get_drvdata(&device->dev));
+ device_destroy(&wmi_bus_class, MKDEV(0, 0));

return 0;
}
@@ -1199,7 +1199,7 @@ static int acpi_wmi_probe(struct platform_device *device)
return 0;

err_remove_busdev:
- device_unregister(wmi_bus_dev);
+ device_destroy(&wmi_bus_class, MKDEV(0, 0));

err_remove_notify_handler:
acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:06 PM9/26/17
to
The initialize routine is:
* class -> bus -> platform

The exit routine is:
* platform -> class -> bus

Fix the exit routine to be:
* platform -> bus -> class

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index e19b074df01d..acbc2b02db3d 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1264,8 +1264,8 @@ static int __init acpi_wmi_init(void)
static void __exit acpi_wmi_exit(void)
{
platform_driver_unregister(&acpi_wmi_driver);
- class_unregister(&wmi_bus_class);
bus_unregister(&wmi_bus_type);
+ class_unregister(&wmi_bus_class);
}

subsys_initcall(acpi_wmi_init);
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
The dell-wmi-smbios driver should be enabled by default when ACPI_WMI
is enabled (like many other WMI drivers).

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index a70bcd8caa72..4f9ca51b1968 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -94,6 +94,7 @@ config ASUS_LAPTOP
config DELL_WMI_SMBIOS
tristate "Dell WMI SMBIOS calling interface"
depends on ACPI_WMI
+ default ACPI_WMI
---help---
This module provides common functions for kernel modules using
Dell SMBIOS over ACPI-WMI.
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
This driver serves the purpose of responding to WMI based notifications
from the DELL_EVENT_GUID (9DBB5994-A997-11DA-B012-B622A1EF5492).
Other GUIDs will be handled by separate drivers.

Update the language used by this driver to avoid future confusion.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
MAINTAINERS | 2 +-
drivers/platform/x86/Kconfig | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2281af4b41b6..5d8ea24a8ee7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3996,7 +3996,7 @@ S: Maintained
F: Documentation/dcdbas.txt
F: drivers/firmware/dcdbas.*

-DELL WMI EXTRAS DRIVER
+DELL WMI NOTIFICATIONS DRIVER
M: Matthew Garrett <mj...@srcf.ucam.org>
M: Pali Rohár <pali....@gmail.com>
S: Maintained
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 80b87954f6dd..9e52f05daa2e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -116,7 +116,7 @@ config DELL_LAPTOP
laptops (except for some models covered by the Compal driver).

config DELL_WMI
- tristate "Dell WMI extras"
+ tristate "Dell WMI notifications"
depends on ACPI_WMI
depends on DMI
depends on INPUT
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
For WMI operations that are only Set or Query read or write sysfs
attributes created by WMI vendor drivers make sense.

For other WMI operations that are run on Method, there needs to be a
way to guarantee to userspace that the results from the method call
belong to the data request to the method call. Sysfs attributes don't
work well in this scenario because two userspace processes may be
competing at reading/writing an attribute and step on each other's
data.

When a WMI vendor driver declares a set of functions in a
file_operations object the WMI bus driver will create a character
device that maps to those file operations.

The WMI vendor drivers will be responsible for managing access to
this character device and proper locking on it.

When a WMI vendor driver is unloaded the WMI bus driver will clean
up the character device.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
include/linux/wmi.h | 1 +
2 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7a05843aff19..5495360ad2da 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -34,7 +34,9 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/acpi.h>
+#include <linux/cdev.h>
#include <linux/device.h>
+#include <linux/idr.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
@@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho");
MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
MODULE_LICENSE("GPL");

+#define WMI_MAX_DEVS MINORMASK
+static DEFINE_IDR(wmi_idr);
+static DEFINE_MUTEX(wmi_minor_lock);
static LIST_HEAD(wmi_block_list);

struct guid_block {
@@ -69,6 +74,8 @@ struct wmi_block {
struct wmi_device dev;
struct list_head list;
struct guid_block gblock;
+ struct cdev *cdev;
+ int minor;
struct acpi_device *acpi_device;
wmi_notify_handler handler;
void *handler_data;
@@ -86,6 +93,7 @@ struct wmi_block {
#define ACPI_WMI_STRING 0x4 /* GUID takes & returns a string */
#define ACPI_WMI_EVENT 0x8 /* GUID is an event */

+static dev_t wmi_devt;
static bool debug_event;
module_param(debug_event, bool, 0444);
MODULE_PARM_DESC(debug_event,
@@ -762,21 +770,88 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
return 0;
}

+static struct class wmi_bus_class = {
+ .name = "wmi_bus",
+};
+
+static int wmi_minor_get(struct wmi_block *wblock)
+{
+ int ret;
+
+ mutex_lock(&wmi_minor_lock);
+ ret = idr_alloc(&wmi_idr, wblock, 0, WMI_MAX_DEVS, GFP_KERNEL);
+ if (ret >= 0)
+ wblock->minor = ret;
+ else if (ret == -ENOSPC)
+ dev_err(&wblock->dev.dev, "too many wmi devices\n");
+
+ mutex_unlock(&wmi_minor_lock);
+ return ret;
+}
+
+static void wmi_minor_free(struct wmi_block *wblock)
+{
+ mutex_lock(&wmi_minor_lock);
+ idr_remove(&wmi_idr, wblock->minor);
+ mutex_unlock(&wmi_minor_lock);
+}
+
+
static int wmi_dev_probe(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;
+ struct device *clsdev;
+ int ret = 0, devno;

if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
dev_warn(dev, "failed to enable device -- probing anyway\n");

+ /* driver wants a character device made */
+ if (wdriver->file_operations) {
+ dev->devt = wmi_devt;
+ wblock->cdev = cdev_alloc();
+ if (!wblock->cdev) {
+ dev_err(dev, "failed to allocate cdev\n");
+ return -ENOMEM;
+ }
+ cdev_init(wblock->cdev, wdriver->file_operations);
+ wblock->cdev->owner = wdriver->file_operations->owner;
+ ret = wmi_minor_get(wblock);
+ if (ret < 0)
+ return ret;
+ devno = MKDEV(MAJOR(wmi_devt), wblock->minor);
+ ret = cdev_add(wblock->cdev, devno, 1);
+ if (ret) {
+ dev_err(dev, "unable to create device %d:%d\n",
+ MAJOR(wmi_devt), wblock->minor);
+ goto err_probe_cdev;
+ }
+ clsdev = device_create(&wmi_bus_class, dev,
+ MKDEV(MAJOR(wmi_devt), wblock->minor),
+ NULL, "wmi-%s", wdriver->driver.name);
+
+ if (IS_ERR(clsdev)) {
+ dev_err(dev, "unable to create device %d:%d\n",
+ MAJOR(wmi_devt), wblock->minor);
+ ret = PTR_ERR(clsdev);
+ goto err_probe_class;
+ }
+ }
+
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;
+
+err_probe_class:
+ cdev_del(wblock->cdev);
+
+err_probe_cdev:
+ wmi_minor_free(wblock);

return ret;
}
@@ -788,6 +863,13 @@ static int wmi_dev_remove(struct device *dev)
container_of(dev->driver, struct wmi_driver, driver);
int ret = 0;

+ if (wdriver->file_operations) {
+ device_destroy(&wmi_bus_class,
+ MKDEV(MAJOR(wmi_devt), wblock->minor));
+ cdev_del(wblock->cdev);
+ wmi_minor_free(wblock);
+ }
+
if (wdriver->remove)
ret = wdriver->remove(dev_to_wdev(dev));

@@ -797,10 +879,6 @@ static int wmi_dev_remove(struct device *dev)
return ret;
}

-static struct class wmi_bus_class = {
- .name = "wmi_bus",
-};
-
static struct bus_type wmi_bus_type = {
.name = "wmi",
.dev_groups = wmi_groups,
@@ -1250,8 +1328,17 @@ static int __init acpi_wmi_init(void)
goto err_unreg_bus;
}

+ error = alloc_chrdev_region(&wmi_devt, 0, WMI_MAX_DEVS, "wmi");
+ if (error < 0) {
+ pr_err("unable to allocate char dev region\n");
+ goto err_unreg_platform;
+ }
+
return 0;

+err_unreg_platform:
+ platform_driver_unregister(&acpi_wmi_driver);
+
err_unreg_bus:
bus_unregister(&wmi_bus_type);

@@ -1263,6 +1350,7 @@ static int __init acpi_wmi_init(void)

static void __exit acpi_wmi_exit(void)
{
+ unregister_chrdev_region(wmi_devt, WMI_MAX_DEVS);
platform_driver_unregister(&acpi_wmi_driver);
bus_unregister(&wmi_bus_type);
class_unregister(&wmi_bus_class);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index cd0d7734dc49..6899ddb6cd30 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -41,6 +41,7 @@ struct wmi_device_id {
struct wmi_driver {
struct device_driver driver;
const struct wmi_device_id *id_table;
+ const struct file_operations *file_operations;

int (*probe)(struct wmi_device *wdev);
int (*remove)(struct wmi_device *wdev);
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
This follows the style of the rest of the platform x86 WMI drivers.

Renaming the driver requires adjusting the other drivers using
dell-smbios to pick up the newly named includes.

While renaming, I noticed that this driver was missing from
MAINTAINERS. Add it to that and myself to the list of people maintaing
it.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
MAINTAINERS | 6 ++++++
drivers/platform/x86/Kconfig | 2 +-
drivers/platform/x86/Makefile | 2 +-
drivers/platform/x86/dell-laptop.c | 2 +-
drivers/platform/x86/{dell-smbios.c => dell-wmi-smbios.c} | 12 ++++++------
drivers/platform/x86/{dell-smbios.h => dell-wmi-smbios.h} | 4 ++--
drivers/platform/x86/dell-wmi.c | 2 +-
7 files changed, 18 insertions(+), 12 deletions(-)
rename drivers/platform/x86/{dell-smbios.c => dell-wmi-smbios.c} (97%)
rename drivers/platform/x86/{dell-smbios.h => dell-wmi-smbios.h} (96%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d8ea24a8ee7..437daa9062e1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4002,6 +4002,12 @@ M: Pali Rohár <pali....@gmail.com>
S: Maintained
F: drivers/platform/x86/dell-wmi.c

+DELL WMI SMBIOS DRIVER
+M: Pali Rohár <pali....@gmail.com>
+M: Mario Limonciello <mario.li...@dell.com>
+S: Maintained
+F: drivers/platform/x86/dell-wmi-smbios.c
+
DELTA ST MEDIA DRIVER
M: Hugues Fruchet <hugues....@st.com>
L: linux...@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 81d61c0f4ef8..a70bcd8caa72 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -91,7 +91,7 @@ config ASUS_LAPTOP

If you have an ACPI-compatible ASUS laptop, say Y or M here.

-config DELL_SMBIOS
+config DELL_WMI_SMBIOS
tristate "Dell WMI SMBIOS calling interface"
depends on ACPI_WMI
---help---
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 91cec1751461..b127a3bc1fab 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -11,11 +11,11 @@ obj-$(CONFIG_EEEPC_WMI) += eeepc-wmi.o
obj-$(CONFIG_MSI_LAPTOP) += msi-laptop.o
obj-$(CONFIG_ACPI_CMPC) += classmate-laptop.o
obj-$(CONFIG_COMPAL_LAPTOP) += compal-laptop.o
-obj-$(CONFIG_DELL_SMBIOS) += dell-smbios.o
obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
obj-$(CONFIG_DELL_WMI) += dell-wmi.o
obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
+obj-$(CONFIG_DELL_WMI_SMBIOS) += dell-wmi-smbios.o
obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
obj-$(CONFIG_DELL_RBTN) += dell-rbtn.o
obj-$(CONFIG_ACER_WMI) += acer-wmi.o
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index f42159fd2031..bf569ea93e9d 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -33,7 +33,7 @@
#include <linux/seq_file.h>
#include <acpi/video.h>
#include "dell-rbtn.h"
-#include "dell-smbios.h"
+#include "dell-wmi-smbios.h"

#define BRIGHTNESS_TOKEN 0x7d
#define KBD_LED_OFF_TOKEN 0x01E1
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
similarity index 97%
rename from drivers/platform/x86/dell-smbios.c
rename to drivers/platform/x86/dell-wmi-smbios.c
index 1fbc40791a48..b0812a8fa860 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -21,7 +21,7 @@
#include <linux/err.h>
#include <linux/mutex.h>
#include <linux/wmi.h>
-#include "dell-smbios.h"
+#include "dell-wmi-smbios.h"

#ifdef CONFIG_DCDBAS
#include "../../firmware/dcdbas.h"
@@ -253,14 +253,14 @@ static const struct wmi_device_id dell_smbios_wmi_id_table[] = {

static struct wmi_driver dell_wmi_smbios_driver = {
.driver = {
- .name = "dell-smbios",
+ .name = "dell-wmi-smbios",
},
.probe = dell_smbios_wmi_probe,
.remove = dell_smbios_wmi_remove,
.id_table = dell_smbios_wmi_id_table,
};

-static int __init dell_smbios_init(void)
+static int __init dell_wmi_smbios_init(void)
{
dmi_walk(find_tokens, NULL);

@@ -287,7 +287,7 @@ static int __init dell_smbios_init(void)
return 0;
}

-static void __exit dell_smbios_exit(void)
+static void __exit dell_wmi_smbios_exit(void)
{
kfree(da_tokens);
#ifdef CONFIG_DCDBAS
@@ -297,8 +297,8 @@ static void __exit dell_smbios_exit(void)
wmi_driver_unregister(&dell_wmi_smbios_driver);
}

-subsys_initcall(dell_smbios_init);
-module_exit(dell_smbios_exit);
+subsys_initcall(dell_wmi_smbios_init);
+module_exit(dell_wmi_smbios_exit);


MODULE_AUTHOR("Matthew Garrett <m...@redhat.com>");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
similarity index 96%
rename from drivers/platform/x86/dell-smbios.h
rename to drivers/platform/x86/dell-wmi-smbios.h
index 2f6fce81ee69..14b7e2ece310 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-wmi-smbios.h
@@ -14,8 +14,8 @@
* published by the Free Software Foundation.
*/

-#ifndef _DELL_SMBIOS_H_
-#define _DELL_SMBIOS_H_
+#ifndef _DELL_WMI_SMBIOS_H_
+#define _DELL_WMI_SMBIOS_H_

struct notifier_block;

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1fbef560ca67..e8b4d412eabc 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -38,7 +38,7 @@
#include <linux/dmi.h>
#include <linux/wmi.h>
#include <acpi/video.h>
-#include "dell-smbios.h"
+#include "dell-wmi-smbios.h"

MODULE_AUTHOR("Matthew Garrett <m...@redhat.com>");
MODULE_AUTHOR("Pali Rohár <pali....@gmail.com>");
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
pr_fmt provides formatting to be used by the driver when
displaying errors and messages.

Reviewed-by: Pali Pohar <pali....@gmail.com>
Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-smbios.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 6654ad44a04c..579be67de2a3 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -12,6 +12,7 @@
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/kernel.h>
#include <linux/module.h>
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
The descriptor GUID is not used to indicate that WMI notifications
in the dell-wmi driver work properly. As such a modalias should
not be present that causes this driver to load on systems with this
GUID.

Reviewed-by: Pali Pohar <pali....@gmail.com>
Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-wmi.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 28d9f8696081..1fbef560ca67 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -51,7 +51,6 @@ MODULE_LICENSE("GPL");
static bool wmi_requires_smbios_request;

MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
-MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);

struct dell_wmi_priv {
struct input_dev *input_dev;
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
The driver currently uses an SMI interface which grants direct access
to physical memory to the platform via a pointer.

Now add a WMI-ACPI interface that is detected by WMI probe and preferred
over the SMI interface.

Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
for a buffer of data storage when platform calls are performed.

This is a safer approach to use in kernel drivers as the platform will
only have access to that OperationRegion.

As a result, this change removes the dependency on this driver on the
dcdbas kernel module. It's now an optional compilation option.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/Kconfig | 8 +-
drivers/platform/x86/dell-smbios.c | 150 ++++++++++++++++++++++++++++++-------
drivers/platform/x86/dell-smbios.h | 15 +++-
3 files changed, 140 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 9e52f05daa2e..81d61c0f4ef8 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -92,13 +92,13 @@ config ASUS_LAPTOP
If you have an ACPI-compatible ASUS laptop, say Y or M here.

config DELL_SMBIOS
- tristate
- select DCDBAS
+ tristate "Dell WMI SMBIOS calling interface"
+ depends on ACPI_WMI
---help---
This module provides common functions for kernel modules using
- Dell SMBIOS.
+ Dell SMBIOS over ACPI-WMI.

- If you have a Dell laptop, say Y or M here.
+ If you have a Dell computer, say Y or M here.

config DELL_LAPTOP
tristate "Dell Laptop Extras"
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 579be67de2a3..1fbc40791a48 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -4,6 +4,7 @@
* Copyright (c) Red Hat <m...@redhat.com>
* Copyright (c) 2014 Gabriele Mazzotta <gabrie...@gmail.com>
* Copyright (c) 2014 Pali Rohár <pali....@gmail.com>
+ * Copyright (c) 2017 Dell Inc.
*
* Based on documentation in the libsmbios package:
* Copyright (C) 2005-2014 Dell Inc.
@@ -19,9 +20,15 @@
#include <linux/dmi.h>
#include <linux/err.h>
#include <linux/mutex.h>
-#include "../../firmware/dcdbas.h"
+#include <linux/wmi.h>
#include "dell-smbios.h"

+#ifdef CONFIG_DCDBAS
+#include "../../firmware/dcdbas.h"
+#endif
+
+#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+
struct calling_interface_structure {
struct dmi_header header;
u16 cmdIOAddress;
@@ -30,12 +37,14 @@ struct calling_interface_structure {
struct calling_interface_token tokens[];
} __packed;

-static struct calling_interface_buffer *buffer;
+static struct calling_interface_buffer *smi_buffer;
+static struct wmi_calling_interface_buffer *wmi_buffer;
static DEFINE_MUTEX(buffer_mutex);

static int da_command_address;
static int da_command_code;
static int da_num_tokens;
+static int has_wmi;
static struct calling_interface_token *da_tokens;

int dell_smbios_error(int value)
@@ -57,13 +66,20 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
{
mutex_lock(&buffer_mutex);
dell_smbios_clear_buffer();
- return buffer;
+ if (has_wmi)
+ return &wmi_buffer->smi;
+ return smi_buffer;
}
EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);

void dell_smbios_clear_buffer(void)
{
- memset(buffer, 0, sizeof(struct calling_interface_buffer));
+ if (has_wmi)
+ memset(wmi_buffer, 0,
+ sizeof(struct wmi_calling_interface_buffer));
+ else
+ memset(smi_buffer, 0,
+ sizeof(struct calling_interface_buffer));
}
EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);

@@ -73,20 +89,60 @@ void dell_smbios_release_buffer(void)
}
EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);

-void dell_smbios_send_request(int class, int select)
+int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
{
- struct smi_cmd command;
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+ struct acpi_buffer input;
+ union acpi_object *obj;
+ acpi_status status;
+
+ input.length = sizeof(struct wmi_calling_interface_buffer);
+ input.pointer = buf;
+
+ status = wmi_evaluate_method(DELL_WMI_SMBIOS_GUID,
+ 0, 1, &input, &output);
+ if (ACPI_FAILURE(status)) {
+ pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
+ buf->smi.class, buf->smi.select,
+ buf->smi.input[0], buf->smi.input[1],
+ buf->smi.input[2], buf->smi.input[3]);
+ return -EIO;
+ }
+ obj = (union acpi_object *)output.pointer;
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ pr_err("invalid type : %d\n", obj->type);
+ return -EIO;
+ }
+ memcpy(buf, obj->buffer.pointer, input.length);

- command.magic = SMI_CMD_MAGIC;
- command.command_address = da_command_address;
- command.command_code = da_command_code;
- command.ebx = virt_to_phys(buffer);
- command.ecx = 0x42534931;
+ return 0;
+}

- buffer->class = class;
- buffer->select = select;
+void dell_smbios_send_request(int class, int select)
+{
+ if (has_wmi) {
+ wmi_buffer->smi.class = class;
+ wmi_buffer->smi.select = select;
+ run_wmi_smbios_call(wmi_buffer);
+ }

- dcdbas_smi_request(&command);
+#ifdef CONFIG_DCDBAS
+ else {
+ if (!smi_buffer)
+ return;
+ struct smi_cmd command;
+
+ smi_buffer->class = class;
+ smi_buffer->select = select;
+ command.magic = SMI_CMD_MAGIC;
+ command.command_address = da_command_address;
+ command.command_code = da_command_code;
+ command.ebx = virt_to_phys(smi_buffer);
+ command.ecx = 0x42534931;
+
+ dcdbas_smi_request(&command);
+ }
+#endif
}
EXPORT_SYMBOL_GPL(dell_smbios_send_request);

@@ -167,10 +223,45 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

-static int __init dell_smbios_init(void)
+static int dell_smbios_wmi_probe(struct wmi_device *wdev)
{
- int ret;
+ /* WMI buffer should be 32k */
+ wmi_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+ if (!wmi_buffer)
+ return -ENOMEM;
+
+#ifdef CONFIG_DCDBAS
+ /* no longer need the SMI page */
+ free_page((unsigned long)smi_buffer);
+ smi_buffer = NULL;
+#endif
+
+ has_wmi = 1;
+ return 0;
+}
+
+static int dell_smbios_wmi_remove(struct wmi_device *wdev)
+{
+ free_pages((unsigned long)wmi_buffer, 3);
+ return 0;
+}
+
+static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
+ { .guid_string = DELL_WMI_SMBIOS_GUID },
+ { },
+};

+static struct wmi_driver dell_wmi_smbios_driver = {
+ .driver = {
+ .name = "dell-smbios",
+ },
+ .probe = dell_smbios_wmi_probe,
+ .remove = dell_smbios_wmi_remove,
+ .id_table = dell_smbios_wmi_id_table,
+};
+
+static int __init dell_smbios_init(void)
+{
dmi_walk(find_tokens, NULL);

if (!da_tokens) {
@@ -178,34 +269,41 @@ static int __init dell_smbios_init(void)
return -ENODEV;
}

+#ifdef CONFIG_DCDBAS
/*
* Allocate buffer below 4GB for SMI data--only 32-bit physical addr
* is passed to SMI handler.
*/
- buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
- if (!buffer) {
- ret = -ENOMEM;
- goto fail_buffer;
+ smi_buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+#else
+ smi_buffer = NULL;
+#endif
+ wmi_driver_register(&dell_wmi_smbios_driver);
+
+ if (!smi_buffer && !has_wmi) {
+ kfree(da_tokens);
+ return -ENOMEM;
}
-
return 0;
-
-fail_buffer:
- kfree(da_tokens);
- return ret;
}

static void __exit dell_smbios_exit(void)
{
kfree(da_tokens);
- free_page((unsigned long)buffer);
+#ifdef CONFIG_DCDBAS
+ if (!has_wmi)
+ free_page((unsigned long)smi_buffer);
+#endif
+ wmi_driver_unregister(&dell_wmi_smbios_driver);
}

subsys_initcall(dell_smbios_init);
module_exit(dell_smbios_exit);

+
MODULE_AUTHOR("Matthew Garrett <m...@redhat.com>");
MODULE_AUTHOR("Gabriele Mazzotta <gabrie...@gmail.com>");
MODULE_AUTHOR("Pali Rohár <pali....@gmail.com>");
+MODULE_AUTHOR("Mario Limonciello <mario.li...@dell.com>");
MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS");
MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 45cbc2292cd3..2f6fce81ee69 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -4,6 +4,7 @@
* Copyright (c) Red Hat <m...@redhat.com>
* Copyright (c) 2014 Gabriele Mazzotta <gabrie...@gmail.com>
* Copyright (c) 2014 Pali Rohár <pali....@gmail.com>
+ * Copyright (c) 2017 Dell Inc.
*
* Based on documentation in the libsmbios package:
* Copyright (C) 2005-2014 Dell Inc.
@@ -18,9 +19,10 @@

struct notifier_block;

-/* This structure will be modified by the firmware when we enter
- * system management mode, hence the volatiles */
-
+/* If called through fallback SMI rather than WMI this structure will be
+ * modified by the firmware when we enter system management mode, hence the
+ * volatiles
+ */
struct calling_interface_buffer {
u16 class;
u16 select;
@@ -28,6 +30,13 @@ struct calling_interface_buffer {
volatile u32 output[4];
} __packed;

+struct wmi_calling_interface_buffer {
+ struct calling_interface_buffer smi;
+ u32 argattrib;
+ u32 blength;
+ u8 data[32724];
+} __packed;
+
struct calling_interface_token {
u16 tokenID;
u16 location;
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
Some cases the wrong type was used for errors and checks can be
done more cleanly.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-wmi-smbios.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
index 232a9dd482b1..290e9853818b 100644
--- a/drivers/platform/x86/dell-wmi-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -340,16 +340,16 @@ int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version)
}
desc_buffer = (u32 *)obj->buffer.pointer;

- if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
- 8, desc_buffer);
+ if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
+ desc_buffer);

if (desc_buffer[2] != 0 && desc_buffer[2] != 1)
- dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
desc_buffer[2]);

if (desc_buffer[3] != 4096)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\n",
desc_buffer[3]);

*version = desc_buffer[2];
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:07 PM9/26/17
to
The Dell WMI descriptor check is used as an indication that WMI
calls are safe to run both when used with the notification
ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.

As some code in dell-wmi-smbios is already a prerequisite for
dell-wmi, move the code for performing the descriptor check into
dell-wmi-smbios and let both drivers use it from there.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-wmi-smbios.c | 84 ++++++++++++++++++++++++++++++++++
drivers/platform/x86/dell-wmi-smbios.h | 3 ++
drivers/platform/x86/dell-wmi.c | 75 +-----------------------------
3 files changed, 88 insertions(+), 74 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
index b0812a8fa860..699757f3e154 100644
--- a/drivers/platform/x86/dell-wmi-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -28,6 +28,7 @@
#endif

#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"

struct calling_interface_structure {
struct dmi_header header;
@@ -223,13 +224,92 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

+/*
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ * Name Offset Length Value
+ * Vendor Signature 0 4 "DELL"
+ * Object Signature 4 4 " WMI"
+ * WMI Interface Version 8 4 <version>
+ * WMI buffer length 12 4 4096
+ */
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version)
+{
+ union acpi_object *obj = NULL;
+ struct wmi_device *desc_dev;
+ u32 *desc_buffer;
+ int ret;
+
+ desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
+ if (!desc_dev) {
+ dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
+ return -ENODEV;
+ }
+
+ obj = wmidev_block_query(desc_dev, 0);
+ if (!obj) {
+ dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
+ ret = -EIO;
+ goto out;
+ }
+
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (obj->buffer.length != 128) {
+ dev_err(&wdev->dev,
+ "Dell descriptor buffer has invalid length (%d)\n",
+ obj->buffer.length);
+ if (obj->buffer.length < 16) {
+ ret = -EINVAL;
+ goto out;
+ }
+ }
+ desc_buffer = (u32 *)obj->buffer.pointer;
+
+ if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
+ 8, desc_buffer);
+
+ if (desc_buffer[2] != 0 && desc_buffer[2] != 1)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
+ desc_buffer[2]);
+
+ if (desc_buffer[3] != 4096)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
+ desc_buffer[3]);
+
+ *version = desc_buffer[2];
+ ret = 0;
+
+ dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
+ *version);
+
+out:
+ kfree(obj);
+ put_device(&desc_dev->dev);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_check_descriptor_buffer);
+
+
static int dell_smbios_wmi_probe(struct wmi_device *wdev)
{
+ int ret;
+ u32 interface_version;
+
/* WMI buffer should be 32k */
wmi_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
if (!wmi_buffer)
return -ENOMEM;

+ ret = dell_wmi_check_descriptor_buffer(wdev, &interface_version);
+ if (ret)
+ goto fail_wmi_probe;
+
#ifdef CONFIG_DCDBAS
/* no longer need the SMI page */
free_page((unsigned long)smi_buffer);
@@ -238,6 +318,10 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)

has_wmi = 1;
return 0;
+
+fail_wmi_probe:
+ free_pages((unsigned long)wmi_buffer, 3);
+ return ret;
}

static int dell_smbios_wmi_remove(struct wmi_device *wdev)
diff --git a/drivers/platform/x86/dell-wmi-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
index 14b7e2ece310..0e8b57f7d0f0 100644
--- a/drivers/platform/x86/dell-wmi-smbios.h
+++ b/drivers/platform/x86/dell-wmi-smbios.h
@@ -17,6 +17,8 @@
#ifndef _DELL_WMI_SMBIOS_H_
#define _DELL_WMI_SMBIOS_H_

+#include <linux/wmi.h>
+
struct notifier_block;

/* If called through fallback SMI rather than WMI this structure will be
@@ -62,5 +64,6 @@ enum dell_laptop_notifier_actions {
int dell_laptop_register_notifier(struct notifier_block *nb);
int dell_laptop_unregister_notifier(struct notifier_block *nb);
void dell_laptop_call_notifier(unsigned long action, void *data);
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version);

#endif
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index e8b4d412eabc..e7011792127f 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -46,7 +46,6 @@ MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver");
MODULE_LICENSE("GPL");

#define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
-#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"

static bool wmi_requires_smbios_request;

@@ -617,78 +616,6 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
input_unregister_device(priv->input_dev);
}

-/*
- * Descriptor buffer is 128 byte long and contains:
- *
- * Name Offset Length Value
- * Vendor Signature 0 4 "DELL"
- * Object Signature 4 4 " WMI"
- * WMI Interface Version 8 4 <version>
- * WMI buffer length 12 4 4096
- */
-static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
-{
- struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
- union acpi_object *obj = NULL;
- struct wmi_device *desc_dev;
- u32 *buffer;
- int ret;
-
- desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
- if (!desc_dev) {
- dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
- return -ENODEV;
- }
-
- obj = wmidev_block_query(desc_dev, 0);
- if (!obj) {
- dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
- ret = -EIO;
- goto out;
- }
-
- if (obj->type != ACPI_TYPE_BUFFER) {
- dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
- ret = -EINVAL;
- goto out;
- }
-
- if (obj->buffer.length != 128) {
- dev_err(&wdev->dev,
- "Dell descriptor buffer has invalid length (%d)\n",
- obj->buffer.length);
- if (obj->buffer.length < 16) {
- ret = -EINVAL;
- goto out;
- }
- }
-
- buffer = (u32 *)obj->buffer.pointer;
-
- if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
- 8, buffer);
-
- if (buffer[2] != 0 && buffer[2] != 1)
- dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
- buffer[2]);
-
- if (buffer[3] != 4096)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
- buffer[3]);
-
- priv->interface_version = buffer[2];
- ret = 0;
-
- dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
- priv->interface_version);
-
-out:
- kfree(obj);
- put_device(&desc_dev->dev);
- return ret;
-}
-
/*
* According to Dell SMBIOS documentation:
*
@@ -732,7 +659,7 @@ static int dell_wmi_probe(struct wmi_device *wdev)
return -ENOMEM;
dev_set_drvdata(&wdev->dev, priv);

- err = dell_wmi_check_descriptor_buffer(wdev);
+ err = dell_wmi_check_descriptor_buffer(wdev, &priv->interface_version);
if (err)
return err;

--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:08 PM9/26/17
to
These aren't needed to compile the module.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-smbios.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 0a5723468bff..6654ad44a04c 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -17,10 +17,7 @@
#include <linux/module.h>
#include <linux/dmi.h>
#include <linux/err.h>
-#include <linux/gfp.h>
#include <linux/mutex.h>
-#include <linux/slab.h>
-#include <linux/io.h>
#include "../../firmware/dcdbas.h"
#include "dell-smbios.h"

--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:13 PM9/26/17
to
This userspace character device will be used to perform SMBIOS calls
from any applications.

It contains 3 operating IOCTL's
1) sending a properly formatted 4k calling interface buffer.
2) Querying number of tokens in the given system
3) Copying the status of all these tokens to a properly formatted
userspace buffer.

This character device is intended to deprecate the dcdbas kernel module
and the interface that it provides to userspace.

It's important for the driver to provide a R/W ioctl to ensure that
two competing userspace processes don't race to provide or read each
others data.

The character device will only be created if the WMI interface was
found.

The API for interacting with this interface is defined in documentation
as well as a uapi header provides the format of the structures.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
Documentation/ABI/testing/dell-wmi-smbios | 10 +++
drivers/platform/x86/dell-wmi-smbios.c | 110 +++++++++++++++++++++++++++---
drivers/platform/x86/dell-wmi-smbios.h | 28 +-------
include/uapi/linux/dell-wmi-smbios.h | 55 +++++++++++++++
4 files changed, 165 insertions(+), 38 deletions(-)
create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
create mode 100644 include/uapi/linux/dell-wmi-smbios.h

diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
new file mode 100644
index 000000000000..8ec98e9b7b34
--- /dev/null
+++ b/Documentation/ABI/testing/dell-wmi-smbios
@@ -0,0 +1,10 @@
+What: /dev/wmi-dell-wmi-smbios
+Date: October 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.li...@dell.com>
+Description:
+ Perform SMBIOS calls on supported Dell machines.
+ through the Dell ACPI-WMI interface.
+
+ IOCTL's and buffer formats are defined in:
+ <uapi/linux/dell-wmi-smbios.h>
\ No newline at end of file
diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
index 699757f3e154..232a9dd482b1 100644
--- a/drivers/platform/x86/dell-wmi-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -21,6 +21,7 @@
#include <linux/err.h>
#include <linux/mutex.h>
#include <linux/wmi.h>
+#include <linux/uaccess.h>
#include "dell-wmi-smbios.h"

#ifdef CONFIG_DCDBAS
@@ -39,7 +40,8 @@ struct calling_interface_structure {
} __packed;

static struct calling_interface_buffer *smi_buffer;
-static struct wmi_calling_interface_buffer *wmi_buffer;
+static struct wmi_calling_interface_buffer *internal_wmi_buffer;
+static struct wmi_calling_interface_buffer *sysfs_wmi_buffer;
static DEFINE_MUTEX(buffer_mutex);

static int da_command_address;
@@ -68,7 +70,7 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
mutex_lock(&buffer_mutex);
dell_smbios_clear_buffer();
if (has_wmi)
- return &wmi_buffer->smi;
+ return &internal_wmi_buffer->smi;
return smi_buffer;
}
EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
@@ -76,7 +78,7 @@ EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
void dell_smbios_clear_buffer(void)
{
if (has_wmi)
- memset(wmi_buffer, 0,
+ memset(internal_wmi_buffer, 0,
sizeof(struct wmi_calling_interface_buffer));
else
memset(smi_buffer, 0,
@@ -122,9 +124,9 @@ int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
void dell_smbios_send_request(int class, int select)
{
if (has_wmi) {
- wmi_buffer->smi.class = class;
- wmi_buffer->smi.select = select;
- run_wmi_smbios_call(wmi_buffer);
+ internal_wmi_buffer->smi.class = class;
+ internal_wmi_buffer->smi.select = select;
+ run_wmi_smbios_call(internal_wmi_buffer);
}

#ifdef CONFIG_DCDBAS
@@ -224,6 +226,74 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

+static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
+{
+ return nonseekable_open(inode, file);
+}
+
+static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct token_ioctl_buffer *tokens_buffer;
+ void __user *p = (void __user *) arg;
+ size_t size;
+ int ret = 0;
+
+ if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
+ return -ENOTTY;
+
+ switch (cmd) {
+ case DELL_WMI_SMBIOS_CALL_CMD:
+ size = sizeof(struct wmi_calling_interface_buffer);
+ mutex_lock(&buffer_mutex);
+ if (copy_from_user(sysfs_wmi_buffer, p, size)) {
+ ret = -EFAULT;
+ goto fail_smbios_cmd;
+ }
+ ret = run_wmi_smbios_call(sysfs_wmi_buffer);
+ if (ret != 0)
+ goto fail_smbios_cmd;
+ if (copy_to_user(p, sysfs_wmi_buffer, size))
+ ret = -EFAULT;
+fail_smbios_cmd:
+ mutex_unlock(&buffer_mutex);
+ break;
+ case DELL_WMI_SMBIOS_GET_NUM_TOKENS_CMD:
+ if (copy_to_user(p, &da_num_tokens, sizeof(u32)))
+ ret = -EFAULT;
+ break;
+ case DELL_WMI_SMBIOS_GET_TOKENS_CMD:
+ tokens_buffer = kmalloc(sizeof(struct token_ioctl_buffer),
+ GFP_KERNEL);
+ size = sizeof(struct token_ioctl_buffer);
+ if (copy_from_user(tokens_buffer, p, size)) {
+ ret = -EFAULT;
+ goto fail_get_tokens_cmd;
+ }
+ if (tokens_buffer->num_tokens < da_num_tokens) {
+ ret = -EOVERFLOW;
+ goto fail_get_tokens_cmd;
+ }
+ size = sizeof(struct calling_interface_token) * da_num_tokens;
+ if (copy_to_user(tokens_buffer->tokens, da_tokens, size)) {
+ ret = -EFAULT;
+ goto fail_get_tokens_cmd;
+ }
+fail_get_tokens_cmd:
+ kfree(tokens_buffer);
+ break;
+ default:
+ pr_err("unsupported ioctl: %d.\n", cmd);
+ ret = -ENOIOCTLCMD;
+ }
+ return ret;
+}
+
/*
* Descriptor buffer is 128 byte long and contains:
*
@@ -301,11 +371,17 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
int ret;
u32 interface_version;

- /* WMI buffer should be 32k */
- wmi_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
- if (!wmi_buffer)
+ /* WMI buffers should be 32k */
+ internal_wmi_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+ if (!internal_wmi_buffer)
return -ENOMEM;

+ sysfs_wmi_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+ if (!sysfs_wmi_buffer) {
+ ret = -ENOMEM;
+ goto fail_sysfs_wmi_buffer;
+ }
+
ret = dell_wmi_check_descriptor_buffer(wdev, &interface_version);
if (ret)
goto fail_wmi_probe;
@@ -320,13 +396,17 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
return 0;

fail_wmi_probe:
- free_pages((unsigned long)wmi_buffer, 3);
+ free_pages((unsigned long)sysfs_wmi_buffer, 3);
+
+fail_sysfs_wmi_buffer:
+ free_pages((unsigned long)internal_wmi_buffer, 3);
return ret;
}

static int dell_smbios_wmi_remove(struct wmi_device *wdev)
{
- free_pages((unsigned long)wmi_buffer, 3);
+ free_pages((unsigned long)internal_wmi_buffer, 3);
+ free_pages((unsigned long)sysfs_wmi_buffer, 3);
return 0;
}

@@ -335,6 +415,13 @@ static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
{ },
};

+static const struct file_operations dell_wmi_smbios_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = dell_wmi_smbios_ioctl,
+ .open = dell_wmi_smbios_open,
+ .release = dell_wmi_smbios_release,
+};
+
static struct wmi_driver dell_wmi_smbios_driver = {
.driver = {
.name = "dell-wmi-smbios",
@@ -342,6 +429,7 @@ static struct wmi_driver dell_wmi_smbios_driver = {
.probe = dell_smbios_wmi_probe,
.remove = dell_smbios_wmi_remove,
.id_table = dell_smbios_wmi_id_table,
+ .file_operations = &dell_wmi_smbios_fops,
};

static int __init dell_wmi_smbios_init(void)
diff --git a/drivers/platform/x86/dell-wmi-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
index 0e8b57f7d0f0..9bc9c42db167 100644
--- a/drivers/platform/x86/dell-wmi-smbios.h
+++ b/drivers/platform/x86/dell-wmi-smbios.h
@@ -18,36 +18,10 @@
#define _DELL_WMI_SMBIOS_H_

#include <linux/wmi.h>
+#include <uapi/linux/dell-wmi-smbios.h>

struct notifier_block;

-/* If called through fallback SMI rather than WMI this structure will be
- * modified by the firmware when we enter system management mode, hence the
- * volatiles
- */
-struct calling_interface_buffer {
- u16 class;
- u16 select;
- volatile u32 input[4];
- volatile u32 output[4];
-} __packed;
-
-struct wmi_calling_interface_buffer {
- struct calling_interface_buffer smi;
- u32 argattrib;
- u32 blength;
- u8 data[32724];
-} __packed;
-
-struct calling_interface_token {
- u16 tokenID;
- u16 location;
- union {
- u16 value;
- u16 stringlength;
- };
-};
-
int dell_smbios_error(int value);

struct calling_interface_buffer *dell_smbios_get_buffer(void);
diff --git a/include/uapi/linux/dell-wmi-smbios.h b/include/uapi/linux/dell-wmi-smbios.h
new file mode 100644
index 000000000000..743035d9443a
--- /dev/null
+++ b/include/uapi/linux/dell-wmi-smbios.h
@@ -0,0 +1,55 @@
+#ifndef _UAPI_DELL_WMI_SMBIOS_H_
+#define _UAPI_DELL_WMI_SMBIOS_H_
+
+#include <linux/ioctl.h>
+
+/* If called through fallback SMI rather than WMI this structure will be
+ * modified by the firmware when we enter system management mode, hence the
+ * volatiles
+ */
+struct calling_interface_buffer {
+ u16 class;
+ u16 select;
+ volatile u32 input[4];
+ volatile u32 output[4];
+} __packed;
+
+struct wmi_calling_interface_buffer {
+ struct calling_interface_buffer smi;
+ u32 argattrib;
+ u32 blength;
+ u8 data[32724];
+} __packed;
+
+struct calling_interface_token {
+ u16 tokenID;
+ u16 location;
+ union {
+ u16 value;
+ u16 stringlength;
+ };
+};
+
+struct token_ioctl_buffer {
+ struct calling_interface_token *tokens;
+ u32 num_tokens;
+};
+
+#define DELL_WMI_SMBIOS_IOC 'D'
+/* run SMBIOS calling interface command
+ * note - 32k is too big for size, so this can not be encoded in macro properly
+ */
+#define DELL_WMI_SMBIOS_CALL_CMD _IOWR(DELL_WMI_SMBIOS_IOC, 0, u8)
+
+/* query the number of DA tokens on system */
+#define DELL_WMI_SMBIOS_GET_NUM_TOKENS_CMD _IOR(DELL_WMI_SMBIOS_IOC, 1, \
+ u32)
+/* query the status, location, and value of all DA tokens from bootup
+ * expects userspace to prepare buffer in advance with the number of tokens
+ * from DELL_WMI_SMBIOS_GET_NUM_TOKENS_CMD
+ */
+#define DELL_WMI_SMBIOS_GET_TOKENS_CMD _IOWR(DELL_WMI_SMBIOS_IOC, 2, \
+ struct token_ioctl_buffer)
+
+
+#endif /* _UAPI_DELL_WMI_SMBIOS_H_ */
--
2.14.1

Mario Limonciello

unread,
Sep 26, 2017, 3:00:16 PM9/26/17
to
The include list is randomly assembled right now. Sort in alphabetical
order.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 0765b1797d4c..e19b074df01d 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -33,17 +33,17 @@

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/types.h>
+#include <linux/acpi.h>
#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
#include <linux/list.h>
-#include <linux/acpi.h>
-#include <linux/slab.h>
#include <linux/module.h>
#include <linux/platform_device.h>
-#include <linux/wmi.h>
+#include <linux/slab.h>
+#include <linux/types.h>
#include <linux/uuid.h>
+#include <linux/wmi.h>

ACPI_MODULE_NAME("wmi");
MODULE_AUTHOR("Carlos Corbacho");
--
2.14.1

Andy Shevchenko

unread,
Sep 26, 2017, 3:10:07 PM9/26/17
to
On Tue, Sep 26, 2017 at 9:50 PM, Mario Limonciello
<mario.li...@dell.com> wrote:
> This userspace character device will be used to perform SMBIOS calls
> from any applications.
>
> It contains 3 operating IOCTL's
> 1) sending a properly formatted 4k calling interface buffer.
> 2) Querying number of tokens in the given system
> 3) Copying the status of all these tokens to a properly formatted
> userspace buffer.
>
> This character device is intended to deprecate the dcdbas kernel module
> and the interface that it provides to userspace.
>
> It's important for the driver to provide a R/W ioctl to ensure that
> two competing userspace processes don't race to provide or read each
> others data.
>
> The character device will only be created if the WMI interface was
> found.
>
> The API for interacting with this interface is defined in documentation
> as well as a uapi header provides the format of the structures.


> + <uapi/linux/dell-wmi-smbios.h>
> \ No newline at end of file

Oh là là

Andy Shevchenko

unread,
Sep 26, 2017, 3:10:08 PM9/26/17
to
Thanks for an update, looks better now.
I'm in the middle of going through it and noticed that patches 6 and 7
sound like a fix to me.
Perhaps better to move it to be first part of the series.

Please, include Andy Lutomirski to Cc list, I'm pretty sure he is
interested in changes like these to WMI.

Mario.Li...@dell.com

unread,
Sep 26, 2017, 3:20:07 PM9/26/17
to
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.sh...@gmail.com]
> Sent: Tuesday, September 26, 2017 3:06 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
In v2 I'd say that all of 1 through 7 are fixes that even if the rest of the
series doesn't get pulled in immediately should land sooner.
I'll reorder 6 and 7 for the beginning though for v3 if v3 is necessary.

8 through 13 are the real "meat", and 14 is a fixup.

>
> Please, include Andy Lutomirski to Cc list, I'm pretty sure he is
> interested in changes like these to WMI.

Added. Andy you weren't CC'ed on the original or v2 submission, but if this
requires a v3 I'll include you.

Pali Rohár

unread,
Sep 26, 2017, 4:10:10 PM9/26/17
to
On Tuesday 26 September 2017 20:50:07 Mario Limonciello wrote:
> This follows the style of the rest of the platform x86 WMI drivers.
>
> Renaming the driver requires adjusting the other drivers using
> dell-smbios to pick up the newly named includes.
>
> While renaming, I noticed that this driver was missing from
> MAINTAINERS. Add it to that and myself to the list of people
> maintaing it.
>
> Signed-off-by: Mario Limonciello <mario.li...@dell.com>

Now when driver supports both old and new WMI call interface, I think
that renaming is not needed...

--
Pali Rohár
pali....@gmail.com
signature.asc

Mario.Li...@dell.com

unread,
Sep 26, 2017, 4:20:08 PM9/26/17
to
> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Tuesday, September 26, 2017 4:06 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
So all other drivers that use WMI have "WMI" in the name, this would be
breaking previously set precedence. Daren, Andy, what would you like
me to do?

Mario.Li...@dell.com

unread,
Sep 26, 2017, 4:20:08 PM9/26/17
to
> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Tuesday, September 26, 2017 4:10 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; Andy Shevchenko <andy.sh...@gmail.com>;
> LKML <linux-...@vger.kernel.org>; platform-...@vger.kernel.org;
> quas...@google.com
> Subject: Re: [PATCH v2 12/14] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
>
> Should be "wmi" part of device name, even if whole driver work without
> WMI?

Well actually the driver won't create a character device unless WMI is
used. Exposing a character device for non-WMI is just duplicating dcdbas
at another endpoint without any security advantage.

>
> I think that "wmi" should be part of device name only if it is related
> to WMI bus functions, and not in case when it exports some vendor (Dell)
> specific API which may be implemented via WMI.
>
> E.g. we have /dev/i2c-* for raw access to i2c devices. But we do not
> have /dev/i2c-something for e.g. RTC device connected via i2c...

I see what you mean here, but I think we need Darren and Andy to weigh
in here.

The character device does use WMI bus methods (wmi_evaluate_method).
The primary reason to create with wmi in the name is to show that the character
devices are supposed to be created and managed by the WMI bus. Vendor
drivers should not create their "own". In my mind this will be especially relevant
once WMI bus can interpret MOF and create a set of mappings for attributes
defined in MOF.

Pali Rohár

unread,
Sep 26, 2017, 4:20:09 PM9/26/17
to
On Tuesday 26 September 2017 20:50:12 Mario Limonciello wrote:
> + if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0)

Should not be this memcmp? IIRC we want to compare that buffer is equal
to some bytes, right?

--
Pali Rohár
pali....@gmail.com
signature.asc

Pali Rohár

unread,
Sep 26, 2017, 4:20:14 PM9/26/17
to
Should be "wmi" part of device name, even if whole driver work without
WMI?

I think that "wmi" should be part of device name only if it is related
to WMI bus functions, and not in case when it exports some vendor (Dell)
specific API which may be implemented via WMI.

E.g. we have /dev/i2c-* for raw access to i2c devices. But we do not
have /dev/i2c-something for e.g. RTC device connected via i2c...

--
Pali Rohár
pali....@gmail.com
signature.asc

Mario.Li...@dell.com

unread,
Sep 26, 2017, 4:30:06 PM9/26/17
to
> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Tuesday, September 26, 2017 4:12 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; Andy Shevchenko <andy.sh...@gmail.com>;
> LKML <linux-...@vger.kernel.org>; platform-...@vger.kernel.org;
> quas...@google.com
> Subject: Re: [PATCH v2 14/14] platform/x86: dell-wmi-smbios: clean up wmi
> descriptor check
>
> On Tuesday 26 September 2017 20:50:12 Mario Limonciello wrote:
> > + if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0)
>
> Should not be this memcmp? IIRC we want to compare that buffer is equal
> to some bytes, right?
>

It used to be comparing the bytes, but Andy S. thought this wasn't very clear.
That's why I adjusted it to compare to the strings that the bytes used instead.

Darren Hart

unread,
Sep 27, 2017, 11:50:06 AM9/27/17
to
Naming is hard, but this seems like a reasonable refinement without
over-specifying.

--
Darren Hart
VMware Open Source Technology Center

Darren Hart

unread,
Sep 27, 2017, 12:50:06 PM9/27/17
to
On Mon, Sep 25, 2017 at 07:28:57PM +0000, Mario.Li...@dell.com wrote:
>
>
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali....@gmail.com]
> > Sent: Monday, September 25, 2017 12:19 PM
> > To: Limonciello, Mario <Mario_Li...@Dell.com>
> > Cc: dvh...@infradead.org; LKML <linux-...@vger.kernel.org>; platform-driver-
> > x...@vger.kernel.org; quas...@google.com
> > Subject: Re: [PATCH 04/12] platform/x86: dell-smbios: Switch to a WMI-ACPI
> > interface
> >
> > On Thursday 21 September 2017 08:57:09 Mario Limonciello wrote:
> > > The driver currently uses an SMI interface which grants direct access
> > > to physical memory to the platform via a pointer.
> > >
> > > Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
> > > for a buffer of data storage when platform calls are performed.
> > >
> > > This is a safer approach to use in kernel drivers as the platform will
> > > only have access to that OperationRegion.
> >
> > In my opinion direct access is safer then using ACPI wrapper for same
> > functionality.
>
> I'd like to hear how this is safer.

Again, I think the disconnect is around the term "platform". I think above you
can s/platform/SMM/ right?

Darren Hart

unread,
Sep 27, 2017, 12:50:08 PM9/27/17
to
On Mon, Sep 25, 2017 at 07:27:24PM +0000, Mario.Li...@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali....@gmail.com]
> > Sent: Monday, September 25, 2017 12:49 PM
> > To: Limonciello, Mario <Mario_Li...@Dell.com>
> > Cc: dvh...@infradead.org; linux-...@vger.kernel.org; platform-driver-
> > x...@vger.kernel.org; quas...@google.com
> > Subject: Re: [PATCH 00/12] Introduce support for Dell SMBIOS over WMI
> >
> > On Monday 25 September 2017 16:32:52 Mario.Li...@dell.com wrote:
> > > Hi Pali,
> > >
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali....@gmail.com]
> > > > Sent: Monday, September 25, 2017 12:14 PM
> > > > To: Limonciello, Mario <Mario_Li...@Dell.com>
> > > > Cc: dvh...@infradead.org; LKML <linux-...@vger.kernel.org>; platform-
> > driver-
> > > > x...@vger.kernel.org; quas...@google.com
> > > > Subject: Re: [PATCH 00/12] Introduce support for Dell SMBIOS over WMI
> > > >
> > > > On Thursday 21 September 2017 08:57:05 Mario Limonciello wrote:
> > > > > The existing way that the dell-smbios helper module and associated
> > > > > other drivers (dell-laptop, dell-wmi) communicate with the platform
> > > > > really isn't secure. It requires creating a buffer in physical
> > > > > DMA32 memory space and passing that to the platform via SMM.
> > > > >
> > > > > Since the platform got a physical memory pointer, you've just got
> > > > > to trust that the platform has only modified (and accessed) memory
> > > > > within that buffer.
> > > >
> > > > And what is the problem? The whole memory management is done by kernel
> > > > itself, so you already need to trust it.
> > >
> > > There's a lot of ifs, but it's not that crazy of a scenario.
> > >
> > > The problem is that if a malicious payload was delivered to the platform
> > > and exercised a vulnerability in the platform code that payload could
> > > potentially modify memory that it wasn't intended to modify and the OS
> > > would not be aware as operating in SMM.
> > >
> > > >
> > > > > Dell Platform designers recognize this security risk and offer a
> > > > > safer way to communicate with the platform over ACPI. This is
> > > > > in turn exposed via a WMI interface to the OS.
> > > >
> > > > Hm... I cannot understand how some proprietary ACPI bytecode interpreted
> > > > by kernel can be safer as kernel code itself.
> > > >
> > >
> > > Inherently ACPI can only operate on operation regions and not physical memory.
> > > Data passed into ACPI needs to be copied to an operation region for any ACPI
> > > calls to use it.
> >
> > But operation regions access is implemented by ACPI interpreter, which
> > is again kernel code.
>
> So isn't that making my point?
> * Kernel can control operation region accessibility. SMM can't operate outside
> of this region.
> * Direct SMI gives platform access to everything < 4G, kernel can't control this.

I think there may be some confusion with the term "platform code" - it means
different things to different people. I believe Mario is talking about limiting
memory access to the SMI/SMM code through the use of the ACPI op region in place
of the physical memory pointer, which is not visible nor can it be audited.

Darren Hart

unread,
Sep 27, 2017, 1:00:06 PM9/27/17
to
On Mon, Sep 25, 2017 at 05:31:05PM +0000, Mario.Li...@dell.com wrote:
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andy.sh...@gmail.com]
> > Sent: Monday, September 25, 2017 1:04 PM
> > To: Pali Rohár <pali....@gmail.com>
> > Cc: Limonciello, Mario <Mario_Li...@Dell.com>; dvh...@infradead.org;
> > LKML <linux-...@vger.kernel.org>; Platform Driver <platform-driver-
> > x...@vger.kernel.org>; quas...@google.com
> > Subject: Re: [PATCH 06/12] platform/x86: dell-wmi-smbios: Add a sysfs interface
> > for SMBIOS tokens
> >
> > On Mon, Sep 25, 2017 at 7:23 PM, Pali Rohár <pali....@gmail.com> wrote:
> > > On Thursday 21 September 2017 08:57:11 Mario Limonciello wrote:
> > >> Currently userspace tools can access system tokens via the dcdbas
> > >> kernel module and a SMI call that will cause the platform to execute
> > >> SMM code.
> > >>
> > >> With a goal in mind of deprecating the dcdbas kernel module a different
> > >> method for accessing these tokens from userspace needs to be created.
> > >>
> > >> This is intentionally marked to only be readable as root as it can
> > >> contain sensitive information about the platform's configuration.
> > >
> > > Darren, Andy, any comments? I'm not quite sure if such API is suitable
> > > for long term in kernel.
> >
> > I would try to avoid sysfs interfaces for some particular devices.
> > Besides we are creating a character device. Would it be suitable there?
>
> If the character device having 2 different ioctls for different needs is
> acceptable I'm happy to adjust the series to do this instead.

One piece of feedback I had re the char device was to see if we could avoid the
need for the IOCTL altogether, I'd like to have that discussion before we add
another.

>
> >
> > > Basically tokens are list of tuples <id, location, value> with
> > > possibility to active them, right?
> > >
>
> I didn't add a way to activate them through this, it was only for
> reading purpose. Activating them should be possible through the
> SMBIOS calling interface though.
>

These are read-only as I understood it, and only with the right privileges.
Sysfs seemed appropriate for this to me.

Darren Hart

unread,
Sep 27, 2017, 1:10:05 PM9/27/17
to
On Mon, Sep 25, 2017 at 05:46:54PM +0000, Mario.Li...@dell.com wrote:
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andy.sh...@gmail.com]
> > Sent: Monday, September 25, 2017 12:58 PM
> > To: Pali Rohár <pali....@gmail.com>
> > Cc: Limonciello, Mario <Mario_Li...@Dell.com>; dvh...@infradead.org;
> > LKML <linux-...@vger.kernel.org>; Platform Driver <platform-driver-
> > x...@vger.kernel.org>; quas...@google.com
> > Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> > device for userspace
> >
> > On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali....@gmail.com> wrote:
> > > On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
> > >> This userspace character device will be used to perform SMBIOS calls
> > >> from any applications sending a properly formatted 4k calling interface
> > >> buffer.
> > >>
> > >> This character device is intended to deprecate the dcdbas kernel module
> > >> and the interface that it provides to userspace.
> > >>
> > >> It's important for the driver to provide a R/W ioctl to ensure that
> > >> two competing userspace processes don't race to provide or read each
> > >> others data.
> >
> > >> +What: /dev/wmi-dell-wmi-smbios
> > >
> > > What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
> > > related and I think userspace does not have to care if it is via WMI or
> > > direct SMM mode... Important is that it provides character device for
> > > SMBIOS API and not how it is implemented.

I agree with this point, the implementation (WMI under the covers) is
not relevant. That said, this is an example of exposing WMI
functionality to userspace, and I DO NOT want to have a naming
discussion for every single WMI GUID that we choose to expose.

> > >
> > > Anyway, Darren, Andy, do we have some convention for naming platform
> > > character devices?
> >
> > For me, looking to this case, seems better to expose a folder like
> > /dev/smbios/
> > with actual vendor device nodes inside like
> > /dev/smbios/dell
>
> I disagree with this. Dell exposes smbios calling in this kernel interface but
> other vendor drivers may also expose different methods for character devices
> that are not SMBIOS.
>
> I'm envisioning that this is just the first kernel module that will use a WMI
> character device to userspace. That's why I wanted to use a generic namespace.
>
> I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you want
> something more generic.
> As long as it's discovereable from uevent that's fine to me.
>
> >
> > Darren?

I would like to see an automated and definiitive way to export WMI
GUIDs. Something we can look at, compare to a set of rules, and say
yay/nay. From that perspective, I like Mario's general proposal. It is
not clear to me if the $driver- prefix adds any value though - would we
ever have need of DRIVER_X-GUID_A and DRIVER_Y-GUID_A ??? I'm thinking
not.

/dev/wmi/$GUID

Darren Hart

unread,
Sep 27, 2017, 1:20:06 PM9/27/17
to
On Tue, Sep 26, 2017 at 07:17:17PM +0000, Mario.Li...@dell.com wrote:
> >
> > Please, include Andy Lutomirski to Cc list, I'm pretty sure he is
> > interested in changes like these to WMI.
>
> Added. Andy you weren't CC'ed on the original or v2 submission, but if this
> requires a v3 I'll include you.

Andy should be Cc'd on all patches to platform drivers x86 as we
co-maintain the subsystem together.

The get_maintainer.pl script is helpful when determining the Cc list.

Darren Hart

unread,
Sep 27, 2017, 1:30:05 PM9/27/17
to
On Tue, Sep 26, 2017 at 01:50:01PM -0500, Mario Limonciello wrote:
> The descriptor GUID is not used to indicate that WMI notifications
> in the dell-wmi driver work properly. As such a modalias should
> not be present that causes this driver to load on systems with this
> GUID.
>
> Reviewed-by: Pali Pohar <pali....@gmail.com>
> Signed-off-by: Mario Limonciello <mario.li...@dell.com>

I can do this manually, but for future reference:

Nit, in general, the author's sign off should be first, followed by
reviewers and testers. The maintainer will add their sign off at the end
- this keeps things clear regarding the development and delivery path.
In general, assume chronological order and you'll have the right idea.

Informational tags, like Reported, Suggested, Tested, and Fixes can come
before the Author sign off, which is consistent with the chronological
order.

Mario.Li...@dell.com

unread,
Sep 27, 2017, 1:40:06 PM9/27/17
to
> -----Original Message-----
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Wednesday, September 27, 2017 1:12 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: andy.sh...@gmail.com; lu...@kernel.org; linux-...@vger.kernel.org;
> platform-...@vger.kernel.org; quas...@google.com;
> pali....@gmail.com
> Subject: Re: [PATCH v2 00/14] Introduce support for Dell SMBIOS over WMI
>
> On Tue, Sep 26, 2017 at 07:17:17PM +0000, Mario.Li...@dell.com wrote:
> > >
> > > Please, include Andy Lutomirski to Cc list, I'm pretty sure he is
> > > interested in changes like these to WMI.
> >
> > Added. Andy you weren't CC'ed on the original or v2 submission, but if this
> > requires a v3 I'll include you.
>
> Andy should be Cc'd on all patches to platform drivers x86 as we
> co-maintain the subsystem together.

Andy S. was CC'ed on all of these, Andy S was recommending that I add Andy L.

Mario.Li...@dell.com

unread,
Sep 27, 2017, 1:40:06 PM9/27/17
to
> -----Original Message-----
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Wednesday, September 27, 2017 1:22 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: Andy Shevchenko <andy.sh...@gmail.com>; LKML <linux-
> ker...@vger.kernel.org>; platform-...@vger.kernel.org;
> quas...@google.com; pali....@gmail.com
> Subject: Re: [PATCH v2 03/14] platform/x86: dell-wmi: Don't match on descriptor
> GUID modalias
>
Operationally is there is a flag I'm missing in git-format-patch that does
this? git format-patch -s puts my sign off at the very bottom, so this is a
manual action to re-order if I'm adding in Reviewed-by: in the patch
description.

Darren Hart

unread,
Sep 27, 2017, 1:40:06 PM9/27/17
to
Ah, of course. Sorry, too many Andys :-)

Darren Hart

unread,
Sep 27, 2017, 1:50:06 PM9/27/17
to
On Tue, Sep 26, 2017 at 01:50:00PM -0500, Mario Limonciello wrote:
> These aren't needed to compile the module.
>
> Signed-off-by: Mario Limonciello <mario.li...@dell.com>

Hrm...

CONFIG_DELL_SMBIOS=m

$ make drivers/platform/x86/dell-smbios.ko

CC [M] drivers/platform/x86/dell-smbios.o
drivers/platform/x86/dell-smbios.c: In function ‘parse_da_table’:
drivers/platform/x86/dell-smbios.c:142:18: error: implicit declaration of function ‘krealloc’ [-Werror=implicit-function-declaration]
new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
^~~~~~~~
drivers/platform/x86/dell-smbios.c:142:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
^
drivers/platform/x86/dell-smbios.c: In function ‘dell_smbios_init’:
drivers/platform/x86/dell-smbios.c:193:2: error: implicit declaration of function ‘kfree’ [-Werror=implicit-function-declaration]
kfree(da_tokens);
^~~~~

> ---
> drivers/platform/x86/dell-smbios.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index 0a5723468bff..6654ad44a04c 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -17,10 +17,7 @@
> #include <linux/module.h>
> #include <linux/dmi.h>
> #include <linux/err.h>
> -#include <linux/gfp.h>
> #include <linux/mutex.h>
> -#include <linux/slab.h>

Appears you do require slab.h...

> -#include <linux/io.h>
> #include "../../firmware/dcdbas.h"
> #include "dell-smbios.h"
>
> --
> 2.14.1



>
>

Darren Hart

unread,
Sep 27, 2017, 1:50:06 PM9/27/17
to
Yes, this is a manual step. Git will add your sign off for you, but it doesn't
do it contextually, always at the end. Works for the first writing, after that
you have to manage the tags manually. (or at least that is what I do)

Mario.Li...@dell.com

unread,
Sep 27, 2017, 1:50:06 PM9/27/17
to
> -----Original Message-----
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Wednesday, September 27, 2017 1:40 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: Andy Shevchenko <andy.sh...@gmail.com>; LKML <linux-
> ker...@vger.kernel.org>; platform-...@vger.kernel.org;
> quas...@google.com; pali....@gmail.com
Thanks, I've obviously been testing this and didn't notice it.
Not sure why that wasn't exploding on my end.
Anyway I'll fix for v3.

Darren Hart

unread,
Sep 27, 2017, 2:00:06 PM9/27/17
to
On Wed, Sep 27, 2017 at 10:40:19AM -0700, Darren Hart wrote:
> On Tue, Sep 26, 2017 at 01:50:00PM -0500, Mario Limonciello wrote:
> > These aren't needed to compile the module.
> >
> > Signed-off-by: Mario Limonciello <mario.li...@dell.com>
>
> Hrm...
>
> CONFIG_DELL_SMBIOS=m
>
> $ make drivers/platform/x86/dell-smbios.ko
>
> CC [M] drivers/platform/x86/dell-smbios.o
> drivers/platform/x86/dell-smbios.c: In function ‘parse_da_table’:
> drivers/platform/x86/dell-smbios.c:142:18: error: implicit declaration of function ‘krealloc’ [-Werror=implicit-function-declaration]
> new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> ^~~~~~~~
> drivers/platform/x86/dell-smbios.c:142:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> ^
> drivers/platform/x86/dell-smbios.c: In function ‘dell_smbios_init’:
> drivers/platform/x86/dell-smbios.c:193:2: error: implicit declaration of function ‘kfree’ [-Werror=implicit-function-declaration]
> kfree(da_tokens);
> ^~~~~
>
> > ---
> > drivers/platform/x86/dell-smbios.c | 3 ---
...
> > -#include <linux/slab.h>
>
> Appears you do require slab.h...
>

I've dropped this one line from this patch, and build is working again. Patches
1-7 look ready to go to me. I'm pushing these to my review branch from some
additional automated testing.

Let me know if there are any objections for 1-7. If we can get these queued now,
we can simplify the review effort for the major functional changes in 8-14.

Mario.Li...@dell.com

unread,
Sep 27, 2017, 2:20:05 PM9/27/17
to
> -----Original Message-----
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Wednesday, September 27, 2017 1:00 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
Yes, I agree let's make sure it's stable and automatic for all drivers that will use it.

>
> > > >
> > > > Anyway, Darren, Andy, do we have some convention for naming platform
> > > > character devices?
> > >
> > > For me, looking to this case, seems better to expose a folder like
> > > /dev/smbios/
> > > with actual vendor device nodes inside like
> > > /dev/smbios/dell
> >
> > I disagree with this. Dell exposes smbios calling in this kernel interface but
> > other vendor drivers may also expose different methods for character devices
> > that are not SMBIOS.
> >
> > I'm envisioning that this is just the first kernel module that will use a WMI
> > character device to userspace. That's why I wanted to use a generic namespace.
> >
> > I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you want
> > something more generic.
> > As long as it's discovereable from uevent that's fine to me.
> >
> > >
> > > Darren?
>
> I would like to see an automated and definiitive way to export WMI
> GUIDs. Something we can look at, compare to a set of rules, and say
> yay/nay. From that perspective, I like Mario's general proposal. It is
> not clear to me if the $driver- prefix adds any value though - would we
> ever have need of DRIVER_X-GUID_A and DRIVER_Y-GUID_A ??? I'm thinking
> not.
>
> /dev/wmi/$GUID
>

I don't think you should allow two drivers to use the same GUID, but what if
a driver needs to use multiple GUIDs?

This obviously isn't a problem yet.
I envision that as we get MOF interpretation as part of the bus we "may" get into
that situation with more complicated MOF design. The vendor driver could
register with the bus for each GUID and request a character device for each but
that's meaning userspace has to worry about knowing which GUIDs do what.
I think it's potentially asking for a complicated interface to userspace for the future.

Here's a hypothetical scenario based on some more advanced MOF design I know of:

Separate GUID provided for each:
* All integer settings (data type)
* All string settings (data type)
* Notifications (event type)
* enumeration types (data type)
* Applying a Setting (method type)
* Managing users (method type)

How would a new driver for that look? I would think you would want one driver to register
and encapsulate at least all of those GUIDs.

I would say all the data should be exported into sysfs, but that you really only want
one character device that is used for both methods.

The interface to userspace would then have two IOCTL's that the driver would internally
apply to the correct GUID. Example:
"/dev/wmi/$driver" supports 2 different IOCTL's.
Internally the driver would then ferry the request through the WMI bus to the correct
GUID based on the ioctl call number.

If you went with the pass the GUID information up and create character device for each
GUID, userspace will have to know:
"I use /dev/wmi/$GUID_X" for applying the setting IOCTL on Acme Inc machine.
And
"I use /dev/wmi/$GUID_Y" for managing users IOCTL on Acme Inc machine.

And what if the notifications are valuable to userspace? Would you want to create another
character device for those? I think you would just want to keep them in the same device
and add a poll/select file operation.

Mario.Li...@dell.com

unread,
Sep 27, 2017, 2:30:05 PM9/27/17
to
> -----Original Message-----
> From: Darren Hart [mailto:dvh...@infradead.org]
From my OEM hat we don't really refer to SMM as an entity, but "Yeah" if that
makes it clearer I'd agree.

Mario.Li...@dell.com

unread,
Sep 27, 2017, 2:30:05 PM9/27/17
to
> -----Original Message-----
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Wednesday, September 27, 2017 12:51 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
My original design was sysfs files for everything but it was raised by several folks
that you run into the potential of two userspace processes stomping on each
other's data when they run the ACPI call. That's why I need to have a mutex to
protect and make sure that userspace calls get the right results.

>
> >
> > >
> > > > Basically tokens are list of tuples <id, location, value> with
> > > > possibility to active them, right?
> > > >
> >
> > I didn't add a way to activate them through this, it was only for
> > reading purpose. Activating them should be possible through the
> > SMBIOS calling interface though.
> >
>
> These are read-only as I understood it, and only with the right privileges.
> Sysfs seemed appropriate for this to me.

Andy S was against having this data as another sysfs file. From a userspace
perspective I think it's simpler to just parse a sysfs file with read only static
data as root. With the current ioctl based solution it requires userspace to run
an ioctl to determine how many tokens exist, then allocate a chunk of memory
big enough to hold all the token data and then run another ioctl to get all the tokens.

Andy S, given this change between v1 and v2 what do you feel is better?

Andy Shevchenko

unread,
Sep 27, 2017, 2:40:05 PM9/27/17
to
I have no strong opinion on this. That's why I recommended to listen to Andy L.

Darren Hart

unread,
Sep 27, 2017, 3:00:06 PM9/27/17
to
+Andy Lutomirski

Andy L, any preference on your part regarding exporting these tokens via sysfs
or through an additional IOCTL in the chardev?

Darren Hart

unread,
Sep 27, 2017, 3:00:06 PM9/27/17
to
Thanks for the concrete example, this helps a lot.

One guiding principle of this series and something Pali correctly pointed out,
is that for Linux, we're keeping WMI as the implementation and exposing features
to userspace through char devs and IOCTLS. You points above are consistent with
that theme, and the GUIDs are not an appropriate namespace.

The driver namespace does make more sense as you suggested above:

/dev/wmi/$driver

This still has wmi in the name, but given that is the bus name, I think it's a
reasonable way organize the files - and is still a well-defined naming scheme.

Andy Lutomirski

unread,
Sep 27, 2017, 3:50:05 PM9/27/17
to
Not really. If this is indeed static data that is potentially useful
for scripts and such, than sysfs is kind of nice.

Andy Lutomirski

unread,
Sep 27, 2017, 3:50:05 PM9/27/17
to
On 09/21/2017 06:57 AM, Mario Limonciello wrote:
> The driver currently uses an SMI interface which grants direct access
> to physical memory to the platform via a pointer.
>
> Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
> for a buffer of data storage when platform calls are performed.
>
> This is a safer approach to use in kernel drivers as the platform will
> only have access to that OperationRegion.
>
> As a result, this change removes the dependency on this driver on the
> dcdbas kernel module.
>

> + status = wmi_evaluate_method(DELL_WMI_SMBIOS_GUID,
> + 0, 1, &input, &output);

It might be nice to add a wmidev_method_evaluate(struct wmi_device
*wdev, ...) function to better aligh with the new world order. The only
reason I didn't do it is that I didn't convert any drivers that wanted it.

Mario.Li...@dell.com

unread,
Sep 27, 2017, 4:00:06 PM9/27/17
to
> -----Original Message-----
> From: Andy Lutomirski [mailto:lu...@kernel.org]
> Sent: Wednesday, September 27, 2017 3:49 PM
> To: Darren Hart <dvh...@infradead.org>; Andy Shevchenko
> <andy.sh...@gmail.com>; Andy Lutomirski <lu...@amacapital.net>
> Cc: Limonciello, Mario <Mario_Li...@Dell.com>; Pali Rohár
> <pali....@gmail.com>; linux-...@vger.kernel.org; Platform Driver <platform-
> drive...@vger.kernel.org>; quas...@google.com
> Subject: Re: [PATCH 06/12] platform/x86: dell-wmi-smbios: Add a sysfs interface
> for SMBIOS tokens
>
OK thanks. I'll switch back to sysfs for v3.

Mario.Li...@dell.com

unread,
Sep 27, 2017, 5:20:06 PM9/27/17
to


> -----Original Message-----
> From: Andy Lutomirski [mailto:lu...@kernel.org]
> Sent: Wednesday, September 27, 2017 3:48 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>; dvh...@infradead.org
> Cc: LKML <linux-...@vger.kernel.org>; platform-...@vger.kernel.org;
> quas...@google.com; pali....@gmail.com
> Subject: Re: [PATCH 04/12] platform/x86: dell-smbios: Switch to a WMI-ACPI
> interface
>
Great idea. I'll apply that for my next patch revision.
I'm not about to convert all the existing drivers, but once all existing drivers
conceptualize using the bus, storing a wmi_device pointer internally,and
using this the old wmi_evaluate_method can be dropped too.



Mario.Li...@dell.com

unread,
Sep 27, 2017, 5:20:07 PM9/27/17
to
Thanks Darren. I'll adjust the patch.
Would you like me to document this chosen policy somewhere in Documentation?
If so, where?
Or to put more about our conversation in the commit message?

Darren Hart

unread,
Sep 27, 2017, 6:00:05 PM9/27/17
to
On Wed, Sep 27, 2017 at 09:12:34PM +0000, Mario.Li...@dell.com wrote:
...
> > Thanks for the concrete example, this helps a lot.
> >
> > One guiding principle of this series and something Pali correctly pointed out,
> > is that for Linux, we're keeping WMI as the implementation and exposing features
> > to userspace through char devs and IOCTLS. You points above are consistent with
> > that theme, and the GUIDs are not an appropriate namespace.
> >
> > The driver namespace does make more sense as you suggested above:
> >
> > /dev/wmi/$driver
> >
> > This still has wmi in the name, but given that is the bus name, I think it's a
> > reasonable way organize the files - and is still a well-defined naming scheme.
> >
>
> Thanks Darren. I'll adjust the patch.
> Would you like me to document this chosen policy somewhere in Documentation?
> If so, where?
> Or to put more about our conversation in the commit message?
>

Some justification in the commit message for now would be best. Thanks!

Darren Hart

unread,
Sep 27, 2017, 6:20:13 PM9/27/17
to
On Tue, Sep 26, 2017 at 01:50:06PM -0500, Mario Limonciello wrote:
> The driver currently uses an SMI interface which grants direct access
> to physical memory to the platform via a pointer.
>
> Now add a WMI-ACPI interface that is detected by WMI probe and preferred
> over the SMI interface.
>
> Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
> for a buffer of data storage when platform calls are performed.
>
> This is a safer approach to use in kernel drivers as the platform will
> only have access to that OperationRegion.

As we discussed, in v3 please update the "platform" language here to clarify
when read from the Linux kernel developer perspective (one who is working on
code in the platform drivers subsystem ;-)

>
> As a result, this change removes the dependency on this driver on the
> dcdbas kernel module. It's now an optional compilation option.

Some update tot he Kconfig help informing the user of how to decide if they
should go and enable DCDBAS or not would be appropriate.

> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
...
> -#include "../../firmware/dcdbas.h"
> +#include <linux/wmi.h>
> #include "dell-smbios.h"
>
> +#ifdef CONFIG_DCDBAS
> +#include "../../firmware/dcdbas.h"
> +#endif

Perhaps this is the conditional include causing the build breakage?

> +
> +#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
> +
> struct calling_interface_structure {
> struct dmi_header header;
> u16 cmdIOAddress;
> @@ -30,12 +37,14 @@ struct calling_interface_structure {
> struct calling_interface_token tokens[];
> } __packed;
>
> -static struct calling_interface_buffer *buffer;
> +static struct calling_interface_buffer *smi_buffer;
> +static struct wmi_calling_interface_buffer *wmi_buffer;
> static DEFINE_MUTEX(buffer_mutex);
>
> static int da_command_address;
> static int da_command_code;
> static int da_num_tokens;
> +static int has_wmi;
> static struct calling_interface_token *da_tokens;
>
> int dell_smbios_error(int value)
> @@ -57,13 +66,20 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
> {
> mutex_lock(&buffer_mutex);
> dell_smbios_clear_buffer();
> - return buffer;
> + if (has_wmi)
> + return &wmi_buffer->smi;
> + return smi_buffer;
> }
> EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
>
> void dell_smbios_clear_buffer(void)
> {
> - memset(buffer, 0, sizeof(struct calling_interface_buffer));
> + if (has_wmi)
> + memset(wmi_buffer, 0,
> + sizeof(struct wmi_calling_interface_buffer));
> + else
> + memset(smi_buffer, 0,
> + sizeof(struct calling_interface_buffer));
> }
> EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);

Would it be possible to dynamically setup "buffer" and "buflen" during
init or something, and avoid all this if/else dance at runtime?

>
> @@ -73,20 +89,60 @@ void dell_smbios_release_buffer(void)
> }
> EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);
>
> -void dell_smbios_send_request(int class, int select)
> +int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
> {
> - struct smi_cmd command;
> + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> + struct acpi_buffer input;
> + union acpi_object *obj;
> + acpi_status status;
> +
> + input.length = sizeof(struct wmi_calling_interface_buffer);
> + input.pointer = buf;
> +
> + status = wmi_evaluate_method(DELL_WMI_SMBIOS_GUID,
> + 0, 1, &input, &output);
> + if (ACPI_FAILURE(status)) {
> + pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
> + buf->smi.class, buf->smi.select,
> + buf->smi.input[0], buf->smi.input[1],
> + buf->smi.input[2], buf->smi.input[3]);
> + return -EIO;
> + }
> + obj = (union acpi_object *)output.pointer;
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + pr_err("invalid type : %d\n", obj->type);
> + return -EIO;
> + }
> + memcpy(buf, obj->buffer.pointer, input.length);
>
> - command.magic = SMI_CMD_MAGIC;
> - command.command_address = da_command_address;
> - command.command_code = da_command_code;
> - command.ebx = virt_to_phys(buffer);
> - command.ecx = 0x42534931;
> + return 0;
> +}
>
> - buffer->class = class;
> - buffer->select = select;
> +void dell_smbios_send_request(int class, int select)
> +{
> + if (has_wmi) {
> + wmi_buffer->smi.class = class;
> + wmi_buffer->smi.select = select;
> + run_wmi_smbios_call(wmi_buffer);
> + }
>
> - dcdbas_smi_request(&command);
> +#ifdef CONFIG_DCDBAS

Rather than ifdef blocks of code, the preferred method is to modify the
declaration of things so the tests here can do the right thing. For
example, if CONFIG_DCDBAS is not set, can that be made equivalent to:

else if (smi_buffer) {

At this point in the code?

See coding-style.rst Section 20) Conditional Compilation
for more specific guidance. Apply throughout.

Darren Hart

unread,
Sep 27, 2017, 6:40:05 PM9/27/17
to
On Tue, Sep 26, 2017 at 08:18:54PM +0000, Mario.Li...@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali....@gmail.com]
> > Sent: Tuesday, September 26, 2017 4:06 PM
> > To: Limonciello, Mario <Mario_Li...@Dell.com>
> > Cc: dvh...@infradead.org; Andy Shevchenko <andy.sh...@gmail.com>;
> > LKML <linux-...@vger.kernel.org>; platform-...@vger.kernel.org;
> > quas...@google.com
> > Subject: Re: [PATCH v2 09/14] platform/x86: dell-smbios: rename to dell-wmi-
> > smbios
> >
> > On Tuesday 26 September 2017 20:50:07 Mario Limonciello wrote:
> > > This follows the style of the rest of the platform x86 WMI drivers.
> > >
> > > Renaming the driver requires adjusting the other drivers using
> > > dell-smbios to pick up the newly named includes.
> > >
> > > While renaming, I noticed that this driver was missing from
> > > MAINTAINERS. Add it to that and myself to the list of people
> > > maintaing it.
> > >
> > > Signed-off-by: Mario Limonciello <mario.li...@dell.com>
> >
> > Now when driver supports both old and new WMI call interface, I think
> > that renaming is not needed...
> >
>
> So all other drivers that use WMI have "WMI" in the name, this would be
> breaking previously set precedence. Daren, Andy, what would you like
> me to do?

I'm leaning toward Pali's point of view here. In fact, I think the driver name
should reflect WHAT it does as opposed to HOW it does it. vendor-hotkeys is a
much better name than vendor-wmi. We have a lot of drivers that are not nicely
granular and use more generic terms like -acpi or -laptop, but those shouldn't
be the first choice in my opinion. Now, is SMBIOS any better than WMI? :-) Not
really, but it is at least as good as, and less change makes it the winner IMO.

Mario Limonciello

unread,
Sep 28, 2017, 12:10:04 AM9/28/17
to
The existing way that the dell-smbios helper module and associated
other drivers (dell-laptop, dell-wmi) communicate with the platform
really isn't secure. It requires creating a buffer in physical
DMA32 memory space and passing that to the platform via SMM.

Since the platform got a physical memory pointer, you've just got
to trust that the platform has only modified (and accessed) memory
within that buffer.

Dell Platform designers recognize this security risk and offer a
safer way to communicate with the platform over ACPI. This is
in turn exposed via a WMI interface to the OS.

When communicating over WMI-ACPI the communication doesn't occur
with physical memory pointers. When the ASL is invoked, the fixed
length ACPI buffer is copied to a small operating region. The ASL
will invoke the SMI, and SMM will only have access to this operating
region. When the ASL returns the buffer is copied back for the OS
to process.

This method of communication should also deprecate the usage of the
dcdbas kernel module and software dependent upon it's interface.
Instead offer a character device interface for communicating with this
ASL method to allow userspace to use instead.

To faciliate that this patch series introduces a generic way for WMI
drivers to be able to create discoverable character devices through
the WMI bus when desired.
Requiring WMI drivers to explicitly ask for this functionality will
act as an effective vendor whitelist to character device creation.

changes between v2 and v3:
* Drop patches 1-7, they're in Darren's review tree now
* Add missing newline on new Documentation
* Add Reviewed by from Edward O'Callaghan
* Adjust path of character device from /dev/wmi-$driver to
/dev/wmi/$driver
* Store wmi_device pointer rather than using a boolean has_wmi
to indicate driver is running in WMI mode
* Don't guard free_page from freeing NULL (this is OK)
* New patch: add wmidev_evaluate_method to wmi bus as recommended
by Andy L
* Adjust ACPI-WMI interface for this patch change ^
* Add back in sysfs token patch, drop 2nd and 3rd ioctls per discussion
on mailing list.
* On sysfs interface: adjust the delimiter to be tabs
* Drop the rename dell-smbios -> dell-wmi-smbios patch
* Remove/move some unnecessary tests for CONFIG_DCDBAS
* Reword s/platform/SMM/ in the WMI-ACPI patch.
* Update Kconfig to recommend using CONFIG_DCDBAS on old machines.
* Allocate buffer to the same pointer regardless of the struct
assigned to it. Keep track of the buffer size for cleaning up.
* Explain policy around character device creation in commit message
changes between v1 and v2:
* Introduce another patch to sort the includes in wmi.c
* Introduce another patch to cleanup dell_wmi_check_descriptor_buffer
checks.
* Add a commit message to the pr_fmt commit
* Introduce includes to wmi.c in proper location
* Add Reviewed-by to relevant patches from Pali
* Make the WMI introduction patch fallback to legacy SMI
if compiled with CONFIG_DCDBAS
* Separate format of WMI and SMI buffers. WMI buffer supports more
arguments and data.
* Adjust the rename patch for changes to fallback
* Drop sysfs token creation patch
* Adjust WMI descriptor check patch for changes to fallback
* introduce another patch to remove needless includes in dell-smbios.c
* Add token ioctl interface to character device.
- Can query number of tokens
- Can query values in all tokens
* Expose format of all buffers and IOCTL commands to uapi header
* Drop the read interface from character device. It doesn't make
sense with multiple different ioctl methods.
* Default WMI interface to 32k (This would normally be queried via
MOF, but that's not possible yet)
* Create separate buffers for WMI and SMI. If WMI is available,
free the SMI buffer.
* Reorder patches so all fixups come first in the series.

Mario Limonciello (8):
platform/x86: wmi: Add new method wmidev_evaluate_method
platform/x86: dell-smbios: Introduce a WMI-ACPI interface
platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check
platform/x86: wmi: create character devices when requested by drivers
platform/x86: dell-wmi-smbios: introduce character device for
userspace
platform/x86: dell-wmi-smbios: Add a sysfs interface for SMBIOS tokens
platform/x86: Kconfig: Change the default settings for dell-wmi-smbios
platform/x86: dell-wmi-smbios: clean up wmi descriptor check

Documentation/ABI/testing/dell-wmi-smbios | 11 +
.../ABI/testing/sysfs-platform-dell-wmi-smbios | 16 ++
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 15 +-
drivers/platform/x86/dell-smbios.c | 315 +++++++++++++++++++--
drivers/platform/x86/dell-smbios.h | 15 +-
drivers/platform/x86/dell-wmi.c | 75 +----
drivers/platform/x86/wmi.c | 126 ++++++++-
include/linux/wmi.h | 7 +
include/uapi/linux/dell-wmi-smbios.h | 30 ++
10 files changed, 498 insertions(+), 118 deletions(-)
create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
create mode 100644 include/uapi/linux/dell-wmi-smbios.h

--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:04 AM9/28/17
to
The Dell WMI descriptor check is used as an indication that WMI
calls are safe to run both when used with the notification
ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.

As some code in dell-wmi-smbios is already a prerequisite for
dell-wmi, move the code for performing the descriptor check into
dell-wmi-smbios and let both drivers use it from there.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/dell-smbios.c | 81 ++++++++++++++++++++++++++++++++++++++
drivers/platform/x86/dell-smbios.h | 3 ++
drivers/platform/x86/dell-wmi.c | 75 +----------------------------------
3 files changed, 85 insertions(+), 74 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 4472817ee045..5d793b012e5e 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -31,6 +31,7 @@
#endif

#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"

struct calling_interface_structure {
struct dmi_header header;
@@ -226,8 +227,87 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

+/*
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ * Name Offset Length Value
+ * Vendor Signature 0 4 "DELL"
+ * Object Signature 4 4 " WMI"
+ * WMI Interface Version 8 4 <version>
+ * WMI buffer length 12 4 4096
+ */
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version)
+{
+ union acpi_object *obj = NULL;
+ struct wmi_device *desc_dev;
+ u32 *desc_buffer;
+ int ret;
+
+ desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
+ if (!desc_dev) {
+ dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
+ return -ENODEV;
+ }
+
+ obj = wmidev_block_query(desc_dev, 0);
+ if (!obj) {
+ dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
+ ret = -EIO;
+ goto out;
+ }
+
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (obj->buffer.length != 128) {
+ dev_err(&wdev->dev,
+ "Dell descriptor buffer has invalid length (%d)\n",
+ obj->buffer.length);
+ if (obj->buffer.length < 16) {
+ ret = -EINVAL;
+ goto out;
+ }
+ }
+ desc_buffer = (u32 *)obj->buffer.pointer;
+
+ if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
+ 8, desc_buffer);
+
+ if (desc_buffer[2] != 0 && desc_buffer[2] != 1)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
+ desc_buffer[2]);
+
+ if (desc_buffer[3] != 4096)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
+ desc_buffer[3]);
+
+ *version = desc_buffer[2];
+ ret = 0;
+
+ dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
+ *version);
+
+out:
+ kfree(obj);
+ put_device(&desc_dev->dev);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_check_descriptor_buffer);
+
+
static int dell_smbios_wmi_probe(struct wmi_device *wdev)
{
+ int ret;
+ u32 interface_version;
+
+ ret = dell_wmi_check_descriptor_buffer(wdev, &interface_version);
+ if (ret)
+ return -ENODEV;
+
/* no longer need the SMI page */
free_page((unsigned long)buffer);

@@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
if (!buffer)
return -ENOMEM;
bufferlen = sizeof(struct wmi_calling_interface_buffer);
+
wmi_dev = wdev;
return 0;
}
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 2f6fce81ee69..be9ec1ccf8bf 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -17,6 +17,8 @@
#ifndef _DELL_SMBIOS_H_
#define _DELL_SMBIOS_H_

+#include <linux/wmi.h>
+
struct notifier_block;

/* If called through fallback SMI rather than WMI this structure will be
@@ -62,5 +64,6 @@ enum dell_laptop_notifier_actions {
int dell_laptop_register_notifier(struct notifier_block *nb);
int dell_laptop_unregister_notifier(struct notifier_block *nb);
void dell_laptop_call_notifier(unsigned long action, void *data);
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version);

#endif
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1fbef560ca67..b3bb8cdd27bf 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -46,7 +46,6 @@ MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver");
MODULE_LICENSE("GPL");

#define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
-#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"

static bool wmi_requires_smbios_request;

@@ -617,78 +616,6 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
input_unregister_device(priv->input_dev);
}

-/*
- * Descriptor buffer is 128 byte long and contains:
- *
- * Name Offset Length Value
- * Vendor Signature 0 4 "DELL"
- * Object Signature 4 4 " WMI"
- * WMI Interface Version 8 4 <version>
- * WMI buffer length 12 4 4096
- */
-static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
-{
- struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
- union acpi_object *obj = NULL;
- struct wmi_device *desc_dev;
- u32 *buffer;
- int ret;
-
- desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
- if (!desc_dev) {
- dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
- return -ENODEV;
- }
-
- obj = wmidev_block_query(desc_dev, 0);
- if (!obj) {
- dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
- ret = -EIO;
- goto out;
- }
-
- if (obj->type != ACPI_TYPE_BUFFER) {
- dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
- ret = -EINVAL;
- goto out;
- }
-
- if (obj->buffer.length != 128) {
- dev_err(&wdev->dev,
- "Dell descriptor buffer has invalid length (%d)\n",
- obj->buffer.length);
- if (obj->buffer.length < 16) {
- ret = -EINVAL;
- goto out;
- }
- }
-
- buffer = (u32 *)obj->buffer.pointer;
-
- if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
- 8, buffer);
-
- if (buffer[2] != 0 && buffer[2] != 1)
- dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
- buffer[2]);
-
- if (buffer[3] != 4096)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
- buffer[3]);
-
- priv->interface_version = buffer[2];
- ret = 0;
-
- dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
- priv->interface_version);
-
-out:
- kfree(obj);
- put_device(&desc_dev->dev);
- return ret;
-}
-
/*
* According to Dell SMBIOS documentation:
*
@@ -732,7 +659,7 @@ static int dell_wmi_probe(struct wmi_device *wdev)
return -ENOMEM;
dev_set_drvdata(&wdev->dev, priv);

- err = dell_wmi_check_descriptor_buffer(wdev);
+ err = dell_wmi_check_descriptor_buffer(wdev, &priv->interface_version);
if (err)
return err;

--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:04 AM9/28/17
to
Currently userspace tools can access system tokens via the dcdbas
kernel module and a SMI call that will cause the platform to execute
SMM code.

With a goal in mind of deprecating the dcdbas kernel module a different
method for accessing these tokens from userspace needs to be created.

This is intentionally marked to only be readable as root as it can
contain sensitive information about the platform's configuration.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
.../ABI/testing/sysfs-platform-dell-wmi-smbios | 16 ++++++++++
drivers/platform/x86/dell-smbios.c | 36 ++++++++++++++++++++++
2 files changed, 52 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios

diff --git a/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios b/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
new file mode 100644
index 000000000000..4cbff5ffe380
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
@@ -0,0 +1,16 @@
+What: /sys/devices/platform/<platform>/tokens
+Date: October 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.li...@dell.com>
+Description:
+ A read-only description of Dell platform tokens
+ available on the machine.
+
+ The tokens will be displayed in the following
+ machine readable format with each token on a
+ new line:
+
+ ID Location value
+
+ For example token:
+ 5 5 3
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 4174afbade13..ac176953e46e 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -229,6 +229,34 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

+static ssize_t tokens_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ size_t off = 0;
+ int i;
+
+ for (i = 0; i < da_num_tokens; i++) {
+ if (off > PAGE_SIZE)
+ break;
+ off += scnprintf(buf+off, PAGE_SIZE-off, "%04x\t%04x\t%04x\n",
+ da_tokens[i].tokenID, da_tokens[i].location,
+ da_tokens[i].value);
+ }
+
+ return off;
+}
+
+DEVICE_ATTR(tokens, 0400, tokens_show, NULL);
+
+static struct attribute *smbios_attrs[] = {
+ &dev_attr_tokens.attr,
+ NULL
+};
+
+static const struct attribute_group smbios_attribute_group = {
+ .attrs = smbios_attrs,
+};
+
static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
{
return nonseekable_open(inode, file);
@@ -367,10 +395,16 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
ret = -ENOMEM;
goto fail_devfs_buffer;
}
+ ret = sysfs_create_group(&wdev->dev.kobj, &smbios_attribute_group);
+ if (ret)
+ goto fail_create_group;

wmi_dev = wdev;
+ kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
return 0;

+fail_create_group:
+ free_pages((unsigned long)devfs_buffer, 3);
fail_devfs_buffer:
free_pages((unsigned long)internal_buffer, 3);
return ret;
@@ -381,6 +415,8 @@ static int dell_smbios_wmi_remove(struct wmi_device *wdev)
wmi_dev = NULL;
free_pages((unsigned long)internal_buffer, 3);
free_pages((unsigned long)devfs_buffer, 3);
+ sysfs_remove_group(&wdev->dev.kobj, &smbios_attribute_group);
+ kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
return 0;
}

--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:04 AM9/28/17
to
For WMI operations that are only Set or Query read or write sysfs
attributes created by WMI vendor drivers make sense.

For other WMI operations that are run on Method, there needs to be a
way to guarantee to userspace that the results from the method call
belong to the data request to the method call. Sysfs attributes don't
work well in this scenario because two userspace processes may be
competing at reading/writing an attribute and step on each other's
data.

When a WMI vendor driver declares a set of functions in a
file_operations object the WMI bus driver will create a character
device that maps to those file operations.

That character device will correspond to this path:
/dev/wmi/$driver

This policy is selected as one driver may map and use multiple
GUIDs and it would be better to only expose a single character
device.

The WMI vendor drivers will be responsible for managing access to
this character device and proper locking on it.

When a WMI vendor driver is unloaded the WMI bus driver will clean
up the character device.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
include/linux/wmi.h | 1 +
2 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4d73a87c2ddf..17399df87948 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -34,7 +34,9 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/acpi.h>
+#include <linux/cdev.h>
#include <linux/device.h>
+#include <linux/idr.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
@@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho");
MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
MODULE_LICENSE("GPL");

+#define WMI_MAX_DEVS MINORMASK
+static DEFINE_IDR(wmi_idr);
+static DEFINE_MUTEX(wmi_minor_lock);
static LIST_HEAD(wmi_block_list);

struct guid_block {
@@ -69,6 +74,8 @@ struct wmi_block {
struct wmi_device dev;
struct list_head list;
struct guid_block gblock;
+ struct cdev *cdev;
+ int minor;
struct acpi_device *acpi_device;
wmi_notify_handler handler;
void *handler_data;
@@ -86,6 +93,7 @@ struct wmi_block {
#define ACPI_WMI_STRING 0x4 /* GUID takes & returns a string */
#define ACPI_WMI_EVENT 0x8 /* GUID is an event */

+static dev_t wmi_devt;
static bool debug_event;
module_param(debug_event, bool, 0444);
MODULE_PARM_DESC(debug_event,
@@ -782,21 +790,88 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
return 0;
}

+static struct class wmi_bus_class = {
+ .name = "wmi_bus",
+};
+
+static int wmi_minor_get(struct wmi_block *wblock)
+{
+ int ret;
+
+ mutex_lock(&wmi_minor_lock);
+ ret = idr_alloc(&wmi_idr, wblock, 0, WMI_MAX_DEVS, GFP_KERNEL);
+ if (ret >= 0)
+ wblock->minor = ret;
+ else if (ret == -ENOSPC)
+ dev_err(&wblock->dev.dev, "too many wmi devices\n");
+
+ mutex_unlock(&wmi_minor_lock);
+ return ret;
+}
+
+static void wmi_minor_free(struct wmi_block *wblock)
+{
+ mutex_lock(&wmi_minor_lock);
+ idr_remove(&wmi_idr, wblock->minor);
+ mutex_unlock(&wmi_minor_lock);
+}
+
+
static int wmi_dev_probe(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;
+ struct device *clsdev;
+ int ret = 0, devno;

if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
dev_warn(dev, "failed to enable device -- probing anyway\n");

+ /* driver wants a character device made */
+ if (wdriver->file_operations) {
+ dev->devt = wmi_devt;
+ wblock->cdev = cdev_alloc();
+ if (!wblock->cdev) {
+ dev_err(dev, "failed to allocate cdev\n");
+ return -ENOMEM;
+ }
+ cdev_init(wblock->cdev, wdriver->file_operations);
+ wblock->cdev->owner = wdriver->file_operations->owner;
+ ret = wmi_minor_get(wblock);
+ if (ret < 0)
+ return ret;
+ devno = MKDEV(MAJOR(wmi_devt), wblock->minor);
+ ret = cdev_add(wblock->cdev, devno, 1);
+ if (ret) {
+ dev_err(dev, "unable to create device %d:%d\n",
+ MAJOR(wmi_devt), wblock->minor);
+ goto err_probe_cdev;
+ }
+ clsdev = device_create(&wmi_bus_class, dev,
+ MKDEV(MAJOR(wmi_devt), wblock->minor),
+ NULL, "wmi/%s", wdriver->driver.name);
+
+ if (IS_ERR(clsdev)) {
+ dev_err(dev, "unable to create device %d:%d\n",
+ MAJOR(wmi_devt), wblock->minor);
+ ret = PTR_ERR(clsdev);
+ goto err_probe_class;
+ }
+ }
+
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;
+
+err_probe_class:
+ cdev_del(wblock->cdev);
+
+err_probe_cdev:
+ wmi_minor_free(wblock);

return ret;
}
@@ -808,6 +883,13 @@ static int wmi_dev_remove(struct device *dev)
container_of(dev->driver, struct wmi_driver, driver);
int ret = 0;

+ if (wdriver->file_operations) {
+ device_destroy(&wmi_bus_class,
+ MKDEV(MAJOR(wmi_devt), wblock->minor));
+ cdev_del(wblock->cdev);
+ wmi_minor_free(wblock);
+ }
+
if (wdriver->remove)
ret = wdriver->remove(dev_to_wdev(dev));

@@ -817,10 +899,6 @@ static int wmi_dev_remove(struct device *dev)
return ret;
}

-static struct class wmi_bus_class = {
- .name = "wmi_bus",
-};
-
static struct bus_type wmi_bus_type = {
.name = "wmi",
.dev_groups = wmi_groups,
@@ -1270,8 +1348,17 @@ static int __init acpi_wmi_init(void)
goto err_unreg_bus;
}

+ error = alloc_chrdev_region(&wmi_devt, 0, WMI_MAX_DEVS, "wmi");
+ if (error < 0) {
+ pr_err("unable to allocate char dev region\n");
+ goto err_unreg_platform;
+ }
+
return 0;

+err_unreg_platform:
+ platform_driver_unregister(&acpi_wmi_driver);
+
err_unreg_bus:
bus_unregister(&wmi_bus_type);

@@ -1283,6 +1370,7 @@ static int __init acpi_wmi_init(void)

static void __exit acpi_wmi_exit(void)
{
+ unregister_chrdev_region(wmi_devt, WMI_MAX_DEVS);
platform_driver_unregister(&acpi_wmi_driver);
bus_unregister(&wmi_bus_type);
class_unregister(&wmi_bus_class);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 2cd10c3b89e9..ba5f75a32f4c 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -47,6 +47,7 @@ struct wmi_device_id {
struct wmi_driver {
struct device_driver driver;
const struct wmi_device_id *id_table;
+ const struct file_operations *file_operations;

int (*probe)(struct wmi_device *wdev);
int (*remove)(struct wmi_device *wdev);
--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:05 AM9/28/17
to
Some cases the wrong type was used for errors and checks can be
done more cleanly.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
Reviewed-by: Edward O'Callaghan <quas...@google.com>
---
drivers/platform/x86/dell-smbios.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index ac176953e46e..d75b6971588e 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -346,16 +346,16 @@ int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version)
}
desc_buffer = (u32 *)obj->buffer.pointer;

- if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
- 8, desc_buffer);
+ if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
+ desc_buffer);

if (desc_buffer[2] != 0 && desc_buffer[2] != 1)
- dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
desc_buffer[2]);

if (desc_buffer[3] != 4096)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
+ dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\n",
desc_buffer[3]);

*version = desc_buffer[2];
--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:05 AM9/28/17
to
This userspace character device will be used to perform SMBIOS calls
from any applications.

It provides an ioctl that will allow passing the 32k WMI calling
interface buffer between userspace and kernel space.

This character device is intended to deprecate the dcdbas kernel module
and the interface that it provides to userspace.

It's important for the driver to provide a R/W ioctl to ensure that
two competing userspace processes don't race to provide or read each
others data.

The character device will only be created if the WMI interface was
found.

The API for interacting with this interface is defined in documentation
as well as a uapi header provides the format of the structures.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
Documentation/ABI/testing/dell-wmi-smbios | 11 ++++
drivers/platform/x86/dell-smbios.c | 100 ++++++++++++++++++++++++------
drivers/platform/x86/dell-smbios.h | 19 +-----
include/uapi/linux/dell-wmi-smbios.h | 30 +++++++++
4 files changed, 124 insertions(+), 36 deletions(-)
create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
create mode 100644 include/uapi/linux/dell-wmi-smbios.h

diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
new file mode 100644
index 000000000000..0a4200688cb0
--- /dev/null
+++ b/Documentation/ABI/testing/dell-wmi-smbios
@@ -0,0 +1,11 @@
+What: /dev/wmi-dell-wmi-smbios
+Date: October 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.li...@dell.com>
+Description:
+ Perform SMBIOS calls on supported Dell machines.
+ through the Dell ACPI-WMI interface.
+
+ IOCTL's and buffer formats are defined in:
+ <uapi/linux/dell-wmi-smbios.h>
+
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 5d793b012e5e..4174afbade13 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -24,6 +24,7 @@
#include <linux/slab.h>
#include <linux/io.h>
#include <linux/wmi.h>
+#include <linux/uaccess.h>
#include "dell-smbios.h"

#ifdef CONFIG_DCDBAS
@@ -41,8 +42,9 @@ struct calling_interface_structure {
struct calling_interface_token tokens[];
} __packed;

-static void *buffer;
-static size_t bufferlen;
+static void *internal_buffer;
+static size_t internal_bufferlen;
+static struct wmi_calling_interface_buffer *devfs_buffer;
static DEFINE_MUTEX(buffer_mutex);

static int da_command_address;
@@ -70,15 +72,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
{
mutex_lock(&buffer_mutex);
dell_smbios_clear_buffer();
- if (wmi_dev)
- return &((struct wmi_calling_interface_buffer *) buffer)->smi;
- return buffer;
+ if (!wmi_dev)
+ return internal_buffer;
+ return &((struct wmi_calling_interface_buffer *) internal_buffer)->smi;
}
EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);

void dell_smbios_clear_buffer(void)
{
- memset(buffer, 0, bufferlen);
+ memset(internal_buffer, 0, internal_bufferlen);
}
EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);

@@ -135,13 +137,13 @@ static void run_smi_smbios_call(struct calling_interface_buffer *buf) {}
void dell_smbios_send_request(int class, int select)
{
if (wmi_dev) {
- struct wmi_calling_interface_buffer *buf = buffer;
+ struct wmi_calling_interface_buffer *buf = internal_buffer;

buf->smi.class = class;
buf->smi.select = select;
run_wmi_smbios_call(buf);
} else {
- struct calling_interface_buffer *buf = buffer;
+ struct calling_interface_buffer *buf = internal_buffer;

buf->class = class;
buf->select = select;
@@ -227,6 +229,49 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

+static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
+{
+ return nonseekable_open(inode, file);
+}
+
+static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ void __user *p = (void __user *) arg;
+ size_t size;
+ int ret = 0;
+
+ if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
+ return -ENOTTY;
+
+ switch (cmd) {
+ case DELL_WMI_SMBIOS_CALL_CMD:
+ size = sizeof(struct wmi_calling_interface_buffer);
+ mutex_lock(&buffer_mutex);
+ if (copy_from_user(devfs_buffer, p, size)) {
+ ret = -EFAULT;
+ goto fail_smbios_cmd;
+ }
+ ret = run_wmi_smbios_call(devfs_buffer);
+ if (ret != 0)
+ goto fail_smbios_cmd;
+ if (copy_to_user(p, devfs_buffer, size))
+ ret = -EFAULT;
+fail_smbios_cmd:
+ mutex_unlock(&buffer_mutex);
+ break;
+ default:
+ pr_err("unsupported ioctl: %d.\n", cmd);
+ ret = -ENOIOCTLCMD;
+ }
+ return ret;
+}
+
/*
* Descriptor buffer is 128 byte long and contains:
*
@@ -309,22 +354,33 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
return -ENODEV;

/* no longer need the SMI page */
- free_page((unsigned long)buffer);
+ free_page((unsigned long)internal_buffer);

/* WMI buffer should be 32k */
- buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
- if (!buffer)
+ internal_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+ if (!internal_buffer)
return -ENOMEM;
- bufferlen = sizeof(struct wmi_calling_interface_buffer);
+ internal_bufferlen = sizeof(struct wmi_calling_interface_buffer);
+
+ devfs_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+ if (!devfs_buffer) {
+ ret = -ENOMEM;
+ goto fail_devfs_buffer;
+ }

wmi_dev = wdev;
return 0;
+
+fail_devfs_buffer:
+ free_pages((unsigned long)internal_buffer, 3);
+ return ret;
}

static int dell_smbios_wmi_remove(struct wmi_device *wdev)
{
wmi_dev = NULL;
- free_pages((unsigned long)buffer, 3);
+ free_pages((unsigned long)internal_buffer, 3);
+ free_pages((unsigned long)devfs_buffer, 3);
return 0;
}

@@ -333,6 +389,13 @@ static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
{ },
};

+static const struct file_operations dell_wmi_smbios_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = dell_wmi_smbios_ioctl,
+ .open = dell_wmi_smbios_open,
+ .release = dell_wmi_smbios_release,
+};
+
static struct wmi_driver dell_wmi_smbios_driver = {
.driver = {
.name = "dell-smbios",
@@ -340,6 +403,7 @@ static struct wmi_driver dell_wmi_smbios_driver = {
.probe = dell_smbios_wmi_probe,
.remove = dell_smbios_wmi_remove,
.id_table = dell_smbios_wmi_id_table,
+ .file_operations = &dell_wmi_smbios_fops,
};

static int __init dell_smbios_init(void)
@@ -356,14 +420,14 @@ static int __init dell_smbios_init(void)
* Allocate buffer below 4GB for SMI data--only 32-bit physical addr
* is passed to SMI handler.
*/
- buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
- bufferlen = sizeof(struct calling_interface_buffer);
+ internal_buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+ internal_bufferlen = sizeof(struct calling_interface_buffer);
#else
- buffer = NULL;
+ internal_buffer = NULL;
#endif /* CONFIG_DCDBAS */
wmi_driver_register(&dell_wmi_smbios_driver);

- if (!buffer) {
+ if (!internal_buffer) {
kfree(da_tokens);
return -ENOMEM;
}
@@ -373,7 +437,7 @@ static int __init dell_smbios_init(void)
static void __exit dell_smbios_exit(void)
{
kfree(da_tokens);
- free_page((unsigned long)buffer);
+ free_page((unsigned long)internal_buffer);
wmi_driver_unregister(&dell_wmi_smbios_driver);
}

diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index be9ec1ccf8bf..3b0e2e3f6ede 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -18,27 +18,10 @@
#define _DELL_SMBIOS_H_

#include <linux/wmi.h>
+#include <uapi/linux/dell-wmi-smbios.h>

struct notifier_block;

-/* If called through fallback SMI rather than WMI this structure will be
- * modified by the firmware when we enter system management mode, hence the
- * volatiles
- */
-struct calling_interface_buffer {
- u16 class;
- u16 select;
- volatile u32 input[4];
- volatile u32 output[4];
-} __packed;
-
-struct wmi_calling_interface_buffer {
- struct calling_interface_buffer smi;
- u32 argattrib;
- u32 blength;
- u8 data[32724];
-} __packed;
-
struct calling_interface_token {
u16 tokenID;
u16 location;
diff --git a/include/uapi/linux/dell-wmi-smbios.h b/include/uapi/linux/dell-wmi-smbios.h
new file mode 100644
index 000000000000..adbe57dd055b
--- /dev/null
+++ b/include/uapi/linux/dell-wmi-smbios.h
@@ -0,0 +1,30 @@
+#ifndef _UAPI_DELL_WMI_SMBIOS_H_
+#define _UAPI_DELL_WMI_SMBIOS_H_
+
+#include <linux/ioctl.h>
+
+/* If called through fallback SMI rather than WMI this structure will be
+ * modified by the firmware when we enter system management mode, hence the
+ * volatiles
+ */
+struct calling_interface_buffer {
+ u16 class;
+ u16 select;
+ volatile u32 input[4];
+ volatile u32 output[4];
+} __packed;
+
+struct wmi_calling_interface_buffer {
+ struct calling_interface_buffer smi;
+ u32 argattrib;
+ u32 blength;
+ u8 data[32724];
+} __packed;
+
+#define DELL_WMI_SMBIOS_IOC 'D'
+/* run SMBIOS calling interface command
+ * note - 32k is too big for size, so this can not be encoded in macro properly
+ */
+#define DELL_WMI_SMBIOS_CALL_CMD _IOWR(DELL_WMI_SMBIOS_IOC, 0, u8)
+
+#endif /* _UAPI_DELL_WMI_SMBIOS_H_ */
--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:05 AM9/28/17
to
The driver currently uses an SMI interface which grants direct access
to physical memory to the firmware SMM methods via a pointer.

Now add a WMI-ACPI interface that is detected by WMI probe and preferred
over the SMI interface.

Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
for a buffer of data storage when SMM calls are performed.

This is a safer approach to use in kernel drivers as the SMM will
only have access to that OperationRegion.

As a result, this change removes the dependency on this driver on the
dcdbas kernel module. It's now an optional compilation option.

When modifying this, add myself to MAINTAINERS.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
MAINTAINERS | 6 ++
drivers/platform/x86/Kconfig | 14 ++--
drivers/platform/x86/dell-smbios.c | 128 ++++++++++++++++++++++++++++++++-----
drivers/platform/x86/dell-smbios.h | 15 ++++-
4 files changed, 138 insertions(+), 25 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 08b96f77f618..6d76b09f46cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3990,6 +3990,12 @@ S: Maintained
F: drivers/hwmon/dell-smm-hwmon.c
F: include/uapi/linux/i8k.h

+DELL SMBIOS DRIVER
+M: Pali Rohár <pali....@gmail.com>
+M: Mario Limonciello <mario.li...@dell.com>
+S: Maintained
+F: drivers/platform/x86/dell-smbios.*
+
DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
M: Doug Warzecha <Douglas_...@dell.com>
S: Maintained
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1f7959ff055c..415886d7a857 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -92,13 +92,17 @@ config ASUS_LAPTOP
If you have an ACPI-compatible ASUS laptop, say Y or M here.

config DELL_SMBIOS
- tristate
- select DCDBAS
+ tristate "Dell SMBIOS calling interface"
+ depends on ACPI_WMI
---help---
- This module provides common functions for kernel modules using
- Dell SMBIOS.
+ This module provides common functions for kernel modules and
+ userspace using Dell SMBIOS.
+
+ If you have a Dell computer, say Y or M here.

- If you have a Dell laptop, say Y or M here.
+ If you have a machine from < 2007 without a WMI interface you
+ may also want to enable CONFIG_DCDBAS to allow this driver to
+ work.

config DELL_LAPTOP
tristate "Dell Laptop Extras"
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index e9b1ca07c872..4472817ee045 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -4,6 +4,7 @@
* Copyright (c) Red Hat <m...@redhat.com>
* Copyright (c) 2014 Gabriele Mazzotta <gabrie...@gmail.com>
* Copyright (c) 2014 Pali Rohár <pali....@gmail.com>
+ * Copyright (c) 2017 Dell Inc.
*
* Based on documentation in the libsmbios package:
* Copyright (C) 2005-2014 Dell Inc.
@@ -22,9 +23,15 @@
#include <linux/mutex.h>
#include <linux/slab.h>
#include <linux/io.h>
-#include "../../firmware/dcdbas.h"
+#include <linux/wmi.h>
#include "dell-smbios.h"

+#ifdef CONFIG_DCDBAS
+#include "../../firmware/dcdbas.h"
+#endif
+
+#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+
struct calling_interface_structure {
struct dmi_header header;
u16 cmdIOAddress;
@@ -33,12 +40,14 @@ struct calling_interface_structure {
struct calling_interface_token tokens[];
} __packed;

-static struct calling_interface_buffer *buffer;
+static void *buffer;
+static size_t bufferlen;
static DEFINE_MUTEX(buffer_mutex);

static int da_command_address;
static int da_command_code;
static int da_num_tokens;
+struct wmi_device *wmi_dev;
static struct calling_interface_token *da_tokens;

int dell_smbios_error(int value)
@@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
{
mutex_lock(&buffer_mutex);
dell_smbios_clear_buffer();
+ if (wmi_dev)
+ return &((struct wmi_calling_interface_buffer *) buffer)->smi;
return buffer;
}
EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);

void dell_smbios_clear_buffer(void)
{
- memset(buffer, 0, sizeof(struct calling_interface_buffer));
+ memset(buffer, 0, bufferlen);
}
EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);

@@ -76,7 +87,36 @@ void dell_smbios_release_buffer(void)
}
EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);

-void dell_smbios_send_request(int class, int select)
+static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
+{
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+ struct acpi_buffer input;
+ union acpi_object *obj;
+ acpi_status status;
+
+ input.length = sizeof(struct wmi_calling_interface_buffer);
+ input.pointer = buf;
+
+ status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output);
+ if (ACPI_FAILURE(status)) {
+ pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
+ buf->smi.class, buf->smi.select,
+ buf->smi.input[0], buf->smi.input[1],
+ buf->smi.input[2], buf->smi.input[3]);
+ return -EIO;
+ }
+ obj = (union acpi_object *)output.pointer;
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ pr_err("invalid type : %d\n", obj->type);
+ return -EIO;
+ }
+ memcpy(buf, obj->buffer.pointer, input.length);
+
+ return 0;
+}
+
+#ifdef CONFIG_DCDBAS
+static void run_smi_smbios_call(struct calling_interface_buffer *buf)
{
struct smi_cmd command;

@@ -85,12 +125,28 @@ void dell_smbios_send_request(int class, int select)
command.command_code = da_command_code;
command.ebx = virt_to_phys(buffer);
command.ecx = 0x42534931;
-
- buffer->class = class;
- buffer->select = select;
-
dcdbas_smi_request(&command);
}
+#else
+static void run_smi_smbios_call(struct calling_interface_buffer *buf) {}
+#endif /* CONFIG_DCDBAS */
+
+void dell_smbios_send_request(int class, int select)
+{
+ if (wmi_dev) {
+ struct wmi_calling_interface_buffer *buf = buffer;
+
+ buf->smi.class = class;
+ buf->smi.select = select;
+ run_wmi_smbios_call(buf);
+ } else {
+ struct calling_interface_buffer *buf = buffer;
+
+ buf->class = class;
+ buf->select = select;
+ run_smi_smbios_call(buf);
+ }
+}
EXPORT_SYMBOL_GPL(dell_smbios_send_request);

struct calling_interface_token *dell_smbios_find_token(int tokenid)
@@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
}
}

-static int __init dell_smbios_init(void)
+static int dell_smbios_wmi_probe(struct wmi_device *wdev)
+{
+ /* no longer need the SMI page */
+ free_page((unsigned long)buffer);
+
+ /* WMI buffer should be 32k */
+ buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+ if (!buffer)
+ return -ENOMEM;
+ bufferlen = sizeof(struct wmi_calling_interface_buffer);
+ wmi_dev = wdev;
+ return 0;
+}
+
+static int dell_smbios_wmi_remove(struct wmi_device *wdev)
{
- int ret;
+ wmi_dev = NULL;
+ free_pages((unsigned long)buffer, 3);
+ return 0;
+}

+static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
+ { .guid_string = DELL_WMI_SMBIOS_GUID },
+ { },
+};
+
+static struct wmi_driver dell_wmi_smbios_driver = {
+ .driver = {
+ .name = "dell-smbios",
+ },
+ .probe = dell_smbios_wmi_probe,
+ .remove = dell_smbios_wmi_remove,
+ .id_table = dell_smbios_wmi_id_table,
+};
+
+static int __init dell_smbios_init(void)
+{
dmi_walk(find_tokens, NULL);

if (!da_tokens) {
@@ -181,34 +270,39 @@ static int __init dell_smbios_init(void)
return -ENODEV;
}

+#ifdef CONFIG_DCDBAS
/*
* Allocate buffer below 4GB for SMI data--only 32-bit physical addr
* is passed to SMI handler.
*/
buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+ bufferlen = sizeof(struct calling_interface_buffer);
+#else
+ buffer = NULL;
+#endif /* CONFIG_DCDBAS */
+ wmi_driver_register(&dell_wmi_smbios_driver);
+
if (!buffer) {
- ret = -ENOMEM;
- goto fail_buffer;
+ kfree(da_tokens);
+ return -ENOMEM;
}
-
return 0;
-
-fail_buffer:
- kfree(da_tokens);
- return ret;
}

static void __exit dell_smbios_exit(void)
{
kfree(da_tokens);
free_page((unsigned long)buffer);
+ wmi_driver_unregister(&dell_wmi_smbios_driver);
}

subsys_initcall(dell_smbios_init);
module_exit(dell_smbios_exit);

+
MODULE_AUTHOR("Matthew Garrett <m...@redhat.com>");
MODULE_AUTHOR("Gabriele Mazzotta <gabrie...@gmail.com>");
MODULE_AUTHOR("Pali Rohár <pali....@gmail.com>");
+MODULE_AUTHOR("Mario Limonciello <mario.li...@dell.com>");
MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS");
MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 45cbc2292cd3..2f6fce81ee69 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -4,6 +4,7 @@
* Copyright (c) Red Hat <m...@redhat.com>
* Copyright (c) 2014 Gabriele Mazzotta <gabrie...@gmail.com>
* Copyright (c) 2014 Pali Rohár <pali....@gmail.com>
+ * Copyright (c) 2017 Dell Inc.
*
* Based on documentation in the libsmbios package:
* Copyright (C) 2005-2014 Dell Inc.
@@ -18,9 +19,10 @@

struct notifier_block;

-/* This structure will be modified by the firmware when we enter
- * system management mode, hence the volatiles */
-
+/* If called through fallback SMI rather than WMI this structure will be
+ * modified by the firmware when we enter system management mode, hence the
+ * volatiles
+ */
struct calling_interface_buffer {
u16 class;
u16 select;
@@ -28,6 +30,13 @@ struct calling_interface_buffer {
volatile u32 output[4];
} __packed;

+struct wmi_calling_interface_buffer {
+ struct calling_interface_buffer smi;
+ u32 argattrib;
+ u32 blength;
+ u8 data[32724];
+} __packed;
+
struct calling_interface_token {
u16 tokenID;
u16 location;
--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:08 AM9/28/17
to
The dell-wmi-smbios driver should be enabled by default when ACPI_WMI
is enabled (like many other WMI drivers).

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 415886d7a857..a83c275019a4 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -94,6 +94,7 @@ config ASUS_LAPTOP
config DELL_SMBIOS
tristate "Dell SMBIOS calling interface"
depends on ACPI_WMI
+ default ACPI_WMI
---help---
This module provides common functions for kernel modules and
userspace using Dell SMBIOS.
--
2.14.1

Mario Limonciello

unread,
Sep 28, 2017, 12:10:08 AM9/28/17
to
Drivers properly using the wmibus can pass their wmi_device
pointer rather than the GUID back to the WMI bus to evaluate
the proper methods.

Any "new" drivers added that use the WMI bus should use this
rather than the old wmi_evaluate_method that would take the
GUID.

Signed-off-by: Mario Limonciello <mario.li...@dell.com>
---
drivers/platform/x86/wmi.c | 28 ++++++++++++++++++++++++----
include/linux/wmi.h | 6 ++++++
2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7a05843aff19..4d73a87c2ddf 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -200,6 +200,28 @@ static acpi_status wmi_method_enable(struct wmi_block *wblock, int enable)
*/
acpi_status wmi_evaluate_method(const char *guid_string, u8 instance,
u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
+{
+ struct wmi_block *wblock = NULL;
+
+ if (!find_guid(guid_string, &wblock))
+ return AE_ERROR;
+ return wmidev_evaluate_method(&wblock->dev, instance, method_id,
+ in, out);
+}
+EXPORT_SYMBOL_GPL(wmi_evaluate_method);
+
+/**
+ * wmidev_evaluate_method - Evaluate a WMI method
+ * @wdev: A wmi bus device from a driver
+ * @instance: Instance index
+ * @method_id: Method ID to call
+ * &in: Buffer containing input for the method call
+ * &out: Empty buffer to return the method results
+ *
+ * Call an ACPI-WMI method
+ */
+acpi_status wmidev_evaluate_method(struct wmi_device *wdev, u8 instance,
+ u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
{
struct guid_block *block = NULL;
struct wmi_block *wblock = NULL;
@@ -209,9 +231,7 @@ u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
union acpi_object params[3];
char method[5] = "WM";

- if (!find_guid(guid_string, &wblock))
- return AE_ERROR;
-
+ wblock = container_of(wdev, struct wmi_block, dev);
block = &wblock->gblock;
handle = wblock->acpi_device->handle;

@@ -246,7 +266,7 @@ u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)

return status;
}
-EXPORT_SYMBOL_GPL(wmi_evaluate_method);
+EXPORT_SYMBOL_GPL(wmidev_evaluate_method);

static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
struct acpi_buffer *out)
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index cd0d7734dc49..2cd10c3b89e9 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -26,6 +26,12 @@ struct wmi_device {
bool setable;
};

+/* evaluate the ACPI method associated with this device */
+extern acpi_status wmidev_evaluate_method(struct wmi_device *wdev,
+ u8 instance, u32 method_id,
+ const struct acpi_buffer *in,
+ struct acpi_buffer *out);
+
/* Caller must kfree the result. */
extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
u8 instance);
--
2.14.1

Pali Rohár

unread,
Sep 28, 2017, 3:00:06 AM9/28/17
to
This code does not seem to be safe. dell_smbios_wmi_probe and
dell_smbios_wmi_remove are called at any time when kernel register new
device which matches some properties OR when user manually bind this
driver to that device.

buffer and wmi_dev is shared as a global variable which means that when
there are two devices which want to bind to this driver, kernel would
get double free at removing time.

Devices from the bus subsystem should not use global shared variables,
but rather allocate own memory...

Also note that user can at any time unbound device from driver and also
can bind it again.
Pali Rohár
pali....@gmail.com
signature.asc

Mario.Li...@dell.com

unread,
Sep 28, 2017, 6:50:05 PM9/28/17
to
> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Thursday, September 28, 2017 2:54 AM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: dvh...@infradead.org; Andy Shevchenko <andy.sh...@gmail.com>;
> LKML <linux-...@vger.kernel.org>; platform-...@vger.kernel.org; Andy
> Lutomirski <lu...@kernel.org>; quas...@google.com
> Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI
> interface
>
I'll adjust for the assumptions I made about it only happening at module init.

> buffer and wmi_dev is shared as a global variable which means that when
> there are two devices which want to bind to this driver, kernel would
> get double free at removing time.

But there is only one GUID in id_table. How can two devices bind to this?
This should be an impossible scenario.

>
> Devices from the bus subsystem should not use global shared variables,
> but rather allocate own memory...

Part of the problem is that this module can operate in two modes now.
I think there will have to be global shared variables when operating in SMI
mode. Or maybe put those bits into a platform_device for SMI mode usage?

The problem I think then becomes how do you handle dell_smbios_send_request?
Without global shared memory for the module the dell-laptop and dell-wmi
modules won't be able to use this.

>
> Also note that user can at any time unbound device from driver and also
> can bind it again.
I'll make some adjustments to account for this.

Pali Rohár

unread,
Sep 29, 2017, 3:40:06 AM9/29/17
to
That is truth, but ...

> How can two devices bind to this?
> This should be an impossible scenario.

... in ACPI DSDT can be more WMI _WDG buffers which could lead to more
wmi buses and each could have own GUID. Therefore there is a theoretical
chance that specially prepared ACPI DSDT code can cause this problem.

> > Devices from the bus subsystem should not use global shared variables,
> > but rather allocate own memory...
>
> Part of the problem is that this module can operate in two modes now.
> I think there will have to be global shared variables when operating in SMI
> mode. Or maybe put those bits into a platform_device for SMI mode usage?
>
> The problem I think then becomes how do you handle dell_smbios_send_request?
> Without global shared memory for the module the dell-laptop and dell-wmi
> modules won't be able to use this.

The whole problem is that SMBIOS calls are implemented via singleton
pattern. And this singleton "instance" needs to use something which is
not a singleton, but a dynamic objects (wmi device <--> driver pattern).

Idea how to handle it:
* put wmi smbios call function into own driver
* put smm smbios call function into own driver
* create dispatcher function which take first available device of one of
the above driver and call on them smbios call function

This problem is very similar to problems in objects world... driver as a
class and device as a instance.

(Or if somebody has a better idea, let us know...)

> > Also note that user can at any time unbound device from driver and also
> > can bind it again.
> I'll make some adjustments to account for this.

To prevent crashing kernel, this needs to be written correctly. And user
should not be able to crash kernel just when trying to unbound driver
from the device.

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

Darren Hart

unread,
Sep 29, 2017, 9:00:05 PM9/29/17
to
On Wed, Sep 27, 2017 at 11:02:14PM -0500, Mario Limonciello wrote:
> The driver currently uses an SMI interface which grants direct access
> to physical memory to the firmware SMM methods via a pointer.
>
> Now add a WMI-ACPI interface that is detected by WMI probe and preferred
> over the SMI interface.
>
> Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
> for a buffer of data storage when SMM calls are performed.
>
> This is a safer approach to use in kernel drivers as the SMM will
> only have access to that OperationRegion.
>
> As a result, this change removes the dependency on this driver on the
> dcdbas kernel module. It's now an optional compilation option.
>
> When modifying this, add myself to MAINTAINERS.
>
> Signed-off-by: Mario Limonciello <mario.li...@dell.com>
> ---

...

> +DELL SMBIOS DRIVER
> +M: Pali Rohár <pali....@gmail.com>
> +M: Mario Limonciello <mario.li...@dell.com>
> +S: Maintained
> +F: drivers/platform/x86/dell-smbios.*

Pali, do you agree with this?

...

> int dell_smbios_error(int value)
> @@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
> {
> mutex_lock(&buffer_mutex);
> dell_smbios_clear_buffer();
> + if (wmi_dev)
> + return &((struct wmi_calling_interface_buffer *) buffer)->smi;
> return buffer;

Hrm, my hope had been to make this transparent at this level. ... This
may need more thought. I don't care for the casting here.... hopefully
enlightenment lies below...

> +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
> +{
> + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> + struct acpi_buffer input;
> + union acpi_object *obj;
> + acpi_status status;
> +
> + input.length = sizeof(struct wmi_calling_interface_buffer);
> + input.pointer = buf;
> +
> + status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output);
> + if (ACPI_FAILURE(status)) {
> + pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
> + buf->smi.class, buf->smi.select,
> + buf->smi.input[0], buf->smi.input[1],
> + buf->smi.input[2], buf->smi.input[3]);
> + return -EIO;
> + }
> + obj = (union acpi_object *)output.pointer;
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + pr_err("invalid type : %d\n", obj->type);
> + return -EIO;
> + }

We ensure we don't write beyond buf, but we havne't ensured we don't read beyond
obj.


if (obj->length != input.length) { // or maybe >= ??
pr_err("invalid buffer length : %d\n", obj->length);
return -EINVAL;
}

> -static int __init dell_smbios_init(void)
> +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> +{
> + /* no longer need the SMI page */
> + free_page((unsigned long)buffer);
> +
> + /* WMI buffer should be 32k */
> + buffer = (void *)__get_free_pages(GFP_KERNEL, 3);

Assuming PAGE_SIZE here (I know, this driver, this architecture,
etc...). But, please use get_order() to determine number of pages
from a linear size:

__get_free_pages(GFP_KERNEL, get_order(32768));

> + if (!buffer)
> + return -ENOMEM;
> + bufferlen = sizeof(struct wmi_calling_interface_buffer);

Use a consistent way to allocate and set the len. Maybe set bufferlen
above and use get_order(bufferlen).
If this cannot be avoided, then use IS_ENABLED(CONFIG_DCDBAS).

I still think we should be to come up with a cleaner way to deal with
the two buffers than a bunch of #ifdefs and if (wdev) {} else {} blocks.

...
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
...
> -/* This structure will be modified by the firmware when we enter
> - * system management mode, hence the volatiles */
> -
> +/* If called through fallback SMI rather than WMI this structure will be
> + * modified by the firmware when we enter system management mode, hence the
> + * volatiles
> + */

Follow coding style when modifying comment blocks (even when they
didn't before) please. See coding-style 8) COmmenting.

/*
* If called ...
...
* volatiles.
*/

Darren Hart

unread,
Sep 29, 2017, 9:30:05 PM9/29/17
to
On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote:
> The Dell WMI descriptor check is used as an indication that WMI
> calls are safe to run both when used with the notification
> ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.
>
> As some code in dell-wmi-smbios is already a prerequisite for
> dell-wmi, move the code for performing the descriptor check into
> dell-wmi-smbios and let both drivers use it from there.
>
> Signed-off-by: Mario Limonciello <mario.li...@dell.com>
> ---
...
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
...
>
> +/*
> + * Descriptor buffer is 128 byte long and contains:
...
> + if (obj->buffer.length != 128) {
> + dev_err(&wdev->dev,
> + "Dell descriptor buffer has invalid length (%d)\n",
> + obj->buffer.length);

This seems odd. We call it an error (not a warning) if != 128, but we only abort
and return an error if it's < 16.

If it's an error, we should return an error code, if anything above 16 is
acceptable but 128 is preferred, the above should be a warning at best. (this
scenario seems unlikely).

> + if (obj->buffer.length < 16) {
> + ret = -EINVAL;
> + goto out;
> + }
> + }
> + desc_buffer = (u32 *)obj->buffer.pointer;
> +
> + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)

This seems like it should be an || ?

(I see this is fixed in a later patch)

I would have suggested fixing it, then moving it - just from a pure ease of
review perspective. Perhaps not in the comment above that this is a verbatim
move, some fixes are coming in subsequent patches. This is worth noting for a
future develop who may be choosing what to backport.

> static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> {
> + int ret;
> + u32 interface_version;

Declarations in order of decreasing line length please.

> @@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> if (!buffer)
> return -ENOMEM;
> bufferlen = sizeof(struct wmi_calling_interface_buffer);
> +

Stray whitespace, should have been in the previous patch I guess?

Darren Hart

unread,
Sep 29, 2017, 10:00:05 PM9/29/17
to
+Greg, Rafael, Matthew, and Christoph

You each provided feedback regarding the method of exposing WMI methods
to userspace. This and subsequent patches from Mario lay some of the
core groundwork.

They implement an implicit whitelist as only drivers requesting the char
dev will see it created.

https://lkml.org/lkml/2017/9/28/8

Darren Hart

unread,
Sep 29, 2017, 10:10:05 PM9/29/17
to
/dev/wmi/dell-smbios
A large portion of this changeset is dedicated to renaming these two variables,
but it isn't clear why, and not mentioned in the changelog. Seems like
unnecessary churn. Or if we really should rename it... can it be done earlier in
the series when we're working with those buffers for functional reasons?

Darren Hart

unread,
Sep 29, 2017, 10:20:04 PM9/29/17
to
On Wed, Sep 27, 2017 at 11:02:18PM -0500, Mario Limonciello wrote:
> Currently userspace tools can access system tokens via the dcdbas
> kernel module and a SMI call that will cause the platform to execute
> SMM code.
>
> With a goal in mind of deprecating the dcdbas kernel module a different
> method for accessing these tokens from userspace needs to be created.
>
> This is intentionally marked to only be readable as root as it can
> contain sensitive information about the platform's configuration.
>
> Signed-off-by: Mario Limonciello <mario.li...@dell.com>
> ---
...
> +static ssize_t tokens_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + size_t off = 0;
> + int i;
> +
> + for (i = 0; i < da_num_tokens; i++) {
> + if (off > PAGE_SIZE)
> + break;
> + off += scnprintf(buf+off, PAGE_SIZE-off, "%04x\t%04x\t%04x\n",

Minor Coding Style nit - spaces around binary operators: 3.1) Spaces
It is loading more messages.
0 new messages