Am 10.01.25 um 23:11 schrieb Derek John Clark:
On Thu, Jan 9, 2025 at 2:35 PM Armin Wolf <W_Armin@xxxxxx> wrote:
Am 02.01.25 um 01:47 schrieb Derek J. Clark:
Adds lenovo-wmi-capdata01.c which provides a driver for the
LENOVO_CAPABILITY_DATA_01 WMI data block that comes on "Other Method"
enabled hardware. Provides an interface for querying if a given
attribute is supported by the hardware, as well as its default_value,
max_value, min_value, and step increment.
v2:
- Use devm_kzalloc to ensure driver can be instanced, remove global
reference.
- Ensure reverse Christmas tree for all variable declarations.
- Remove extra whitespace.
- Use guard(mutex) in all mutex instances, global mutex.
- Use pr_fmt instead of adding the driver name to each pr_err.
- Remove noisy pr_info usage.
- Rename capdata_wmi to lenovo_wmi_cd01_priv and cd01_wmi to priv.
- Use list to get the lenovo_wmi_cd01_priv instance in
lenovo_wmi_capdata01_get as none of the data provided by the macros
that will use it can pass a member of the struct for use in
container_of.
Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
---
MAINTAINERS | 1 +
drivers/platform/x86/Kconfig | 11 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/lenovo-wmi-capdata01.c | 131 ++++++++++++++++++++
drivers/platform/x86/lenovo-wmi.h | 20 +++
5 files changed, 164 insertions(+)
create mode 100644 drivers/platform/x86/lenovo-wmi-capdata01.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8f8a6aec6b92..c9374c395905 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13038,6 +13038,7 @@ LENOVO WMI drivers
M: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
L: platform-driver-x86@xxxxxxxxxxxxxxx
S: Maintained
+F: drivers/platform/x86/lenovo-wmi-capdata01.c
F: drivers/platform/x86/lenovo-wmi-gamezone.c
F: drivers/platform/x86/lenovo-wmi.h
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 9a6ac7fdec9f..a2c1ab47ad9e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -470,6 +470,17 @@ config LENOVO_WMI_GAMEZONE
To compile this driver as a module, choose M here: the module will
be called lenovo_wmi_gamezone.
+config LENOVO_WMI_DATA01
+ tristate "Lenovo Legion WMI capability Data 01 Driver"
+ depends on ACPI_WMI
+ help
+ Say Y here if you have a WMI aware Lenovo Legion device in the "Gaming Series"
+ line of hardware. This interface is a dependency for exposing tunable power
+ settings.
+
+ To compile this driver as a module, choose M here: the module will
+ be called lenovo_wmi_capdata01.
Could it be that the resulting module will be called lenovo-wmi-capdata01? Also if its a mere
dependency without any value when being used alone then it would make sense to hide it from
users by removing the help texts:
config LENOVO_WMI_DATA01
tristate
depends on ACPI_WMI
Makes sense, Will do
+
config IDEAPAD_LAPTOP
tristate "Lenovo IdeaPad Laptop Extras"
depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 7cb29a480ed2..6c96cc3f3855 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_YOGABOOK) += lenovo-yogabook.o
obj-$(CONFIG_YT2_1380) += lenovo-yoga-tab2-pro-1380-fastcharger.o
obj-$(CONFIG_LENOVO_WMI_CAMERA) += lenovo-wmi-camera.o
obj-$(CONFIG_LENOVO_WMI_GAMEZONE) += lenovo-wmi-gamezone.o
+obj-$(CONFIG_LENOVO_WMI_DATA01) += lenovo-wmi-capdata01.o
# Intel
obj-y += intel/
diff --git a/drivers/platform/x86/lenovo-wmi-capdata01.c b/drivers/platform/x86/lenovo-wmi-capdata01.c
new file mode 100644
index 000000000000..b10a6e4b320f
--- /dev/null
+++ b/drivers/platform/x86/lenovo-wmi-capdata01.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * LENOVO_CAPABILITY_DATA_01 WMI data block driver. This interface provides
+ * information on tunable attributes used by the "Other Method" WMI interface,
+ * including if it is supported by the hardware, the default_value, max_value,
+ * min_value, and step increment.
+ *
+ * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@xxxxxxxxx>
+ */
+
+#include <linux/list.h>
+#include "lenovo-wmi.h"
+
+#define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
+
+static DEFINE_MUTEX(cd01_call_mutex);
Does the WMI interface really rely on such mutual exclusion of callers? If no then
please remove this mutex.
As with the other drivers, will remove.
+static DEFINE_MUTEX(cd01_list_mutex);
+static LIST_HEAD(cd01_wmi_list);
+
+static const struct wmi_device_id lenovo_wmi_capdata01_id_table[] = {
+ { LENOVO_CAPABILITY_DATA_01_GUID, NULL },
+ {}
+};
+
+struct lenovo_wmi_cd01_priv {
+ struct wmi_device *wdev;
+ struct list_head list;
+};
+
+static inline struct lenovo_wmi_cd01_priv *get_first_wmi_priv(void)
+{
+ guard(mutex)(&cd01_list_mutex);
+ return list_first_entry_or_null(&cd01_wmi_list,
+ struct lenovo_wmi_cd01_priv, list);
Two things:
1. This will cause issues should a WMI device in this list be removed while a
consumer is using it. In this case you will need extend the scope of the list mutex.
2. Do multiple capdata01 WMI devices exist in any systems? If no then please consider
using the component framework (https://docs.kernel.org/driver-api/component.html) which
will simplify the interop between the consumer driver of capdata01 and this driver.
I looked into this and struggled with it for a while, do you have any
good examples I can reference?
Will this allow me to pass struct lenovo_wmi_cd01_priv *priv to this
function from the other mode driver? If so, should I avoid calling it
priv since it will be accessible outside the driver?
You can use the i915 GSC proxy code as a reference.
For the component supplier (capdata01):
- use drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c as an example
1. Register a component with component_add().
2. Use the .bind callback inside struct component_ops to pass a pointer to an array of capdata01 items to the component master.
For the component master (lenovo-wmi-other):
- use drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c as an example
1. Add a match function using component_match_add(). I suggest that you implement a public function inside the capdata01 driver
which does the following (pseudo code):
int lenovo_wmi_capdata01_match(struct device *dev, void *data)
{
return dev->driver == &lenovo_wmi_capdata01_driver.driver;
}
EXPORT_SYMBOL_GPL(lenovo_wmi_capdata01_match);
This function can then be used to match against the component provided by lenovo-wmi-capdata01.
2. Add the component master using component_master_add_with_match(). Use the .bind callback inside
struct component_master_ops to register the firmware attribute class device. You can use component_bind_all()
to pass a pointer to the bound components. In this case this pointer can be used by the capdata01 driver
to pass an array of capdata01 items to you.
Keep in mind that you cannot use devres inside the .bind callbacks.
+}
+
+int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id,
+ struct capability_data_01 *cap_data)
+{
+ u32 attribute_id = *(int *)&attr_id;
This will cause alignment issues, please use FIELD_GET()/FIELD_PREP() to construct a u32 to
pass to this function.
Can do.
+ struct lenovo_wmi_cd01_priv *priv;
+ union acpi_object *ret_obj;
+ int instance_idx;
+ int count;
+
+ priv = get_first_wmi_priv();
+ if (!priv)
+ return -ENODEV;
+
+ guard(mutex)(&cd01_call_mutex);
+ count = wmidev_instance_count(priv->wdev);
+ pr_info("Got instance count: %u\n", count);
+
+ for (instance_idx = 0; instance_idx < count; instance_idx++) {
+ ret_obj = wmidev_block_query(priv->wdev, instance_idx);
+ if (!ret_obj) {
+ pr_err("WMI Data block query failed.\n");
+ continue;
+ }
+
+ if (ret_obj->type != ACPI_TYPE_BUFFER) {
+ pr_err("WMI Data block query returned wrong type.\n");
+ kfree(ret_obj);
+ continue;
+ }
+
+ if (ret_obj->buffer.length != sizeof(*cap_data)) {
+ pr_err("WMI Data block query returned wrong buffer length: %u vice expected %lu.\n",
+ ret_obj->buffer.length, sizeof(*cap_data));
+ kfree(ret_obj);
+ continue;
+ }
+
+ memcpy(cap_data, ret_obj->buffer.pointer,
+ ret_obj->buffer.length);
+ kfree(ret_obj);
+
+ if (cap_data->id != attribute_id)
+ continue;
+ break;
+ }
Maybe it would make sense to read this data during device initialization and store it
inside an array? This way looking up capability data would be _much_ faster especially
since WMI calls are usually quite slow.
I was looking into this as I agree that would be preferable but wasn't
able to get a working version. Since I don't know the array length at
compile time I tried using krealloc_array after getting
wmidev_instance_count to resize a capdata array stored in priv, but
that would result in the driver crashing for some reason. I'll take
another shot at it.
I suggest you use devm_kmalloc_array(), with the first argument being the return value of wmidev_instance_count().
The second parameter will be the size of a capdata01 structure.
Then you just need to fill-in the capdata01 structures using a for-loop.
Also this function is way to noisy when it comes to error messages. Please leave this
to the caller of this function.
Can do. If I don't get a ret_obj should I quit the loop here?
You can continue the loop in such a case, so that the other attributes are still available to the lenovo-wmi-other driver.
However please print a warning message if you skip an instance so that users can find out why some attributes are missing.
Thanks,
Armin Wolf
+
+ if (cap_data->id != attribute_id) {
+ pr_err("Unable to find capability data for attribute_id %x\n",
+ attribute_id);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(lenovo_wmi_capdata01_get, "CAPDATA_WMI");
+
+static int lenovo_wmi_capdata01_probe(struct wmi_device *wdev,
+ const void *context)
+
+{
+ struct lenovo_wmi_cd01_priv *priv;
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->wdev = wdev;
+
+ guard(mutex)(&cd01_list_mutex);
+ list_add_tail(&priv->list, &cd01_wmi_list);
+
+ return 0;
+}
+
+static void lenovo_wmi_capdata01_remove(struct wmi_device *wdev)
+{
+ struct lenovo_wmi_cd01_priv *priv = dev_get_drvdata(&wdev->dev);
+
+ guard(mutex)(&cd01_list_mutex);
+ list_del(&priv->list);
+}
+
+static struct wmi_driver lenovo_wmi_capdata01_driver = {
+ .driver = { .name = "lenovo_wmi_capdata01" },
Please set ".probe_type = PROBE_PREFER_ASYNCHRONOUS".
Ack
+ .id_table = lenovo_wmi_capdata01_id_table,
+ .probe = lenovo_wmi_capdata01_probe,
+ .remove = lenovo_wmi_capdata01_remove,
Please set ".no_singleton = true".
Ack
Thanks,
Derek
Thanks,
Armin Wolf
+};
+
+module_wmi_driver(lenovo_wmi_capdata01_driver);
+
+MODULE_DEVICE_TABLE(wmi, lenovo_wmi_capdata01_id_table);
+MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@xxxxxxxxx>");
+MODULE_DESCRIPTION("Lenovo Capability Data 01 WMI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h
index 8a302c6c47cb..53cea84a956b 100644
--- a/drivers/platform/x86/lenovo-wmi.h
+++ b/drivers/platform/x86/lenovo-wmi.h
@@ -36,6 +36,22 @@ struct wmi_method_args {
u32 arg1;
};
+struct lenovo_wmi_attr_id {
+ u32 mode_id : 16; /* Fan profile */
+ u32 feature_id : 8; /* Attribute (SPL/SPPT/...) */
+ u32 device_id : 8; /* CPU/GPU/... */
+} __packed;
+
+/* Data struct for LENOVO_CAPABILITY_DATA_01 */
+struct capability_data_01 {
+ u32 id;
+ u32 supported;
+ u32 default_value;
+ u32 step;
+ u32 min_value;
+ u32 max_value;
+};
+
/* General Use functions */
static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance,
u32 method_id, struct acpi_buffer *in,
@@ -102,4 +118,8 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
0, retval);
}
+/* LENOVO_CAPABILITY_DATA_01 exported functions */
+int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id,
+ struct capability_data_01 *cap_data);
+
#endif /* !_LENOVO_WMI_H_ */
