* [PATCH v5 01/14] platform/x86: wmi: Add new method wmidev_evaluate_method
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 02/14] platform/x86: dell-wmi: increase severity of some failures Mario Limonciello
` (12 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
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.limonciello@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
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 02/14] platform/x86: dell-wmi: increase severity of some failures
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 01/14] platform/x86: wmi: Add new method wmidev_evaluate_method Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 03/14] platform/x86: dell-wmi: clean up wmi descriptor check Mario Limonciello
` (11 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
There is a lot of error checking in place for the format of the WMI
descriptor buffer, but some of the potentially raised issues should
be considered critical failures.
If the buffer size or header don't match, this is a good indication
that the buffer format changed in a way that the rest of the data
should not be relied upon.
For the remaining data set vectors, continue to notate a warning
in undefined results, but as those are fields that the descriptor
intended to refer to other applications, don't fail if they're new
values.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
drivers/platform/x86/dell-wmi.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1fbef560ca67..2cfaaa8faf0a 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -657,17 +657,18 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
dev_err(&wdev->dev,
"Dell descriptor buffer has invalid length (%d)\n",
obj->buffer.length);
- if (obj->buffer.length < 16) {
- ret = -EINVAL;
- goto out;
- }
+ 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",
+ if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720) {
+ dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
8, buffer);
+ ret = -EINVAL;
+ goto out;
+ }
if (buffer[2] != 0 && buffer[2] != 1)
dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 03/14] platform/x86: dell-wmi: clean up wmi descriptor check
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 01/14] platform/x86: wmi: Add new method wmidev_evaluate_method Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 02/14] platform/x86: dell-wmi: increase severity of some failures Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 04/14] platform/x86: dell-wmi: allow 32k return size in the descriptor Mario Limonciello
` (10 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
Some cases the wrong type was used for errors and checks can be
done more cleanly.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Reviewed-by: Edward O'Callaghan <quasisec@google.com>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/platform/x86/dell-wmi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 2cfaaa8faf0a..ece2fe341f01 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -663,19 +663,19 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
buffer = (u32 *)obj->buffer.pointer;
- if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720) {
- dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
- 8, buffer);
+ if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
+ dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
+ buffer);
ret = -EINVAL;
goto out;
}
if (buffer[2] != 0 && 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",
buffer[2]);
if (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",
buffer[3]);
priv->interface_version = buffer[2];
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 04/14] platform/x86: dell-wmi: allow 32k return size in the descriptor
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (2 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 03/14] platform/x86: dell-wmi: clean up wmi descriptor check Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 05/14] platform/x86: dell-wmi-descriptor: split WMI descriptor into it's own driver Mario Limonciello
` (9 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
Some platforms this year will be adopting 32k WMI buffer, so don't
complain when encountering those.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
drivers/platform/x86/dell-wmi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index ece2fe341f01..c8c7f4f9326c 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -624,7 +624,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
* Vendor Signature 0 4 "DELL"
* Object Signature 4 4 " WMI"
* WMI Interface Version 8 4 <version>
- * WMI buffer length 12 4 4096
+ * WMI buffer length 12 4 4096 or 32768
*/
static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
{
@@ -674,7 +674,7 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
buffer[2]);
- if (buffer[3] != 4096)
+ if (desc_buffer[3] != 4096 && desc_buffer[3] != 32768)
dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\n",
buffer[3]);
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 05/14] platform/x86: dell-wmi-descriptor: split WMI descriptor into it's own driver
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (3 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 04/14] platform/x86: dell-wmi: allow 32k return size in the descriptor Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 06/14] platform/x86: wmi: Don't allow drivers to get each other's GUIDs Mario Limonciello
` (8 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
All communication on individual GUIDs should occur in separate drivers.
Allowing a driver to communicate with the bus to another GUID is just
a hack that discourages drivers to adopt the bus model.
The information found from the WMI descriptor driver is now exported
for use by other drivers.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
MAINTAINERS | 5 +
drivers/platform/x86/Kconfig | 5 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/dell-wmi-descriptor.c | 162 +++++++++++++++++++++++++++++
drivers/platform/x86/dell-wmi-descriptor.h | 18 ++++
drivers/platform/x86/dell-wmi.c | 89 ++--------------
6 files changed, 198 insertions(+), 82 deletions(-)
create mode 100644 drivers/platform/x86/dell-wmi-descriptor.c
create mode 100644 drivers/platform/x86/dell-wmi-descriptor.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 08b96f77f618..659dbeec4191 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4002,6 +4002,11 @@ M: Pali Rohár <pali.rohar@gmail.com>
S: Maintained
F: drivers/platform/x86/dell-wmi.c
+DELL WMI DESCRIPTOR DRIVER
+M: Mario Limonciello <mario.limonciello@dell.com>
+S: Maintained
+F: drivers/platform/x86/dell-wmi-descriptor.c
+
DELTA ST MEDIA DRIVER
M: Hugues Fruchet <hugues.fruchet@st.com>
L: linux-media@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1f7959ff055c..7722923c968c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -121,6 +121,7 @@ config DELL_WMI
depends on DMI
depends on INPUT
depends on ACPI_VIDEO || ACPI_VIDEO = n
+ select DELL_WMI_DESCRIPTOR
select DELL_SMBIOS
select INPUT_SPARSEKMAP
---help---
@@ -129,6 +130,10 @@ config DELL_WMI
To compile this driver as a module, choose M here: the module will
be called dell-wmi.
+config DELL_WMI_DESCRIPTOR
+ tristate
+ depends on ACPI_WMI
+
config DELL_WMI_AIO
tristate "WMI Hotkeys for Dell All-In-One series"
depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2b315d0df3b7..8636f5d3424f 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -14,6 +14,7 @@ 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_DESCRIPTOR) += dell-wmi-descriptor.o
obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c
new file mode 100644
index 000000000000..72e317cf0365
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-descriptor.c
@@ -0,0 +1,162 @@
+/*
+ * Dell WMI descriptor driver
+ *
+ * Copyright (C) 2017 Dell Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+#include "dell-wmi-descriptor.h"
+
+#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
+
+struct descriptor_priv {
+ struct list_head list;
+ u32 interface_version;
+ u32 size;
+};
+static LIST_HEAD(wmi_list);
+
+bool dell_wmi_get_interface_version(u32 *version)
+{
+ struct descriptor_priv *priv;
+
+ priv = list_first_entry_or_null(&wmi_list,
+ struct descriptor_priv,
+ list);
+ if (!priv)
+ return false;
+ *version = priv->interface_version;
+ return true;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_get_interface_version);
+
+bool dell_wmi_get_size(u32 *size)
+{
+ struct descriptor_priv *priv;
+
+ priv = list_first_entry_or_null(&wmi_list,
+ struct descriptor_priv,
+ list);
+ if (!priv)
+ return false;
+ *size = priv->size;
+ return true;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_get_size);
+
+/*
+ * 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 or 32768
+ */
+static int dell_wmi_descriptor_probe(struct wmi_device *wdev)
+{
+ union acpi_object *obj = NULL;
+ struct descriptor_priv *priv;
+ u32 *buffer;
+ int ret;
+
+ obj = wmidev_block_query(wdev, 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;
+ }
+
+ /* Although it's not technically a failure, this would lead to
+ * unexpected behavior
+ */
+ if (obj->buffer.length != 128) {
+ dev_err(&wdev->dev,
+ "Dell descriptor buffer has unexpected length (%d)\n",
+ obj->buffer.length);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ buffer = (u32 *)obj->buffer.pointer;
+
+ if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
+ dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
+ buffer);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (buffer[2] != 0 && buffer[2] != 1)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
+ buffer[2]);
+
+ if (buffer[3] != 4096 && buffer[3] != 32768)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unexpected buffer length (%u)\n",
+ buffer[3]);
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(struct descriptor_priv),
+ GFP_KERNEL);
+
+ priv->interface_version = buffer[2];
+ priv->size = buffer[3];
+ ret = 0;
+ dev_set_drvdata(&wdev->dev, priv);
+ list_add_tail(&priv->list, &wmi_list);
+
+ dev_dbg(&wdev->dev, "Detected Dell WMI interface version %u and buffer size %u\n",
+ priv->interface_version, priv->size);
+
+out:
+ kfree(obj);
+ return ret;
+}
+
+static int dell_wmi_descriptor_remove(struct wmi_device *wdev)
+{
+ struct descriptor_priv *priv = dev_get_drvdata(&wdev->dev);
+
+ list_del(&priv->list);
+ return 0;
+}
+
+static const struct wmi_device_id dell_wmi_descriptor_id_table[] = {
+ { .guid_string = DELL_WMI_DESCRIPTOR_GUID },
+ { },
+};
+
+static struct wmi_driver dell_wmi_descriptor_driver = {
+ .driver = {
+ .name = "dell-wmi-descriptor",
+ },
+ .probe = dell_wmi_descriptor_probe,
+ .remove = dell_wmi_descriptor_remove,
+ .id_table = dell_wmi_descriptor_id_table,
+};
+
+module_wmi_driver(dell_wmi_descriptor_driver);
+
+MODULE_ALIAS("wmi:" DELL_WMI_DESCRIPTOR_GUID);
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_DESCRIPTION("Dell WMI descriptor driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-wmi-descriptor.h b/drivers/platform/x86/dell-wmi-descriptor.h
new file mode 100644
index 000000000000..3e652c6da034
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-descriptor.h
@@ -0,0 +1,18 @@
+/*
+ *
+ * Copyright (c) 2017 Dell Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _DELL_WMI_DESCRIPTOR_H_
+#define _DELL_WMI_DESCRIPTOR_H_
+
+#include <linux/wmi.h>
+
+bool dell_wmi_get_interface_version(u32 *version);
+bool dell_wmi_get_size(u32 *size);
+
+#endif
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index c8c7f4f9326c..90d7e8e55e9b 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -39,6 +39,7 @@
#include <linux/wmi.h>
#include <acpi/video.h>
#include "dell-smbios.h"
+#include "dell-wmi-descriptor.h"
MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -46,7 +47,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;
@@ -54,7 +54,6 @@ MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
struct dell_wmi_priv {
struct input_dev *input_dev;
- u32 interface_version;
};
static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -347,9 +346,9 @@ static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
static void dell_wmi_notify(struct wmi_device *wdev,
union acpi_object *obj)
{
- struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
u16 *buffer_entry, *buffer_end;
acpi_size buffer_size;
+ u32 interface_version;
int len, i;
if (obj->type != ACPI_TYPE_BUFFER) {
@@ -376,7 +375,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,
* So to prevent reading garbage from buffer we will process only first
* one event on devices with WMI interface version 0.
*/
- if (priv->interface_version == 0 && buffer_entry < buffer_end)
+ if (!dell_wmi_get_interface_version(&interface_version)) {
+ pr_warn("WMI descriptor driver not ready or unavailable");
+ return;
+ }
+ if (interface_version == 0 && buffer_entry < buffer_end)
if (buffer_end > buffer_entry + buffer_entry[0] + 1)
buffer_end = buffer_entry + buffer_entry[0] + 1;
@@ -617,79 +620,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 or 32768
- */
-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);
- ret = -EINVAL;
- goto out;
- }
-
- buffer = (u32 *)obj->buffer.pointer;
-
- if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
- dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
- buffer);
- ret = -EINVAL;
- goto out;
- }
-
- if (buffer[2] != 0 && buffer[2] != 1)
- dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
- buffer[2]);
-
- if (desc_buffer[3] != 4096 && desc_buffer[3] != 32768)
- dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\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:
*
@@ -725,7 +655,6 @@ static int dell_wmi_events_set_enabled(bool enable)
static int dell_wmi_probe(struct wmi_device *wdev)
{
struct dell_wmi_priv *priv;
- int err;
priv = devm_kzalloc(
&wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
@@ -733,10 +662,6 @@ static int dell_wmi_probe(struct wmi_device *wdev)
return -ENOMEM;
dev_set_drvdata(&wdev->dev, priv);
- err = dell_wmi_check_descriptor_buffer(wdev);
- if (err)
- return err;
-
return dell_wmi_input_setup(wdev);
}
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 06/14] platform/x86: wmi: Don't allow drivers to get each other's GUIDs
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (4 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 05/14] platform/x86: dell-wmi-descriptor: split WMI descriptor into it's own driver Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 07/14] platform/x86: dell-smbios: only run if proper oem string is detected Mario Limonciello
` (7 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
The only driver using this was dell-wmi, and it really was a hack.
The driver was getting a data attribute from another driver and this
type of action should not be encouraged.
Rather drivers that need to interact with one another should pass
data back and forth via exported functions.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
drivers/platform/x86/wmi.c | 17 -----------------
include/linux/wmi.h | 4 ----
2 files changed, 21 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4d73a87c2ddf..bcb41c1c7f52 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -368,23 +368,6 @@ union acpi_object *wmidev_block_query(struct wmi_device *wdev, u8 instance)
}
EXPORT_SYMBOL_GPL(wmidev_block_query);
-struct wmi_device *wmidev_get_other_guid(struct wmi_device *wdev,
- const char *guid_string)
-{
- struct wmi_block *this_wb = container_of(wdev, struct wmi_block, dev);
- struct wmi_block *other_wb;
-
- if (!find_guid(guid_string, &other_wb))
- return NULL;
-
- if (other_wb->acpi_device != this_wb->acpi_device)
- return NULL;
-
- get_device(&other_wb->dev.dev);
- return &other_wb->dev;
-}
-EXPORT_SYMBOL_GPL(wmidev_get_other_guid);
-
/**
* wmi_set_block - Write to a WMI block
* @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 2cd10c3b89e9..ddee427e0721 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -36,10 +36,6 @@ extern acpi_status wmidev_evaluate_method(struct wmi_device *wdev,
extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
u8 instance);
-/* Gets another device on the same bus. Caller must put_device the result. */
-extern struct wmi_device *wmidev_get_other_guid(struct wmi_device *wdev,
- const char *guid_string);
-
struct wmi_device_id {
const char *guid_string;
};
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 07/14] platform/x86: dell-smbios: only run if proper oem string is detected
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (5 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 06/14] platform/x86: wmi: Don't allow drivers to get each other's GUIDs Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens Mario Limonciello
` (6 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
The proper way to indicate that a system is a 'supported' Dell System
is by the presence of this string in OEM strings.
Allowing the driver to load on non-Dell systems will have undefined
results.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
drivers/platform/x86/dell-smbios.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index e9b1ca07c872..873d1c3f7641 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -172,8 +172,15 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
static int __init dell_smbios_init(void)
{
+ const struct dmi_device *valid;
int ret;
+ valid = dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL);
+ if (!valid) {
+ pr_info("Unable to run on non-Dell system\n");
+ return -ENODEV;
+ }
+
dmi_walk(find_tokens, NULL);
if (!da_tokens) {
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (6 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 07/14] platform/x86: dell-smbios: only run if proper oem string is detected Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 6:54 ` Greg KH
2017-10-07 4:59 ` [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls Mario Limonciello
` (5 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
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.
MAINTAINERS was missing for this driver. Add myself and Pali to
maintainers list for it.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
.../ABI/testing/sysfs-platform-dell-smbios | 16 ++++++
MAINTAINERS | 7 +++
drivers/platform/x86/dell-smbios.c | 64 ++++++++++++++++++++++
3 files changed, 87 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios
diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios b/Documentation/ABI/testing/sysfs-platform-dell-smbios
new file mode 100644
index 000000000000..d97f4bd5bd91
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios
@@ -0,0 +1,16 @@
+What: /sys/devices/platform/<platform>/tokens
+Date: November 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.limonciello@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/MAINTAINERS b/MAINTAINERS
index 659dbeec4191..2e3f2aea0370 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3967,6 +3967,13 @@ M: "Maciej W. Rozycki" <macro@linux-mips.org>
S: Maintained
F: drivers/net/fddi/defxx.*
+DELL SMBIOS DRIVER
+M: Pali Rohár <pali.rohar@gmail.com>
+M: Mario Limonciello <mario.limonciello@dell.com>
+L: platform-driver-x86@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/dell-smbios.*
+
DELL LAPTOP DRIVER
M: Matthew Garrett <mjg59@srcf.ucam.org>
M: Pali Rohár <pali.rohar@gmail.com>
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 873d1c3f7641..7275d1d48190 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -23,6 +23,7 @@
#include <linux/slab.h>
#include <linux/io.h>
#include "../../firmware/dcdbas.h"
+#include <linux/platform_device.h>
#include "dell-smbios.h"
struct calling_interface_structure {
@@ -39,6 +40,7 @@ static DEFINE_MUTEX(buffer_mutex);
static int da_command_address;
static int da_command_code;
static int da_num_tokens;
+static struct platform_device *platform_device;
static struct calling_interface_token *da_tokens;
int dell_smbios_error(int value)
@@ -170,6 +172,40 @@ 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 to_print;
+ int i;
+
+ to_print = min(da_num_tokens, (int)(PAGE_SIZE - 1) / 15);
+ for (i = 0; i < to_print; i++) {
+ 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 struct platform_driver platform_driver = {
+ .driver = {
+ .name = "dell-smbios",
+ },
+};
+
static int __init dell_smbios_init(void)
{
const struct dmi_device *valid;
@@ -197,9 +233,37 @@ static int __init dell_smbios_init(void)
ret = -ENOMEM;
goto fail_buffer;
}
+ ret = platform_driver_register(&platform_driver);
+ if (ret)
+ goto fail_platform_driver;
+
+ platform_device = platform_device_alloc("dell-smbios", 0);
+ if (!platform_device) {
+ ret = -ENOMEM;
+ goto fail_platform_device_alloc;
+ }
+ ret = platform_device_add(platform_device);
+ if (ret)
+ goto fail_platform_device_add;
+ ret = sysfs_create_group(&platform_device->dev.kobj,
+ &smbios_attribute_group);
+ if (ret)
+ goto fail_create_group;
return 0;
+fail_create_group:
+ platform_device_del(platform_device);
+
+fail_platform_device_add:
+ platform_device_put(platform_device);
+
+fail_platform_device_alloc:
+ platform_driver_unregister(&platform_driver);
+
+fail_platform_driver:
+ free_page((unsigned long)buffer);
+
fail_buffer:
kfree(da_tokens);
return ret;
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens
2017-10-07 4:59 ` [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens Mario Limonciello
@ 2017-10-07 6:54 ` Greg KH
2017-10-07 11:56 ` Mario.Limonciello
0 siblings, 1 reply; 31+ messages in thread
From: Greg KH @ 2017-10-07 6:54 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart, Andy Shevchenko, LKML, platform-driver-x86,
Andy Lutomirski, quasisec, pali.rohar, rjw, mjg59, hch
On Fri, Oct 06, 2017 at 11:59:52PM -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.
>
> MAINTAINERS was missing for this driver. Add myself and Pali to
> maintainers list for it.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
> .../ABI/testing/sysfs-platform-dell-smbios | 16 ++++++
> MAINTAINERS | 7 +++
> drivers/platform/x86/dell-smbios.c | 64 ++++++++++++++++++++++
> 3 files changed, 87 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios b/Documentation/ABI/testing/sysfs-platform-dell-smbios
> new file mode 100644
> index 000000000000..d97f4bd5bd91
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios
> @@ -0,0 +1,16 @@
> +What: /sys/devices/platform/<platform>/tokens
> +Date: November 2017
> +KernelVersion: 4.15
> +Contact: "Mario Limonciello" <mario.limonciello@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
That's more than "one value per file" which is what sysfs requires, so
this isn't acceptable, sorry.
> +static ssize_t tokens_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + size_t off = 0;
> + int to_print;
> + int i;
> +
> + to_print = min(da_num_tokens, (int)(PAGE_SIZE - 1) / 15);
> + for (i = 0; i < to_print; i++) {
> + 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);
Huge hint, if you are checking for the max size of the buffer for sysfs,
something is really wrong, and you need to redo your design.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread* RE: [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens
2017-10-07 6:54 ` Greg KH
@ 2017-10-07 11:56 ` Mario.Limonciello
2017-10-07 12:39 ` Greg KH
0 siblings, 1 reply; 31+ messages in thread
From: Mario.Limonciello @ 2017-10-07 11:56 UTC (permalink / raw)
To: greg
Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86, luto,
quasisec, pali.rohar, rjw, mjg59, hch
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Saturday, October 7, 2017 1:54 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Andy Lutomirski <luto@kernel.org>; quasisec@google.com;
> pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for
> SMBIOS tokens
>
> On Fri, Oct 06, 2017 at 11:59:52PM -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.
> >
> > MAINTAINERS was missing for this driver. Add myself and Pali to
> > maintainers list for it.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> > .../ABI/testing/sysfs-platform-dell-smbios | 16 ++++++
> > MAINTAINERS | 7 +++
> > drivers/platform/x86/dell-smbios.c | 64 ++++++++++++++++++++++
> > 3 files changed, 87 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios
> b/Documentation/ABI/testing/sysfs-platform-dell-smbios
> > new file mode 100644
> > index 000000000000..d97f4bd5bd91
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios
> > @@ -0,0 +1,16 @@
> > +What: /sys/devices/platform/<platform>/tokens
> > +Date: November 2017
> > +KernelVersion: 4.15
> > +Contact: "Mario Limonciello" <mario.limonciello@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
>
> That's more than "one value per file" which is what sysfs requires, so
> this isn't acceptable, sorry.
What's more acceptable to you to relay this information?
Binary sysfs attribute and export the structure format in a uapi?
or a collection of sysfs files?
Eg:
tokens/$x/id
tokens/$x/location
tokens/$s/value
I'm guessing the latter is the better way to go.
>
> > +static ssize_t tokens_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + size_t off = 0;
> > + int to_print;
> > + int i;
> > +
> > + to_print = min(da_num_tokens, (int)(PAGE_SIZE - 1) / 15);
> > + for (i = 0; i < to_print; i++) {
> > + 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);
>
> Huge hint, if you are checking for the max size of the buffer for sysfs,
> something is really wrong, and you need to redo your design.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens
2017-10-07 11:56 ` Mario.Limonciello
@ 2017-10-07 12:39 ` Greg KH
0 siblings, 0 replies; 31+ messages in thread
From: Greg KH @ 2017-10-07 12:39 UTC (permalink / raw)
To: Mario.Limonciello
Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86, luto,
quasisec, pali.rohar, rjw, mjg59, hch
On Sat, Oct 07, 2017 at 11:56:04AM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Saturday, October 7, 2017 1:54 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> > Andy Lutomirski <luto@kernel.org>; quasisec@google.com;
> > pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> > Subject: Re: [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for
> > SMBIOS tokens
> >
> > On Fri, Oct 06, 2017 at 11:59:52PM -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.
> > >
> > > MAINTAINERS was missing for this driver. Add myself and Pali to
> > > maintainers list for it.
> > >
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > ---
> > > .../ABI/testing/sysfs-platform-dell-smbios | 16 ++++++
> > > MAINTAINERS | 7 +++
> > > drivers/platform/x86/dell-smbios.c | 64 ++++++++++++++++++++++
> > > 3 files changed, 87 insertions(+)
> > > create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios
> > b/Documentation/ABI/testing/sysfs-platform-dell-smbios
> > > new file mode 100644
> > > index 000000000000..d97f4bd5bd91
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios
> > > @@ -0,0 +1,16 @@
> > > +What: /sys/devices/platform/<platform>/tokens
> > > +Date: November 2017
> > > +KernelVersion: 4.15
> > > +Contact: "Mario Limonciello" <mario.limonciello@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
> >
> > That's more than "one value per file" which is what sysfs requires, so
> > this isn't acceptable, sorry.
>
> What's more acceptable to you to relay this information?
> Binary sysfs attribute and export the structure format in a uapi?
binary sysfs apis are for passing "raw" data to/from firmware/hardware
to userspace, without the kernel knowing anything about the structure
or format of the data. Putting the format in a structure in a uapi
header kind of defeats that purpose :)
> or a collection of sysfs files?
> Eg:
> tokens/$x/id
> tokens/$x/location
> tokens/$s/value
>
> I'm guessing the latter is the better way to go.
Good guess :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (7 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 08/14] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-08 15:48 ` Andy Shevchenko
2017-10-07 4:59 ` [PATCH v5 10/14] platform/x86: dell-smbios: add filtering capability for requests Mario Limonciello
` (4 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
This splits up the dell-smbios driver into two drivers:
* dell-smbios
* dell-smbios-smm
dell-smbios can operate with multiple different dispatcher drivers to
perform SMBIOS operations.
Also modify the interface that dell-laptop and dell-wmi use align to this
model more closely. Rather than a single global buffer being allocated
for all drivers, each driver will allocate and be responsible for it's own
buffer. The pointer will be passed to the calling function and each
dispatcher driver will then internally copy it to the proper location to
perform it's call.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 16 ++-
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/dell-laptop.c | 255 ++++++++++++---------------------
drivers/platform/x86/dell-smbios-smm.c | 136 ++++++++++++++++++
drivers/platform/x86/dell-smbios.c | 123 ++++++++++------
drivers/platform/x86/dell-smbios.h | 13 +-
drivers/platform/x86/dell-wmi.c | 11 +-
8 files changed, 338 insertions(+), 223 deletions(-)
create mode 100644 drivers/platform/x86/dell-smbios-smm.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 2e3f2aea0370..8faf08ebcfee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3974,6 +3974,12 @@ L: platform-driver-x86@vger.kernel.org
S: Maintained
F: drivers/platform/x86/dell-smbios.*
+DELL SMBIOS SMM DRIVER
+M: Mario Limonciello <mario.limonciello@dell.com>
+L: platform-driver-x86@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/dell-smbios-smm.c
+
DELL LAPTOP DRIVER
M: Matthew Garrett <mjg59@srcf.ucam.org>
M: Pali Rohár <pali.rohar@gmail.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7722923c968c..7cc91519bec8 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -93,12 +93,20 @@ config ASUS_LAPTOP
config DELL_SMBIOS
tristate
- select DCDBAS
+ depends on DELL_SMBIOS_SMM
+
+config DELL_SMBIOS_SMM
+ tristate "Dell SMBIOS calling interface (SMM implementation)"
+ depends on DCDBAS
+ default DCDBAS
+ select DELL_SMBIOS
---help---
- This module provides common functions for kernel modules using
- Dell SMBIOS.
+ This provides an implementation for the Dell SMBIOS calling interface
+ communicated over SMI/SMM.
- If you have a Dell laptop, say Y or M here.
+ If you have a Dell computer from <=2017 you should say Y or M here.
+ If you aren't sure and this module doesn't work for your computer
+ it just won't load.
config DELL_LAPTOP
tristate "Dell Laptop Extras"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8636f5d3424f..e743615241f8 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ 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_SMBIOS_SMM) += dell-smbios-smm.o
obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
obj-$(CONFIG_DELL_WMI) += dell-wmi.o
obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index f42159fd2031..2f65f5c15d53 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -85,6 +85,7 @@ static struct platform_driver platform_driver = {
}
};
+static struct calling_interface_buffer *buffer;
static struct platform_device *platform_device;
static struct backlight_device *dell_backlight_device;
static struct rfkill *wifi_rfkill;
@@ -283,6 +284,23 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
{ }
};
+int dell_send_request(u16 class, u16 select, u32 arg0, u32 arg1, u32 arg2,
+ u32 arg3)
+{
+ int ret;
+
+ buffer->class = class;
+ buffer->select = select;
+ buffer->input[0] = arg0;
+ buffer->input[1] = arg1;
+ buffer->input[2] = arg2;
+ buffer->input[3] = arg3;
+ ret = dell_smbios_call(buffer);
+ if (ret != 0)
+ return ret;
+ return dell_smbios_error(buffer->output[0]);
+}
+
/*
* Derived from information in smbios-wireless-ctl:
*
@@ -405,7 +423,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
static int dell_rfkill_set(void *data, bool blocked)
{
- struct calling_interface_buffer *buffer;
int disable = blocked ? 1 : 0;
unsigned long radio = (unsigned long)data;
int hwswitch_bit = (unsigned long)data - 1;
@@ -413,20 +430,14 @@ static int dell_rfkill_set(void *data, bool blocked)
int status;
int ret;
- buffer = dell_smbios_get_buffer();
-
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0, 0, 0, 0);
+ if (ret)
+ return ret;
status = buffer->output[1];
- if (ret != 0)
- goto out;
-
- dell_smbios_clear_buffer();
-
- buffer->input[0] = 0x2;
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0x2, 0, 0, 0);
+ if (ret)
+ return ret;
hwswitch = buffer->output[1];
/* If the hardware switch controls this radio, and the hardware
@@ -435,28 +446,19 @@ static int dell_rfkill_set(void *data, bool blocked)
(status & BIT(0)) && !(status & BIT(16)))
disable = 1;
- dell_smbios_clear_buffer();
-
- buffer->input[0] = (1 | (radio<<8) | (disable << 16));
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
-
- out:
- dell_smbios_release_buffer();
- return dell_smbios_error(ret);
+ ret = dell_send_request(17, 11, (1 | (radio<<8) | (disable << 16)),
+ 0, 0, 0);
+ return ret;
}
-/* Must be called with the buffer held */
static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
- int status,
- struct calling_interface_buffer *buffer)
+ int status)
{
if (status & BIT(0)) {
/* Has hw-switch, sync sw_state to BIOS */
int block = rfkill_blocked(rfkill);
- dell_smbios_clear_buffer();
- buffer->input[0] = (1 | (radio << 8) | (block << 16));
- dell_smbios_send_request(17, 11);
+ dell_send_request(17, 11, (1 | (radio << 8) | (block << 16)),
+ 0, 0, 0);
} else {
/* No hw-switch, sync BIOS state to sw_state */
rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
@@ -472,32 +474,21 @@ static void dell_rfkill_update_hw_state(struct rfkill *rfkill, int radio,
static void dell_rfkill_query(struct rfkill *rfkill, void *data)
{
- struct calling_interface_buffer *buffer;
int radio = ((unsigned long)data & 0xF);
int hwswitch;
int status;
int ret;
- buffer = dell_smbios_get_buffer();
-
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0, 0, 0, 0);
status = buffer->output[1];
if (ret != 0 || !(status & BIT(0))) {
- dell_smbios_release_buffer();
return;
}
- dell_smbios_clear_buffer();
-
- buffer->input[0] = 0x2;
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0x2, 0, 0, 0);
hwswitch = buffer->output[1];
- dell_smbios_release_buffer();
-
if (ret != 0)
return;
@@ -513,27 +504,21 @@ static struct dentry *dell_laptop_dir;
static int dell_debugfs_show(struct seq_file *s, void *data)
{
- struct calling_interface_buffer *buffer;
int hwswitch_state;
int hwswitch_ret;
int status;
int ret;
- buffer = dell_smbios_get_buffer();
-
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0, 0, 0, 0);
+ if (ret)
+ return ret;
status = buffer->output[1];
- dell_smbios_clear_buffer();
-
- buffer->input[0] = 0x2;
- dell_smbios_send_request(17, 11);
- hwswitch_ret = buffer->output[0];
+ hwswitch_ret = dell_send_request(17, 11, 0x2, 0, 0, 0);
+ if (hwswitch_ret)
+ return hwswitch_ret;
hwswitch_state = buffer->output[1];
- dell_smbios_release_buffer();
-
seq_printf(s, "return:\t%d\n", ret);
seq_printf(s, "status:\t0x%X\n", status);
seq_printf(s, "Bit 0 : Hardware switch supported: %lu\n",
@@ -613,46 +598,34 @@ static const struct file_operations dell_debugfs_fops = {
static void dell_update_rfkill(struct work_struct *ignored)
{
- struct calling_interface_buffer *buffer;
int hwswitch = 0;
int status;
int ret;
- buffer = dell_smbios_get_buffer();
-
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0, 0, 0, 0);
status = buffer->output[1];
if (ret != 0)
- goto out;
-
- dell_smbios_clear_buffer();
+ return;
- buffer->input[0] = 0x2;
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0x2, 0, 0, 0);
if (ret == 0 && (status & BIT(0)))
hwswitch = buffer->output[1];
if (wifi_rfkill) {
dell_rfkill_update_hw_state(wifi_rfkill, 1, status, hwswitch);
- dell_rfkill_update_sw_state(wifi_rfkill, 1, status, buffer);
+ dell_rfkill_update_sw_state(wifi_rfkill, 1, status);
}
if (bluetooth_rfkill) {
dell_rfkill_update_hw_state(bluetooth_rfkill, 2, status,
hwswitch);
- dell_rfkill_update_sw_state(bluetooth_rfkill, 2, status,
- buffer);
+ dell_rfkill_update_sw_state(bluetooth_rfkill, 2, status);
}
if (wwan_rfkill) {
dell_rfkill_update_hw_state(wwan_rfkill, 3, status, hwswitch);
- dell_rfkill_update_sw_state(wwan_rfkill, 3, status, buffer);
+ dell_rfkill_update_sw_state(wwan_rfkill, 3, status);
}
-
- out:
- dell_smbios_release_buffer();
}
static DECLARE_DELAYED_WORK(dell_rfkill_work, dell_update_rfkill);
@@ -696,7 +669,6 @@ static struct notifier_block dell_laptop_rbtn_notifier = {
static int __init dell_setup_rfkill(void)
{
- struct calling_interface_buffer *buffer;
int status, ret, whitelisted;
const char *product;
@@ -712,11 +684,8 @@ static int __init dell_setup_rfkill(void)
if (!force_rfkill && !whitelisted)
return 0;
- buffer = dell_smbios_get_buffer();
- dell_smbios_send_request(17, 11);
- ret = buffer->output[0];
+ ret = dell_send_request(17, 11, 0, 0, 0, 0);
status = buffer->output[1];
- dell_smbios_release_buffer();
/* dell wireless info smbios call is not supported */
if (ret != 0)
@@ -869,53 +838,42 @@ static void dell_cleanup_rfkill(void)
static int dell_send_intensity(struct backlight_device *bd)
{
- struct calling_interface_buffer *buffer;
struct calling_interface_token *token;
int ret;
+ u8 sel;
token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
if (!token)
return -ENODEV;
- buffer = dell_smbios_get_buffer();
- buffer->input[0] = token->location;
- buffer->input[1] = bd->props.brightness;
-
if (power_supply_is_system_supplied() > 0)
- dell_smbios_send_request(1, 2);
+ sel = 2;
else
- dell_smbios_send_request(1, 1);
+ sel = 1;
+ ret = dell_send_request(1, sel, token->location, bd->props.brightness,
+ 0, 0);
- ret = dell_smbios_error(buffer->output[0]);
-
- dell_smbios_release_buffer();
return ret;
}
static int dell_get_intensity(struct backlight_device *bd)
{
- struct calling_interface_buffer *buffer;
struct calling_interface_token *token;
int ret;
+ u8 sel;
token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
if (!token)
return -ENODEV;
- buffer = dell_smbios_get_buffer();
- buffer->input[0] = token->location;
-
if (power_supply_is_system_supplied() > 0)
- dell_smbios_send_request(0, 2);
+ sel = 2;
else
- dell_smbios_send_request(0, 1);
+ sel = 1;
+ ret = dell_send_request(0, sel, token->location, 0, 0, 0);
- if (buffer->output[0])
- ret = dell_smbios_error(buffer->output[0]);
- else
+ if (ret == 0)
ret = buffer->output[1];
-
- dell_smbios_release_buffer();
return ret;
}
@@ -1179,20 +1137,12 @@ static DEFINE_MUTEX(kbd_led_mutex);
static int kbd_get_info(struct kbd_info *info)
{
- struct calling_interface_buffer *buffer;
u8 units;
int ret;
- buffer = dell_smbios_get_buffer();
-
- buffer->input[0] = 0x0;
- dell_smbios_send_request(4, 11);
- ret = buffer->output[0];
-
- if (ret) {
- ret = dell_smbios_error(ret);
- goto out;
- }
+ ret = dell_send_request(4, 11, 0, 0, 0, 0);
+ if (ret)
+ return ret;
info->modes = buffer->output[1] & 0xFFFF;
info->type = (buffer->output[1] >> 24) & 0xFF;
@@ -1209,8 +1159,6 @@ static int kbd_get_info(struct kbd_info *info)
if (units & BIT(3))
info->days = (buffer->output[3] >> 24) & 0xFF;
- out:
- dell_smbios_release_buffer();
return ret;
}
@@ -1269,19 +1217,11 @@ static int kbd_set_level(struct kbd_state *state, u8 level)
static int kbd_get_state(struct kbd_state *state)
{
- struct calling_interface_buffer *buffer;
int ret;
- buffer = dell_smbios_get_buffer();
-
- buffer->input[0] = 0x1;
- dell_smbios_send_request(4, 11);
- ret = buffer->output[0];
-
- if (ret) {
- ret = dell_smbios_error(ret);
- goto out;
- }
+ ret = dell_send_request(4, 11, 0x1, 0, 0, 0);
+ if (ret)
+ return ret;
state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
if (state->mode_bit != 0)
@@ -1296,31 +1236,26 @@ static int kbd_get_state(struct kbd_state *state)
state->timeout_value_ac = (buffer->output[2] >> 24) & 0x3F;
state->timeout_unit_ac = (buffer->output[2] >> 30) & 0x3;
- out:
- dell_smbios_release_buffer();
return ret;
}
static int kbd_set_state(struct kbd_state *state)
{
- struct calling_interface_buffer *buffer;
int ret;
+ u32 input1;
+ u32 input2;
+
+ input1 = BIT(state->mode_bit) & 0xFFFF;
+ input1 |= (state->triggers & 0xFF) << 16;
+ input1 |= (state->timeout_value & 0x3F) << 24;
+ input1 |= (state->timeout_unit & 0x3) << 30;
+ input2 = state->als_setting & 0xFF;
+ input2 |= (state->level & 0xFF) << 16;
+ input2 |= (state->timeout_value_ac & 0x3F) << 24;
+ input2 |= (state->timeout_unit_ac & 0x3) << 30;
+ ret = dell_send_request(4, 11, 0x2, input1, input2, 0);
- buffer = dell_smbios_get_buffer();
- buffer->input[0] = 0x2;
- buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
- buffer->input[1] |= (state->triggers & 0xFF) << 16;
- buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
- buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
- buffer->input[2] = state->als_setting & 0xFF;
- buffer->input[2] |= (state->level & 0xFF) << 16;
- buffer->input[2] |= (state->timeout_value_ac & 0x3F) << 24;
- buffer->input[2] |= (state->timeout_unit_ac & 0x3) << 30;
- dell_smbios_send_request(4, 11);
- ret = buffer->output[0];
- dell_smbios_release_buffer();
-
- return dell_smbios_error(ret);
+ return ret;
}
static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
@@ -1345,7 +1280,6 @@ static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
static int kbd_set_token_bit(u8 bit)
{
- struct calling_interface_buffer *buffer;
struct calling_interface_token *token;
int ret;
@@ -1356,19 +1290,13 @@ static int kbd_set_token_bit(u8 bit)
if (!token)
return -EINVAL;
- buffer = dell_smbios_get_buffer();
- buffer->input[0] = token->location;
- buffer->input[1] = token->value;
- dell_smbios_send_request(1, 0);
- ret = buffer->output[0];
- dell_smbios_release_buffer();
+ ret = dell_send_request(1, 0, token->location, token->value, 0, 0);
- return dell_smbios_error(ret);
+ return ret;
}
static int kbd_get_token_bit(u8 bit)
{
- struct calling_interface_buffer *buffer;
struct calling_interface_token *token;
int ret;
int val;
@@ -1380,15 +1308,12 @@ static int kbd_get_token_bit(u8 bit)
if (!token)
return -EINVAL;
- buffer = dell_smbios_get_buffer();
- buffer->input[0] = token->location;
- dell_smbios_send_request(0, 0);
- ret = buffer->output[0];
+
+ ret = dell_send_request(0, 0, token->location, 0, 0, 0);
val = buffer->output[1];
- dell_smbios_release_buffer();
if (ret)
- return dell_smbios_error(ret);
+ return ret;
return (val == token->value);
}
@@ -2102,7 +2027,6 @@ static struct notifier_block dell_laptop_notifier = {
int dell_micmute_led_set(int state)
{
- struct calling_interface_buffer *buffer;
struct calling_interface_token *token;
if (state == 0)
@@ -2115,11 +2039,7 @@ int dell_micmute_led_set(int state)
if (!token)
return -ENODEV;
- buffer = dell_smbios_get_buffer();
- buffer->input[0] = token->location;
- buffer->input[1] = token->value;
- dell_smbios_send_request(1, 0);
- dell_smbios_release_buffer();
+ dell_send_request(1, 0, token->location, token->value, 0, 0);
return state;
}
@@ -2127,7 +2047,6 @@ EXPORT_SYMBOL_GPL(dell_micmute_led_set);
static int __init dell_init(void)
{
- struct calling_interface_buffer *buffer;
struct calling_interface_token *token;
int max_intensity = 0;
int ret;
@@ -2158,6 +2077,10 @@ static int __init dell_init(void)
goto fail_rfkill;
}
+ buffer = (void *)get_zeroed_page(GFP_KERNEL);
+ if (!buffer)
+ goto fail_buffer;
+
if (quirks && quirks->touchpad_led)
touchpad_led_init(&platform_device->dev);
@@ -2175,12 +2098,9 @@ static int __init dell_init(void)
token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
if (token) {
- buffer = dell_smbios_get_buffer();
- buffer->input[0] = token->location;
- dell_smbios_send_request(0, 2);
- if (buffer->output[0] == 0)
+ ret = dell_send_request(0, 2, token->location, 0, 0, 0);
+ if (ret)
max_intensity = buffer->output[3];
- dell_smbios_release_buffer();
}
if (max_intensity) {
@@ -2214,6 +2134,8 @@ static int __init dell_init(void)
fail_get_brightness:
backlight_device_unregister(dell_backlight_device);
fail_backlight:
+ free_page((unsigned long)buffer);
+fail_buffer:
dell_cleanup_rfkill();
fail_rfkill:
platform_device_del(platform_device);
@@ -2233,6 +2155,7 @@ static void __exit dell_exit(void)
touchpad_led_exit();
kbd_led_exit();
backlight_device_unregister(dell_backlight_device);
+ free_page((unsigned long)buffer);
dell_cleanup_rfkill();
if (platform_device) {
platform_device_unregister(platform_device);
diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c
new file mode 100644
index 000000000000..223531e43fea
--- /dev/null
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -0,0 +1,136 @@
+/*
+ * SMI methods for use with dell-smbios
+ *
+ * Copyright (c) Red Hat <mjg@redhat.com>
+ * Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ * Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
+ * Copyright (c) 2017 Dell Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/gfp.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include "../../firmware/dcdbas.h"
+#include "dell-smbios.h"
+
+static int da_command_address;
+static int da_command_code;
+static struct calling_interface_buffer *buffer;
+struct platform_device *platform_device;
+static DEFINE_MUTEX(smm_mutex);
+
+static const struct dmi_system_id dell_device_table[] __initconst = {
+ {
+ .ident = "Dell laptop",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_CHASSIS_TYPE, "8"),
+ },
+ },
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_CHASSIS_TYPE, "9"), /*Laptop*/
+ },
+ },
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_CHASSIS_TYPE, "10"), /*Notebook*/
+ },
+ },
+ {
+ .ident = "Dell Computer Corporation",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer Corporation"),
+ DMI_MATCH(DMI_CHASSIS_TYPE, "8"),
+ },
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(dmi, dell_device_table);
+
+int dell_smbios_smm_call(struct calling_interface_buffer *input)
+{
+ struct smi_cmd command;
+ size_t size;
+
+ size = sizeof(struct calling_interface_buffer);
+ 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;
+
+ mutex_lock(&smm_mutex);
+ memcpy(buffer, input, size);
+ dcdbas_smi_request(&command);
+ memcpy(input, buffer, size);
+ mutex_unlock(&smm_mutex);
+ return 0;
+}
+
+static int __init dell_smbios_smm_init(void)
+{
+ int ret;
+ /*
+ * 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)
+ return -ENOMEM;
+ dell_smbios_get_smm_address(&da_command_address, &da_command_code);
+
+ platform_device = platform_device_alloc("dell-smbios", 1);
+ if (!platform_device) {
+ ret = -ENOMEM;
+ goto fail_platform_device_alloc;
+ }
+
+ ret = platform_device_add(platform_device);
+ if (ret)
+ goto fail_platform_device_add;
+
+ ret = dell_smbios_register_device(&platform_device->dev,
+ &dell_smbios_smm_call);
+ if (ret)
+ goto fail_register;
+
+ return 0;
+
+fail_register:
+ platform_device_del(platform_device);
+
+fail_platform_device_add:
+ platform_device_put(platform_device);
+
+fail_platform_device_alloc:
+ free_page((unsigned long)buffer);
+ return ret;
+}
+
+static void __exit dell_smbios_smm_exit(void)
+{
+ if (platform_device) {
+ dell_smbios_unregister_device(&platform_device->dev);
+ platform_device_unregister(platform_device);
+ free_page((unsigned long)buffer);
+ }
+}
+
+subsys_initcall(dell_smbios_smm_init);
+module_exit(dell_smbios_smm_exit);
+
+MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_DESCRIPTION("Dell SMBIOS communications over SMI");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 7275d1d48190..2f90ba5346bc 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -18,11 +18,9 @@
#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/platform_device.h>
#include "dell-smbios.h"
@@ -34,14 +32,27 @@ struct calling_interface_structure {
struct calling_interface_token tokens[];
} __packed;
-static struct calling_interface_buffer *buffer;
-static DEFINE_MUTEX(buffer_mutex);
-
static int da_command_address;
static int da_command_code;
static int da_num_tokens;
static struct platform_device *platform_device;
static struct calling_interface_token *da_tokens;
+static DEFINE_MUTEX(smbios_mutex);
+
+struct smbios_device {
+ struct list_head list;
+ struct device *device;
+ int (*call_fn)(struct calling_interface_buffer *);
+};
+
+static LIST_HEAD(smbios_device_list);
+
+void dell_smbios_get_smm_address(int *address, int *code)
+{
+ *address = da_command_address;
+ *code = da_command_code;
+}
+EXPORT_SYMBOL_GPL(dell_smbios_get_smm_address);
int dell_smbios_error(int value)
{
@@ -58,42 +69,71 @@ int dell_smbios_error(int value)
}
EXPORT_SYMBOL_GPL(dell_smbios_error);
-struct calling_interface_buffer *dell_smbios_get_buffer(void)
-{
- mutex_lock(&buffer_mutex);
- dell_smbios_clear_buffer();
- return buffer;
-}
-EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
-
-void dell_smbios_clear_buffer(void)
+int dell_smbios_register_device(struct device *d, void *call_fn)
{
- memset(buffer, 0, sizeof(struct calling_interface_buffer));
+ struct smbios_device *priv;
+
+ priv = devm_kzalloc(d, sizeof(struct smbios_device), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ get_device(d);
+ priv->device = d;
+ priv->call_fn = call_fn;
+ mutex_lock(&smbios_mutex);
+ list_add_tail(&priv->list, &smbios_device_list);
+ mutex_unlock(&smbios_mutex);
+ dev_dbg(d, "Added device: %s\n", d->driver->name);
+ return 0;
}
-EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
+EXPORT_SYMBOL_GPL(dell_smbios_register_device);
-void dell_smbios_release_buffer(void)
+void dell_smbios_unregister_device(struct device *d)
{
- mutex_unlock(&buffer_mutex);
+ struct smbios_device *priv;
+
+ mutex_lock(&smbios_mutex);
+ list_for_each_entry(priv, &smbios_device_list, list) {
+ if (priv->device == d) {
+ list_del(&priv->list);
+ put_device(d);
+ break;
+ }
+ };
+ mutex_unlock(&smbios_mutex);
+ dev_dbg(d, "Remove device: %s\n", d->driver->name);
}
-EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);
+EXPORT_SYMBOL_GPL(dell_smbios_unregister_device);
-void dell_smbios_send_request(int class, int select)
+int dell_smbios_call(struct calling_interface_buffer *buffer)
{
- struct smi_cmd command;
+ int (*call_fn)(struct calling_interface_buffer *) = NULL;
+ struct device *selected_dev = NULL;
+ struct smbios_device *priv;
+ int ret;
- 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;
+ mutex_lock(&smbios_mutex);
+ list_for_each_entry(priv, &smbios_device_list, list) {
+ if (!selected_dev || priv->device->id >= selected_dev->id) {
+ dev_dbg(priv->device, "Trying device ID: %d\n",
+ priv->device->id);
+ call_fn = priv->call_fn;
+ selected_dev = priv->device;
+ }
+ };
+
+ if (!selected_dev) {
+ ret = -ENODEV;
+ pr_err("No dell-smbios drivers are loaded\n");
+ goto out_smbios_call;
+ }
- buffer->class = class;
- buffer->select = select;
+ ret = call_fn(buffer);
- dcdbas_smi_request(&command);
+out_smbios_call:
+ mutex_unlock(&smbios_mutex);
+ return ret;
}
-EXPORT_SYMBOL_GPL(dell_smbios_send_request);
+EXPORT_SYMBOL_GPL(dell_smbios_call);
struct calling_interface_token *dell_smbios_find_token(int tokenid)
{
@@ -224,15 +264,6 @@ 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);
- if (!buffer) {
- ret = -ENOMEM;
- goto fail_buffer;
- }
ret = platform_driver_register(&platform_driver);
if (ret)
goto fail_platform_driver;
@@ -262,17 +293,21 @@ static int __init dell_smbios_init(void)
platform_driver_unregister(&platform_driver);
fail_platform_driver:
- free_page((unsigned long)buffer);
-
-fail_buffer:
kfree(da_tokens);
return ret;
}
static void __exit dell_smbios_exit(void)
{
- kfree(da_tokens);
- free_page((unsigned long)buffer);
+ mutex_lock(&smbios_mutex);
+ if (platform_device) {
+ sysfs_remove_group(&platform_device->dev.kobj,
+ &smbios_attribute_group);
+ platform_device_unregister(platform_device);
+ platform_driver_unregister(&platform_driver);
+ kfree(da_tokens);
+ }
+ mutex_unlock(&smbios_mutex);
}
subsys_initcall(dell_smbios_init);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 45cbc2292cd3..fe28964da499 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -16,6 +16,8 @@
#ifndef _DELL_SMBIOS_H_
#define _DELL_SMBIOS_H_
+#include <linux/device.h>
+
struct notifier_block;
/* This structure will be modified by the firmware when we enter
@@ -37,12 +39,13 @@ struct calling_interface_token {
};
};
-int dell_smbios_error(int value);
-struct calling_interface_buffer *dell_smbios_get_buffer(void);
-void dell_smbios_clear_buffer(void);
-void dell_smbios_release_buffer(void);
-void dell_smbios_send_request(int class, int select);
+int dell_smbios_register_device(struct device *d, void *call_fn);
+void dell_smbios_unregister_device(struct device *d);
+
+void dell_smbios_get_smm_address(int *address, int *command);
+int dell_smbios_error(int value);
+int dell_smbios_call(struct calling_interface_buffer *buffer);
struct calling_interface_token *dell_smbios_find_token(int tokenid);
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 90d7e8e55e9b..b74f48d17116 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -641,13 +641,16 @@ static int dell_wmi_events_set_enabled(bool enable)
struct calling_interface_buffer *buffer;
int ret;
- buffer = dell_smbios_get_buffer();
+ buffer = (void *)get_zeroed_page(GFP_KERNEL);
+ buffer->class = 17;
+ buffer->select = 3;
buffer->input[0] = 0x10000;
buffer->input[1] = 0x51534554;
buffer->input[3] = enable;
- dell_smbios_send_request(17, 3);
- ret = buffer->output[0];
- dell_smbios_release_buffer();
+ ret = dell_smbios_call(buffer);
+ if (ret == 0)
+ ret = buffer->output[0];
+ free_page((unsigned long)buffer);
return dell_smbios_error(ret);
}
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls
2017-10-07 4:59 ` [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls Mario Limonciello
@ 2017-10-08 15:48 ` Andy Shevchenko
2017-10-08 18:13 ` Andy Shevchenko
0 siblings, 1 reply; 31+ messages in thread
From: Andy Shevchenko @ 2017-10-08 15:48 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart@infradead.org, LKML, Platform Driver, Andy Lutomirski,
quasisec, Pali Rohár, Rafael J. Wysocki, mjg59,
Christoph Hellwig, Greg KH
On Sat, Oct 7, 2017 at 7:59 AM, Mario Limonciello
<mario.limonciello@dell.com> wrote:
> This splits up the dell-smbios driver into two drivers:
> * dell-smbios
> * dell-smbios-smm
>
> dell-smbios can operate with multiple different dispatcher drivers to
> perform SMBIOS operations.
>
> Also modify the interface that dell-laptop and dell-wmi use align to this
> model more closely. Rather than a single global buffer being allocated
> for all drivers, each driver will allocate and be responsible for it's own
> buffer. The pointer will be passed to the calling function and each
> dispatcher driver will then internally copy it to the proper location to
> perform it's call.
> config DELL_SMBIOS
> tristate
> + depends on DELL_SMBIOS_SMM
> +
If the above dependency is true (which I'm quite sure not) this split
should never happen.
> +config DELL_SMBIOS_SMM
> + tristate "Dell SMBIOS calling interface (SMM implementation)"
> + depends on DCDBAS
> + default DCDBAS
> + select DELL_SMBIOS
> ---help---
> + This provides an implementation for the Dell SMBIOS calling interface
> + communicated over SMI/SMM.
>
> + If you have a Dell computer from <=2017 you should say Y or M here.
> + If you aren't sure and this module doesn't work for your computer
> + it just won't load.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls
2017-10-08 15:48 ` Andy Shevchenko
@ 2017-10-08 18:13 ` Andy Shevchenko
2017-10-08 21:45 ` Mario.Limonciello
0 siblings, 1 reply; 31+ messages in thread
From: Andy Shevchenko @ 2017-10-08 18:13 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart@infradead.org, LKML, Platform Driver, Andy Lutomirski,
quasisec, Pali Rohár, Rafael J. Wysocki, mjg59,
Christoph Hellwig, Greg KH
On Sun, Oct 8, 2017 at 6:48 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Oct 7, 2017 at 7:59 AM, Mario Limonciello
> <mario.limonciello@dell.com> wrote:
>> This splits up the dell-smbios driver into two drivers:
>> * dell-smbios
>> * dell-smbios-smm
>>
>> dell-smbios can operate with multiple different dispatcher drivers to
>> perform SMBIOS operations.
>>
>> Also modify the interface that dell-laptop and dell-wmi use align to this
>> model more closely. Rather than a single global buffer being allocated
>> for all drivers, each driver will allocate and be responsible for it's own
>> buffer. The pointer will be passed to the calling function and each
>> dispatcher driver will then internally copy it to the proper location to
>> perform it's call.
>
>> config DELL_SMBIOS
>> tristate
>
>> + depends on DELL_SMBIOS_SMM
>> +
>
> If the above dependency is true (which I'm quite sure not) this split
> should never happen.
Hmm... One more thought here.
Which is library and which is not? IOW the question is "can
DELL_SMBIOS be a standalone working module"?
>
>> +config DELL_SMBIOS_SMM
>> + tristate "Dell SMBIOS calling interface (SMM implementation)"
>> + depends on DCDBAS
>> + default DCDBAS
>> + select DELL_SMBIOS
>> ---help---
>> + This provides an implementation for the Dell SMBIOS calling interface
>> + communicated over SMI/SMM.
>>
>> + If you have a Dell computer from <=2017 you should say Y or M here.
>> + If you aren't sure and this module doesn't work for your computer
>> + it just won't load.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls
2017-10-08 18:13 ` Andy Shevchenko
@ 2017-10-08 21:45 ` Mario.Limonciello
2017-10-08 23:10 ` Andy Shevchenko
0 siblings, 1 reply; 31+ messages in thread
From: Mario.Limonciello @ 2017-10-08 21:45 UTC (permalink / raw)
To: andy.shevchenko
Cc: dvhart, linux-kernel, platform-driver-x86, luto, quasisec,
pali.rohar, rjw, mjg59, hch, greg
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Sunday, October 8, 2017 1:13 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; LKML <linux-kernel@vger.kernel.org>; Platform Driver
> <platform-driver-x86@vger.kernel.org>; Andy Lutomirski <luto@kernel.org>;
> quasisec@google.com; Pali Rohár <pali.rohar@gmail.com>; Rafael J. Wysocki
> <rjw@rjwysocki.net>; mjg59@google.com; Christoph Hellwig <hch@lst.de>;
> Greg KH <greg@kroah.com>
> Subject: Re: [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for
> SMM calls
>
> On Sun, Oct 8, 2017 at 6:48 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Oct 7, 2017 at 7:59 AM, Mario Limonciello
> > <mario.limonciello@dell.com> wrote:
> >> This splits up the dell-smbios driver into two drivers:
> >> * dell-smbios
> >> * dell-smbios-smm
> >>
> >> dell-smbios can operate with multiple different dispatcher drivers to
> >> perform SMBIOS operations.
> >>
> >> Also modify the interface that dell-laptop and dell-wmi use align to this
> >> model more closely. Rather than a single global buffer being allocated
> >> for all drivers, each driver will allocate and be responsible for it's own
> >> buffer. The pointer will be passed to the calling function and each
> >> dispatcher driver will then internally copy it to the proper location to
> >> perform it's call.
> >
> >> config DELL_SMBIOS
> >> tristate
> >
> >> + depends on DELL_SMBIOS_SMM
> >> +
> >
> > If the above dependency is true (which I'm quite sure not) this split
> > should never happen.
>
> Hmm... One more thought here.
>
> Which is library and which is not? IOW the question is "can
> DELL_SMBIOS be a standalone working module"?
>
It can technically be compiled as a standalone module, but it won't do
anything as a standalone module as of this patch.
After the sysfs tokens patch it could be used to provide token information
without the SMM or WMI drivers.
(although this interface will have to change based upon Greg's most recent
feedback).
In terms of transitioning people moving from older kernels to new, it makes
most sense to me that it's automatically selecting DELL_SMBIOS_SMM if you
had it selected previously, but I'll defer to you judgement if the dependency
should be dropped since it can be a standalone module that just doesn't do
anything.
> >
> >> +config DELL_SMBIOS_SMM
> >> + tristate "Dell SMBIOS calling interface (SMM implementation)"
> >> + depends on DCDBAS
> >> + default DCDBAS
> >> + select DELL_SMBIOS
> >> ---help---
> >> + This provides an implementation for the Dell SMBIOS calling interface
> >> + communicated over SMI/SMM.
> >>
> >> + If you have a Dell computer from <=2017 you should say Y or M here.
> >> + If you aren't sure and this module doesn't work for your computer
> >> + it just won't load.
>
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls
2017-10-08 21:45 ` Mario.Limonciello
@ 2017-10-08 23:10 ` Andy Shevchenko
0 siblings, 0 replies; 31+ messages in thread
From: Andy Shevchenko @ 2017-10-08 23:10 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart@infradead.org, linux-kernel@vger.kernel.org,
Platform Driver, Andy Lutomirski, quasisec, Pali Rohár,
Rafael J. Wysocki, mjg59, Christoph Hellwig, Greg KH
On Mon, Oct 9, 2017 at 12:45 AM, <Mario.Limonciello@dell.com> wrote:
>> >> config DELL_SMBIOS
>> >> tristate
>> >
>> >> + depends on DELL_SMBIOS_SMM
>> >> +
>> >
>> > If the above dependency is true (which I'm quite sure not) this split
>> > should never happen.
>>
>> Hmm... One more thought here.
>>
>> Which is library and which is not? IOW the question is "can
>> DELL_SMBIOS be a standalone working module"?
>>
>
> It can technically be compiled as a standalone module, but it won't do
> anything as a standalone module as of this patch.
>
> After the sysfs tokens patch it could be used to provide token information
> without the SMM or WMI drivers.
> (although this interface will have to change based upon Greg's most recent
> feedback).
>
> In terms of transitioning people moving from older kernels to new, it makes
> most sense to me that it's automatically selecting DELL_SMBIOS_SMM if you
> had it selected previously, but I'll defer to you judgement if the dependency
> should be dropped since it can be a standalone module that just doesn't do
> anything.
Okay, if we can call it "a library" then remove the reverse dependency
as I mentioned in first comment.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5 10/14] platform/x86: dell-smbios: add filtering capability for requests
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (8 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 7:43 ` Greg KH
2017-10-07 4:59 ` [PATCH v5 11/14] platform/x86: dell-smbios-wmi: Add new WMI dispatcher driver Mario Limonciello
` (3 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
There are some categories of tokens and SMBIOS calls that it makes
sense to protect userspace from accessing. These are calls that
may write to one time use fields or activate hardware debugging
capabilities. They are not intended for general purpose use.
This same functionality may be be later extended to also intercept
calls that may cause kernel functionality to get out of sync if
the same functions are used by other drivers.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
drivers/platform/x86/dell-smbios.c | 76 ++++++++++++++++++++++++++++++++++++++
drivers/platform/x86/dell-smbios.h | 2 +
2 files changed, 78 insertions(+)
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 2f90ba5346bc..d1908f159be3 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -32,6 +32,7 @@ struct calling_interface_structure {
struct calling_interface_token tokens[];
} __packed;
+static u32 da_supported_commands;
static int da_command_address;
static int da_command_code;
static int da_num_tokens;
@@ -45,6 +46,14 @@ struct smbios_device {
int (*call_fn)(struct calling_interface_buffer *);
};
+static u32 token_black[] = {
+ 0x0175, 0x0176, 0x0195, 0x0196, 0x0197, 0x01DC, 0x01DD, 0x027D, 0x027E,
+ 0x027F, 0x0280, 0x0281, 0x0282, 0x0283, 0x0284, 0x02E3, 0x02FF, 0x0300,
+ 0x0301, 0x0302, 0x0325, 0x0326, 0x0332, 0x0333, 0x0334, 0x0335, 0x0350,
+ 0x0363, 0x0368, 0x03F6, 0x03F7, 0x049E, 0x049F, 0x04A0, 0x04A1, 0x04A2,
+ 0x04A3, 0x04E6, 0x04E7, 0x9000, 0x9001
+};
+
static LIST_HEAD(smbios_device_list);
void dell_smbios_get_smm_address(int *address, int *code)
@@ -104,6 +113,65 @@ void dell_smbios_unregister_device(struct device *d)
}
EXPORT_SYMBOL_GPL(dell_smbios_unregister_device);
+int dell_smbios_call_filter(struct device *d,
+ struct calling_interface_buffer *buffer)
+{
+ int i;
+ int j;
+ u32 t;
+
+ /* can't make calls over 30 */
+ if (buffer->class > 30) {
+ dev_dbg(d, "buffer->class too big: %d\n", buffer->class);
+ return -EINVAL;
+ }
+
+ /* supported calls on the particular system */
+ if (!(da_supported_commands & (1 << buffer->class))) {
+ dev_dbg(d, "invalid command, supported commands: 0x%8x\n",
+ da_supported_commands);
+ return -EINVAL;
+ }
+
+ /* diagonstics, debugging information or write once */
+ if ((buffer->class == 01 && buffer->select == 07) ||
+ (buffer->class == 06 && buffer->select == 05) ||
+ (buffer->class == 11 && buffer->select == 03) ||
+ (buffer->class == 11 && buffer->select == 07) ||
+ (buffer->class == 11 && buffer->select == 11) ||
+ buffer->class == 19) {
+ dev_dbg(d, "blacklisted command: %d/%d\n",
+ buffer->class, buffer->select);
+ return -EINVAL;
+ }
+
+ /* reading/writing tokens*/
+ if ((buffer->class == 0 && buffer->select < 3) ||
+ (buffer->class == 1 && buffer->select < 3)) {
+ for (i = 0; i < da_num_tokens; i++) {
+ if (da_tokens[i].location != buffer->input[0])
+ continue;
+ /*blacklist reading and writing these */
+ t = da_tokens[i].tokenID;
+ if ((t >= 0x4000 && t <= 0x7FFF) ||
+ (t >= 0xA000 && t <= 0xBFFF) ||
+ (t >= 0xEFF0 && t <= 0xEFFF))
+ return -EINVAL;
+ for (j = 0; j < ARRAY_SIZE(token_black); j++)
+ if (t == token_black[j])
+ return -EINVAL;
+ /* token exists and is OK */
+ return 0;
+ }
+ /* token didn't exist */
+ dev_dbg(d, "token at location %u doesn't exist\n",
+ buffer->input[0]);
+ return -EINVAL;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dell_smbios_call_filter);
+
int dell_smbios_call(struct calling_interface_buffer *buffer)
{
int (*call_fn)(struct calling_interface_buffer *) = NULL;
@@ -127,6 +195,13 @@ int dell_smbios_call(struct calling_interface_buffer *buffer)
goto out_smbios_call;
}
+ if (dell_smbios_call_filter(selected_dev, buffer)) {
+ ret = -EINVAL;
+ dev_err(selected_dev, "Invalid call %d/%d:%8x\n",
+ buffer->class, buffer->select, buffer->input[0]);
+ goto out_smbios_call;
+ }
+
ret = call_fn(buffer);
out_smbios_call:
@@ -184,6 +259,7 @@ static void __init parse_da_table(const struct dmi_header *dm)
da_command_address = table->cmdIOAddress;
da_command_code = table->cmdIOCode;
+ da_supported_commands = table->supportedCmds;
new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
sizeof(struct calling_interface_token),
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index fe28964da499..c743c58831e5 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -45,6 +45,8 @@ void dell_smbios_unregister_device(struct device *d);
void dell_smbios_get_smm_address(int *address, int *command);
int dell_smbios_error(int value);
+int dell_smbios_call_filter(struct device *d,
+ struct calling_interface_buffer *buffer);
int dell_smbios_call(struct calling_interface_buffer *buffer);
struct calling_interface_token *dell_smbios_find_token(int tokenid);
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v5 10/14] platform/x86: dell-smbios: add filtering capability for requests
2017-10-07 4:59 ` [PATCH v5 10/14] platform/x86: dell-smbios: add filtering capability for requests Mario Limonciello
@ 2017-10-07 7:43 ` Greg KH
0 siblings, 0 replies; 31+ messages in thread
From: Greg KH @ 2017-10-07 7:43 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart, Andy Shevchenko, LKML, platform-driver-x86,
Andy Lutomirski, quasisec, pali.rohar, rjw, mjg59, hch
On Fri, Oct 06, 2017 at 11:59:54PM -0500, Mario Limonciello wrote:
> There are some categories of tokens and SMBIOS calls that it makes
> sense to protect userspace from accessing. These are calls that
> may write to one time use fields or activate hardware debugging
> capabilities. They are not intended for general purpose use.
>
> This same functionality may be be later extended to also intercept
> calls that may cause kernel functionality to get out of sync if
> the same functions are used by other drivers.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
> drivers/platform/x86/dell-smbios.c | 76 ++++++++++++++++++++++++++++++++++++++
> drivers/platform/x86/dell-smbios.h | 2 +
> 2 files changed, 78 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index 2f90ba5346bc..d1908f159be3 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -32,6 +32,7 @@ struct calling_interface_structure {
> struct calling_interface_token tokens[];
> } __packed;
>
> +static u32 da_supported_commands;
> static int da_command_address;
> static int da_command_code;
> static int da_num_tokens;
> @@ -45,6 +46,14 @@ struct smbios_device {
> int (*call_fn)(struct calling_interface_buffer *);
> };
>
> +static u32 token_black[] = {
> + 0x0175, 0x0176, 0x0195, 0x0196, 0x0197, 0x01DC, 0x01DD, 0x027D, 0x027E,
> + 0x027F, 0x0280, 0x0281, 0x0282, 0x0283, 0x0284, 0x02E3, 0x02FF, 0x0300,
> + 0x0301, 0x0302, 0x0325, 0x0326, 0x0332, 0x0333, 0x0334, 0x0335, 0x0350,
> + 0x0363, 0x0368, 0x03F6, 0x03F7, 0x049E, 0x049F, 0x04A0, 0x04A1, 0x04A2,
> + 0x04A3, 0x04E6, 0x04E7, 0x9000, 0x9001
> +};
Any hint as to what these values represent?
> static LIST_HEAD(smbios_device_list);
>
> void dell_smbios_get_smm_address(int *address, int *code)
> @@ -104,6 +113,65 @@ void dell_smbios_unregister_device(struct device *d)
> }
> EXPORT_SYMBOL_GPL(dell_smbios_unregister_device);
>
> +int dell_smbios_call_filter(struct device *d,
> + struct calling_interface_buffer *buffer)
> +{
> + int i;
> + int j;
> + u32 t;
> +
> + /* can't make calls over 30 */
> + if (buffer->class > 30) {
> + dev_dbg(d, "buffer->class too big: %d\n", buffer->class);
> + return -EINVAL;
> + }
> +
> + /* supported calls on the particular system */
> + if (!(da_supported_commands & (1 << buffer->class))) {
> + dev_dbg(d, "invalid command, supported commands: 0x%8x\n",
> + da_supported_commands);
> + return -EINVAL;
> + }
> +
> + /* diagonstics, debugging information or write once */
> + if ((buffer->class == 01 && buffer->select == 07) ||
> + (buffer->class == 06 && buffer->select == 05) ||
> + (buffer->class == 11 && buffer->select == 03) ||
> + (buffer->class == 11 && buffer->select == 07) ||
> + (buffer->class == 11 && buffer->select == 11) ||
> + buffer->class == 19) {
A structure of class/select that is not allowed might be easier to
maintain over time, right?
> + dev_dbg(d, "blacklisted command: %d/%d\n",
> + buffer->class, buffer->select);
> + return -EINVAL;
> + }
> +
> + /* reading/writing tokens*/
> + if ((buffer->class == 0 && buffer->select < 3) ||
> + (buffer->class == 1 && buffer->select < 3)) {
> + for (i = 0; i < da_num_tokens; i++) {
> + if (da_tokens[i].location != buffer->input[0])
> + continue;
> + /*blacklist reading and writing these */
"/* " ???
thanks,
greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5 11/14] platform/x86: dell-smbios-wmi: Add new WMI dispatcher driver
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (9 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 10/14] platform/x86: dell-smbios: add filtering capability for requests Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 12/14] platform/x86: dell-smbios-smm: test for WSMT Mario Limonciello
` (2 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
The dell-smbios stack only currently uses an SMI interface which grants
direct access to physical memory to the firmware SMM methods via a pointer.
This dispatcher driver adds a WMI-ACPI interface that is detected by WMI
probe and preferred over the SMI interface in dell-smbios.
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.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 16 ++-
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/dell-smbios-wmi.c | 229 +++++++++++++++++++++++++++++++++
4 files changed, 251 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/x86/dell-smbios-wmi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8faf08ebcfee..e7514b616e13 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3980,6 +3980,12 @@ L: platform-driver-x86@vger.kernel.org
S: Maintained
F: drivers/platform/x86/dell-smbios-smm.c
+DELL SMBIOS WMI DRIVER
+M: Mario Limonciello <mario.limonciello@dell.com>
+L: platform-driver-x86@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/dell-smbios-wmi.c
+
DELL LAPTOP DRIVER
M: Matthew Garrett <mjg59@srcf.ucam.org>
M: Pali Rohár <pali.rohar@gmail.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7cc91519bec8..7b0b87777379 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -93,7 +93,21 @@ config ASUS_LAPTOP
config DELL_SMBIOS
tristate
- depends on DELL_SMBIOS_SMM
+ depends on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM
+
+config DELL_SMBIOS_WMI
+ tristate "Dell SMBIOS calling interface (WMI implementation)"
+ depends on ACPI_WMI
+ select DELL_WMI_DESCRIPTOR
+ default ACPI_WMI
+ select DELL_SMBIOS
+ ---help---
+ This provides an implementation for the Dell SMBIOS calling interface
+ communicated over ACPI-WMI.
+
+ If you have a Dell computer from >2007 you should say Y or M here.
+ If you aren't sure and this module doesn't work for your computer
+ it just won't load.
config DELL_SMBIOS_SMM
tristate "Dell SMBIOS calling interface (SMM implementation)"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e743615241f8..1c4234861de0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ 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_SMBIOS_WMI) += dell-smbios-wmi.o
obj-$(CONFIG_DELL_SMBIOS_SMM) += dell-smbios-smm.o
obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
obj-$(CONFIG_DELL_WMI) += dell-wmi.o
diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
new file mode 100644
index 000000000000..3de8abea38f8
--- /dev/null
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -0,0 +1,229 @@
+/*
+ * WMI methods for use with dell-smbios
+ *
+ * Copyright (c) 2017 Dell Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/dmi.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/uaccess.h>
+#include <linux/wmi.h>
+#include "dell-smbios.h"
+#include "dell-wmi-descriptor.h"
+static DEFINE_MUTEX(call_mutex);
+static DEFINE_MUTEX(list_mutex);
+static int wmi_supported;
+
+struct misc_bios_flags_structure {
+ struct dmi_header header;
+ u16 flags0;
+} __packed;
+#define FLAG_HAS_ACPI_WMI 0x02
+
+#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+
+struct wmi_extensions {
+ __u32 argattrib;
+ __u32 blength;
+ __u8 data[];
+} __packed;
+
+struct wmi_smbios_buffer {
+ struct calling_interface_buffer std;
+ struct wmi_extensions ext;
+} __packed;
+
+struct wmi_smbios_priv {
+ struct wmi_smbios_buffer *buf;
+ struct list_head list;
+ struct wmi_device *wdev;
+ struct device *child;
+ u32 buffer_size;
+};
+static LIST_HEAD(wmi_list);
+
+static inline struct wmi_smbios_priv *get_first_smbios_priv(void)
+{
+ return list_first_entry_or_null(&wmi_list,
+ struct wmi_smbios_priv,
+ list);
+}
+
+static int run_smbios_call(struct wmi_device *wdev)
+{
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+ struct wmi_smbios_priv *priv;
+ struct acpi_buffer input;
+ union acpi_object *obj;
+ acpi_status status;
+
+ priv = dev_get_drvdata(&wdev->dev);
+ input.length = priv->buffer_size;
+ input.pointer = &priv->buf->std;
+
+ dev_dbg(&wdev->dev, "evaluating: %x/%x [%x,%x,%x,%x]\n",
+ priv->buf->std.class, priv->buf->std.select,
+ priv->buf->std.input[0], priv->buf->std.input[1],
+ priv->buf->std.input[2], priv->buf->std.input[3]);
+
+ status = wmidev_evaluate_method(wdev, 0, 1, &input, &output);
+ if (ACPI_FAILURE(status))
+ return -EIO;
+ obj = (union acpi_object *)output.pointer;
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ dev_dbg(&wdev->dev, "received type: %d\n", obj->type);
+ if (obj->type == ACPI_TYPE_INTEGER)
+ dev_dbg(&wdev->dev, "SMBIOS call failed: %llu\n",
+ obj->integer.value);
+ return -EIO;
+ }
+ memcpy(&priv->buf->std, obj->buffer.pointer, obj->buffer.length);
+
+ return 0;
+}
+
+int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
+{
+ struct wmi_smbios_priv *priv;
+ size_t difference;
+ size_t size;
+ int ret;
+
+ priv = get_first_smbios_priv();
+ if (!priv)
+ return -ENODEV;
+
+ size = sizeof(struct calling_interface_buffer);
+ difference = priv->buffer_size - sizeof(u64) - size;
+
+ mutex_lock(&call_mutex);
+ memset(&priv->buf->ext, 0, difference);
+ memcpy(&priv->buf->std, buffer, size);
+ ret = run_smbios_call(priv->wdev);
+ memcpy(buffer, &priv->buf->std, size);
+ mutex_unlock(&call_mutex);
+
+ return ret;
+}
+
+static int dell_smbios_wmi_probe(struct wmi_device *wdev)
+{
+ struct wmi_smbios_priv *priv;
+ int count;
+ int ret;
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(struct wmi_smbios_priv),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* WMI buffer size will be either 4k or 32k depending on machine */
+ if (!dell_wmi_get_size(&priv->buffer_size))
+ return -EINVAL;
+
+ count = get_order(priv->buffer_size);
+ priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
+ if (!priv->buf)
+ return -ENOMEM;
+
+ /* ID is used by dell-smbios to set priority of drivers */
+ wdev->dev.id = 1;
+ ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
+ if (ret)
+ goto fail_register;
+
+ priv->wdev = wdev;
+ dev_set_drvdata(&wdev->dev, priv);
+ mutex_lock(&list_mutex);
+ list_add_tail(&priv->list, &wmi_list);
+ mutex_unlock(&list_mutex);
+
+ return 0;
+
+fail_register:
+ free_pages((unsigned long)priv->buf, count);
+ return ret;
+}
+
+static int dell_smbios_wmi_remove(struct wmi_device *wdev)
+{
+ struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
+ int count;
+
+ mutex_lock(&call_mutex);
+ mutex_lock(&list_mutex);
+ list_del(&priv->list);
+ mutex_unlock(&list_mutex);
+ dell_smbios_unregister_device(&wdev->dev);
+ count = get_order(priv->buffer_size);
+ free_pages((unsigned long)priv->buf, count);
+ mutex_unlock(&call_mutex);
+ return 0;
+}
+
+static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
+ { .guid_string = DELL_WMI_SMBIOS_GUID },
+ { },
+};
+
+static void __init parse_b1_table(const struct dmi_header *dm)
+{
+ struct misc_bios_flags_structure *flags =
+ container_of(dm, struct misc_bios_flags_structure, header);
+
+ /* 4 bytes header, 8 bytes flags */
+ if (dm->length < 12)
+ return;
+ if (dm->handle != 0xb100)
+ return;
+ if ((flags->flags0 & FLAG_HAS_ACPI_WMI))
+ wmi_supported = 1;
+}
+
+static void __init find_b1(const struct dmi_header *dm, void *dummy)
+{
+ switch (dm->type) {
+ case 0xb1: /* misc bios flags */
+ parse_b1_table(dm);
+ break;
+ }
+}
+
+static struct wmi_driver dell_smbios_wmi_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 init_dell_smbios_wmi(void)
+{
+ dmi_walk(find_b1, NULL);
+
+ if (!wmi_supported)
+ return -ENODEV;
+
+ return wmi_driver_register(&dell_smbios_wmi_driver);
+}
+
+static void __exit exit_dell_smbios_wmi(void)
+{
+ wmi_driver_unregister(&dell_smbios_wmi_driver);
+}
+
+module_init(init_dell_smbios_wmi);
+module_exit(exit_dell_smbios_wmi);
+
+MODULE_ALIAS("wmi:" DELL_WMI_SMBIOS_GUID);
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_DESCRIPTION("Dell SMBIOS communications over WMI");
+MODULE_LICENSE("GPL");
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 12/14] platform/x86: dell-smbios-smm: test for WSMT
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (10 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 11/14] platform/x86: dell-smbios-wmi: Add new WMI dispatcher driver Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 13/14] platform/x86: wmi: create character devices when requested by drivers Mario Limonciello
2017-10-07 4:59 ` [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface Mario Limonciello
13 siblings, 0 replies; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
WSMT is as an attestation to the OS that the platform won't
modify memory outside of pre-defined areas.
If a platform has WSMT enabled in BIOS setup, SMM calls through
dcdbas will fail. The only way to access platform data in these
instances is through the WMI SMBIOS calling interface.
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
drivers/platform/x86/dell-smbios-smm.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c
index 223531e43fea..ba315753e847 100644
--- a/drivers/platform/x86/dell-smbios-smm.c
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -25,6 +25,8 @@ static struct calling_interface_buffer *buffer;
struct platform_device *platform_device;
static DEFINE_MUTEX(smm_mutex);
+#define WSMT_EN_TOKEN 0x04EC
+
static const struct dmi_system_id dell_device_table[] __initconst = {
{
.ident = "Dell laptop",
@@ -76,6 +78,30 @@ int dell_smbios_smm_call(struct calling_interface_buffer *input)
return 0;
}
+static int test_wsmt_enabled(void)
+{
+ struct calling_interface_token *token;
+
+ /* if token doesn't exist, SMM will work */
+ token = dell_smbios_find_token(WSMT_EN_TOKEN);
+ if (!token)
+ return 0;
+
+ /* if token exists, try to access over SMM */
+ buffer->class = 0;
+ buffer->select = 0;
+ memset(buffer, 0, sizeof(struct calling_interface_buffer));
+ buffer->input[0] = token->location;
+ dell_smbios_smm_call(buffer);
+
+ /* if lookup failed, we know WSMT was enabled */
+ if (buffer->output[0] != 0)
+ return 1;
+
+ /* query token status if it didn't fail */
+ return (buffer->output[1] == token->value);
+}
+
static int __init dell_smbios_smm_init(void)
{
int ret;
@@ -88,6 +114,13 @@ static int __init dell_smbios_smm_init(void)
return -ENOMEM;
dell_smbios_get_smm_address(&da_command_address, &da_command_code);
+ ret = test_wsmt_enabled();
+ pr_debug("WSMT enable test: %d\n", ret);
+ if (ret) {
+ ret = -ENODEV;
+ goto fail_wsmt;
+ }
+
platform_device = platform_device_alloc("dell-smbios", 1);
if (!platform_device) {
ret = -ENOMEM;
@@ -111,6 +144,7 @@ static int __init dell_smbios_smm_init(void)
fail_platform_device_add:
platform_device_put(platform_device);
+fail_wsmt:
fail_platform_device_alloc:
free_page((unsigned long)buffer);
return ret;
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v5 13/14] platform/x86: wmi: create character devices when requested by drivers
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (11 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 12/14] platform/x86: dell-smbios-smm: test for WSMT Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 7:34 ` Greg KH
2017-10-07 4:59 ` [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface Mario Limonciello
13 siblings, 1 reply; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
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 an ioctl callback in the wmi_driver
the WMI bus driver will create a character device that maps to that
function.
That character device will correspond to this path:
/dev/wmi/$driver
The WMI bus driver will interpret the IOCTL calls, test them for
a valid instance and pass them on to the vendor driver to run.
This creates an implicit policy that only driver per character
device. If a module matches multiple GUID's, the wmi_devices
will need to be all handled by the same wmi_driver if the same
character device is used.
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.limonciello@dell.com>
---
MAINTAINERS | 1 +
drivers/platform/x86/wmi.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/wmi.h | 5 +++
include/uapi/linux/wmi.h | 19 +++++++++++
4 files changed, 110 insertions(+)
create mode 100644 include/uapi/linux/wmi.h
diff --git a/MAINTAINERS b/MAINTAINERS
index e7514b616e13..2a99ee9fd883 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -372,6 +372,7 @@ ACPI WMI DRIVER
L: platform-driver-x86@vger.kernel.org
S: Orphan
F: drivers/platform/x86/wmi.c
+F: include/uapi/linux/wmi.h
AD1889 ALSA SOUND DRIVER
M: Thibaut Varene <T-Bone@parisc-linux.org>
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index bcb41c1c7f52..114d28aafa16 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -38,6 +38,7 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
+#include <linux/miscdevice.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
@@ -69,6 +70,7 @@ struct wmi_block {
struct wmi_device dev;
struct list_head list;
struct guid_block gblock;
+ struct miscdevice misc_dev;
struct acpi_device *acpi_device;
wmi_notify_handler handler;
void *handler_data;
@@ -765,12 +767,90 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
return 0;
}
+static int match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
+ int compat)
+{
+ struct wmi_driver *wdriver = NULL;
+ struct wmi_block *wblock = NULL;
+ const char *driver_name;
+ struct list_head *p;
+
+ if (_IOC_TYPE(cmd) != WMI_IOC)
+ return -ENOTTY;
+
+ driver_name = filp->f_path.dentry->d_iname;
+
+ list_for_each(p, &wmi_block_list) {
+ wblock = list_entry(p, struct wmi_block, list);
+ wdriver = container_of(wblock->dev.dev.driver,
+ struct wmi_driver, driver);
+ if (!wdriver)
+ continue;
+ if (strcmp(driver_name, wdriver->driver.name) == 0)
+ break;
+ }
+
+ if (!wdriver)
+ return -ENODEV;
+
+ /* make sure we're not calling a higher instance than exists*/
+ if (_IOC_NR(cmd) > wblock->gblock.instance_count - 1)
+ return -EINVAL;
+
+ if (!compat)
+ return wdriver->unlocked_ioctl(&wblock->dev, cmd, arg);
+
+ /* not all vendor drivers may specify this */
+ if (!wdriver->compat_ioctl)
+ return -ENODEV;
+
+ return wdriver->compat_ioctl(&wblock->dev, cmd, arg);
+}
+
+static long wmi_unlocked_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ return match_ioctl(filp, cmd, arg, 0);
+}
+
+static long wmi_compat_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ return match_ioctl(filp, cmd, arg, 1);
+}
+
+static const struct file_operations wmi_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = wmi_unlocked_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = wmi_compat_ioctl,
+#endif
+};
+
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;
+ char *buf;
+
+ /* driver wants a character device made */
+ if (wdriver->unlocked_ioctl) {
+ buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+ sprintf(buf, "wmi/%s", wdriver->driver.name);
+ wblock->misc_dev.minor = MISC_DYNAMIC_MINOR;
+ wblock->misc_dev.name = buf;
+ wblock->misc_dev.fops = &wmi_fops;
+ ret = misc_register(&wblock->misc_dev);
+ if (ret) {
+ dev_warn(dev, "failed to register char dev: %d", ret);
+ kfree(buf);
+ return -ENOMEM;
+ }
+ }
if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
dev_warn(dev, "failed to enable device -- probing anyway\n");
@@ -791,6 +871,11 @@ static int wmi_dev_remove(struct device *dev)
container_of(dev->driver, struct wmi_driver, driver);
int ret = 0;
+ if (wdriver->unlocked_ioctl) {
+ misc_deregister(&wblock->misc_dev);
+ kfree(wblock->misc_dev.name);
+ }
+
if (wdriver->remove)
ret = wdriver->remove(dev_to_wdev(dev));
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index ddee427e0721..a70699dc0f44 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -18,6 +18,7 @@
#include <linux/device.h>
#include <linux/acpi.h>
+#include <uapi/linux/wmi.h>
struct wmi_device {
struct device dev;
@@ -47,6 +48,10 @@ struct wmi_driver {
int (*probe)(struct wmi_device *wdev);
int (*remove)(struct wmi_device *wdev);
void (*notify)(struct wmi_device *device, union acpi_object *data);
+ long (*unlocked_ioctl)(struct wmi_device *wdev, unsigned int cmd,
+ unsigned long arg);
+ long (*compat_ioctl)(struct wmi_device *wdev, unsigned int cmd,
+ unsigned long arg);
};
extern int __must_check __wmi_driver_register(struct wmi_driver *driver,
diff --git a/include/uapi/linux/wmi.h b/include/uapi/linux/wmi.h
new file mode 100644
index 000000000000..2d8285ae4bd1
--- /dev/null
+++ b/include/uapi/linux/wmi.h
@@ -0,0 +1,19 @@
+/*
+ * User API methods for ACPI-WMI mapping driver
+ *
+ * Copyright (C) 2017 Dell, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _UAPI_LINUX_WMI_H
+#define _UAPI_LINUX_WMI_H
+
+#define WMI_IOC 'W'
+#define WMI_IO(instance) _IO(WMI_IOC, instance)
+#define WMI_IOR(instance, type) _IOR(WMI_IOC, instance, type)
+#define WMI_IOW(instance, type) _IOW(WMI_IOC, instance, type)
+#define WMI_IOWR(instance, type) _IOWR(WMI_IOC, instance, type)
+
+#endif
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v5 13/14] platform/x86: wmi: create character devices when requested by drivers
2017-10-07 4:59 ` [PATCH v5 13/14] platform/x86: wmi: create character devices when requested by drivers Mario Limonciello
@ 2017-10-07 7:34 ` Greg KH
2017-10-07 11:59 ` Mario.Limonciello
0 siblings, 1 reply; 31+ messages in thread
From: Greg KH @ 2017-10-07 7:34 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart, Andy Shevchenko, LKML, platform-driver-x86,
Andy Lutomirski, quasisec, pali.rohar, rjw, mjg59, hch
On Fri, Oct 06, 2017 at 11:59:57PM -0500, Mario Limonciello 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 an ioctl callback in the wmi_driver
> the WMI bus driver will create a character device that maps to that
> function.
>
> That character device will correspond to this path:
> /dev/wmi/$driver
>
> The WMI bus driver will interpret the IOCTL calls, test them for
> a valid instance and pass them on to the vendor driver to run.
>
> This creates an implicit policy that only driver per character
> device. If a module matches multiple GUID's, the wmi_devices
> will need to be all handled by the same wmi_driver if the same
> character device is used.
>
> 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.
What prevents the vendor driver from being unloaded while the ioctl is
being called in it? I don't see any protection here from that at all :(
> +static long wmi_unlocked_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + return match_ioctl(filp, cmd, arg, 0);
> +}
> +
> +static long wmi_compat_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + return match_ioctl(filp, cmd, arg, 1);
> +}
Why a compat ioctl at all? That's for older interfaces, not for brand
new ones where you design the ioctl structures "correctly" to work on
both 32 and 64 bits at the same time. That should not be needed at all.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread* RE: [PATCH v5 13/14] platform/x86: wmi: create character devices when requested by drivers
2017-10-07 7:34 ` Greg KH
@ 2017-10-07 11:59 ` Mario.Limonciello
2017-10-07 12:38 ` Greg KH
0 siblings, 1 reply; 31+ messages in thread
From: Mario.Limonciello @ 2017-10-07 11:59 UTC (permalink / raw)
To: greg
Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86, luto,
quasisec, pali.rohar, rjw, mjg59, hch
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Saturday, October 7, 2017 2:35 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Andy Lutomirski <luto@kernel.org>; quasisec@google.com;
> pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v5 13/14] platform/x86: wmi: create character devices when
> requested by drivers
>
> On Fri, Oct 06, 2017 at 11:59:57PM -0500, Mario Limonciello 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 an ioctl callback in the wmi_driver
> > the WMI bus driver will create a character device that maps to that
> > function.
> >
> > That character device will correspond to this path:
> > /dev/wmi/$driver
> >
> > The WMI bus driver will interpret the IOCTL calls, test them for
> > a valid instance and pass them on to the vendor driver to run.
> >
> > This creates an implicit policy that only driver per character
> > device. If a module matches multiple GUID's, the wmi_devices
> > will need to be all handled by the same wmi_driver if the same
> > character device is used.
> >
> > 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.
>
> What prevents the vendor driver from being unloaded while the ioctl is
> being called in it? I don't see any protection here from that at all :(
>
In my driver I take a mutex that blocks unloading while running ioctl,
but you mean you want one in the bus driver more generally, OK.
> > +static long wmi_unlocked_ioctl(struct file *filp, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return match_ioctl(filp, cmd, arg, 0);
> > +}
> > +
> > +static long wmi_compat_ioctl(struct file *filp, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return match_ioctl(filp, cmd, arg, 1);
> > +}
>
> Why a compat ioctl at all? That's for older interfaces, not for brand
> new ones where you design the ioctl structures "correctly" to work on
> both 32 and 64 bits at the same time. That should not be needed at all.
>
> thanks,
>
> greg k-h
I was trying to make sure that any other future vendor drivers had it for an
option. I wasn't aware that new code shouldn't use it. OK, I'll remove it.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v5 13/14] platform/x86: wmi: create character devices when requested by drivers
2017-10-07 11:59 ` Mario.Limonciello
@ 2017-10-07 12:38 ` Greg KH
0 siblings, 0 replies; 31+ messages in thread
From: Greg KH @ 2017-10-07 12:38 UTC (permalink / raw)
To: Mario.Limonciello
Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86, luto,
quasisec, pali.rohar, rjw, mjg59, hch
On Sat, Oct 07, 2017 at 11:59:52AM +0000, Mario.Limonciello@dell.com wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Saturday, October 7, 2017 2:35 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> > Andy Lutomirski <luto@kernel.org>; quasisec@google.com;
> > pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> > Subject: Re: [PATCH v5 13/14] platform/x86: wmi: create character devices when
> > requested by drivers
> >
> > On Fri, Oct 06, 2017 at 11:59:57PM -0500, Mario Limonciello 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 an ioctl callback in the wmi_driver
> > > the WMI bus driver will create a character device that maps to that
> > > function.
> > >
> > > That character device will correspond to this path:
> > > /dev/wmi/$driver
> > >
> > > The WMI bus driver will interpret the IOCTL calls, test them for
> > > a valid instance and pass them on to the vendor driver to run.
> > >
> > > This creates an implicit policy that only driver per character
> > > device. If a module matches multiple GUID's, the wmi_devices
> > > will need to be all handled by the same wmi_driver if the same
> > > character device is used.
> > >
> > > 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.
> >
> > What prevents the vendor driver from being unloaded while the ioctl is
> > being called in it? I don't see any protection here from that at all :(
> >
>
> In my driver I take a mutex that blocks unloading while running ioctl,
> but you mean you want one in the bus driver more generally, OK.
No, you need to increment the module reference count before calling into
it, which prevents anyone from even attempting to unload it. Much
simpler than trying to rely on a mutex that is not obviously documented
that this is what it does (hint, sharing a mutex across modules is not a
good idea...)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface
2017-10-07 4:59 [PATCH v5 00/14] Introduce support for Dell SMBIOS over WMI Mario Limonciello
` (12 preceding siblings ...)
2017-10-07 4:59 ` [PATCH v5 13/14] platform/x86: wmi: create character devices when requested by drivers Mario Limonciello
@ 2017-10-07 4:59 ` Mario Limonciello
2017-10-07 7:41 ` Greg KH
13 siblings, 1 reply; 31+ messages in thread
From: Mario Limonciello @ 2017-10-07 4:59 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, Andy Lutomirski, quasisec, pali.rohar,
rjw, mjg59, hch, Greg KH, Mario Limonciello
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.
This userspace character device will be used to perform SMBIOS calls
from any applications.
It provides an ioctl that will allow passing the 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.
To use the character device the buffer needed for the machine will
also be needed. This information is exported to a sysfs attribute.
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.limonciello@dell.com>
---
Documentation/ABI/testing/dell-smbios-wmi | 41 ++++++++
.../ABI/testing/sysfs-platform-dell-smbios-wmi | 10 ++
MAINTAINERS | 1 +
drivers/platform/x86/dell-smbios-wmi.c | 104 ++++++++++++++++++---
drivers/platform/x86/dell-smbios.h | 11 +--
include/uapi/linux/dell-smbios.h | 42 +++++++++
6 files changed, 188 insertions(+), 21 deletions(-)
create mode 100644 Documentation/ABI/testing/dell-smbios-wmi
create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
create mode 100644 include/uapi/linux/dell-smbios.h
diff --git a/Documentation/ABI/testing/dell-smbios-wmi b/Documentation/ABI/testing/dell-smbios-wmi
new file mode 100644
index 000000000000..e067e955fcc9
--- /dev/null
+++ b/Documentation/ABI/testing/dell-smbios-wmi
@@ -0,0 +1,41 @@
+What: /dev/wmi/dell-smbios
+Date: November 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.limonciello@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-smbios.h>
+
+ 1) To perform a call from userspace, you'll need to first
+ determine the minimum size of the calling interface buffer
+ for your machine.
+ Platforms that contain larger buffers can return larger
+ objects from the system firmware.
+ Commonly this size is either 4k or 32k.
+
+ To determine the size of the buffer, refer to:
+ sysfs-platform-dell-smbios-wmi
+
+ 2) After you've determined the minimum size of the calling
+ interface buffer, you can allocate a structure that represents
+ the structure documented above.
+
+ 3) In the 'length' object store the size of the buffer you
+ determined above and allocated.
+
+ 4) In this buffer object, prepare as necessary for the SMBIOS
+ call you're interested in. Typically SMBIOS buffers have
+ "class", "select", and "input" defined to values that coincide
+ with the data you are interested in.
+ Documenting class/select/input values is outside of the scope
+ of this documentation. Check with the libsmbios project for
+ further documentation on these values.
+
+ 6) Run the call by using ioctl() as described in the header.
+
+ 7) The output will be returned in the buffer object.
+
+ 8) Be sure to free up your allocated object.
diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
new file mode 100644
index 000000000000..6a0513703a3c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
@@ -0,0 +1,10 @@
+What: /sys/devices/platform/<platform>/buffer_size
+Date: November 2017
+KernelVersion: 4.15
+Contact: "Mario Limonciello" <mario.limonciello@dell.com>
+Description:
+ A read-only description of the size of a calling
+ interface buffer that can be passed to Dell
+ firmware.
+
+ Commonly this size is either 4k or 32k.
diff --git a/MAINTAINERS b/MAINTAINERS
index 2a99ee9fd883..4940f3c7481b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3986,6 +3986,7 @@ M: Mario Limonciello <mario.limonciello@dell.com>
L: platform-driver-x86@vger.kernel.org
S: Maintained
F: drivers/platform/x86/dell-smbios-wmi.c
+F: include/uapi/linux/dell-smbios.h
DELL LAPTOP DRIVER
M: Matthew Garrett <mjg59@srcf.ucam.org>
diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
index 3de8abea38f8..2b78aba68755 100644
--- a/drivers/platform/x86/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -15,6 +15,7 @@
#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/wmi.h>
+#include <uapi/linux/dell-smbios.h>
#include "dell-smbios.h"
#include "dell-wmi-descriptor.h"
static DEFINE_MUTEX(call_mutex);
@@ -29,19 +30,9 @@ struct misc_bios_flags_structure {
#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
-struct wmi_extensions {
- __u32 argattrib;
- __u32 blength;
- __u8 data[];
-} __packed;
-
-struct wmi_smbios_buffer {
- struct calling_interface_buffer std;
- struct wmi_extensions ext;
-} __packed;
-
struct wmi_smbios_priv {
struct wmi_smbios_buffer *buf;
+ struct device_attribute buffer_size_attr;
struct list_head list;
struct wmi_device *wdev;
struct device *child;
@@ -113,6 +104,78 @@ int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
return ret;
}
+static long dell_smbios_wmi_ioctl(struct wmi_device *wdev, unsigned int cmd,
+ unsigned long arg)
+{
+ struct wmi_smbios_buffer __user *input =
+ (struct wmi_smbios_buffer __user *) arg;
+ struct wmi_smbios_priv *priv;
+ int ret = 0;
+ size_t size;
+
+ switch (cmd) {
+ case DELL_WMI_SMBIOS_CMD:
+ priv = dev_get_drvdata(&wdev->dev);
+ if (!priv)
+ return -ENODEV;
+ size = sizeof(struct wmi_smbios_buffer);
+ mutex_lock(&call_mutex);
+ if (copy_from_user(priv->buf, input, size)) {
+ dev_dbg(&wdev->dev, "Copy %lu from user failed\n",
+ size);
+ ret = -EFAULT;
+ goto fail_smbios_cmd;
+ }
+ if (priv->buf->length < priv->buffer_size) {
+ dev_err(&wdev->dev,
+ "Buffer %lld too small, need at least %d\n",
+ priv->buf->length, priv->buffer_size);
+ ret = -EINVAL;
+ goto fail_smbios_cmd;
+ }
+ if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
+ dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
+ priv->buf->std.class, priv->buf->std.select,
+ priv->buf->std.input[0]);
+ ret = -EFAULT;
+ goto fail_smbios_cmd;
+ }
+ size = priv->buffer_size - sizeof(struct wmi_smbios_buffer);
+ if (copy_from_user(priv->buf->ext.data,
+ input->ext.data, size)) {
+ dev_dbg(&wdev->dev, "Copy %lu from user failed\n",
+ size);
+ ret = -EFAULT;
+ goto fail_smbios_cmd;
+ }
+ ret = run_smbios_call(priv->wdev);
+ if (ret != 0)
+ goto fail_smbios_cmd;
+ if (copy_to_user(input, priv->buf, priv->buffer_size)) {
+ dev_dbg(&wdev->dev, "Copy %d to user failed\n",
+ priv->buffer_size);
+ ret = -EFAULT;
+ }
+fail_smbios_cmd:
+ mutex_unlock(&call_mutex);
+ break;
+ default:
+ dev_dbg(&wdev->dev, "unsupported ioctl: %d [%d, %d, %d, %d].\n",
+ cmd, _IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd),
+ _IOC_SIZE(cmd));
+ ret = -ENOIOCTLCMD;
+ }
+ return ret;
+}
+
+static ssize_t buffer_size_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct wmi_smbios_priv *priv = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", priv->buffer_size);
+}
+
static int dell_smbios_wmi_probe(struct wmi_device *wdev)
{
struct wmi_smbios_priv *priv;
@@ -127,12 +190,23 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
/* WMI buffer size will be either 4k or 32k depending on machine */
if (!dell_wmi_get_size(&priv->buffer_size))
return -EINVAL;
+ /* add in the length object we will use internally with ioctl */
+ priv->buffer_size += sizeof(u64);
count = get_order(priv->buffer_size);
priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
if (!priv->buf)
return -ENOMEM;
+ sysfs_attr_init(&priv->buffer_size_attr);
+ priv->buffer_size_attr.attr.name = "buffer_size";
+ priv->buffer_size_attr.attr.mode = 0444;
+ priv->buffer_size_attr.show = buffer_size_show;
+
+ ret = device_create_file(&wdev->dev, &priv->buffer_size_attr);
+ if (ret)
+ goto fail_create_sysfs;
+
/* ID is used by dell-smbios to set priority of drivers */
wdev->dev.id = 1;
ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
@@ -148,6 +222,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
return 0;
fail_register:
+ device_remove_file(&wdev->dev, &priv->buffer_size_attr);
+
+fail_create_sysfs:
free_pages((unsigned long)priv->buf, count);
return ret;
}
@@ -162,6 +239,7 @@ static int dell_smbios_wmi_remove(struct wmi_device *wdev)
list_del(&priv->list);
mutex_unlock(&list_mutex);
dell_smbios_unregister_device(&wdev->dev);
+ device_remove_file(&wdev->dev, &priv->buffer_size_attr);
count = get_order(priv->buffer_size);
free_pages((unsigned long)priv->buf, count);
mutex_unlock(&call_mutex);
@@ -203,6 +281,10 @@ static struct wmi_driver dell_smbios_wmi_driver = {
.probe = dell_smbios_wmi_probe,
.remove = dell_smbios_wmi_remove,
.id_table = dell_smbios_wmi_id_table,
+ .unlocked_ioctl = dell_smbios_wmi_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = dell_smbios_wmi_ioctl,
+#endif
};
static int __init init_dell_smbios_wmi(void)
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index c743c58831e5..04995136114c 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -17,19 +17,10 @@
#define _DELL_SMBIOS_H_
#include <linux/device.h>
+#include <uapi/linux/dell-smbios.h>
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];
-} __packed;
-
struct calling_interface_token {
u16 tokenID;
u16 location;
diff --git a/include/uapi/linux/dell-smbios.h b/include/uapi/linux/dell-smbios.h
new file mode 100644
index 000000000000..8ce142b95f15
--- /dev/null
+++ b/include/uapi/linux/dell-smbios.h
@@ -0,0 +1,42 @@
+/*
+ * User API for WMI methods for use with dell-smbios
+ *
+ * Copyright (c) 2017 Dell Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _UAPI_DELL_SMBIOS_H_
+#define _UAPI_DELL_SMBIOS_H_
+
+#include <linux/ioctl.h>
+#include <linux/wmi.h>
+
+/* This structure may be modified by the firmware when we enter
+ * system management mode through SMM, hence the volatiles
+ */
+struct calling_interface_buffer {
+ __u16 class;
+ __u16 select;
+ volatile __u32 input[4];
+ volatile __u32 output[4];
+} __packed;
+
+struct wmi_extensions {
+ __u32 argattrib;
+ __u32 blength;
+ __u8 data[];
+} __packed;
+
+struct wmi_smbios_buffer {
+ __u64 length;
+ struct calling_interface_buffer std;
+ struct wmi_extensions ext;
+} __packed;
+
+/* SMBIOS calling IOCTL command */
+#define DELL_WMI_SMBIOS_CMD WMI_IOWR(0, struct wmi_smbios_buffer)
+
+#endif /* _UAPI_DELL_SMBIOS_H_ */
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface
2017-10-07 4:59 ` [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface Mario Limonciello
@ 2017-10-07 7:41 ` Greg KH
2017-10-07 7:43 ` Greg KH
2017-10-07 12:15 ` Mario.Limonciello
0 siblings, 2 replies; 31+ messages in thread
From: Greg KH @ 2017-10-07 7:41 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart, Andy Shevchenko, LKML, platform-driver-x86,
Andy Lutomirski, quasisec, pali.rohar, rjw, mjg59, hch
On Fri, Oct 06, 2017 at 11:59:58PM -0500, Mario Limonciello wrote:
> 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.
>
> This userspace character device will be used to perform SMBIOS calls
> from any applications.
>
> It provides an ioctl that will allow passing the 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.
>
> To use the character device the buffer needed for the machine will
> also be needed. This information is exported to a sysfs attribute.
>
> 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.limonciello@dell.com>
> ---
> Documentation/ABI/testing/dell-smbios-wmi | 41 ++++++++
> .../ABI/testing/sysfs-platform-dell-smbios-wmi | 10 ++
> MAINTAINERS | 1 +
> drivers/platform/x86/dell-smbios-wmi.c | 104 ++++++++++++++++++---
> drivers/platform/x86/dell-smbios.h | 11 +--
> include/uapi/linux/dell-smbios.h | 42 +++++++++
> 6 files changed, 188 insertions(+), 21 deletions(-)
> create mode 100644 Documentation/ABI/testing/dell-smbios-wmi
> create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
> create mode 100644 include/uapi/linux/dell-smbios.h
>
> diff --git a/Documentation/ABI/testing/dell-smbios-wmi b/Documentation/ABI/testing/dell-smbios-wmi
> new file mode 100644
> index 000000000000..e067e955fcc9
> --- /dev/null
> +++ b/Documentation/ABI/testing/dell-smbios-wmi
> @@ -0,0 +1,41 @@
> +What: /dev/wmi/dell-smbios
> +Date: November 2017
> +KernelVersion: 4.15
> +Contact: "Mario Limonciello" <mario.limonciello@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-smbios.h>
> +
> + 1) To perform a call from userspace, you'll need to first
> + determine the minimum size of the calling interface buffer
> + for your machine.
> + Platforms that contain larger buffers can return larger
> + objects from the system firmware.
> + Commonly this size is either 4k or 32k.
> +
> + To determine the size of the buffer, refer to:
> + sysfs-platform-dell-smbios-wmi
> +
> + 2) After you've determined the minimum size of the calling
> + interface buffer, you can allocate a structure that represents
> + the structure documented above.
> +
> + 3) In the 'length' object store the size of the buffer you
> + determined above and allocated.
> +
> + 4) In this buffer object, prepare as necessary for the SMBIOS
> + call you're interested in. Typically SMBIOS buffers have
> + "class", "select", and "input" defined to values that coincide
> + with the data you are interested in.
> + Documenting class/select/input values is outside of the scope
> + of this documentation. Check with the libsmbios project for
> + further documentation on these values.
> +
> + 6) Run the call by using ioctl() as described in the header.
> +
> + 7) The output will be returned in the buffer object.
> +
> + 8) Be sure to free up your allocated object.
> diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
> new file mode 100644
> index 000000000000..6a0513703a3c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
> @@ -0,0 +1,10 @@
> +What: /sys/devices/platform/<platform>/buffer_size
> +Date: November 2017
> +KernelVersion: 4.15
> +Contact: "Mario Limonciello" <mario.limonciello@dell.com>
> +Description:
> + A read-only description of the size of a calling
> + interface buffer that can be passed to Dell
> + firmware.
> +
> + Commonly this size is either 4k or 32k.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2a99ee9fd883..4940f3c7481b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3986,6 +3986,7 @@ M: Mario Limonciello <mario.limonciello@dell.com>
> L: platform-driver-x86@vger.kernel.org
> S: Maintained
> F: drivers/platform/x86/dell-smbios-wmi.c
> +F: include/uapi/linux/dell-smbios.h
>
> DELL LAPTOP DRIVER
> M: Matthew Garrett <mjg59@srcf.ucam.org>
> diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
> index 3de8abea38f8..2b78aba68755 100644
> --- a/drivers/platform/x86/dell-smbios-wmi.c
> +++ b/drivers/platform/x86/dell-smbios-wmi.c
> @@ -15,6 +15,7 @@
> #include <linux/mutex.h>
> #include <linux/uaccess.h>
> #include <linux/wmi.h>
> +#include <uapi/linux/dell-smbios.h>
> #include "dell-smbios.h"
> #include "dell-wmi-descriptor.h"
> static DEFINE_MUTEX(call_mutex);
> @@ -29,19 +30,9 @@ struct misc_bios_flags_structure {
>
> #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
>
> -struct wmi_extensions {
> - __u32 argattrib;
> - __u32 blength;
> - __u8 data[];
> -} __packed;
> -
> -struct wmi_smbios_buffer {
> - struct calling_interface_buffer std;
> - struct wmi_extensions ext;
> -} __packed;
> -
> struct wmi_smbios_priv {
> struct wmi_smbios_buffer *buf;
> + struct device_attribute buffer_size_attr;
> struct list_head list;
> struct wmi_device *wdev;
> struct device *child;
> @@ -113,6 +104,78 @@ int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
> return ret;
> }
>
> +static long dell_smbios_wmi_ioctl(struct wmi_device *wdev, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct wmi_smbios_buffer __user *input =
> + (struct wmi_smbios_buffer __user *) arg;
Why not just always define arg as "struct wmi_smbios_buffer __user *"?
No need to pass down a vague type here, right?
Unless you need to better name your structure to show that it is a dell
type only :)
> + struct wmi_smbios_priv *priv;
> + int ret = 0;
> + size_t size;
> +
> + switch (cmd) {
> + case DELL_WMI_SMBIOS_CMD:
> + priv = dev_get_drvdata(&wdev->dev);
> + if (!priv)
> + return -ENODEV;
> + size = sizeof(struct wmi_smbios_buffer);
> + mutex_lock(&call_mutex);
> + if (copy_from_user(priv->buf, input, size)) {
> + dev_dbg(&wdev->dev, "Copy %lu from user failed\n",
> + size);
> + ret = -EFAULT;
> + goto fail_smbios_cmd;
> + }
> + if (priv->buf->length < priv->buffer_size) {
> + dev_err(&wdev->dev,
> + "Buffer %lld too small, need at least %d\n",
> + priv->buf->length, priv->buffer_size);
> + ret = -EINVAL;
> + goto fail_smbios_cmd;
> + }
No checking for too big of a length? Any other fields you should check
for validity? Like too small?
> + if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
> + dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
> + priv->buf->std.class, priv->buf->std.select,
> + priv->buf->std.input[0]);
> + ret = -EFAULT;
> + goto fail_smbios_cmd;
> + }
> + size = priv->buffer_size - sizeof(struct wmi_smbios_buffer);
What if size just went too small and wrapped around? :(
Remember, "All input is evil". Go print that out and put it on the wall
when you are designing this user/kernel api. You can trust no one, you
have to validate _everything_.
> --- /dev/null
> +++ b/include/uapi/linux/dell-smbios.h
> @@ -0,0 +1,42 @@
> +/*
> + * User API for WMI methods for use with dell-smbios
> + *
> + * Copyright (c) 2017 Dell Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _UAPI_DELL_SMBIOS_H_
> +#define _UAPI_DELL_SMBIOS_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/wmi.h>
> +
> +/* This structure may be modified by the firmware when we enter
> + * system management mode through SMM, hence the volatiles
> + */
> +struct calling_interface_buffer {
> + __u16 class;
> + __u16 select;
> + volatile __u32 input[4];
> + volatile __u32 output[4];
> +} __packed;
I still don't buy the use of volatile, but oh well, I'm assuming you
properly tested this?
> +struct wmi_extensions {
> + __u32 argattrib;
> + __u32 blength;
> + __u8 data[];
> +} __packed;
> +
> +struct wmi_smbios_buffer {
> + __u64 length;
> + struct calling_interface_buffer std;
> + struct wmi_extensions ext;
> +} __packed;
Much better, right? Now why do you feel you need a compat ioctl for
this structure? If you do, where is that logic?
And I still didn't see where you were verifying the list of commands in
this structure, was that in some other patch?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface
2017-10-07 7:41 ` Greg KH
@ 2017-10-07 7:43 ` Greg KH
2017-10-07 12:15 ` Mario.Limonciello
1 sibling, 0 replies; 31+ messages in thread
From: Greg KH @ 2017-10-07 7:43 UTC (permalink / raw)
To: Mario Limonciello
Cc: dvhart, Andy Shevchenko, LKML, platform-driver-x86,
Andy Lutomirski, quasisec, pali.rohar, rjw, mjg59, hch
On Sat, Oct 07, 2017 at 09:41:32AM +0200, Greg KH wrote:
> And I still didn't see where you were verifying the list of commands in
> this structure, was that in some other patch?
Oh nevermind, that was a different patch, found it...
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface
2017-10-07 7:41 ` Greg KH
2017-10-07 7:43 ` Greg KH
@ 2017-10-07 12:15 ` Mario.Limonciello
2017-10-07 12:36 ` Greg KH
1 sibling, 1 reply; 31+ messages in thread
From: Mario.Limonciello @ 2017-10-07 12:15 UTC (permalink / raw)
To: greg
Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86, luto,
quasisec, pali.rohar, rjw, mjg59, hch
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Saturday, October 7, 2017 2:42 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Andy Lutomirski <luto@kernel.org>; quasisec@google.com;
> pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
>
> On Fri, Oct 06, 2017 at 11:59:58PM -0500, Mario Limonciello wrote:
> > 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.
> >
> > This userspace character device will be used to perform SMBIOS calls
> > from any applications.
> >
> > It provides an ioctl that will allow passing the 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.
> >
> > To use the character device the buffer needed for the machine will
> > also be needed. This information is exported to a sysfs attribute.
> >
> > 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.limonciello@dell.com>
> > ---
> > Documentation/ABI/testing/dell-smbios-wmi | 41 ++++++++
> > .../ABI/testing/sysfs-platform-dell-smbios-wmi | 10 ++
> > MAINTAINERS | 1 +
> > drivers/platform/x86/dell-smbios-wmi.c | 104 ++++++++++++++++++---
> > drivers/platform/x86/dell-smbios.h | 11 +--
> > include/uapi/linux/dell-smbios.h | 42 +++++++++
> > 6 files changed, 188 insertions(+), 21 deletions(-)
> > create mode 100644 Documentation/ABI/testing/dell-smbios-wmi
> > create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios-
> wmi
> > create mode 100644 include/uapi/linux/dell-smbios.h
> >
> > diff --git a/Documentation/ABI/testing/dell-smbios-wmi
> b/Documentation/ABI/testing/dell-smbios-wmi
> > new file mode 100644
> > index 000000000000..e067e955fcc9
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/dell-smbios-wmi
> > @@ -0,0 +1,41 @@
> > +What: /dev/wmi/dell-smbios
> > +Date: November 2017
> > +KernelVersion: 4.15
> > +Contact: "Mario Limonciello" <mario.limonciello@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-smbios.h>
> > +
> > + 1) To perform a call from userspace, you'll need to first
> > + determine the minimum size of the calling interface buffer
> > + for your machine.
> > + Platforms that contain larger buffers can return larger
> > + objects from the system firmware.
> > + Commonly this size is either 4k or 32k.
> > +
> > + To determine the size of the buffer, refer to:
> > + sysfs-platform-dell-smbios-wmi
> > +
> > + 2) After you've determined the minimum size of the calling
> > + interface buffer, you can allocate a structure that represents
> > + the structure documented above.
> > +
> > + 3) In the 'length' object store the size of the buffer you
> > + determined above and allocated.
> > +
> > + 4) In this buffer object, prepare as necessary for the SMBIOS
> > + call you're interested in. Typically SMBIOS buffers have
> > + "class", "select", and "input" defined to values that coincide
> > + with the data you are interested in.
> > + Documenting class/select/input values is outside of the scope
> > + of this documentation. Check with the libsmbios project for
> > + further documentation on these values.
> > +
> > + 6) Run the call by using ioctl() as described in the header.
> > +
> > + 7) The output will be returned in the buffer object.
> > +
> > + 8) Be sure to free up your allocated object.
> > diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
> b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
> > new file mode 100644
> > index 000000000000..6a0513703a3c
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
> > @@ -0,0 +1,10 @@
> > +What: /sys/devices/platform/<platform>/buffer_size
> > +Date: November 2017
> > +KernelVersion: 4.15
> > +Contact: "Mario Limonciello" <mario.limonciello@dell.com>
> > +Description:
> > + A read-only description of the size of a calling
> > + interface buffer that can be passed to Dell
> > + firmware.
> > +
> > + Commonly this size is either 4k or 32k.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2a99ee9fd883..4940f3c7481b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3986,6 +3986,7 @@ M: Mario Limonciello <mario.limonciello@dell.com>
> > L: platform-driver-x86@vger.kernel.org
> > S: Maintained
> > F: drivers/platform/x86/dell-smbios-wmi.c
> > +F: include/uapi/linux/dell-smbios.h
> >
> > DELL LAPTOP DRIVER
> > M: Matthew Garrett <mjg59@srcf.ucam.org>
> > diff --git a/drivers/platform/x86/dell-smbios-wmi.c
> b/drivers/platform/x86/dell-smbios-wmi.c
> > index 3de8abea38f8..2b78aba68755 100644
> > --- a/drivers/platform/x86/dell-smbios-wmi.c
> > +++ b/drivers/platform/x86/dell-smbios-wmi.c
> > @@ -15,6 +15,7 @@
> > #include <linux/mutex.h>
> > #include <linux/uaccess.h>
> > #include <linux/wmi.h>
> > +#include <uapi/linux/dell-smbios.h>
> > #include "dell-smbios.h"
> > #include "dell-wmi-descriptor.h"
> > static DEFINE_MUTEX(call_mutex);
> > @@ -29,19 +30,9 @@ struct misc_bios_flags_structure {
> >
> > #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-
> B622A1EF5492"
> >
> > -struct wmi_extensions {
> > - __u32 argattrib;
> > - __u32 blength;
> > - __u8 data[];
> > -} __packed;
> > -
> > -struct wmi_smbios_buffer {
> > - struct calling_interface_buffer std;
> > - struct wmi_extensions ext;
> > -} __packed;
> > -
> > struct wmi_smbios_priv {
> > struct wmi_smbios_buffer *buf;
> > + struct device_attribute buffer_size_attr;
> > struct list_head list;
> > struct wmi_device *wdev;
> > struct device *child;
> > @@ -113,6 +104,78 @@ int dell_smbios_wmi_call(struct
> calling_interface_buffer *buffer)
> > return ret;
> > }
> >
> > +static long dell_smbios_wmi_ioctl(struct wmi_device *wdev, unsigned int
> cmd,
> > + unsigned long arg)
> > +{
> > + struct wmi_smbios_buffer __user *input =
> > + (struct wmi_smbios_buffer __user *) arg;
>
> Why not just always define arg as "struct wmi_smbios_buffer __user *"?
> No need to pass down a vague type here, right?
>
> Unless you need to better name your structure to show that it is a dell
> type only :)
Great idea thanks.
>
> > + struct wmi_smbios_priv *priv;
> > + int ret = 0;
> > + size_t size;
> > +
> > + switch (cmd) {
> > + case DELL_WMI_SMBIOS_CMD:
> > + priv = dev_get_drvdata(&wdev->dev);
> > + if (!priv)
> > + return -ENODEV;
> > + size = sizeof(struct wmi_smbios_buffer);
> > + mutex_lock(&call_mutex);
> > + if (copy_from_user(priv->buf, input, size)) {
> > + dev_dbg(&wdev->dev, "Copy %lu from user failed\n",
> > + size);
> > + ret = -EFAULT;
> > + goto fail_smbios_cmd;
> > + }
> > + if (priv->buf->length < priv->buffer_size) {
> > + dev_err(&wdev->dev,
> > + "Buffer %lld too small, need at least %d\n",
> > + priv->buf->length, priv->buffer_size);
> > + ret = -EINVAL;
> > + goto fail_smbios_cmd;
> > + }
>
> No checking for too big of a length? Any other fields you should check
> for validity? Like too small?
Too big is actually intentionally ignored.
I split the copy into two segments to check for this.
1. First copy the size of the structure
(if userspace didn't allocate at least sizeof(struct wmi_smbios_buffer) that's a problem)
2. Verify the size claimed is "at least" what we internally are looking for.
3. Copy the rest of the size internally needed. If userspace sent more it's just not copied.
4. When sending it back I only send back up to the "at least" internal size.
>
> > + if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
> > + dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
> > + priv->buf->std.class, priv->buf->std.select,
> > + priv->buf->std.input[0]);
> > + ret = -EFAULT;
> > + goto fail_smbios_cmd;
> > + }
> > + size = priv->buffer_size - sizeof(struct wmi_smbios_buffer);
>
> What if size just went too small and wrapped around? :(
>
> Remember, "All input is evil". Go print that out and put it on the wall
> when you are designing this user/kernel api. You can trust no one, you
> have to validate _everything_.
priv->buffer_size can't be set by userspace.
>
> > --- /dev/null
> > +++ b/include/uapi/linux/dell-smbios.h
> > @@ -0,0 +1,42 @@
> > +/*
> > + * User API for WMI methods for use with dell-smbios
> > + *
> > + * Copyright (c) 2017 Dell Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef _UAPI_DELL_SMBIOS_H_
> > +#define _UAPI_DELL_SMBIOS_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/wmi.h>
> > +
> > +/* This structure may be modified by the firmware when we enter
> > + * system management mode through SMM, hence the volatiles
> > + */
> > +struct calling_interface_buffer {
> > + __u16 class;
> > + __u16 select;
> > + volatile __u32 input[4];
> > + volatile __u32 output[4];
> > +} __packed;
>
> I still don't buy the use of volatile, but oh well, I'm assuming you
> properly tested this?
If I wasn't hanging onto the SMM legacy driver I'd drop this, but it's
used when the SMM driver works on this structure.
I've tested it with both the SMM and WMI drivers though, yes.
>
> > +struct wmi_extensions {
> > + __u32 argattrib;
> > + __u32 blength;
> > + __u8 data[];
> > +} __packed;
> > +
> > +struct wmi_smbios_buffer {
> > + __u64 length;
> > + struct calling_interface_buffer std;
> > + struct wmi_extensions ext;
> > +} __packed;
>
> Much better, right? Now why do you feel you need a compat ioctl for
> this structure? If you do, where is that logic?
Yes, thank you this is much cleaner.
Other "newish" drivers provide both stubs even if they point to the same
function.
Eg:
https://github.com/torvalds/linux/commit/2521ea3e0d1b285cea371a38772d3eefa0490c71
I did check that as things are in v5 it works both in 32 and 64 applications.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface
2017-10-07 12:15 ` Mario.Limonciello
@ 2017-10-07 12:36 ` Greg KH
2017-10-07 13:13 ` Mario.Limonciello
0 siblings, 1 reply; 31+ messages in thread
From: Greg KH @ 2017-10-07 12:36 UTC (permalink / raw)
To: Mario.Limonciello
Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86, luto,
quasisec, pali.rohar, rjw, mjg59, hch
On Sat, Oct 07, 2017 at 12:15:18PM +0000, Mario.Limonciello@dell.com wrote:
> > > + struct wmi_smbios_priv *priv;
> > > + int ret = 0;
> > > + size_t size;
> > > +
> > > + switch (cmd) {
> > > + case DELL_WMI_SMBIOS_CMD:
> > > + priv = dev_get_drvdata(&wdev->dev);
> > > + if (!priv)
> > > + return -ENODEV;
> > > + size = sizeof(struct wmi_smbios_buffer);
> > > + mutex_lock(&call_mutex);
> > > + if (copy_from_user(priv->buf, input, size)) {
Wait, how do you know that input is size big?
> > > + dev_dbg(&wdev->dev, "Copy %lu from user failed\n",
> > > + size);
> > > + ret = -EFAULT;
> > > + goto fail_smbios_cmd;
> > > + }
> > > + if (priv->buf->length < priv->buffer_size) {
> > > + dev_err(&wdev->dev,
> > > + "Buffer %lld too small, need at least %d\n",
> > > + priv->buf->length, priv->buffer_size);
> > > + ret = -EINVAL;
> > > + goto fail_smbios_cmd;
> > > + }
> >
> > No checking for too big of a length? Any other fields you should check
> > for validity? Like too small?
>
> Too big is actually intentionally ignored.
That seems "odd"...
> I split the copy into two segments to check for this.
> 1. First copy the size of the structure
> (if userspace didn't allocate at least sizeof(struct wmi_smbios_buffer) that's a problem)
> 2. Verify the size claimed is "at least" what we internally are looking for.
> 3. Copy the rest of the size internally needed. If userspace sent more it's just not copied.
> 4. When sending it back I only send back up to the "at least" internal size.
That feels strange, are you sure this is correct? Why the odd two step
process here?
What if 'length' is set to an invalid value (too big or small), will you
catch that correctly here?
> > > + if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
> > > + dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
> > > + priv->buf->std.class, priv->buf->std.select,
> > > + priv->buf->std.input[0]);
> > > + ret = -EFAULT;
> > > + goto fail_smbios_cmd;
> > > + }
> > > + size = priv->buffer_size - sizeof(struct wmi_smbios_buffer);
> >
> > What if size just went too small and wrapped around? :(
> >
> > Remember, "All input is evil". Go print that out and put it on the wall
> > when you are designing this user/kernel api. You can trust no one, you
> > have to validate _everything_.
>
> priv->buffer_size can't be set by userspace.
Who sets it? Your structure naming here doesn't make it obvious which
data is from the kernel and which from userspace, making this very hard
to audit :(
thanks,
greg k-h
^ permalink raw reply [flat|nested] 31+ messages in thread* RE: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface
2017-10-07 12:36 ` Greg KH
@ 2017-10-07 13:13 ` Mario.Limonciello
0 siblings, 0 replies; 31+ messages in thread
From: Mario.Limonciello @ 2017-10-07 13:13 UTC (permalink / raw)
To: greg
Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86, luto,
quasisec, pali.rohar, rjw, mjg59, hch
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Saturday, October 7, 2017 7:37 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org;
> quasisec@google.com; pali.rohar@gmail.com; rjw@rjwysocki.net;
> mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
>
> On Sat, Oct 07, 2017 at 12:15:18PM +0000, Mario.Limonciello@dell.com wrote:
> > > > + struct wmi_smbios_priv *priv;
> > > > + int ret = 0;
> > > > + size_t size;
> > > > +
> > > > + switch (cmd) {
> > > > + case DELL_WMI_SMBIOS_CMD:
> > > > + priv = dev_get_drvdata(&wdev->dev);
> > > > + if (!priv)
> > > > + return -ENODEV;
> > > > + size = sizeof(struct wmi_smbios_buffer);
> > > > + mutex_lock(&call_mutex);
> > > > + if (copy_from_user(priv->buf, input, size)) {
>
> Wait, how do you know that input is size big?
How can you check this? I guess just read the first u64 for the data first (where length
is stored). Or is there a better way?
>
> > > > + dev_dbg(&wdev->dev, "Copy %lu from user failed\n",
> > > > + size);
> > > > + ret = -EFAULT;
> > > > + goto fail_smbios_cmd;
> > > > + }
> > > > + if (priv->buf->length < priv->buffer_size) {
> > > > + dev_err(&wdev->dev,
> > > > + "Buffer %lld too small, need at least %d\n",
> > > > + priv->buf->length, priv->buffer_size);
> > > > + ret = -EINVAL;
> > > > + goto fail_smbios_cmd;
> > > > + }
> > >
> > > No checking for too big of a length? Any other fields you should check
> > > for validity? Like too small?
> >
> > Too big is actually intentionally ignored.
>
> That seems "odd"...
>
> > I split the copy into two segments to check for this.
> > 1. First copy the size of the structure
> > (if userspace didn't allocate at least sizeof(struct wmi_smbios_buffer) that's a
> problem)
> > 2. Verify the size claimed is "at least" what we internally are looking for.
> > 3. Copy the rest of the size internally needed. If userspace sent more it's just
> not copied.
> > 4. When sending it back I only send back up to the "at least" internal size.
>
> That feels strange, are you sure this is correct? Why the odd two step
> process here?
>
> What if 'length' is set to an invalid value (too big or small), will you
> catch that correctly here?
Too small is definitely caught, too big won't cause problems due to above logic.
The remaining problem is if length doesn't really represent how much memory
Userspace allocated. I'm not sure how you can actually check that.
>
> > > > + if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
> > > > + dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
> > > > + priv->buf->std.class, priv->buf->std.select,
> > > > + priv->buf->std.input[0]);
> > > > + ret = -EFAULT;
> > > > + goto fail_smbios_cmd;
> > > > + }
> > > > + size = priv->buffer_size - sizeof(struct wmi_smbios_buffer);
> > >
> > > What if size just went too small and wrapped around? :(
> > >
> > > Remember, "All input is evil". Go print that out and put it on the wall
> > > when you are designing this user/kernel api. You can trust no one, you
> > > have to validate _everything_.
> >
> > priv->buffer_size can't be set by userspace.
>
> Who sets it? Your structure naming here doesn't make it obvious which
> data is from the kernel and which from userspace, making this very hard
> to audit :(
It's set during the probe routine. I'll rename it to something more obvious.
I'll also add some comments to the ioctl so it's more apparent.
^ permalink raw reply [flat|nested] 31+ messages in thread