| Message ID | 20250102004854.14874-5-derekjohn.clark@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: Add Lenovo Gaming Series WMI Drivers | expand |
On 1/1/25 18:47, Derek J. Clark wrote: > Adds lenovo-wmi-other.c which provides a driver for the Lenovo > "Other Mode" WMI interface that comes on some Lenovo "Gaming > Series" hardware. Provides a firmware-attributes class which > enables the use of tunable knobs for SPL, SPPT, and FPPT. > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. > - Use list to get the lenovo_wmi_om_priv instance in some macro > called functions as the data provided by the macros that use it > doesn't pass a member of the struct for use in container_of. > - Do not rely on GameZone interface to grab the current fan mode. > > Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> > --- > MAINTAINERS | 1 + > drivers/platform/x86/Kconfig | 12 + > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ > drivers/platform/x86/lenovo-wmi.h | 116 +++++++ > 5 files changed, 515 insertions(+) > create mode 100644 drivers/platform/x86/lenovo-wmi-other.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index c9374c395905..318e1e517eed 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org > S: Maintained > F: drivers/platform/x86/lenovo-wmi-capdata01.c > F: drivers/platform/x86/lenovo-wmi-gamezone.c > +F: drivers/platform/x86/lenovo-wmi-other.c > F: drivers/platform/x86/lenovo-wmi.h > > LETSKETCH HID TABLET DRIVER > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index a2c1ab47ad9e..e2285ab987c5 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 > To compile this driver as a module, choose M here: the module will > be called lenovo_wmi_capdata01. > > +config LENOVO_WMI_TUNING > + tristate "Lenovo Other Method WMI Driver" > + depends on LENOVO_WMI_DATA01 > + select FW_ATTR_CLASS > + help > + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the > + firmware_attributes API to control various tunable settings typically exposed by > + Lenovo software in Windows. > + > + To compile this driver as a module, choose M here: the module will > + be called lenovo_wmi_other. > + > config IDEAPAD_LAPTOP > tristate "Lenovo IdeaPad Laptop Extras" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 6c96cc3f3855..3e059b3c3647 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -70,6 +70,7 @@ 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 > +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o > > # Intel > obj-y += intel/ > diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c > new file mode 100644 > index 000000000000..2392faa74973 > --- /dev/null > +++ b/drivers/platform/x86/lenovo-wmi-other.c > @@ -0,0 +1,385 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes > + * class to expose the various WMI functions provided by the "Other Method" WMI > + * interface. This enables CPU and GPU power limit as well as various other > + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop > + * devices. Each attribute exposed by the "Other Method"" interface has a > + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to > + * probe details about the attribute such as set/get support, step, min, max, > + * and default value. Each attibute has multiple pages, one for each of the > + * fan profiles managed by the GameZone interface, so it must be probed prior > + * to returning the current_value. > + * > + * These attributes typically don't fit anywhere else in the sysfs and are set > + * in Windows using one of Lenovo's multiple user applications. > + * > + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> > + */ > + > +#include <linux/list.h> > +#include "lenovo-wmi.h" > +#include "firmware_attributes_class.h" > + > +#define FW_ATTR_FOLDER "lenovo-wmi-other" > +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" > + > +/* Device IDs */ > +#define WMI_DEVICE_ID_CPU 0x01 > + > +/* WMI_DEVICE_ID_CPU feature IDs */ > +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ > +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ > +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ > +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ What exactly is WMI_FEATURE_ID_CPU_FPPT_BAD? I don't see it used in the code at all. > + > +/* Method IDs */ > +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ > +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ > + > +static DEFINE_MUTEX(call_mutex); > +static DEFINE_MUTEX(om_list_mutex); > +static LIST_HEAD(om_wmi_list); > + > +struct lenovo_wmi_om_priv { > + struct wmi_device *wdev; > + struct device *fw_attr_dev; > + struct kset *fw_attr_kset; > + struct list_head list; > +}; > + > +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) > +{ > + guard(mutex)(&om_list_mutex); > + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, > + list); > +} > + > +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { > + { LENOVO_OTHER_METHOD_GUID, NULL }, > + {} > +}; > + > +/* Tunable Attributes */ > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, > + .feature_id = WMI_FEATURE_ID_CPU_SPL }; > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, > + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, > + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; > + > +struct capdata01_attr_group { > + const struct attribute_group *attr_group; > + struct tunable_attr_01 *tunable_attr; > +}; > + > +static const struct class *fw_attr_class; > + > +/** > + * attr_capdata01_setup() - Get the data of the specified attribute > + * from LENOVO_CAPABILITY_DATA_01 and store it. > + * @tunable_attr: The attribute to be populated. > + * > + * Returns: Either 0 or an error. > + */ > +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) > +{ > + struct capability_data_01 cap_data; > + int mode = SMARTFAN_MODE_CUSTOM; > + int err; > + Why the whitespace here? Seems unnecessary. > + struct lenovo_wmi_attr_id attr_id = { mode << 8, As mode is only used here, I would just do: { SMARTFAN_MODE_CUSTOM << 8, To avoid the extra variable. > + tunable_attr->feature_id, > + tunable_attr->device_id }; > + > + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); > + if (err) { > + pr_err("Failed to get capability data: %u\n", err); > + return err; > + } > + > + if (cap_data.supported < 1) > + return -EOPNOTSUPP; > + > + tunable_attr->capdata = cap_data; > + > + return 0; > +} > + > +/** > + * attr_capdata01_show() - Get the value of the specified attribute property > + * from LENOVO_CAPABILITY_DATA_01. > + * @kobj: Pointer to the driver object. > + * @kobj_attribute: Pointer to the attribute calling this function. > + * @buf: The buffer to write to. > + * @tunable_attr: The attribute to be read. > + * @prop: The property of this attribute to be read. > + * > + * This function is intended to be generic so it can be called from any "_show" > + * attribute which works only with integers. > + * > + * If the WMI is success, then the sysfs attribute is notified. > + * > + * Returns: Either count, or an error. > + */ > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf, struct tunable_attr_01 *tunable_attr, > + enum attribute_property prop) > +{ > + struct capability_data_01 cap_data; > + int retval; > + > + cap_data = tunable_attr->capdata; > + > + switch (prop) { > + case DEFAULT_VAL: > + retval = cap_data.default_value; > + break; > + case MAX_VAL: > + retval = cap_data.max_value; > + break; > + case MIN_VAL: > + retval = cap_data.min_value; > + break; > + case STEP_VAL: > + retval = cap_data.step; > + break; > + default: > + return -EINVAL; > + } > + return sysfs_emit(buf, "%u\n", retval); > +} > + > +/* Simple attribute creation */ > + > +/* > + * att_current_value_store() - Set the current value of the given attribute > + * @kobj: Pointer to the driver object. > + * @kobj_attribute: Pointer to the attribute calling this function. > + * @buf: The buffer to read from, this is parsed to `int` type. > + * @count: Required by sysfs attribute macros, pass in from the callee attr. > + * @tunable_attr: The attribute to be stored. > + * > + * This function is intended to be generic so it can be called from any > + * attribute's "current_value_store" which works only with integers. The > + * integer to be sent to the WMI method is range checked and an error returned > + * if out of range. > + * > + * If the value is valid and WMI is success, then the sysfs attribute is > + * notified. > + * > + * Returns: Either count, or an error. > + */ > +ssize_t attr_current_value_store(struct kobject *kobj, > + struct kobj_attribute *attr, const char *buf, > + size_t count, > + struct tunable_attr_01 *tunable_attr) > +{ > + struct capability_data_01 cap_data; > + struct lenovo_wmi_om_priv *priv; > + int mode = SMARTFAN_MODE_CUSTOM; > + u32 value; > + int err; > + > + struct lenovo_wmi_attr_id attr_id = { mode << 8, Similar comment about the mode here too. > + tunable_attr->feature_id, > + tunable_attr->device_id }; > + > + err = kstrtouint(buf, 10, &value); > + if (err) { > + pr_err("Error converting value to int: %u\n", err); > + return err; > + } > + > + cap_data = tunable_attr->capdata; > + > + if (value < cap_data.min_value || value > cap_data.max_value) > + return -EINVAL; > + > + priv = get_first_wmi_priv(); > + if (!priv) > + return -ENODEV; > + > + guard(mutex)(&call_mutex); > + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, > + WMI_METHOD_ID_VALUE_SET, > + *(int *)&attr_id, value, NULL); > + > + if (err) { > + pr_err("Error setting attribute: %u\n", err); > + return err; > + } > + > + tunable_attr->store_value = value; > + > + sysfs_notify(kobj, NULL, attr->attr.name); > + > + return count; > +}; > + > +/* > + * attr_current_value_show() - Get the current value of the given attribute > + * @kobj: Pointer to the driver object. > + * @kobj_attribute: Pointer to the attribute calling this function. > + * @buf: The buffer to write to. > + * @tunable_attr: The attribute to be read. > + * > + * This function is intended to be generic so it can be called from any "_show" > + * attribute which works only with integers. > + * > + * If the WMI is success, then the sysfs attribute is notified. > + * > + * Returns: Either count, or an error. > + */ > +ssize_t attr_current_value_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf, > + struct tunable_attr_01 *tunable_attr) > +{ > + struct lenovo_wmi_om_priv *priv; > + int mode = SMARTFAN_MODE_CUSTOM; > + int retval; > + int err; > + > + struct lenovo_wmi_attr_id attr_id = { mode << 8, Same comment about SMARTFAN_MODE_CUSTOM here. > + tunable_attr->feature_id, > + tunable_attr->device_id }; > + > + priv = get_first_wmi_priv(); > + if (!priv) > + return -ENODEV; > + > + guard(mutex)(&call_mutex); > + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, > + WMI_METHOD_ID_VALUE_GET, > + *(int *)&attr_id, &retval); > + > + if (err) { > + pr_err("Error getting attribute: %u\n", err); > + return err; > + } > + > + return sysfs_emit(buf, "%u\n", retval); > +} > + > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", > + "Set the CPU sustained power limit"); > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", > + "Set the CPU slow package power tracking limit"); > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", > + "Set the CPU fast package power tracking limit"); > + > +static const struct capdata01_attr_group capdata01_attr_groups[] = { > + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, > + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, > + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, > + {}, > +}; > + > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) > +{ > + int err, i; > + > + err = fw_attributes_class_get(&fw_attr_class); > + if (err) { > + pr_err("Failed to get firmware_attributes_class: %u\n", err); > + return err; > + } > + > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > + NULL, "%s", FW_ATTR_FOLDER); > + if (IS_ERR(priv->fw_attr_dev)) { > + err = PTR_ERR(priv->fw_attr_dev); > + pr_err("Failed to create firmware_attributes_class device: %u\n", > + err); > + goto fail_class_get; > + } > + > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, > + &priv->fw_attr_dev->kobj); > + if (!priv->fw_attr_kset) { > + err = -ENOMEM; > + pr_err("Failed to create firmware_attributes_class kset: %u\n", > + err); > + goto err_destroy_classdev; > + } > + > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { > + err = attr_capdata01_setup( > + capdata01_attr_groups[i].tunable_attr); > + if (err) { > + pr_err("Failed to populate capability data for %s: %u\n", > + capdata01_attr_groups[i].attr_group->name, err); This specific error could be a bit noisy because it's a dependency on the other driver in case one attribute returns not supported. Could you instead detect EOPNOTSUPP specifically and only show error if not EOPNOTSUPP? > + continue; > + } > + > + err = sysfs_create_group(&priv->fw_attr_kset->kobj, > + capdata01_attr_groups[i].attr_group); > + if (err) { > + pr_err("Failed to create sysfs-group for %s: %u\n", > + capdata01_attr_groups[i].attr_group->name, err); > + goto err_remove_groups; > + } > + } > + > + return 0; > + > +err_remove_groups: > + while (i-- > 0) { > + sysfs_remove_group(&priv->fw_attr_kset->kobj, > + capdata01_attr_groups[i].attr_group); > + } > + > + return err; > + > +err_destroy_classdev: > + device_destroy(fw_attr_class, MKDEV(0, 0)); > + > + return err; > + > +fail_class_get: > + fw_attributes_class_put(); > + > + return err; > +} > + > +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context) > +{ > + struct lenovo_wmi_om_priv *priv; > + > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->wdev = wdev; > + > + guard(mutex)(&om_list_mutex); > + list_add_tail(&priv->list, &om_wmi_list); > + > + return other_method_fw_attr_add(priv); > +} > + > +static void lenovo_wmi_other_remove(struct wmi_device *wdev) > +{ > + struct lenovo_wmi_om_priv *priv = dev_get_drvdata(&wdev->dev); > + > + guard(mutex)(&om_list_mutex); > + list_del(&priv->list); > + kset_unregister(priv->fw_attr_kset); > + device_destroy(fw_attr_class, MKDEV(0, 0)); > + fw_attributes_class_put(); > +} > + > +static struct wmi_driver lenovo_wmi_other_driver = { > + .driver = { .name = "lenovo_wmi_other" }, > + .id_table = lenovo_wmi_other_id_table, > + .probe = lenovo_wmi_other_probe, > + .remove = lenovo_wmi_other_remove, > +}; > + > +module_wmi_driver(lenovo_wmi_other_driver); > + > +MODULE_IMPORT_NS("CAPDATA_WMI"); > +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table); > +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>"); > +MODULE_DESCRIPTION("Lenovo Other Method WMI Driver"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h > index 53cea84a956b..1c8358551ba6 100644 > --- a/drivers/platform/x86/lenovo-wmi.h > +++ b/drivers/platform/x86/lenovo-wmi.h > @@ -42,6 +42,14 @@ struct lenovo_wmi_attr_id { > u32 device_id : 8; /* CPU/GPU/... */ > } __packed; > > +enum attribute_property { > + DEFAULT_VAL, > + MAX_VAL, > + MIN_VAL, > + STEP_VAL, > + SUPPORTED, > +}; > + > /* Data struct for LENOVO_CAPABILITY_DATA_01 */ > struct capability_data_01 { > u32 id; > @@ -52,6 +60,14 @@ struct capability_data_01 { > u32 max_value; > }; > > +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */ > +struct tunable_attr_01 { > + struct capability_data_01 capdata; > + u32 device_id; > + u32 feature_id; > + u32 store_value; > +}; > + > /* General Use functions */ > static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, > u32 method_id, struct acpi_buffer *in, > @@ -122,4 +138,104 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance, > int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id, > struct capability_data_01 *cap_data); > > +/* Other Method attribute functions */ > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf, struct tunable_attr_01 *tunable_attr, > + enum attribute_property prop); > + > +ssize_t attr_current_value_store(struct kobject *kobj, > + struct kobj_attribute *attr, const char *buf, > + size_t count, > + struct tunable_attr_01 *tunable_attr); > + > +ssize_t attr_current_value_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf, > + struct tunable_attr_01 *tunable_attr); > + > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf); > + > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "integer\n"); > +} > + > +/* Other Method attribute macros */ > +#define __LL_ATTR_RO(_func, _name) \ > + { \ > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > + .show = _func##_##_name##_show, \ > + } > + > +#define __LL_ATTR_RO_AS(_name, _show) \ > + { \ > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > + .show = _show, \ > + } > + > +#define __LL_ATTR_RW(_func, _name) \ > + __ATTR(_name, 0644, _func##_##_name##_show, _func##_##_name##_store) > + > +/* Shows a formatted static variable */ > +#define __ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val) \ > + static ssize_t _attrname##_##_prop##_show( \ > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > + { \ > + return sysfs_emit(buf, _fmt, _val); \ > + } \ > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > + __LL_ATTR_RO(_attrname, _prop) > + > +/* Attribute current_value show/store */ > +#define __LL_TUNABLE_RW_CAP01(_attrname) \ > + static ssize_t _attrname##_current_value_store( \ > + struct kobject *kobj, struct kobj_attribute *attr, \ > + const char *buf, size_t count) \ > + { \ > + return attr_current_value_store(kobj, attr, buf, count, \ > + &_attrname); \ > + } \ > + static ssize_t _attrname##_current_value_show( \ > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > + { \ > + return attr_current_value_show(kobj, attr, buf, &_attrname); \ > + } \ > + static struct kobj_attribute attr_##_attrname##_current_value = \ > + __LL_ATTR_RW(_attrname, current_value) > + > +/* Attribute property show only */ > +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \ > + static ssize_t _attrname##_##_prop##_show( \ > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > + { \ > + return attr_capdata01_show(kobj, attr, buf, &_attrname, \ > + _prop_type); \ > + } \ > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > + __LL_ATTR_RO(_attrname, _prop) > + > +#define ATTR_GROUP_LL_TUNABLE_CAP01(_attrname, _fsname, _dispname) \ > + __LL_TUNABLE_RW_CAP01(_attrname); \ > + __LL_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL); \ > + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \ > + __LL_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL); \ > + __LL_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL); \ > + __LL_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL); \ > + static struct kobj_attribute attr_##_attrname##_type = \ > + __LL_ATTR_RO_AS(type, int_type_show); \ > + static struct attribute *_attrname##_attrs[] = { \ > + &attr_##_attrname##_current_value.attr, \ > + &attr_##_attrname##_default_value.attr, \ > + &attr_##_attrname##_display_name.attr, \ > + &attr_##_attrname##_max_value.attr, \ > + &attr_##_attrname##_min_value.attr, \ > + &attr_##_attrname##_scalar_increment.attr, \ > + &attr_##_attrname##_type.attr, \ > + NULL, \ > + }; \ > + static const struct attribute_group _attrname##_attr_group = { \ > + .name = _fsname, .attrs = _attrname##_attrs \ > + } > + > #endif /* !_LENOVO_WMI_H_ */
Hi Derek, kernel test robot noticed the following build errors: [auto build test ERROR on amd-pstate/linux-next] [also build test ERROR on amd-pstate/bleeding-edge linus/master v6.13-rc5 next-20241220] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Derek-J-Clark/platform-x86-Add-lenovo-wmi-drivers-Documentation/20250102-085149 base: https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git linux-next patch link: https://lore.kernel.org/r/20250102004854.14874-5-derekjohn.clark%40gmail.com patch subject: [PATCH v2 4/4] platform/x86: Add Lenovo Other Mode WMI Driver config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20250102/202501021728.uZ2voPKr-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250102/202501021728.uZ2voPKr-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501021728.uZ2voPKr-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/platform/x86/lenovo-wmi-other.c: In function 'other_method_fw_attr_add': >> drivers/platform/x86/lenovo-wmi-other.c:288:64: error: implicit declaration of function 'MKDEV' [-Werror=implicit-function-declaration] 288 | priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), | ^~~~~ cc1: some warnings being treated as errors vim +/MKDEV +288 drivers/platform/x86/lenovo-wmi-other.c 277 278 static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) 279 { 280 int err, i; 281 282 err = fw_attributes_class_get(&fw_attr_class); 283 if (err) { 284 pr_err("Failed to get firmware_attributes_class: %u\n", err); 285 return err; 286 } 287 > 288 priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), 289 NULL, "%s", FW_ATTR_FOLDER); 290 if (IS_ERR(priv->fw_attr_dev)) { 291 err = PTR_ERR(priv->fw_attr_dev); 292 pr_err("Failed to create firmware_attributes_class device: %u\n", 293 err); 294 goto fail_class_get; 295 } 296 297 priv->fw_attr_kset = kset_create_and_add("attributes", NULL, 298 &priv->fw_attr_dev->kobj); 299 if (!priv->fw_attr_kset) { 300 err = -ENOMEM; 301 pr_err("Failed to create firmware_attributes_class kset: %u\n", 302 err); 303 goto err_destroy_classdev; 304 } 305 306 for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { 307 err = attr_capdata01_setup( 308 capdata01_attr_groups[i].tunable_attr); 309 if (err) { 310 pr_err("Failed to populate capability data for %s: %u\n", 311 capdata01_attr_groups[i].attr_group->name, err); 312 continue; 313 } 314 315 err = sysfs_create_group(&priv->fw_attr_kset->kobj, 316 capdata01_attr_groups[i].attr_group); 317 if (err) { 318 pr_err("Failed to create sysfs-group for %s: %u\n", 319 capdata01_attr_groups[i].attr_group->name, err); 320 goto err_remove_groups; 321 } 322 } 323 324 return 0; 325 326 err_remove_groups: 327 while (i-- > 0) { 328 sysfs_remove_group(&priv->fw_attr_kset->kobj, 329 capdata01_attr_groups[i].attr_group); 330 } 331 332 return err; 333 334 err_destroy_classdev: 335 device_destroy(fw_attr_class, MKDEV(0, 0)); 336 337 return err; 338 339 fail_class_get: 340 fw_attributes_class_put(); 341 342 return err; 343 } 344
On Wed, Jan 1, 2025 at 7:40 PM Mario Limonciello <superm1@kernel.org> wrote: > > > > On 1/1/25 18:47, Derek J. Clark wrote: > > Adds lenovo-wmi-other.c which provides a driver for the Lenovo > > "Other Mode" WMI interface that comes on some Lenovo "Gaming > > Series" hardware. Provides a firmware-attributes class which > > enables the use of tunable knobs for SPL, SPPT, and FPPT. > > > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. > > - Use list to get the lenovo_wmi_om_priv instance in some macro > > called functions as the data provided by the macros that use it > > doesn't pass a member of the struct for use in container_of. > > - Do not rely on GameZone interface to grab the current fan mode. > > > > Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> > > --- > > MAINTAINERS | 1 + > > drivers/platform/x86/Kconfig | 12 + > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ > > drivers/platform/x86/lenovo-wmi.h | 116 +++++++ > > 5 files changed, 515 insertions(+) > > create mode 100644 drivers/platform/x86/lenovo-wmi-other.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index c9374c395905..318e1e517eed 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org > > S: Maintained > > F: drivers/platform/x86/lenovo-wmi-capdata01.c > > F: drivers/platform/x86/lenovo-wmi-gamezone.c > > +F: drivers/platform/x86/lenovo-wmi-other.c > > F: drivers/platform/x86/lenovo-wmi.h > > > > LETSKETCH HID TABLET DRIVER > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > index a2c1ab47ad9e..e2285ab987c5 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 > > To compile this driver as a module, choose M here: the module will > > be called lenovo_wmi_capdata01. > > > > +config LENOVO_WMI_TUNING > > + tristate "Lenovo Other Method WMI Driver" > > + depends on LENOVO_WMI_DATA01 > > + select FW_ATTR_CLASS > > + help > > + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the > > + firmware_attributes API to control various tunable settings typically exposed by > > + Lenovo software in Windows. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called lenovo_wmi_other. > > + > > config IDEAPAD_LAPTOP > > tristate "Lenovo IdeaPad Laptop Extras" > > depends on ACPI > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > index 6c96cc3f3855..3e059b3c3647 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -70,6 +70,7 @@ 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 > > +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o > > > > # Intel > > obj-y += intel/ > > diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c > > new file mode 100644 > > index 000000000000..2392faa74973 > > --- /dev/null > > +++ b/drivers/platform/x86/lenovo-wmi-other.c > > @@ -0,0 +1,385 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes > > + * class to expose the various WMI functions provided by the "Other Method" WMI > > + * interface. This enables CPU and GPU power limit as well as various other > > + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop > > + * devices. Each attribute exposed by the "Other Method"" interface has a > > + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to > > + * probe details about the attribute such as set/get support, step, min, max, > > + * and default value. Each attibute has multiple pages, one for each of the > > + * fan profiles managed by the GameZone interface, so it must be probed prior > > + * to returning the current_value. > > + * > > + * These attributes typically don't fit anywhere else in the sysfs and are set > > + * in Windows using one of Lenovo's multiple user applications. > > + * > > + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> > > + */ > > + > > +#include <linux/list.h> > > +#include "lenovo-wmi.h" > > +#include "firmware_attributes_class.h" > > + > > +#define FW_ATTR_FOLDER "lenovo-wmi-other" > > +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" > > + > > +/* Device IDs */ > > +#define WMI_DEVICE_ID_CPU 0x01 > > + > > +/* WMI_DEVICE_ID_CPU feature IDs */ > > +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ > > +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ > > +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ > > +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ > > What exactly is WMI_FEATURE_ID_CPU_FPPT_BAD? I don't see it used in the > code at all. > Something I was going to add a quirk for based on some bad gouge but missed in my cleanup. I'll remove. > > + > > +/* Method IDs */ > > +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ > > +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ > > + > > +static DEFINE_MUTEX(call_mutex); > > +static DEFINE_MUTEX(om_list_mutex); > > +static LIST_HEAD(om_wmi_list); > > + > > +struct lenovo_wmi_om_priv { > > + struct wmi_device *wdev; > > + struct device *fw_attr_dev; > > + struct kset *fw_attr_kset; > > + struct list_head list; > > +}; > > + > > +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) > > +{ > > + guard(mutex)(&om_list_mutex); > > + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, > > + list); > > +} > > + > > +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { > > + { LENOVO_OTHER_METHOD_GUID, NULL }, > > + {} > > +}; > > + > > +/* Tunable Attributes */ > > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, > > + .feature_id = WMI_FEATURE_ID_CPU_SPL }; > > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, > > + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; > > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, > > + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; > > + > > +struct capdata01_attr_group { > > + const struct attribute_group *attr_group; > > + struct tunable_attr_01 *tunable_attr; > > +}; > > + > > +static const struct class *fw_attr_class; > > + > > +/** > > + * attr_capdata01_setup() - Get the data of the specified attribute > > + * from LENOVO_CAPABILITY_DATA_01 and store it. > > + * @tunable_attr: The attribute to be populated. > > + * > > + * Returns: Either 0 or an error. > > + */ > > +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) > > +{ > > + struct capability_data_01 cap_data; > > + int mode = SMARTFAN_MODE_CUSTOM; > > + int err; > > + > > Why the whitespace here? Seems unnecessary. > > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > As mode is only used here, I would just do: > > { SMARTFAN_MODE_CUSTOM << 8, > > To avoid the extra variable. > Acked. > > + tunable_attr->feature_id, > > + tunable_attr->device_id }; > > + > > + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); > > + if (err) { > > + pr_err("Failed to get capability data: %u\n", err); > > + return err; > > + } > > + > > + if (cap_data.supported < 1) > > + return -EOPNOTSUPP; > > + > > + tunable_attr->capdata = cap_data; > > + > > + return 0; > > +} > > + > > +/** > > + * attr_capdata01_show() - Get the value of the specified attribute property > > + * from LENOVO_CAPABILITY_DATA_01. > > + * @kobj: Pointer to the driver object. > > + * @kobj_attribute: Pointer to the attribute calling this function. > > + * @buf: The buffer to write to. > > + * @tunable_attr: The attribute to be read. > > + * @prop: The property of this attribute to be read. > > + * > > + * This function is intended to be generic so it can be called from any "_show" > > + * attribute which works only with integers. > > + * > > + * If the WMI is success, then the sysfs attribute is notified. > > + * > > + * Returns: Either count, or an error. > > + */ > > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf, struct tunable_attr_01 *tunable_attr, > > + enum attribute_property prop) > > +{ > > + struct capability_data_01 cap_data; > > + int retval; > > + > > + cap_data = tunable_attr->capdata; > > + > > + switch (prop) { > > + case DEFAULT_VAL: > > + retval = cap_data.default_value; > > + break; > > + case MAX_VAL: > > + retval = cap_data.max_value; > > + break; > > + case MIN_VAL: > > + retval = cap_data.min_value; > > + break; > > + case STEP_VAL: > > + retval = cap_data.step; > > + break; > > + default: > > + return -EINVAL; > > + } > > + return sysfs_emit(buf, "%u\n", retval); > > +} > > + > > +/* Simple attribute creation */ > > + > > +/* > > + * att_current_value_store() - Set the current value of the given attribute > > + * @kobj: Pointer to the driver object. > > + * @kobj_attribute: Pointer to the attribute calling this function. > > + * @buf: The buffer to read from, this is parsed to `int` type. > > + * @count: Required by sysfs attribute macros, pass in from the callee attr. > > + * @tunable_attr: The attribute to be stored. > > + * > > + * This function is intended to be generic so it can be called from any > > + * attribute's "current_value_store" which works only with integers. The > > + * integer to be sent to the WMI method is range checked and an error returned > > + * if out of range. > > + * > > + * If the value is valid and WMI is success, then the sysfs attribute is > > + * notified. > > + * > > + * Returns: Either count, or an error. > > + */ > > +ssize_t attr_current_value_store(struct kobject *kobj, > > + struct kobj_attribute *attr, const char *buf, > > + size_t count, > > + struct tunable_attr_01 *tunable_attr) > > +{ > > + struct capability_data_01 cap_data; > > + struct lenovo_wmi_om_priv *priv; > > + int mode = SMARTFAN_MODE_CUSTOM; > > + u32 value; > > + int err; > > + > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > Similar comment about the mode here too. > > > + tunable_attr->feature_id, > > + tunable_attr->device_id }; > > + > > + err = kstrtouint(buf, 10, &value); > > + if (err) { > > + pr_err("Error converting value to int: %u\n", err); > > + return err; > > + } > > + > > + cap_data = tunable_attr->capdata; > > + > > + if (value < cap_data.min_value || value > cap_data.max_value) > > + return -EINVAL; > > + > > + priv = get_first_wmi_priv(); > > + if (!priv) > > + return -ENODEV; > > + > > + guard(mutex)(&call_mutex); > > + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, > > + WMI_METHOD_ID_VALUE_SET, > > + *(int *)&attr_id, value, NULL); > > + > > + if (err) { > > + pr_err("Error setting attribute: %u\n", err); > > + return err; > > + } > > + > > + tunable_attr->store_value = value; > > + > > + sysfs_notify(kobj, NULL, attr->attr.name); > > + > > + return count; > > +}; > > + > > +/* > > + * attr_current_value_show() - Get the current value of the given attribute > > + * @kobj: Pointer to the driver object. > > + * @kobj_attribute: Pointer to the attribute calling this function. > > + * @buf: The buffer to write to. > > + * @tunable_attr: The attribute to be read. > > + * > > + * This function is intended to be generic so it can be called from any "_show" > > + * attribute which works only with integers. > > + * > > + * If the WMI is success, then the sysfs attribute is notified. > > + * > > + * Returns: Either count, or an error. > > + */ > > +ssize_t attr_current_value_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf, > > + struct tunable_attr_01 *tunable_attr) > > +{ > > + struct lenovo_wmi_om_priv *priv; > > + int mode = SMARTFAN_MODE_CUSTOM; > > + int retval; > > + int err; > > + > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > Same comment about SMARTFAN_MODE_CUSTOM here. > In this case it may be needed, discussion ongoing in thread 0/4. > > + tunable_attr->feature_id, > > + tunable_attr->device_id }; > > + > > + priv = get_first_wmi_priv(); > > + if (!priv) > > + return -ENODEV; > > + > > + guard(mutex)(&call_mutex); > > + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, > > + WMI_METHOD_ID_VALUE_GET, > > + *(int *)&attr_id, &retval); > > + > > + if (err) { > > + pr_err("Error getting attribute: %u\n", err); > > + return err; > > + } > > + > > + return sysfs_emit(buf, "%u\n", retval); > > +} > > + > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", > > + "Set the CPU sustained power limit"); > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", > > + "Set the CPU slow package power tracking limit"); > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", > > + "Set the CPU fast package power tracking limit"); > > + > > +static const struct capdata01_attr_group capdata01_attr_groups[] = { > > + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, > > + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, > > + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, > > + {}, > > +}; > > + > > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) > > +{ > > + int err, i; > > + > > + err = fw_attributes_class_get(&fw_attr_class); > > + if (err) { > > + pr_err("Failed to get firmware_attributes_class: %u\n", err); > > + return err; > > + } > > + > > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > > + NULL, "%s", FW_ATTR_FOLDER); > > + if (IS_ERR(priv->fw_attr_dev)) { > > + err = PTR_ERR(priv->fw_attr_dev); > > + pr_err("Failed to create firmware_attributes_class device: %u\n", > > + err); > > + goto fail_class_get; > > + } > > + > > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, > > + &priv->fw_attr_dev->kobj); > > + if (!priv->fw_attr_kset) { > > + err = -ENOMEM; > > + pr_err("Failed to create firmware_attributes_class kset: %u\n", > > + err); > > + goto err_destroy_classdev; > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { > > + err = attr_capdata01_setup( > > + capdata01_attr_groups[i].tunable_attr); > > + if (err) { > > + pr_err("Failed to populate capability data for %s: %u\n", > > + capdata01_attr_groups[i].attr_group->name, err); > > This specific error could be a bit noisy because it's a dependency on > the other driver in case one attribute returns not supported. > > Could you instead detect EOPNOTSUPP specifically and only show error if > not EOPNOTSUPP? > Easy fix, will do. I'll also add a wmi_dev_exists() here before the loop to exit early. > > + continue; > > + } > > + > > + err = sysfs_create_group(&priv->fw_attr_kset->kobj, > > + capdata01_attr_groups[i].attr_group); > > + if (err) { > > + pr_err("Failed to create sysfs-group for %s: %u\n", > > + capdata01_attr_groups[i].attr_group->name, err); > > + goto err_remove_groups; > > + } > > + } > > + > > + return 0; > > + > > +err_remove_groups: > > + while (i-- > 0) { > > + sysfs_remove_group(&priv->fw_attr_kset->kobj, > > + capdata01_attr_groups[i].attr_group); > > + } > > + > > + return err; > > + > > +err_destroy_classdev: > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > + > > + return err; > > + > > +fail_class_get: > > + fw_attributes_class_put(); > > + > > + return err; > > +} > > + > > +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context) > > +{ > > + struct lenovo_wmi_om_priv *priv; > > + > > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->wdev = wdev; > > + > > + guard(mutex)(&om_list_mutex); > > + list_add_tail(&priv->list, &om_wmi_list); > > + > > + return other_method_fw_attr_add(priv); > > +} > > + > > +static void lenovo_wmi_other_remove(struct wmi_device *wdev) > > +{ > > + struct lenovo_wmi_om_priv *priv = dev_get_drvdata(&wdev->dev); > > + > > + guard(mutex)(&om_list_mutex); > > + list_del(&priv->list); > > + kset_unregister(priv->fw_attr_kset); > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > + fw_attributes_class_put(); > > +} > > + > > +static struct wmi_driver lenovo_wmi_other_driver = { > > + .driver = { .name = "lenovo_wmi_other" }, > > + .id_table = lenovo_wmi_other_id_table, > > + .probe = lenovo_wmi_other_probe, > > + .remove = lenovo_wmi_other_remove, > > +}; > > + > > +module_wmi_driver(lenovo_wmi_other_driver); > > + > > +MODULE_IMPORT_NS("CAPDATA_WMI"); > > +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table); > > +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>"); > > +MODULE_DESCRIPTION("Lenovo Other Method WMI Driver"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h > > index 53cea84a956b..1c8358551ba6 100644 > > --- a/drivers/platform/x86/lenovo-wmi.h > > +++ b/drivers/platform/x86/lenovo-wmi.h > > @@ -42,6 +42,14 @@ struct lenovo_wmi_attr_id { > > u32 device_id : 8; /* CPU/GPU/... */ > > } __packed; > > > > +enum attribute_property { > > + DEFAULT_VAL, > > + MAX_VAL, > > + MIN_VAL, > > + STEP_VAL, > > + SUPPORTED, > > +}; > > + > > /* Data struct for LENOVO_CAPABILITY_DATA_01 */ > > struct capability_data_01 { > > u32 id; > > @@ -52,6 +60,14 @@ struct capability_data_01 { > > u32 max_value; > > }; > > > > +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */ > > +struct tunable_attr_01 { > > + struct capability_data_01 capdata; > > + u32 device_id; > > + u32 feature_id; > > + u32 store_value; > > +}; > > + > > /* General Use functions */ > > static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, > > u32 method_id, struct acpi_buffer *in, > > @@ -122,4 +138,104 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance, > > int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id, > > struct capability_data_01 *cap_data); > > > > +/* Other Method attribute functions */ > > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf, struct tunable_attr_01 *tunable_attr, > > + enum attribute_property prop); > > + > > +ssize_t attr_current_value_store(struct kobject *kobj, > > + struct kobj_attribute *attr, const char *buf, > > + size_t count, > > + struct tunable_attr_01 *tunable_attr); > > + > > +ssize_t attr_current_value_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf, > > + struct tunable_attr_01 *tunable_attr); > > + > > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf); > > + > > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf) > > +{ > > + return sysfs_emit(buf, "integer\n"); > > +} > > + > > +/* Other Method attribute macros */ > > +#define __LL_ATTR_RO(_func, _name) \ > > + { \ > > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > > + .show = _func##_##_name##_show, \ > > + } > > + > > +#define __LL_ATTR_RO_AS(_name, _show) \ > > + { \ > > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > > + .show = _show, \ > > + } > > + > > +#define __LL_ATTR_RW(_func, _name) \ > > + __ATTR(_name, 0644, _func##_##_name##_show, _func##_##_name##_store) > > + > > +/* Shows a formatted static variable */ > > +#define __ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val) \ > > + static ssize_t _attrname##_##_prop##_show( \ > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > + { \ > > + return sysfs_emit(buf, _fmt, _val); \ > > + } \ > > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > > + __LL_ATTR_RO(_attrname, _prop) > > + > > +/* Attribute current_value show/store */ > > +#define __LL_TUNABLE_RW_CAP01(_attrname) \ > > + static ssize_t _attrname##_current_value_store( \ > > + struct kobject *kobj, struct kobj_attribute *attr, \ > > + const char *buf, size_t count) \ > > + { \ > > + return attr_current_value_store(kobj, attr, buf, count, \ > > + &_attrname); \ > > + } \ > > + static ssize_t _attrname##_current_value_show( \ > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > + { \ > > + return attr_current_value_show(kobj, attr, buf, &_attrname); \ > > + } \ > > + static struct kobj_attribute attr_##_attrname##_current_value = \ > > + __LL_ATTR_RW(_attrname, current_value) > > + > > +/* Attribute property show only */ > > +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \ > > + static ssize_t _attrname##_##_prop##_show( \ > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > + { \ > > + return attr_capdata01_show(kobj, attr, buf, &_attrname, \ > > + _prop_type); \ > > + } \ > > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > > + __LL_ATTR_RO(_attrname, _prop) > > + > > +#define ATTR_GROUP_LL_TUNABLE_CAP01(_attrname, _fsname, _dispname) \ > > + __LL_TUNABLE_RW_CAP01(_attrname); \ > > + __LL_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL); \ > > + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \ > > + __LL_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL); \ > > + __LL_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL); \ > > + __LL_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL); \ > > + static struct kobj_attribute attr_##_attrname##_type = \ > > + __LL_ATTR_RO_AS(type, int_type_show); \ > > + static struct attribute *_attrname##_attrs[] = { \ > > + &attr_##_attrname##_current_value.attr, \ > > + &attr_##_attrname##_default_value.attr, \ > > + &attr_##_attrname##_display_name.attr, \ > > + &attr_##_attrname##_max_value.attr, \ > > + &attr_##_attrname##_min_value.attr, \ > > + &attr_##_attrname##_scalar_increment.attr, \ > > + &attr_##_attrname##_type.attr, \ > > + NULL, \ > > + }; \ > > + static const struct attribute_group _attrname##_attr_group = { \ > > + .name = _fsname, .attrs = _attrname##_attrs \ > > + } > > + > > #endif /* !_LENOVO_WMI_H_ */ >
On Thu, 2 Jan 2025, Derek John Clark wrote: > On Wed, Jan 1, 2025 at 7:40 PM Mario Limonciello <superm1@kernel.org> wrote: > > On 1/1/25 18:47, Derek J. Clark wrote: > > > Adds lenovo-wmi-other.c which provides a driver for the Lenovo > > > "Other Mode" WMI interface that comes on some Lenovo "Gaming > > > Series" hardware. Provides a firmware-attributes class which > > > enables the use of tunable knobs for SPL, SPPT, and FPPT. > > > > > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. > > > - Use list to get the lenovo_wmi_om_priv instance in some macro > > > called functions as the data provided by the macros that use it > > > doesn't pass a member of the struct for use in container_of. > > > - Do not rely on GameZone interface to grab the current fan mode. > > > > > > Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/platform/x86/Kconfig | 12 + > > > drivers/platform/x86/Makefile | 1 + > > > drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ > > > drivers/platform/x86/lenovo-wmi.h | 116 +++++++ > > > 5 files changed, 515 insertions(+) > > > create mode 100644 drivers/platform/x86/lenovo-wmi-other.c > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index c9374c395905..318e1e517eed 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org > > > S: Maintained > > > F: drivers/platform/x86/lenovo-wmi-capdata01.c > > > F: drivers/platform/x86/lenovo-wmi-gamezone.c > > > +F: drivers/platform/x86/lenovo-wmi-other.c > > > F: drivers/platform/x86/lenovo-wmi.h > > > > > > LETSKETCH HID TABLET DRIVER > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > > index a2c1ab47ad9e..e2285ab987c5 100644 > > > --- a/drivers/platform/x86/Kconfig > > > +++ b/drivers/platform/x86/Kconfig > > > @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 > > > To compile this driver as a module, choose M here: the module will > > > be called lenovo_wmi_capdata01. > > > > > > +config LENOVO_WMI_TUNING > > > + tristate "Lenovo Other Method WMI Driver" > > > + depends on LENOVO_WMI_DATA01 > > > + select FW_ATTR_CLASS > > > + help > > > + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the > > > + firmware_attributes API to control various tunable settings typically exposed by > > > + Lenovo software in Windows. > > > + > > > + To compile this driver as a module, choose M here: the module will > > > + be called lenovo_wmi_other. > > > + > > > config IDEAPAD_LAPTOP > > > tristate "Lenovo IdeaPad Laptop Extras" > > > depends on ACPI > > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > > index 6c96cc3f3855..3e059b3c3647 100644 > > > --- a/drivers/platform/x86/Makefile > > > +++ b/drivers/platform/x86/Makefile > > > @@ -70,6 +70,7 @@ 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 > > > +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o > > > > > > # Intel > > > obj-y += intel/ > > > diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c > > > new file mode 100644 > > > index 000000000000..2392faa74973 > > > --- /dev/null > > > +++ b/drivers/platform/x86/lenovo-wmi-other.c > > > @@ -0,0 +1,385 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +/* > > > + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes > > > + * class to expose the various WMI functions provided by the "Other Method" WMI > > > + * interface. This enables CPU and GPU power limit as well as various other > > > + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop > > > + * devices. Each attribute exposed by the "Other Method"" interface has a > > > + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to > > > + * probe details about the attribute such as set/get support, step, min, max, > > > + * and default value. Each attibute has multiple pages, one for each of the > > > + * fan profiles managed by the GameZone interface, so it must be probed prior > > > + * to returning the current_value. > > > + * > > > + * These attributes typically don't fit anywhere else in the sysfs and are set > > > + * in Windows using one of Lenovo's multiple user applications. > > > + * > > > + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> > > > + */ > > > + > > > +#include <linux/list.h> > > > +#include "lenovo-wmi.h" > > > +#include "firmware_attributes_class.h" > > > + > > > +#define FW_ATTR_FOLDER "lenovo-wmi-other" > > > +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" > > > + > > > +/* Device IDs */ > > > +#define WMI_DEVICE_ID_CPU 0x01 > > > + > > > +/* WMI_DEVICE_ID_CPU feature IDs */ > > > +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ > > > +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ > > > +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ > > > +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ > > > > What exactly is WMI_FEATURE_ID_CPU_FPPT_BAD? I don't see it used in the > > code at all. > > > > Something I was going to add a quirk for based on some bad gouge but > missed in my cleanup. I'll remove. > > > > + > > > +/* Method IDs */ > > > +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ > > > +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ > > > + > > > +static DEFINE_MUTEX(call_mutex); > > > +static DEFINE_MUTEX(om_list_mutex); > > > +static LIST_HEAD(om_wmi_list); > > > + > > > +struct lenovo_wmi_om_priv { > > > + struct wmi_device *wdev; > > > + struct device *fw_attr_dev; > > > + struct kset *fw_attr_kset; > > > + struct list_head list; > > > +}; > > > + > > > +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) > > > +{ > > > + guard(mutex)(&om_list_mutex); > > > + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, > > > + list); > > > +} > > > + > > > +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { > > > + { LENOVO_OTHER_METHOD_GUID, NULL }, > > > + {} > > > +}; > > > + > > > +/* Tunable Attributes */ > > > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, > > > + .feature_id = WMI_FEATURE_ID_CPU_SPL }; > > > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, > > > + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; > > > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, > > > + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; > > > + > > > +struct capdata01_attr_group { > > > + const struct attribute_group *attr_group; > > > + struct tunable_attr_01 *tunable_attr; > > > +}; > > > + > > > +static const struct class *fw_attr_class; > > > + > > > +/** > > > + * attr_capdata01_setup() - Get the data of the specified attribute > > > + * from LENOVO_CAPABILITY_DATA_01 and store it. > > > + * @tunable_attr: The attribute to be populated. > > > + * > > > + * Returns: Either 0 or an error. > > > + */ > > > +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) > > > +{ > > > + struct capability_data_01 cap_data; > > > + int mode = SMARTFAN_MODE_CUSTOM; > > > + int err; > > > + > > > > Why the whitespace here? Seems unnecessary. > > > > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > > > As mode is only used here, I would just do: > > > > { SMARTFAN_MODE_CUSTOM << 8, > > > > To avoid the extra variable. > > > > Acked. Where does that << 8 come from? It smells like a field inside mode_id? If that's the case, FIELD_PREP() should be used instead of the open-coded shift. > > > + tunable_attr->feature_id, > > > + tunable_attr->device_id }; > > > + > > > + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); > > > + if (err) { > > > + pr_err("Failed to get capability data: %u\n", err); > > > + return err; > > > + } > > > + > > > + if (cap_data.supported < 1) > > > + return -EOPNOTSUPP; > > > + > > > + tunable_attr->capdata = cap_data; > > > + > > > + return 0; > > > +} > > > + > > > +/** > > > + * attr_capdata01_show() - Get the value of the specified attribute property > > > + * from LENOVO_CAPABILITY_DATA_01. > > > + * @kobj: Pointer to the driver object. > > > + * @kobj_attribute: Pointer to the attribute calling this function. > > > + * @buf: The buffer to write to. > > > + * @tunable_attr: The attribute to be read. > > > + * @prop: The property of this attribute to be read. > > > + * > > > + * This function is intended to be generic so it can be called from any "_show" > > > + * attribute which works only with integers. > > > + * > > > + * If the WMI is success, then the sysfs attribute is notified. > > > + * > > > + * Returns: Either count, or an error. > > > + */ > > > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > > > + char *buf, struct tunable_attr_01 *tunable_attr, > > > + enum attribute_property prop) > > > +{ > > > + struct capability_data_01 cap_data; > > > + int retval; > > > + > > > + cap_data = tunable_attr->capdata; > > > + > > > + switch (prop) { > > > + case DEFAULT_VAL: > > > + retval = cap_data.default_value; > > > + break; > > > + case MAX_VAL: > > > + retval = cap_data.max_value; > > > + break; > > > + case MIN_VAL: > > > + retval = cap_data.min_value; > > > + break; > > > + case STEP_VAL: > > > + retval = cap_data.step; > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + return sysfs_emit(buf, "%u\n", retval); > > > +} > > > + > > > +/* Simple attribute creation */ > > > + > > > +/* > > > + * att_current_value_store() - Set the current value of the given attribute > > > + * @kobj: Pointer to the driver object. > > > + * @kobj_attribute: Pointer to the attribute calling this function. > > > + * @buf: The buffer to read from, this is parsed to `int` type. > > > + * @count: Required by sysfs attribute macros, pass in from the callee attr. > > > + * @tunable_attr: The attribute to be stored. > > > + * > > > + * This function is intended to be generic so it can be called from any > > > + * attribute's "current_value_store" which works only with integers. The > > > + * integer to be sent to the WMI method is range checked and an error returned > > > + * if out of range. > > > + * > > > + * If the value is valid and WMI is success, then the sysfs attribute is > > > + * notified. > > > + * > > > + * Returns: Either count, or an error. > > > + */ > > > +ssize_t attr_current_value_store(struct kobject *kobj, > > > + struct kobj_attribute *attr, const char *buf, > > > + size_t count, > > > + struct tunable_attr_01 *tunable_attr) > > > +{ > > > + struct capability_data_01 cap_data; > > > + struct lenovo_wmi_om_priv *priv; > > > + int mode = SMARTFAN_MODE_CUSTOM; > > > + u32 value; > > > + int err; > > > + > > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > > > Similar comment about the mode here too. > > > > > + tunable_attr->feature_id, > > > + tunable_attr->device_id }; > > > + > > > + err = kstrtouint(buf, 10, &value); > > > + if (err) { > > > + pr_err("Error converting value to int: %u\n", err); > > > + return err; > > > + } > > > + > > > + cap_data = tunable_attr->capdata; > > > + > > > + if (value < cap_data.min_value || value > cap_data.max_value) > > > + return -EINVAL; > > > + > > > + priv = get_first_wmi_priv(); > > > + if (!priv) > > > + return -ENODEV; > > > + > > > + guard(mutex)(&call_mutex); > > > + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, > > > + WMI_METHOD_ID_VALUE_SET, > > > + *(int *)&attr_id, value, NULL); > > > + > > > + if (err) { > > > + pr_err("Error setting attribute: %u\n", err); > > > + return err; > > > + } > > > + > > > + tunable_attr->store_value = value; > > > + > > > + sysfs_notify(kobj, NULL, attr->attr.name); > > > + > > > + return count; > > > +}; > > > + > > > +/* > > > + * attr_current_value_show() - Get the current value of the given attribute > > > + * @kobj: Pointer to the driver object. > > > + * @kobj_attribute: Pointer to the attribute calling this function. > > > + * @buf: The buffer to write to. > > > + * @tunable_attr: The attribute to be read. > > > + * > > > + * This function is intended to be generic so it can be called from any "_show" > > > + * attribute which works only with integers. > > > + * > > > + * If the WMI is success, then the sysfs attribute is notified. > > > + * > > > + * Returns: Either count, or an error. > > > + */ > > > +ssize_t attr_current_value_show(struct kobject *kobj, > > > + struct kobj_attribute *attr, char *buf, > > > + struct tunable_attr_01 *tunable_attr) > > > +{ > > > + struct lenovo_wmi_om_priv *priv; > > > + int mode = SMARTFAN_MODE_CUSTOM; > > > + int retval; > > > + int err; > > > + > > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > > > Same comment about SMARTFAN_MODE_CUSTOM here. > > > > In this case it may be needed, discussion ongoing in thread 0/4. > > > > + tunable_attr->feature_id, > > > + tunable_attr->device_id }; > > > + > > > + priv = get_first_wmi_priv(); > > > + if (!priv) > > > + return -ENODEV; > > > + > > > + guard(mutex)(&call_mutex); > > > + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, > > > + WMI_METHOD_ID_VALUE_GET, > > > + *(int *)&attr_id, &retval); > > > + > > > + if (err) { > > > + pr_err("Error getting attribute: %u\n", err); > > > + return err; > > > + } > > > + > > > + return sysfs_emit(buf, "%u\n", retval); > > > +} > > > + > > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", > > > + "Set the CPU sustained power limit"); > > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", > > > + "Set the CPU slow package power tracking limit"); > > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", > > > + "Set the CPU fast package power tracking limit"); > > > + > > > +static const struct capdata01_attr_group capdata01_attr_groups[] = { > > > + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, > > > + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, > > > + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, > > > + {}, > > > +}; > > > + > > > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) > > > +{ > > > + int err, i; > > > + > > > + err = fw_attributes_class_get(&fw_attr_class); > > > + if (err) { > > > + pr_err("Failed to get firmware_attributes_class: %u\n", err); > > > + return err; > > > + } > > > + > > > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > > > + NULL, "%s", FW_ATTR_FOLDER); > > > + if (IS_ERR(priv->fw_attr_dev)) { > > > + err = PTR_ERR(priv->fw_attr_dev); > > > + pr_err("Failed to create firmware_attributes_class device: %u\n", > > > + err); > > > + goto fail_class_get; > > > + } > > > + > > > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, > > > + &priv->fw_attr_dev->kobj); > > > + if (!priv->fw_attr_kset) { > > > + err = -ENOMEM; > > > + pr_err("Failed to create firmware_attributes_class kset: %u\n", > > > + err); > > > + goto err_destroy_classdev; > > > + } > > > + > > > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { > > > + err = attr_capdata01_setup( > > > + capdata01_attr_groups[i].tunable_attr); > > > + if (err) { > > > + pr_err("Failed to populate capability data for %s: %u\n", > > > + capdata01_attr_groups[i].attr_group->name, err); > > > > This specific error could be a bit noisy because it's a dependency on > > the other driver in case one attribute returns not supported. > > > > Could you instead detect EOPNOTSUPP specifically and only show error if > > not EOPNOTSUPP? > > > > Easy fix, will do. I'll also add a wmi_dev_exists() here before the > loop to exit early. > > > > + continue; > > > + } > > > + > > > + err = sysfs_create_group(&priv->fw_attr_kset->kobj, > > > + capdata01_attr_groups[i].attr_group); > > > + if (err) { > > > + pr_err("Failed to create sysfs-group for %s: %u\n", > > > + capdata01_attr_groups[i].attr_group->name, err); > > > + goto err_remove_groups; > > > + } > > > + } > > > + > > > + return 0; > > > + > > > +err_remove_groups: > > > + while (i-- > 0) { > > > + sysfs_remove_group(&priv->fw_attr_kset->kobj, > > > + capdata01_attr_groups[i].attr_group); > > > + } > > > + > > > + return err; > > > + > > > +err_destroy_classdev: > > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > > + > > > + return err; > > > + > > > +fail_class_get: > > > + fw_attributes_class_put(); > > > + > > > + return err; I highly suspect the intermediate return errs in the previous labels will cause leaks. Don't you want to rollback everything on error?
On Tue, Jan 7, 2025 at 10:21 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > On Thu, 2 Jan 2025, Derek John Clark wrote: > > On Wed, Jan 1, 2025 at 7:40 PM Mario Limonciello <superm1@kernel.org> wrote: > > > On 1/1/25 18:47, Derek J. Clark wrote: > > > > Adds lenovo-wmi-other.c which provides a driver for the Lenovo > > > > "Other Mode" WMI interface that comes on some Lenovo "Gaming > > > > Series" hardware. Provides a firmware-attributes class which > > > > enables the use of tunable knobs for SPL, SPPT, and FPPT. > > > > > > > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. > > > > - Use list to get the lenovo_wmi_om_priv instance in some macro > > > > called functions as the data provided by the macros that use it > > > > doesn't pass a member of the struct for use in container_of. > > > > - Do not rely on GameZone interface to grab the current fan mode. > > > > > > > > Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/platform/x86/Kconfig | 12 + > > > > drivers/platform/x86/Makefile | 1 + > > > > drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ > > > > drivers/platform/x86/lenovo-wmi.h | 116 +++++++ > > > > 5 files changed, 515 insertions(+) > > > > create mode 100644 drivers/platform/x86/lenovo-wmi-other.c > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > index c9374c395905..318e1e517eed 100644 > > > > --- a/MAINTAINERS > > > > +++ b/MAINTAINERS > > > > @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org > > > > S: Maintained > > > > F: drivers/platform/x86/lenovo-wmi-capdata01.c > > > > F: drivers/platform/x86/lenovo-wmi-gamezone.c > > > > +F: drivers/platform/x86/lenovo-wmi-other.c > > > > F: drivers/platform/x86/lenovo-wmi.h > > > > > > > > LETSKETCH HID TABLET DRIVER > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > > > index a2c1ab47ad9e..e2285ab987c5 100644 > > > > --- a/drivers/platform/x86/Kconfig > > > > +++ b/drivers/platform/x86/Kconfig > > > > @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 > > > > To compile this driver as a module, choose M here: the module will > > > > be called lenovo_wmi_capdata01. > > > > > > > > +config LENOVO_WMI_TUNING > > > > + tristate "Lenovo Other Method WMI Driver" > > > > + depends on LENOVO_WMI_DATA01 > > > > + select FW_ATTR_CLASS > > > > + help > > > > + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the > > > > + firmware_attributes API to control various tunable settings typically exposed by > > > > + Lenovo software in Windows. > > > > + > > > > + To compile this driver as a module, choose M here: the module will > > > > + be called lenovo_wmi_other. > > > > + > > > > config IDEAPAD_LAPTOP > > > > tristate "Lenovo IdeaPad Laptop Extras" > > > > depends on ACPI > > > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > > > index 6c96cc3f3855..3e059b3c3647 100644 > > > > --- a/drivers/platform/x86/Makefile > > > > +++ b/drivers/platform/x86/Makefile > > > > @@ -70,6 +70,7 @@ 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 > > > > +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o > > > > > > > > # Intel > > > > obj-y += intel/ > > > > diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c > > > > new file mode 100644 > > > > index 000000000000..2392faa74973 > > > > --- /dev/null > > > > +++ b/drivers/platform/x86/lenovo-wmi-other.c > > > > @@ -0,0 +1,385 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > > +/* > > > > + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes > > > > + * class to expose the various WMI functions provided by the "Other Method" WMI > > > > + * interface. This enables CPU and GPU power limit as well as various other > > > > + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop > > > > + * devices. Each attribute exposed by the "Other Method"" interface has a > > > > + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to > > > > + * probe details about the attribute such as set/get support, step, min, max, > > > > + * and default value. Each attibute has multiple pages, one for each of the > > > > + * fan profiles managed by the GameZone interface, so it must be probed prior > > > > + * to returning the current_value. > > > > + * > > > > + * These attributes typically don't fit anywhere else in the sysfs and are set > > > > + * in Windows using one of Lenovo's multiple user applications. > > > > + * > > > > + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> > > > > + */ > > > > + > > > > +#include <linux/list.h> > > > > +#include "lenovo-wmi.h" > > > > +#include "firmware_attributes_class.h" > > > > + > > > > +#define FW_ATTR_FOLDER "lenovo-wmi-other" > > > > +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" > > > > + > > > > +/* Device IDs */ > > > > +#define WMI_DEVICE_ID_CPU 0x01 > > > > + > > > > +/* WMI_DEVICE_ID_CPU feature IDs */ > > > > +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ > > > > +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ > > > > +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ > > > > +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ > > > > > > What exactly is WMI_FEATURE_ID_CPU_FPPT_BAD? I don't see it used in the > > > code at all. > > > > > > > Something I was going to add a quirk for based on some bad gouge but > > missed in my cleanup. I'll remove. > > > > > > + > > > > +/* Method IDs */ > > > > +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ > > > > +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ > > > > + > > > > +static DEFINE_MUTEX(call_mutex); > > > > +static DEFINE_MUTEX(om_list_mutex); > > > > +static LIST_HEAD(om_wmi_list); > > > > + > > > > +struct lenovo_wmi_om_priv { > > > > + struct wmi_device *wdev; > > > > + struct device *fw_attr_dev; > > > > + struct kset *fw_attr_kset; > > > > + struct list_head list; > > > > +}; > > > > + > > > > +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) > > > > +{ > > > > + guard(mutex)(&om_list_mutex); > > > > + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, > > > > + list); > > > > +} > > > > + > > > > +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { > > > > + { LENOVO_OTHER_METHOD_GUID, NULL }, > > > > + {} > > > > +}; > > > > + > > > > +/* Tunable Attributes */ > > > > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, > > > > + .feature_id = WMI_FEATURE_ID_CPU_SPL }; > > > > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, > > > > + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; > > > > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, > > > > + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; > > > > + > > > > +struct capdata01_attr_group { > > > > + const struct attribute_group *attr_group; > > > > + struct tunable_attr_01 *tunable_attr; > > > > +}; > > > > + > > > > +static const struct class *fw_attr_class; > > > > + > > > > +/** > > > > + * attr_capdata01_setup() - Get the data of the specified attribute > > > > + * from LENOVO_CAPABILITY_DATA_01 and store it. > > > > + * @tunable_attr: The attribute to be populated. > > > > + * > > > > + * Returns: Either 0 or an error. > > > > + */ > > > > +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) > > > > +{ > > > > + struct capability_data_01 cap_data; > > > > + int mode = SMARTFAN_MODE_CUSTOM; > > > > + int err; > > > > + > > > > > > Why the whitespace here? Seems unnecessary. > > > > > > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > > > > > As mode is only used here, I would just do: > > > > > > { SMARTFAN_MODE_CUSTOM << 8, > > > > > > To avoid the extra variable. > > > > > > > Acked. > > Where does that << 8 come from? It smells like a field inside mode_id? If > that's the case, FIELD_PREP() should be used instead of the open-coded > shift. > This was another thing that was accidentally dropped when I was preparing v2. There is a fourth u8 field, "type_id", that can be used at the end of the u32. It is used in some features that aren't yet implemented and is 0x00 for most attributes. An example of a feature that uses the type ID would be getting the fan speed on a laptop that has two fans, you can select which fan to get with the type ID. I've already fixed this in my working branch for the next version. > > > > + tunable_attr->feature_id, > > > > + tunable_attr->device_id }; > > > > + > > > > + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); > > > > + if (err) { > > > > + pr_err("Failed to get capability data: %u\n", err); > > > > + return err; > > > > + } > > > > + > > > > + if (cap_data.supported < 1) > > > > + return -EOPNOTSUPP; > > > > + > > > > + tunable_attr->capdata = cap_data; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +/** > > > > + * attr_capdata01_show() - Get the value of the specified attribute property > > > > + * from LENOVO_CAPABILITY_DATA_01. > > > > + * @kobj: Pointer to the driver object. > > > > + * @kobj_attribute: Pointer to the attribute calling this function. > > > > + * @buf: The buffer to write to. > > > > + * @tunable_attr: The attribute to be read. > > > > + * @prop: The property of this attribute to be read. > > > > + * > > > > + * This function is intended to be generic so it can be called from any "_show" > > > > + * attribute which works only with integers. > > > > + * > > > > + * If the WMI is success, then the sysfs attribute is notified. > > > > + * > > > > + * Returns: Either count, or an error. > > > > + */ > > > > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > > > > + char *buf, struct tunable_attr_01 *tunable_attr, > > > > + enum attribute_property prop) > > > > +{ > > > > + struct capability_data_01 cap_data; > > > > + int retval; > > > > + > > > > + cap_data = tunable_attr->capdata; > > > > + > > > > + switch (prop) { > > > > + case DEFAULT_VAL: > > > > + retval = cap_data.default_value; > > > > + break; > > > > + case MAX_VAL: > > > > + retval = cap_data.max_value; > > > > + break; > > > > + case MIN_VAL: > > > > + retval = cap_data.min_value; > > > > + break; > > > > + case STEP_VAL: > > > > + retval = cap_data.step; > > > > + break; > > > > + default: > > > > + return -EINVAL; > > > > + } > > > > + return sysfs_emit(buf, "%u\n", retval); > > > > +} > > > > + > > > > +/* Simple attribute creation */ > > > > + > > > > +/* > > > > + * att_current_value_store() - Set the current value of the given attribute > > > > + * @kobj: Pointer to the driver object. > > > > + * @kobj_attribute: Pointer to the attribute calling this function. > > > > + * @buf: The buffer to read from, this is parsed to `int` type. > > > > + * @count: Required by sysfs attribute macros, pass in from the callee attr. > > > > + * @tunable_attr: The attribute to be stored. > > > > + * > > > > + * This function is intended to be generic so it can be called from any > > > > + * attribute's "current_value_store" which works only with integers. The > > > > + * integer to be sent to the WMI method is range checked and an error returned > > > > + * if out of range. > > > > + * > > > > + * If the value is valid and WMI is success, then the sysfs attribute is > > > > + * notified. > > > > + * > > > > + * Returns: Either count, or an error. > > > > + */ > > > > +ssize_t attr_current_value_store(struct kobject *kobj, > > > > + struct kobj_attribute *attr, const char *buf, > > > > + size_t count, > > > > + struct tunable_attr_01 *tunable_attr) > > > > +{ > > > > + struct capability_data_01 cap_data; > > > > + struct lenovo_wmi_om_priv *priv; > > > > + int mode = SMARTFAN_MODE_CUSTOM; > > > > + u32 value; > > > > + int err; > > > > + > > > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > > > > > Similar comment about the mode here too. > > > > > > > + tunable_attr->feature_id, > > > > + tunable_attr->device_id }; > > > > + > > > > + err = kstrtouint(buf, 10, &value); > > > > + if (err) { > > > > + pr_err("Error converting value to int: %u\n", err); > > > > + return err; > > > > + } > > > > + > > > > + cap_data = tunable_attr->capdata; > > > > + > > > > + if (value < cap_data.min_value || value > cap_data.max_value) > > > > + return -EINVAL; > > > > + > > > > + priv = get_first_wmi_priv(); > > > > + if (!priv) > > > > + return -ENODEV; > > > > + > > > > + guard(mutex)(&call_mutex); > > > > + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, > > > > + WMI_METHOD_ID_VALUE_SET, > > > > + *(int *)&attr_id, value, NULL); > > > > + > > > > + if (err) { > > > > + pr_err("Error setting attribute: %u\n", err); > > > > + return err; > > > > + } > > > > + > > > > + tunable_attr->store_value = value; > > > > + > > > > + sysfs_notify(kobj, NULL, attr->attr.name); > > > > + > > > > + return count; > > > > +}; > > > > + > > > > +/* > > > > + * attr_current_value_show() - Get the current value of the given attribute > > > > + * @kobj: Pointer to the driver object. > > > > + * @kobj_attribute: Pointer to the attribute calling this function. > > > > + * @buf: The buffer to write to. > > > > + * @tunable_attr: The attribute to be read. > > > > + * > > > > + * This function is intended to be generic so it can be called from any "_show" > > > > + * attribute which works only with integers. > > > > + * > > > > + * If the WMI is success, then the sysfs attribute is notified. > > > > + * > > > > + * Returns: Either count, or an error. > > > > + */ > > > > +ssize_t attr_current_value_show(struct kobject *kobj, > > > > + struct kobj_attribute *attr, char *buf, > > > > + struct tunable_attr_01 *tunable_attr) > > > > +{ > > > > + struct lenovo_wmi_om_priv *priv; > > > > + int mode = SMARTFAN_MODE_CUSTOM; > > > > + int retval; > > > > + int err; > > > > + > > > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > > > > > Same comment about SMARTFAN_MODE_CUSTOM here. > > > > > > > In this case it may be needed, discussion ongoing in thread 0/4. > > > > > > + tunable_attr->feature_id, > > > > + tunable_attr->device_id }; > > > > + > > > > + priv = get_first_wmi_priv(); > > > > + if (!priv) > > > > + return -ENODEV; > > > > + > > > > + guard(mutex)(&call_mutex); > > > > + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, > > > > + WMI_METHOD_ID_VALUE_GET, > > > > + *(int *)&attr_id, &retval); > > > > + > > > > + if (err) { > > > > + pr_err("Error getting attribute: %u\n", err); > > > > + return err; > > > > + } > > > > + > > > > + return sysfs_emit(buf, "%u\n", retval); > > > > +} > > > > + > > > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", > > > > + "Set the CPU sustained power limit"); > > > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", > > > > + "Set the CPU slow package power tracking limit"); > > > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", > > > > + "Set the CPU fast package power tracking limit"); > > > > + > > > > +static const struct capdata01_attr_group capdata01_attr_groups[] = { > > > > + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, > > > > + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, > > > > + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, > > > > + {}, > > > > +}; > > > > + > > > > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) > > > > +{ > > > > + int err, i; > > > > + > > > > + err = fw_attributes_class_get(&fw_attr_class); > > > > + if (err) { > > > > + pr_err("Failed to get firmware_attributes_class: %u\n", err); > > > > + return err; > > > > + } > > > > + > > > > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > > > > + NULL, "%s", FW_ATTR_FOLDER); > > > > + if (IS_ERR(priv->fw_attr_dev)) { > > > > + err = PTR_ERR(priv->fw_attr_dev); > > > > + pr_err("Failed to create firmware_attributes_class device: %u\n", > > > > + err); > > > > + goto fail_class_get; > > > > + } > > > > + > > > > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, > > > > + &priv->fw_attr_dev->kobj); > > > > + if (!priv->fw_attr_kset) { > > > > + err = -ENOMEM; > > > > + pr_err("Failed to create firmware_attributes_class kset: %u\n", > > > > + err); > > > > + goto err_destroy_classdev; > > > > + } > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { > > > > + err = attr_capdata01_setup( > > > > + capdata01_attr_groups[i].tunable_attr); > > > > + if (err) { > > > > + pr_err("Failed to populate capability data for %s: %u\n", > > > > + capdata01_attr_groups[i].attr_group->name, err); > > > > > > This specific error could be a bit noisy because it's a dependency on > > > the other driver in case one attribute returns not supported. > > > > > > Could you instead detect EOPNOTSUPP specifically and only show error if > > > not EOPNOTSUPP? > > > > > > > Easy fix, will do. I'll also add a wmi_dev_exists() here before the > > loop to exit early. > > > > > > + continue; > > > > + } > > > > + > > > > + err = sysfs_create_group(&priv->fw_attr_kset->kobj, > > > > + capdata01_attr_groups[i].attr_group); > > > > + if (err) { > > > > + pr_err("Failed to create sysfs-group for %s: %u\n", > > > > + capdata01_attr_groups[i].attr_group->name, err); > > > > + goto err_remove_groups; > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > + > > > > +err_remove_groups: > > > > + while (i-- > 0) { > > > > + sysfs_remove_group(&priv->fw_attr_kset->kobj, > > > > + capdata01_attr_groups[i].attr_group); > > > > + } > > > > + > > > > + return err; > > > > + > > > > +err_destroy_classdev: > > > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > > > + > > > > + return err; > > > > + > > > > +fail_class_get: > > > > + fw_attributes_class_put(); > > > > + > > > > + return err; > > I highly suspect the intermediate return errs in the previous labels will > cause leaks. Don't you want to rollback everything on error? To clarify, you mean remove the returns in each fail case before fail_class_get so they will fall through? That would make more sense, yeah. > -- > i. > > Thanks Ilpo, Derek > > > > +} > > > > + > > > > +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context) > > > > +{ > > > > + struct lenovo_wmi_om_priv *priv; > > > > + > > > > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); > > > > + if (!priv) > > > > + return -ENOMEM; > > > > + > > > > + priv->wdev = wdev; > > > > + > > > > + guard(mutex)(&om_list_mutex); > > > > + list_add_tail(&priv->list, &om_wmi_list); > > > > + > > > > + return other_method_fw_attr_add(priv); > > > > +} > > > > + > > > > +static void lenovo_wmi_other_remove(struct wmi_device *wdev) > > > > +{ > > > > + struct lenovo_wmi_om_priv *priv = dev_get_drvdata(&wdev->dev); > > > > + > > > > + guard(mutex)(&om_list_mutex); > > > > + list_del(&priv->list); > > > > + kset_unregister(priv->fw_attr_kset); > > > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > > > + fw_attributes_class_put(); > > > > +} > > > > + > > > > +static struct wmi_driver lenovo_wmi_other_driver = { > > > > + .driver = { .name = "lenovo_wmi_other" }, > > > > + .id_table = lenovo_wmi_other_id_table, > > > > + .probe = lenovo_wmi_other_probe, > > > > + .remove = lenovo_wmi_other_remove, > > > > +}; > > > > + > > > > +module_wmi_driver(lenovo_wmi_other_driver); > > > > + > > > > +MODULE_IMPORT_NS("CAPDATA_WMI"); > > > > +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table); > > > > +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>"); > > > > +MODULE_DESCRIPTION("Lenovo Other Method WMI Driver"); > > > > +MODULE_LICENSE("GPL"); > > > > diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h > > > > index 53cea84a956b..1c8358551ba6 100644 > > > > --- a/drivers/platform/x86/lenovo-wmi.h > > > > +++ b/drivers/platform/x86/lenovo-wmi.h > > > > @@ -42,6 +42,14 @@ struct lenovo_wmi_attr_id { > > > > u32 device_id : 8; /* CPU/GPU/... */ > > > > } __packed; > > > > > > > > +enum attribute_property { > > > > + DEFAULT_VAL, > > > > + MAX_VAL, > > > > + MIN_VAL, > > > > + STEP_VAL, > > > > + SUPPORTED, > > > > +}; > > > > + > > > > /* Data struct for LENOVO_CAPABILITY_DATA_01 */ > > > > struct capability_data_01 { > > > > u32 id; > > > > @@ -52,6 +60,14 @@ struct capability_data_01 { > > > > u32 max_value; > > > > }; > > > > > > > > +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */ > > > > +struct tunable_attr_01 { > > > > + struct capability_data_01 capdata; > > > > + u32 device_id; > > > > + u32 feature_id; > > > > + u32 store_value; > > > > +}; > > > > + > > > > /* General Use functions */ > > > > static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, > > > > u32 method_id, struct acpi_buffer *in, > > > > @@ -122,4 +138,104 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance, > > > > int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id, > > > > struct capability_data_01 *cap_data); > > > > > > > > +/* Other Method attribute functions */ > > > > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > > > > + char *buf, struct tunable_attr_01 *tunable_attr, > > > > + enum attribute_property prop); > > > > + > > > > +ssize_t attr_current_value_store(struct kobject *kobj, > > > > + struct kobj_attribute *attr, const char *buf, > > > > + size_t count, > > > > + struct tunable_attr_01 *tunable_attr); > > > > + > > > > +ssize_t attr_current_value_show(struct kobject *kobj, > > > > + struct kobj_attribute *attr, char *buf, > > > > + struct tunable_attr_01 *tunable_attr); > > > > + > > > > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > > > > + char *buf); > > > > + > > > > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > > > > + char *buf) > > > > +{ > > > > + return sysfs_emit(buf, "integer\n"); > > > > +} > > > > + > > > > +/* Other Method attribute macros */ > > > > +#define __LL_ATTR_RO(_func, _name) \ > > > > + { \ > > > > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > > > > + .show = _func##_##_name##_show, \ > > > > + } > > > > + > > > > +#define __LL_ATTR_RO_AS(_name, _show) \ > > > > + { \ > > > > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > > > > + .show = _show, \ > > > > + } > > > > + > > > > +#define __LL_ATTR_RW(_func, _name) \ > > > > + __ATTR(_name, 0644, _func##_##_name##_show, _func##_##_name##_store) > > > > + > > > > +/* Shows a formatted static variable */ > > > > +#define __ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val) \ > > > > + static ssize_t _attrname##_##_prop##_show( \ > > > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > > > + { \ > > > > + return sysfs_emit(buf, _fmt, _val); \ > > > > + } \ > > > > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > > > > + __LL_ATTR_RO(_attrname, _prop) > > > > + > > > > +/* Attribute current_value show/store */ > > > > +#define __LL_TUNABLE_RW_CAP01(_attrname) \ > > > > + static ssize_t _attrname##_current_value_store( \ > > > > + struct kobject *kobj, struct kobj_attribute *attr, \ > > > > + const char *buf, size_t count) \ > > > > + { \ > > > > + return attr_current_value_store(kobj, attr, buf, count, \ > > > > + &_attrname); \ > > > > + } \ > > > > + static ssize_t _attrname##_current_value_show( \ > > > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > > > + { \ > > > > + return attr_current_value_show(kobj, attr, buf, &_attrname); \ > > > > + } \ > > > > + static struct kobj_attribute attr_##_attrname##_current_value = \ > > > > + __LL_ATTR_RW(_attrname, current_value) > > > > + > > > > +/* Attribute property show only */ > > > > +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \ > > > > + static ssize_t _attrname##_##_prop##_show( \ > > > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > > > + { \ > > > > + return attr_capdata01_show(kobj, attr, buf, &_attrname, \ > > > > + _prop_type); \ > > > > + } \ > > > > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > > > > + __LL_ATTR_RO(_attrname, _prop) > > > > + > > > > +#define ATTR_GROUP_LL_TUNABLE_CAP01(_attrname, _fsname, _dispname) \ > > > > + __LL_TUNABLE_RW_CAP01(_attrname); \ > > > > + __LL_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL); \ > > > > + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \ > > > > + __LL_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL); \ > > > > + __LL_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL); \ > > > > + __LL_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL); \ > > > > + static struct kobj_attribute attr_##_attrname##_type = \ > > > > + __LL_ATTR_RO_AS(type, int_type_show); \ > > > > + static struct attribute *_attrname##_attrs[] = { \ > > > > + &attr_##_attrname##_current_value.attr, \ > > > > + &attr_##_attrname##_default_value.attr, \ > > > > + &attr_##_attrname##_display_name.attr, \ > > > > + &attr_##_attrname##_max_value.attr, \ > > > > + &attr_##_attrname##_min_value.attr, \ > > > > + &attr_##_attrname##_scalar_increment.attr, \ > > > > + &attr_##_attrname##_type.attr, \ > > > > + NULL, \ > > > > + }; \ > > > > + static const struct attribute_group _attrname##_attr_group = { \ > > > > + .name = _fsname, .attrs = _attrname##_attrs \ > > > > + } > > > > + > > > > #endif /* !_LENOVO_WMI_H_ */ > > > > >
On Tue, 7 Jan 2025, Derek John Clark wrote: > On Tue, Jan 7, 2025 at 10:21 AM Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: > > > > On Thu, 2 Jan 2025, Derek John Clark wrote: > > > On Wed, Jan 1, 2025 at 7:40 PM Mario Limonciello <superm1@kernel.org> wrote: > > > > On 1/1/25 18:47, Derek J. Clark wrote: > > > > > Adds lenovo-wmi-other.c which provides a driver for the Lenovo > > > > > "Other Mode" WMI interface that comes on some Lenovo "Gaming > > > > > Series" hardware. Provides a firmware-attributes class which > > > > > enables the use of tunable knobs for SPL, SPPT, and FPPT. > > > > > > > > > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. > > > > > - Use list to get the lenovo_wmi_om_priv instance in some macro > > > > > called functions as the data provided by the macros that use it > > > > > doesn't pass a member of the struct for use in container_of. > > > > > - Do not rely on GameZone interface to grab the current fan mode. > > > > > > > > > > Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> > > > > > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) > > > > > +{ > > > > > + int err, i; > > > > > + > > > > > + err = fw_attributes_class_get(&fw_attr_class); > > > > > + if (err) { > > > > > + pr_err("Failed to get firmware_attributes_class: %u\n", err); > > > > > + return err; > > > > > + } > > > > > + > > > > > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > > > > > + NULL, "%s", FW_ATTR_FOLDER); > > > > > + if (IS_ERR(priv->fw_attr_dev)) { > > > > > + err = PTR_ERR(priv->fw_attr_dev); > > > > > + pr_err("Failed to create firmware_attributes_class device: %u\n", > > > > > + err); > > > > > + goto fail_class_get; > > > > > + } > > > > > + > > > > > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, > > > > > + &priv->fw_attr_dev->kobj); > > > > > + if (!priv->fw_attr_kset) { > > > > > + err = -ENOMEM; > > > > > + pr_err("Failed to create firmware_attributes_class kset: %u\n", > > > > > + err); > > > > > + goto err_destroy_classdev; > > > > > + } > > > > > + > > > > > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { > > > > > + err = attr_capdata01_setup( > > > > > + capdata01_attr_groups[i].tunable_attr); > > > > > + if (err) { > > > > > + pr_err("Failed to populate capability data for %s: %u\n", > > > > > + capdata01_attr_groups[i].attr_group->name, err); > > > > > > > > This specific error could be a bit noisy because it's a dependency on > > > > the other driver in case one attribute returns not supported. > > > > > > > > Could you instead detect EOPNOTSUPP specifically and only show error if > > > > not EOPNOTSUPP? > > > > > > > > > > Easy fix, will do. I'll also add a wmi_dev_exists() here before the > > > loop to exit early. > > > > > > > > + continue; > > > > > + } > > > > > + > > > > > + err = sysfs_create_group(&priv->fw_attr_kset->kobj, > > > > > + capdata01_attr_groups[i].attr_group); > > > > > + if (err) { > > > > > + pr_err("Failed to create sysfs-group for %s: %u\n", > > > > > + capdata01_attr_groups[i].attr_group->name, err); > > > > > + goto err_remove_groups; > > > > > + } > > > > > + } > > > > > + > > > > > + return 0; > > > > > + > > > > > +err_remove_groups: > > > > > + while (i-- > 0) { > > > > > + sysfs_remove_group(&priv->fw_attr_kset->kobj, > > > > > + capdata01_attr_groups[i].attr_group); > > > > > + } > > > > > + > > > > > + return err; > > > > > + > > > > > +err_destroy_classdev: > > > > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > > > > + > > > > > + return err; > > > > > + > > > > > +fail_class_get: > > > > > + fw_attributes_class_put(); > > > > > + > > > > > + return err; > > > > I highly suspect the intermediate return errs in the previous labels will > > cause leaks. Don't you want to rollback everything on error? > > To clarify, you mean remove the returns in each fail case before > fail_class_get so they will fall through? That would make more sense, > yeah. Yes, the returns before the fail_class_get label and before the err_destroy_classdev label. Both seemed to break the usual rollback pattern and it looked to me when I tracked the callchains an error here will lead to a probe failure so I'd expect you want to rollback everything in case of an error, not just the latest step. (In some cases probe is allowed to succeed partially but I didn't see any indication of that here.)
Am 02.01.25 um 01:47 schrieb Derek J. Clark: > Adds lenovo-wmi-other.c which provides a driver for the Lenovo > "Other Mode" WMI interface that comes on some Lenovo "Gaming > Series" hardware. Provides a firmware-attributes class which > enables the use of tunable knobs for SPL, SPPT, and FPPT. > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. > - Use list to get the lenovo_wmi_om_priv instance in some macro > called functions as the data provided by the macros that use it > doesn't pass a member of the struct for use in container_of. > - Do not rely on GameZone interface to grab the current fan mode. > > Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> > --- > MAINTAINERS | 1 + > drivers/platform/x86/Kconfig | 12 + > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ > drivers/platform/x86/lenovo-wmi.h | 116 +++++++ > 5 files changed, 515 insertions(+) > create mode 100644 drivers/platform/x86/lenovo-wmi-other.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index c9374c395905..318e1e517eed 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org > S: Maintained > F: drivers/platform/x86/lenovo-wmi-capdata01.c > F: drivers/platform/x86/lenovo-wmi-gamezone.c > +F: drivers/platform/x86/lenovo-wmi-other.c > F: drivers/platform/x86/lenovo-wmi.h > > LETSKETCH HID TABLET DRIVER > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index a2c1ab47ad9e..e2285ab987c5 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 > To compile this driver as a module, choose M here: the module will > be called lenovo_wmi_capdata01. > > +config LENOVO_WMI_TUNING > + tristate "Lenovo Other Method WMI Driver" > + depends on LENOVO_WMI_DATA01 I think we should use "select LENOVO_WMI_DATA01" here. Ideally CONFIG_LENOVO_WMI_DATA01 will automatically be enabled/disabled if users enable/disable CONFIG_LENOVO_WMI_TUNING. > + select FW_ATTR_CLASS > + help > + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the > + firmware_attributes API to control various tunable settings typically exposed by > + Lenovo software in Windows. > + > + To compile this driver as a module, choose M here: the module will > + be called lenovo_wmi_other. Check the module name again. > + > config IDEAPAD_LAPTOP > tristate "Lenovo IdeaPad Laptop Extras" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 6c96cc3f3855..3e059b3c3647 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -70,6 +70,7 @@ 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 > +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o > > # Intel > obj-y += intel/ > diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c > new file mode 100644 > index 000000000000..2392faa74973 > --- /dev/null > +++ b/drivers/platform/x86/lenovo-wmi-other.c > @@ -0,0 +1,385 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes > + * class to expose the various WMI functions provided by the "Other Method" WMI > + * interface. This enables CPU and GPU power limit as well as various other > + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop > + * devices. Each attribute exposed by the "Other Method"" interface has a > + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to > + * probe details about the attribute such as set/get support, step, min, max, > + * and default value. Each attibute has multiple pages, one for each of the > + * fan profiles managed by the GameZone interface, so it must be probed prior > + * to returning the current_value. > + * > + * These attributes typically don't fit anywhere else in the sysfs and are set > + * in Windows using one of Lenovo's multiple user applications. > + * > + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> > + */ > + > +#include <linux/list.h> > +#include "lenovo-wmi.h" > +#include "firmware_attributes_class.h" > + > +#define FW_ATTR_FOLDER "lenovo-wmi-other" > +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" > + > +/* Device IDs */ > +#define WMI_DEVICE_ID_CPU 0x01 > + > +/* WMI_DEVICE_ID_CPU feature IDs */ > +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ > +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ > +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ > +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ > + > +/* Method IDs */ > +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ > +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ > + > +static DEFINE_MUTEX(call_mutex); Is this mutex really necessary? If not then remove it please. > +static DEFINE_MUTEX(om_list_mutex); > +static LIST_HEAD(om_wmi_list); > + > +struct lenovo_wmi_om_priv { > + struct wmi_device *wdev; > + struct device *fw_attr_dev; > + struct kset *fw_attr_kset; > + struct list_head list; > +}; > + > +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) > +{ > + guard(mutex)(&om_list_mutex); > + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, > + list); > +} > + > +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { > + { LENOVO_OTHER_METHOD_GUID, NULL }, > + {} > +}; Please move the list of device ids closer to the driver struct. > + > +/* Tunable Attributes */ > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, > + .feature_id = WMI_FEATURE_ID_CPU_SPL }; > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, > + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, > + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; > + > +struct capdata01_attr_group { > + const struct attribute_group *attr_group; > + struct tunable_attr_01 *tunable_attr; Would it make sense to do something similar with each attribute, so that each attribute can use container_of() to access lenovo_wmi_om_priv without having to use a list lookup? This would of course mean that each attribute as to be allocated dynamically. Heep in mind that we are currently working on a new API for registering firmware-atrtibute class devices which should fix this. > +}; > + > +static const struct class *fw_attr_class; > + > +/** > + * attr_capdata01_setup() - Get the data of the specified attribute > + * from LENOVO_CAPABILITY_DATA_01 and store it. > + * @tunable_attr: The attribute to be populated. > + * > + * Returns: Either 0 or an error. > + */ > +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) > +{ > + struct capability_data_01 cap_data; > + int mode = SMARTFAN_MODE_CUSTOM; > + int err; > + > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > + tunable_attr->feature_id, > + tunable_attr->device_id }; Please use FIELD_GET()/FIELD_PREP() here. > + > + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); > + if (err) { > + pr_err("Failed to get capability data: %u\n", err); > + return err; > + } > + > + if (cap_data.supported < 1) > + return -EOPNOTSUPP; > + > + tunable_attr->capdata = cap_data; > + > + return 0; > +} > + > +/** > + * attr_capdata01_show() - Get the value of the specified attribute property > + * from LENOVO_CAPABILITY_DATA_01. > + * @kobj: Pointer to the driver object. > + * @kobj_attribute: Pointer to the attribute calling this function. > + * @buf: The buffer to write to. > + * @tunable_attr: The attribute to be read. > + * @prop: The property of this attribute to be read. > + * > + * This function is intended to be generic so it can be called from any "_show" > + * attribute which works only with integers. > + * > + * If the WMI is success, then the sysfs attribute is notified. > + * > + * Returns: Either count, or an error. > + */ > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf, struct tunable_attr_01 *tunable_attr, > + enum attribute_property prop) > +{ > + struct capability_data_01 cap_data; > + int retval; > + > + cap_data = tunable_attr->capdata; > + > + switch (prop) { > + case DEFAULT_VAL: > + retval = cap_data.default_value; > + break; > + case MAX_VAL: > + retval = cap_data.max_value; > + break; > + case MIN_VAL: > + retval = cap_data.min_value; > + break; > + case STEP_VAL: > + retval = cap_data.step; > + break; > + default: > + return -EINVAL; > + } > + return sysfs_emit(buf, "%u\n", retval); > +} > + > +/* Simple attribute creation */ > + > +/* > + * att_current_value_store() - Set the current value of the given attribute > + * @kobj: Pointer to the driver object. > + * @kobj_attribute: Pointer to the attribute calling this function. > + * @buf: The buffer to read from, this is parsed to `int` type. > + * @count: Required by sysfs attribute macros, pass in from the callee attr. > + * @tunable_attr: The attribute to be stored. > + * > + * This function is intended to be generic so it can be called from any > + * attribute's "current_value_store" which works only with integers. The > + * integer to be sent to the WMI method is range checked and an error returned > + * if out of range. > + * > + * If the value is valid and WMI is success, then the sysfs attribute is > + * notified. > + * > + * Returns: Either count, or an error. > + */ > +ssize_t attr_current_value_store(struct kobject *kobj, > + struct kobj_attribute *attr, const char *buf, > + size_t count, > + struct tunable_attr_01 *tunable_attr) > +{ > + struct capability_data_01 cap_data; > + struct lenovo_wmi_om_priv *priv; > + int mode = SMARTFAN_MODE_CUSTOM; > + u32 value; > + int err; > + > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > + tunable_attr->feature_id, > + tunable_attr->device_id }; > + > + err = kstrtouint(buf, 10, &value); > + if (err) { > + pr_err("Error converting value to int: %u\n", err); > + return err; > + } > + > + cap_data = tunable_attr->capdata; > + > + if (value < cap_data.min_value || value > cap_data.max_value) > + return -EINVAL; > + > + priv = get_first_wmi_priv(); > + if (!priv) > + return -ENODEV; > + > + guard(mutex)(&call_mutex); > + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, > + WMI_METHOD_ID_VALUE_SET, > + *(int *)&attr_id, value, NULL); > + > + if (err) { > + pr_err("Error setting attribute: %u\n", err); This error message is unnecessary, please drop it. > + return err; > + } > + > + tunable_attr->store_value = value; Is this value used anywhere? If no then please drop it. > + > + sysfs_notify(kobj, NULL, attr->attr.name); AFAIK this is unnecessary since userspace already expects the attribute value to change after an write access. This is only meant to be used should the value be changed by for example the underlying hardware without user intervention. > + > + return count; > +}; > + > +/* > + * attr_current_value_show() - Get the current value of the given attribute > + * @kobj: Pointer to the driver object. > + * @kobj_attribute: Pointer to the attribute calling this function. > + * @buf: The buffer to write to. > + * @tunable_attr: The attribute to be read. > + * > + * This function is intended to be generic so it can be called from any "_show" > + * attribute which works only with integers. > + * > + * If the WMI is success, then the sysfs attribute is notified. > + * > + * Returns: Either count, or an error. > + */ > +ssize_t attr_current_value_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf, > + struct tunable_attr_01 *tunable_attr) > +{ > + struct lenovo_wmi_om_priv *priv; > + int mode = SMARTFAN_MODE_CUSTOM; > + int retval; > + int err; > + > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > + tunable_attr->feature_id, > + tunable_attr->device_id }; > + > + priv = get_first_wmi_priv(); > + if (!priv) > + return -ENODEV; > + > + guard(mutex)(&call_mutex); > + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, > + WMI_METHOD_ID_VALUE_GET, > + *(int *)&attr_id, &retval); > + > + if (err) { > + pr_err("Error getting attribute: %u\n", err); > + return err; > + } > + > + return sysfs_emit(buf, "%u\n", retval); > +} > + > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", > + "Set the CPU sustained power limit"); > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", > + "Set the CPU slow package power tracking limit"); > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", > + "Set the CPU fast package power tracking limit"); > + > +static const struct capdata01_attr_group capdata01_attr_groups[] = { > + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, > + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, > + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, > + {}, > +}; > + > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) > +{ > + int err, i; > + > + err = fw_attributes_class_get(&fw_attr_class); > + if (err) { > + pr_err("Failed to get firmware_attributes_class: %u\n", err); > + return err; > + } > + > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > + NULL, "%s", FW_ATTR_FOLDER); > + if (IS_ERR(priv->fw_attr_dev)) { > + err = PTR_ERR(priv->fw_attr_dev); > + pr_err("Failed to create firmware_attributes_class device: %u\n", > + err); > + goto fail_class_get; > + } > + > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, > + &priv->fw_attr_dev->kobj); > + if (!priv->fw_attr_kset) { > + err = -ENOMEM; > + pr_err("Failed to create firmware_attributes_class kset: %u\n", > + err); > + goto err_destroy_classdev; > + } > + > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { > + err = attr_capdata01_setup( > + capdata01_attr_groups[i].tunable_attr); > + if (err) { > + pr_err("Failed to populate capability data for %s: %u\n", > + capdata01_attr_groups[i].attr_group->name, err); > + continue; > + } > + > + err = sysfs_create_group(&priv->fw_attr_kset->kobj, > + capdata01_attr_groups[i].attr_group); AFAIK there exists sysfs_create_groups(). Together with the *_is_visible callbacks this should simplify this part of your code a lot. > + if (err) { > + pr_err("Failed to create sysfs-group for %s: %u\n", > + capdata01_attr_groups[i].attr_group->name, err); > + goto err_remove_groups; > + } > + } > + > + return 0; > + > +err_remove_groups: > + while (i-- > 0) { > + sysfs_remove_group(&priv->fw_attr_kset->kobj, > + capdata01_attr_groups[i].attr_group); > + } > + > + return err; Please remove this return statement, since the other resources need to be cleaned up too. Also where do you clean up the kset? > + > +err_destroy_classdev: > + device_destroy(fw_attr_class, MKDEV(0, 0)); Please use device_unregister() instead. > + > + return err; ... > + > +fail_class_get: > + fw_attributes_class_put(); > + > + return err; > +} > + > +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context) > +{ > + struct lenovo_wmi_om_priv *priv; > + > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->wdev = wdev; Missing dev_set_drvdata(). > + > + guard(mutex)(&om_list_mutex); > + list_add_tail(&priv->list, &om_wmi_list); > + > + return other_method_fw_attr_add(priv); > +} > + > +static void lenovo_wmi_other_remove(struct wmi_device *wdev) > +{ > + struct lenovo_wmi_om_priv *priv = dev_get_drvdata(&wdev->dev); > + > + guard(mutex)(&om_list_mutex); > + list_del(&priv->list); > + kset_unregister(priv->fw_attr_kset); > + device_destroy(fw_attr_class, MKDEV(0, 0)); > + fw_attributes_class_put(); > +} > + > +static struct wmi_driver lenovo_wmi_other_driver = { > + .driver = { .name = "lenovo_wmi_other" }, .probe_type = PROBE_PREFER_ASYNCHRONOUS > + .id_table = lenovo_wmi_other_id_table, > + .probe = lenovo_wmi_other_probe, > + .remove = lenovo_wmi_other_remove, .no_singleton = true In this case please make sure that the name of the firmware-attributes class device is unique. You can use an IDA (https://docs.kernel.org/core-api/idr.html) for this. > +}; > + > +module_wmi_driver(lenovo_wmi_other_driver); > + > +MODULE_IMPORT_NS("CAPDATA_WMI"); > +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table); > +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>"); > +MODULE_DESCRIPTION("Lenovo Other Method WMI Driver"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h > index 53cea84a956b..1c8358551ba6 100644 > --- a/drivers/platform/x86/lenovo-wmi.h > +++ b/drivers/platform/x86/lenovo-wmi.h > @@ -42,6 +42,14 @@ struct lenovo_wmi_attr_id { > u32 device_id : 8; /* CPU/GPU/... */ > } __packed; > > +enum attribute_property { > + DEFAULT_VAL, > + MAX_VAL, > + MIN_VAL, > + STEP_VAL, > + SUPPORTED, > +}; > + > /* Data struct for LENOVO_CAPABILITY_DATA_01 */ > struct capability_data_01 { > u32 id; > @@ -52,6 +60,14 @@ struct capability_data_01 { > u32 max_value; > }; > > +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */ > +struct tunable_attr_01 { > + struct capability_data_01 capdata; > + u32 device_id; > + u32 feature_id; > + u32 store_value; > +}; > + > /* General Use functions */ > static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, > u32 method_id, struct acpi_buffer *in, > @@ -122,4 +138,104 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance, > int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id, > struct capability_data_01 *cap_data); > > +/* Other Method attribute functions */ > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf, struct tunable_attr_01 *tunable_attr, > + enum attribute_property prop); > + > +ssize_t attr_current_value_store(struct kobject *kobj, > + struct kobj_attribute *attr, const char *buf, > + size_t count, > + struct tunable_attr_01 *tunable_attr); > + > +ssize_t attr_current_value_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf, > + struct tunable_attr_01 *tunable_attr); > + > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf); > + > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "integer\n"); > +} > + > +/* Other Method attribute macros */ > +#define __LL_ATTR_RO(_func, _name) \ > + { \ > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > + .show = _func##_##_name##_show, \ > + } > + > +#define __LL_ATTR_RO_AS(_name, _show) \ > + { \ > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > + .show = _show, \ > + } > + > +#define __LL_ATTR_RW(_func, _name) \ > + __ATTR(_name, 0644, _func##_##_name##_show, _func##_##_name##_store) > + > +/* Shows a formatted static variable */ > +#define __ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val) \ > + static ssize_t _attrname##_##_prop##_show( \ > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > + { \ > + return sysfs_emit(buf, _fmt, _val); \ > + } \ > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > + __LL_ATTR_RO(_attrname, _prop) > + > +/* Attribute current_value show/store */ > +#define __LL_TUNABLE_RW_CAP01(_attrname) \ > + static ssize_t _attrname##_current_value_store( \ > + struct kobject *kobj, struct kobj_attribute *attr, \ > + const char *buf, size_t count) \ > + { \ > + return attr_current_value_store(kobj, attr, buf, count, \ > + &_attrname); \ > + } \ > + static ssize_t _attrname##_current_value_show( \ > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > + { \ > + return attr_current_value_show(kobj, attr, buf, &_attrname); \ > + } \ > + static struct kobj_attribute attr_##_attrname##_current_value = \ > + __LL_ATTR_RW(_attrname, current_value) > + > +/* Attribute property show only */ > +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \ > + static ssize_t _attrname##_##_prop##_show( \ > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > + { \ > + return attr_capdata01_show(kobj, attr, buf, &_attrname, \ > + _prop_type); \ > + } \ > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > + __LL_ATTR_RO(_attrname, _prop) > + > +#define ATTR_GROUP_LL_TUNABLE_CAP01(_attrname, _fsname, _dispname) \ > + __LL_TUNABLE_RW_CAP01(_attrname); \ > + __LL_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL); \ > + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \ > + __LL_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL); \ > + __LL_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL); \ > + __LL_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL); \ > + static struct kobj_attribute attr_##_attrname##_type = \ > + __LL_ATTR_RO_AS(type, int_type_show); \ > + static struct attribute *_attrname##_attrs[] = { \ > + &attr_##_attrname##_current_value.attr, \ > + &attr_##_attrname##_default_value.attr, \ > + &attr_##_attrname##_display_name.attr, \ > + &attr_##_attrname##_max_value.attr, \ > + &attr_##_attrname##_min_value.attr, \ > + &attr_##_attrname##_scalar_increment.attr, \ > + &attr_##_attrname##_type.attr, \ > + NULL, \ > + }; \ > + static const struct attribute_group _attrname##_attr_group = { \ > + .name = _fsname, .attrs = _attrname##_attrs \ > + } Is there a reason why this needs to be put inside the header? If no then please put this inside the driver. Thanks, Armin Wolf > + > #endif /* !_LENOVO_WMI_H_ */
On Thu, Jan 9, 2025 at 3:00 PM Armin Wolf <W_Armin@gmx.de> wrote: > > Am 02.01.25 um 01:47 schrieb Derek J. Clark: > > > Adds lenovo-wmi-other.c which provides a driver for the Lenovo > > "Other Mode" WMI interface that comes on some Lenovo "Gaming > > Series" hardware. Provides a firmware-attributes class which > > enables the use of tunable knobs for SPL, SPPT, and FPPT. > > > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. > > - Use list to get the lenovo_wmi_om_priv instance in some macro > > called functions as the data provided by the macros that use it > > doesn't pass a member of the struct for use in container_of. > > - Do not rely on GameZone interface to grab the current fan mode. > > > > Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> > > --- > > MAINTAINERS | 1 + > > drivers/platform/x86/Kconfig | 12 + > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ > > drivers/platform/x86/lenovo-wmi.h | 116 +++++++ > > 5 files changed, 515 insertions(+) > > create mode 100644 drivers/platform/x86/lenovo-wmi-other.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index c9374c395905..318e1e517eed 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org > > S: Maintained > > F: drivers/platform/x86/lenovo-wmi-capdata01.c > > F: drivers/platform/x86/lenovo-wmi-gamezone.c > > +F: drivers/platform/x86/lenovo-wmi-other.c > > F: drivers/platform/x86/lenovo-wmi.h > > > > LETSKETCH HID TABLET DRIVER > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > index a2c1ab47ad9e..e2285ab987c5 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 > > To compile this driver as a module, choose M here: the module will > > be called lenovo_wmi_capdata01. > > > > +config LENOVO_WMI_TUNING > > + tristate "Lenovo Other Method WMI Driver" > > + depends on LENOVO_WMI_DATA01 > > I think we should use "select LENOVO_WMI_DATA01" here. Ideally CONFIG_LENOVO_WMI_DATA01 > will automatically be enabled/disabled if users enable/disable CONFIG_LENOVO_WMI_TUNING. > Makes sense with the other change. Will do. > > + select FW_ATTR_CLASS > > + help > > + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the > > + firmware_attributes API to control various tunable settings typically exposed by > > + Lenovo software in Windows. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called lenovo_wmi_other. > > Check the module name again. > > > + > > config IDEAPAD_LAPTOP > > tristate "Lenovo IdeaPad Laptop Extras" > > depends on ACPI > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > index 6c96cc3f3855..3e059b3c3647 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -70,6 +70,7 @@ 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 > > +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o > > > > # Intel > > obj-y += intel/ > > diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c > > new file mode 100644 > > index 000000000000..2392faa74973 > > --- /dev/null > > +++ b/drivers/platform/x86/lenovo-wmi-other.c > > @@ -0,0 +1,385 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes > > + * class to expose the various WMI functions provided by the "Other Method" WMI > > + * interface. This enables CPU and GPU power limit as well as various other > > + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop > > + * devices. Each attribute exposed by the "Other Method"" interface has a > > + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to > > + * probe details about the attribute such as set/get support, step, min, max, > > + * and default value. Each attibute has multiple pages, one for each of the > > + * fan profiles managed by the GameZone interface, so it must be probed prior > > + * to returning the current_value. > > + * > > + * These attributes typically don't fit anywhere else in the sysfs and are set > > + * in Windows using one of Lenovo's multiple user applications. > > + * > > + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> > > + */ > > + > > +#include <linux/list.h> > > +#include "lenovo-wmi.h" > > +#include "firmware_attributes_class.h" > > + > > +#define FW_ATTR_FOLDER "lenovo-wmi-other" > > +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" > > + > > +/* Device IDs */ > > +#define WMI_DEVICE_ID_CPU 0x01 > > + > > +/* WMI_DEVICE_ID_CPU feature IDs */ > > +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ > > +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ > > +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ > > +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ > > + > > +/* Method IDs */ > > +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ > > +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ > > + > > +static DEFINE_MUTEX(call_mutex); > > Is this mutex really necessary? If not then remove it please. > Same as other drivers, will remove. > > +static DEFINE_MUTEX(om_list_mutex); > > +static LIST_HEAD(om_wmi_list); > > + > > +struct lenovo_wmi_om_priv { > > + struct wmi_device *wdev; > > + struct device *fw_attr_dev; > > + struct kset *fw_attr_kset; > > + struct list_head list; > > +}; > > + > > +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) > > +{ > > + guard(mutex)(&om_list_mutex); > > + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, > > + list); > > +} > > + > > +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { > > + { LENOVO_OTHER_METHOD_GUID, NULL }, > > + {} > > +}; > > Please move the list of device ids closer to the driver struct. > Will do. > > + > > +/* Tunable Attributes */ > > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, > > + .feature_id = WMI_FEATURE_ID_CPU_SPL }; > > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, > > + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; > > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, > > + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; > > + > > +struct capdata01_attr_group { > > + const struct attribute_group *attr_group; > > + struct tunable_attr_01 *tunable_attr; > > Would it make sense to do something similar with each attribute, so that each attribute > can use container_of() to access lenovo_wmi_om_priv without having to use a list lookup? > > This would of course mean that each attribute as to be allocated dynamically. > > Heep in mind that we are currently working on a new API for registering firmware-atrtibute class > devices which should fix this. > I'm not sure I understand what you mean exactly. I think what you're saying is, instead of an attr_group, allocate each attribute as a struct in priv? > > +}; > > + > > +static const struct class *fw_attr_class; > > + > > +/** > > + * attr_capdata01_setup() - Get the data of the specified attribute > > + * from LENOVO_CAPABILITY_DATA_01 and store it. > > + * @tunable_attr: The attribute to be populated. > > + * > > + * Returns: Either 0 or an error. > > + */ > > +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) > > +{ > > + struct capability_data_01 cap_data; > > + int mode = SMARTFAN_MODE_CUSTOM; > > + int err; > > + > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > + tunable_attr->feature_id, > > + tunable_attr->device_id }; > > Please use FIELD_GET()/FIELD_PREP() here. > Can do. > > + > > + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); > > + if (err) { > > + pr_err("Failed to get capability data: %u\n", err); > > + return err; > > + } > > + > > + if (cap_data.supported < 1) > > + return -EOPNOTSUPP; > > + > > + tunable_attr->capdata = cap_data; > > + > > + return 0; > > +} > > + > > +/** > > + * attr_capdata01_show() - Get the value of the specified attribute property > > + * from LENOVO_CAPABILITY_DATA_01. > > + * @kobj: Pointer to the driver object. > > + * @kobj_attribute: Pointer to the attribute calling this function. > > + * @buf: The buffer to write to. > > + * @tunable_attr: The attribute to be read. > > + * @prop: The property of this attribute to be read. > > + * > > + * This function is intended to be generic so it can be called from any "_show" > > + * attribute which works only with integers. > > + * > > + * If the WMI is success, then the sysfs attribute is notified. > > + * > > + * Returns: Either count, or an error. > > + */ > > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf, struct tunable_attr_01 *tunable_attr, > > + enum attribute_property prop) > > +{ > > + struct capability_data_01 cap_data; > > + int retval; > > + > > + cap_data = tunable_attr->capdata; > > + > > + switch (prop) { > > + case DEFAULT_VAL: > > + retval = cap_data.default_value; > > + break; > > + case MAX_VAL: > > + retval = cap_data.max_value; > > + break; > > + case MIN_VAL: > > + retval = cap_data.min_value; > > + break; > > + case STEP_VAL: > > + retval = cap_data.step; > > + break; > > + default: > > + return -EINVAL; > > + } > > + return sysfs_emit(buf, "%u\n", retval); > > +} > > + > > +/* Simple attribute creation */ > > + > > +/* > > + * att_current_value_store() - Set the current value of the given attribute > > + * @kobj: Pointer to the driver object. > > + * @kobj_attribute: Pointer to the attribute calling this function. > > + * @buf: The buffer to read from, this is parsed to `int` type. > > + * @count: Required by sysfs attribute macros, pass in from the callee attr. > > + * @tunable_attr: The attribute to be stored. > > + * > > + * This function is intended to be generic so it can be called from any > > + * attribute's "current_value_store" which works only with integers. The > > + * integer to be sent to the WMI method is range checked and an error returned > > + * if out of range. > > + * > > + * If the value is valid and WMI is success, then the sysfs attribute is > > + * notified. > > + * > > + * Returns: Either count, or an error. > > + */ > > +ssize_t attr_current_value_store(struct kobject *kobj, > > + struct kobj_attribute *attr, const char *buf, > > + size_t count, > > + struct tunable_attr_01 *tunable_attr) > > +{ > > + struct capability_data_01 cap_data; > > + struct lenovo_wmi_om_priv *priv; > > + int mode = SMARTFAN_MODE_CUSTOM; > > + u32 value; > > + int err; > > + > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > + tunable_attr->feature_id, > > + tunable_attr->device_id }; > > + > > + err = kstrtouint(buf, 10, &value); > > + if (err) { > > + pr_err("Error converting value to int: %u\n", err); > > + return err; > > + } > > + > > + cap_data = tunable_attr->capdata; > > + > > + if (value < cap_data.min_value || value > cap_data.max_value) > > + return -EINVAL; > > + > > + priv = get_first_wmi_priv(); > > + if (!priv) > > + return -ENODEV; > > + > > + guard(mutex)(&call_mutex); > > + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, > > + WMI_METHOD_ID_VALUE_SET, > > + *(int *)&attr_id, value, NULL); > > + > > + if (err) { > > + pr_err("Error setting attribute: %u\n", err); > > This error message is unnecessary, please drop it. > Ack > > + return err; > > + } > > + > > + tunable_attr->store_value = value; > > Is this value used anywhere? If no then please drop it. > It isn't, will do. > > + > > + sysfs_notify(kobj, NULL, attr->attr.name); > > AFAIK this is unnecessary since userspace already expects the attribute value to > change after an write access. This is only meant to be used should the value be > changed by for example the underlying hardware without user intervention. > I can drop this too. > > + > > + return count; > > +}; > > + > > +/* > > + * attr_current_value_show() - Get the current value of the given attribute > > + * @kobj: Pointer to the driver object. > > + * @kobj_attribute: Pointer to the attribute calling this function. > > + * @buf: The buffer to write to. > > + * @tunable_attr: The attribute to be read. > > + * > > + * This function is intended to be generic so it can be called from any "_show" > > + * attribute which works only with integers. > > + * > > + * If the WMI is success, then the sysfs attribute is notified. > > + * > > + * Returns: Either count, or an error. > > + */ > > +ssize_t attr_current_value_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf, > > + struct tunable_attr_01 *tunable_attr) > > +{ > > + struct lenovo_wmi_om_priv *priv; > > + int mode = SMARTFAN_MODE_CUSTOM; > > + int retval; > > + int err; > > + > > + struct lenovo_wmi_attr_id attr_id = { mode << 8, > > + tunable_attr->feature_id, > > + tunable_attr->device_id }; > > + > > + priv = get_first_wmi_priv(); > > + if (!priv) > > + return -ENODEV; > > + > > + guard(mutex)(&call_mutex); > > + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, > > + WMI_METHOD_ID_VALUE_GET, > > + *(int *)&attr_id, &retval); > > + > > + if (err) { > > + pr_err("Error getting attribute: %u\n", err); > > + return err; > > + } > > + > > + return sysfs_emit(buf, "%u\n", retval); > > +} > > + > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", > > + "Set the CPU sustained power limit"); > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", > > + "Set the CPU slow package power tracking limit"); > > +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", > > + "Set the CPU fast package power tracking limit"); > > + > > +static const struct capdata01_attr_group capdata01_attr_groups[] = { > > + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, > > + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, > > + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, > > + {}, > > +}; > > + > > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) > > +{ > > + int err, i; > > + > > + err = fw_attributes_class_get(&fw_attr_class); > > + if (err) { > > + pr_err("Failed to get firmware_attributes_class: %u\n", err); > > + return err; > > + } > > + > > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > > + NULL, "%s", FW_ATTR_FOLDER); > > + if (IS_ERR(priv->fw_attr_dev)) { > > + err = PTR_ERR(priv->fw_attr_dev); > > + pr_err("Failed to create firmware_attributes_class device: %u\n", > > + err); > > + goto fail_class_get; > > + } > > + > > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, > > + &priv->fw_attr_dev->kobj); > > + if (!priv->fw_attr_kset) { > > + err = -ENOMEM; > > + pr_err("Failed to create firmware_attributes_class kset: %u\n", > > + err); > > + goto err_destroy_classdev; > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { > > + err = attr_capdata01_setup( > > + capdata01_attr_groups[i].tunable_attr); > > + if (err) { > > + pr_err("Failed to populate capability data for %s: %u\n", > > + capdata01_attr_groups[i].attr_group->name, err); > > + continue; > > + } > > + > > + err = sysfs_create_group(&priv->fw_attr_kset->kobj, > > + capdata01_attr_groups[i].attr_group); > > AFAIK there exists sysfs_create_groups(). Together with the *_is_visible callbacks this > should simplify this part of your code a lot. > Part of this code is caching the capability data so it doesn't need to be called after probe. If I can get the cached list working in that driver I can drop storing it here and use _is_visible as a macro component. > > + if (err) { > > + pr_err("Failed to create sysfs-group for %s: %u\n", > > + capdata01_attr_groups[i].attr_group->name, err); > > + goto err_remove_groups; > > + } > > + } > > + > > + return 0; > > + > > +err_remove_groups: > > + while (i-- > 0) { > > + sysfs_remove_group(&priv->fw_attr_kset->kobj, > > + capdata01_attr_groups[i].attr_group); > > + } > > + > > + return err; > > Please remove this return statement, since the other resources need to be cleaned up too. Agree. Ilpo noted these as well. > Also where do you clean up the kset? I'll add it here. > > + > > +err_destroy_classdev: > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > Please use device_unregister() instead. > Ack > > + > > + return err; > ... > > + > > +fail_class_get: > > + fw_attributes_class_put(); > > + > > + return err; > > +} > > + > > +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context) > > +{ > > + struct lenovo_wmi_om_priv *priv; > > + > > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->wdev = wdev; > > Missing dev_set_drvdata(). > Ack > > + > > + guard(mutex)(&om_list_mutex); > > + list_add_tail(&priv->list, &om_wmi_list); > > + > > + return other_method_fw_attr_add(priv); > > +} > > + > > +static void lenovo_wmi_other_remove(struct wmi_device *wdev) > > +{ > > + struct lenovo_wmi_om_priv *priv = dev_get_drvdata(&wdev->dev); > > + > > + guard(mutex)(&om_list_mutex); > > + list_del(&priv->list); > > + kset_unregister(priv->fw_attr_kset); > > + device_destroy(fw_attr_class, MKDEV(0, 0)); > > + fw_attributes_class_put(); > > +} > > + > > +static struct wmi_driver lenovo_wmi_other_driver = { > > + .driver = { .name = "lenovo_wmi_other" }, > > .probe_type = PROBE_PREFER_ASYNCHRONOUS > Ack > > + .id_table = lenovo_wmi_other_id_table, > > + .probe = lenovo_wmi_other_probe, > > + .remove = lenovo_wmi_other_remove, > > .no_singleton = true > Ack > In this case please make sure that the name of the firmware-attributes class device is unique. > You can use an IDA (https://docs.kernel.org/core-api/idr.html) for this. Will do, thanks. > > +}; > > + > > +module_wmi_driver(lenovo_wmi_other_driver); > > + > > +MODULE_IMPORT_NS("CAPDATA_WMI"); > > +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table); > > +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>"); > > +MODULE_DESCRIPTION("Lenovo Other Method WMI Driver"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h > > index 53cea84a956b..1c8358551ba6 100644 > > --- a/drivers/platform/x86/lenovo-wmi.h > > +++ b/drivers/platform/x86/lenovo-wmi.h > > @@ -42,6 +42,14 @@ struct lenovo_wmi_attr_id { > > u32 device_id : 8; /* CPU/GPU/... */ > > } __packed; > > > > +enum attribute_property { > > + DEFAULT_VAL, > > + MAX_VAL, > > + MIN_VAL, > > + STEP_VAL, > > + SUPPORTED, > > +}; > > + > > /* Data struct for LENOVO_CAPABILITY_DATA_01 */ > > struct capability_data_01 { > > u32 id; > > @@ -52,6 +60,14 @@ struct capability_data_01 { > > u32 max_value; > > }; > > > > +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */ > > +struct tunable_attr_01 { > > + struct capability_data_01 capdata; > > + u32 device_id; > > + u32 feature_id; > > + u32 store_value; > > +}; > > + > > /* General Use functions */ > > static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, > > u32 method_id, struct acpi_buffer *in, > > @@ -122,4 +138,104 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance, > > int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id, > > struct capability_data_01 *cap_data); > > > > +/* Other Method attribute functions */ > > +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf, struct tunable_attr_01 *tunable_attr, > > + enum attribute_property prop); > > + > > +ssize_t attr_current_value_store(struct kobject *kobj, > > + struct kobj_attribute *attr, const char *buf, > > + size_t count, > > + struct tunable_attr_01 *tunable_attr); > > + > > +ssize_t attr_current_value_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf, > > + struct tunable_attr_01 *tunable_attr); > > + > > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf); > > + > > +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf) > > +{ > > + return sysfs_emit(buf, "integer\n"); > > +} > > + > > +/* Other Method attribute macros */ > > +#define __LL_ATTR_RO(_func, _name) \ > > + { \ > > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > > + .show = _func##_##_name##_show, \ > > + } > > + > > +#define __LL_ATTR_RO_AS(_name, _show) \ > > + { \ > > + .attr = { .name = __stringify(_name), .mode = 0444 }, \ > > + .show = _show, \ > > + } > > + > > +#define __LL_ATTR_RW(_func, _name) \ > > + __ATTR(_name, 0644, _func##_##_name##_show, _func##_##_name##_store) > > + > > +/* Shows a formatted static variable */ > > +#define __ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val) \ > > + static ssize_t _attrname##_##_prop##_show( \ > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > + { \ > > + return sysfs_emit(buf, _fmt, _val); \ > > + } \ > > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > > + __LL_ATTR_RO(_attrname, _prop) > > + > > +/* Attribute current_value show/store */ > > +#define __LL_TUNABLE_RW_CAP01(_attrname) \ > > + static ssize_t _attrname##_current_value_store( \ > > + struct kobject *kobj, struct kobj_attribute *attr, \ > > + const char *buf, size_t count) \ > > + { \ > > + return attr_current_value_store(kobj, attr, buf, count, \ > > + &_attrname); \ > > + } \ > > + static ssize_t _attrname##_current_value_show( \ > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > + { \ > > + return attr_current_value_show(kobj, attr, buf, &_attrname); \ > > + } \ > > + static struct kobj_attribute attr_##_attrname##_current_value = \ > > + __LL_ATTR_RW(_attrname, current_value) > > + > > +/* Attribute property show only */ > > +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \ > > + static ssize_t _attrname##_##_prop##_show( \ > > + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ > > + { \ > > + return attr_capdata01_show(kobj, attr, buf, &_attrname, \ > > + _prop_type); \ > > + } \ > > + static struct kobj_attribute attr_##_attrname##_##_prop = \ > > + __LL_ATTR_RO(_attrname, _prop) > > + > > +#define ATTR_GROUP_LL_TUNABLE_CAP01(_attrname, _fsname, _dispname) \ > > + __LL_TUNABLE_RW_CAP01(_attrname); \ > > + __LL_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL); \ > > + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \ > > + __LL_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL); \ > > + __LL_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL); \ > > + __LL_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL); \ > > + static struct kobj_attribute attr_##_attrname##_type = \ > > + __LL_ATTR_RO_AS(type, int_type_show); \ > > + static struct attribute *_attrname##_attrs[] = { \ > > + &attr_##_attrname##_current_value.attr, \ > > + &attr_##_attrname##_default_value.attr, \ > > + &attr_##_attrname##_display_name.attr, \ > > + &attr_##_attrname##_max_value.attr, \ > > + &attr_##_attrname##_min_value.attr, \ > > + &attr_##_attrname##_scalar_increment.attr, \ > > + &attr_##_attrname##_type.attr, \ > > + NULL, \ > > + }; \ > > + static const struct attribute_group _attrname##_attr_group = { \ > > + .name = _fsname, .attrs = _attrname##_attrs \ > > + } > > Is there a reason why this needs to be put inside the header? If no then please put this > inside the driver. To clarify, you mean the macros? I was under the impression they belonged in headers but I can move them. I will move some of the enums/structs as well which are referenced here and the driver only. > Thanks, > Armin Wolf > > > + > > #endif /* !_LENOVO_WMI_H_ */
Am 10.01.25 um 23:33 schrieb Derek John Clark: > On Thu, Jan 9, 2025 at 3:00 PM Armin Wolf <W_Armin@gmx.de> wrote: >> Am 02.01.25 um 01:47 schrieb Derek J. Clark: >> >>> Adds lenovo-wmi-other.c which provides a driver for the Lenovo >>> "Other Mode" WMI interface that comes on some Lenovo "Gaming >>> Series" hardware. Provides a firmware-attributes class which >>> enables the use of tunable knobs for SPL, SPPT, and FPPT. >>> >>> 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. >>> - Use list to get the lenovo_wmi_om_priv instance in some macro >>> called functions as the data provided by the macros that use it >>> doesn't pass a member of the struct for use in container_of. >>> - Do not rely on GameZone interface to grab the current fan mode. >>> >>> Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> >>> --- >>> MAINTAINERS | 1 + >>> drivers/platform/x86/Kconfig | 12 + >>> drivers/platform/x86/Makefile | 1 + >>> drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ >>> drivers/platform/x86/lenovo-wmi.h | 116 +++++++ >>> 5 files changed, 515 insertions(+) >>> create mode 100644 drivers/platform/x86/lenovo-wmi-other.c >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index c9374c395905..318e1e517eed 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org >>> S: Maintained >>> F: drivers/platform/x86/lenovo-wmi-capdata01.c >>> F: drivers/platform/x86/lenovo-wmi-gamezone.c >>> +F: drivers/platform/x86/lenovo-wmi-other.c >>> F: drivers/platform/x86/lenovo-wmi.h >>> >>> LETSKETCH HID TABLET DRIVER >>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >>> index a2c1ab47ad9e..e2285ab987c5 100644 >>> --- a/drivers/platform/x86/Kconfig >>> +++ b/drivers/platform/x86/Kconfig >>> @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 >>> To compile this driver as a module, choose M here: the module will >>> be called lenovo_wmi_capdata01. >>> >>> +config LENOVO_WMI_TUNING >>> + tristate "Lenovo Other Method WMI Driver" >>> + depends on LENOVO_WMI_DATA01 >> I think we should use "select LENOVO_WMI_DATA01" here. Ideally CONFIG_LENOVO_WMI_DATA01 >> will automatically be enabled/disabled if users enable/disable CONFIG_LENOVO_WMI_TUNING. >> > Makes sense with the other change. Will do. > >>> + select FW_ATTR_CLASS >>> + help >>> + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the >>> + firmware_attributes API to control various tunable settings typically exposed by >>> + Lenovo software in Windows. >>> + >>> + To compile this driver as a module, choose M here: the module will >>> + be called lenovo_wmi_other. >> Check the module name again. >> >>> + >>> config IDEAPAD_LAPTOP >>> tristate "Lenovo IdeaPad Laptop Extras" >>> depends on ACPI >>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile >>> index 6c96cc3f3855..3e059b3c3647 100644 >>> --- a/drivers/platform/x86/Makefile >>> +++ b/drivers/platform/x86/Makefile >>> @@ -70,6 +70,7 @@ 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 >>> +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o >>> >>> # Intel >>> obj-y += intel/ >>> diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c >>> new file mode 100644 >>> index 000000000000..2392faa74973 >>> --- /dev/null >>> +++ b/drivers/platform/x86/lenovo-wmi-other.c >>> @@ -0,0 +1,385 @@ >>> +// SPDX-License-Identifier: GPL-2.0-or-later >>> +/* >>> + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes >>> + * class to expose the various WMI functions provided by the "Other Method" WMI >>> + * interface. This enables CPU and GPU power limit as well as various other >>> + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop >>> + * devices. Each attribute exposed by the "Other Method"" interface has a >>> + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to >>> + * probe details about the attribute such as set/get support, step, min, max, >>> + * and default value. Each attibute has multiple pages, one for each of the >>> + * fan profiles managed by the GameZone interface, so it must be probed prior >>> + * to returning the current_value. >>> + * >>> + * These attributes typically don't fit anywhere else in the sysfs and are set >>> + * in Windows using one of Lenovo's multiple user applications. >>> + * >>> + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> >>> + */ >>> + >>> +#include <linux/list.h> >>> +#include "lenovo-wmi.h" >>> +#include "firmware_attributes_class.h" >>> + >>> +#define FW_ATTR_FOLDER "lenovo-wmi-other" >>> +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" >>> + >>> +/* Device IDs */ >>> +#define WMI_DEVICE_ID_CPU 0x01 >>> + >>> +/* WMI_DEVICE_ID_CPU feature IDs */ >>> +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ >>> +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ >>> +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ >>> +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ >>> + >>> +/* Method IDs */ >>> +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ >>> +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ >>> + >>> +static DEFINE_MUTEX(call_mutex); >> Is this mutex really necessary? If not then remove it please. >> > Same as other drivers, will remove. > >>> +static DEFINE_MUTEX(om_list_mutex); >>> +static LIST_HEAD(om_wmi_list); >>> + >>> +struct lenovo_wmi_om_priv { >>> + struct wmi_device *wdev; >>> + struct device *fw_attr_dev; >>> + struct kset *fw_attr_kset; >>> + struct list_head list; >>> +}; >>> + >>> +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) >>> +{ >>> + guard(mutex)(&om_list_mutex); >>> + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, >>> + list); >>> +} >>> + >>> +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { >>> + { LENOVO_OTHER_METHOD_GUID, NULL }, >>> + {} >>> +}; >> Please move the list of device ids closer to the driver struct. >> > Will do. > >>> + >>> +/* Tunable Attributes */ >>> +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, >>> + .feature_id = WMI_FEATURE_ID_CPU_SPL }; >>> +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, >>> + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; >>> +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, >>> + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; >>> + >>> +struct capdata01_attr_group { >>> + const struct attribute_group *attr_group; >>> + struct tunable_attr_01 *tunable_attr; >> Would it make sense to do something similar with each attribute, so that each attribute >> can use container_of() to access lenovo_wmi_om_priv without having to use a list lookup? >> >> This would of course mean that each attribute as to be allocated dynamically. >> >> Heep in mind that we are currently working on a new API for registering firmware-atrtibute class >> devices which should fix this. >> > I'm not sure I understand what you mean exactly. I think what you're > saying is, instead of an attr_group, allocate each attribute as a > struct in priv? Kind of. I envisioned something like this (pseudo code): struct firmware_attribute { struct kobj_attribute attr; struct lenovo_wmi_om_priv; } This would allow you to use container_of() to access priv, but would force you to allocate each attribute separately. Maybe you can wait with the lenovo-wmi-other driver until the improved firmware-attribute class device API has landed. Meanwhile we can focus on the lenovo-wmi-gamezone driver. >>> +}; >>> + >>> +static const struct class *fw_attr_class; >>> + >>> +/** >>> + * attr_capdata01_setup() - Get the data of the specified attribute >>> + * from LENOVO_CAPABILITY_DATA_01 and store it. >>> + * @tunable_attr: The attribute to be populated. >>> + * >>> + * Returns: Either 0 or an error. >>> + */ >>> +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) >>> +{ >>> + struct capability_data_01 cap_data; >>> + int mode = SMARTFAN_MODE_CUSTOM; >>> + int err; >>> + >>> + struct lenovo_wmi_attr_id attr_id = { mode << 8, >>> + tunable_attr->feature_id, >>> + tunable_attr->device_id }; >> Please use FIELD_GET()/FIELD_PREP() here. >> > Can do. > >>> + >>> + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); >>> + if (err) { >>> + pr_err("Failed to get capability data: %u\n", err); >>> + return err; >>> + } >>> + >>> + if (cap_data.supported < 1) >>> + return -EOPNOTSUPP; >>> + >>> + tunable_attr->capdata = cap_data; >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * attr_capdata01_show() - Get the value of the specified attribute property >>> + * from LENOVO_CAPABILITY_DATA_01. >>> + * @kobj: Pointer to the driver object. >>> + * @kobj_attribute: Pointer to the attribute calling this function. >>> + * @buf: The buffer to write to. >>> + * @tunable_attr: The attribute to be read. >>> + * @prop: The property of this attribute to be read. >>> + * >>> + * This function is intended to be generic so it can be called from any "_show" >>> + * attribute which works only with integers. >>> + * >>> + * If the WMI is success, then the sysfs attribute is notified. >>> + * >>> + * Returns: Either count, or an error. >>> + */ >>> +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, >>> + char *buf, struct tunable_attr_01 *tunable_attr, >>> + enum attribute_property prop) >>> +{ >>> + struct capability_data_01 cap_data; >>> + int retval; >>> + >>> + cap_data = tunable_attr->capdata; >>> + >>> + switch (prop) { >>> + case DEFAULT_VAL: >>> + retval = cap_data.default_value; >>> + break; >>> + case MAX_VAL: >>> + retval = cap_data.max_value; >>> + break; >>> + case MIN_VAL: >>> + retval = cap_data.min_value; >>> + break; >>> + case STEP_VAL: >>> + retval = cap_data.step; >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + return sysfs_emit(buf, "%u\n", retval); >>> +} >>> + >>> +/* Simple attribute creation */ >>> + >>> +/* >>> + * att_current_value_store() - Set the current value of the given attribute >>> + * @kobj: Pointer to the driver object. >>> + * @kobj_attribute: Pointer to the attribute calling this function. >>> + * @buf: The buffer to read from, this is parsed to `int` type. >>> + * @count: Required by sysfs attribute macros, pass in from the callee attr. >>> + * @tunable_attr: The attribute to be stored. >>> + * >>> + * This function is intended to be generic so it can be called from any >>> + * attribute's "current_value_store" which works only with integers. The >>> + * integer to be sent to the WMI method is range checked and an error returned >>> + * if out of range. >>> + * >>> + * If the value is valid and WMI is success, then the sysfs attribute is >>> + * notified. >>> + * >>> + * Returns: Either count, or an error. >>> + */ >>> +ssize_t attr_current_value_store(struct kobject *kobj, >>> + struct kobj_attribute *attr, const char *buf, >>> + size_t count, >>> + struct tunable_attr_01 *tunable_attr) >>> +{ >>> + struct capability_data_01 cap_data; >>> + struct lenovo_wmi_om_priv *priv; >>> + int mode = SMARTFAN_MODE_CUSTOM; >>> + u32 value; >>> + int err; >>> + >>> + struct lenovo_wmi_attr_id attr_id = { mode << 8, >>> + tunable_attr->feature_id, >>> + tunable_attr->device_id }; >>> + >>> + err = kstrtouint(buf, 10, &value); >>> + if (err) { >>> + pr_err("Error converting value to int: %u\n", err); >>> + return err; >>> + } >>> + >>> + cap_data = tunable_attr->capdata; >>> + >>> + if (value < cap_data.min_value || value > cap_data.max_value) >>> + return -EINVAL; >>> + >>> + priv = get_first_wmi_priv(); >>> + if (!priv) >>> + return -ENODEV; >>> + >>> + guard(mutex)(&call_mutex); >>> + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, >>> + WMI_METHOD_ID_VALUE_SET, >>> + *(int *)&attr_id, value, NULL); >>> + >>> + if (err) { >>> + pr_err("Error setting attribute: %u\n", err); >> This error message is unnecessary, please drop it. >> > Ack > >>> + return err; >>> + } >>> + >>> + tunable_attr->store_value = value; >> Is this value used anywhere? If no then please drop it. >> > It isn't, will do. >>> + >>> + sysfs_notify(kobj, NULL, attr->attr.name); >> AFAIK this is unnecessary since userspace already expects the attribute value to >> change after an write access. This is only meant to be used should the value be >> changed by for example the underlying hardware without user intervention. >> > I can drop this too. > >>> + >>> + return count; >>> +}; >>> + >>> +/* >>> + * attr_current_value_show() - Get the current value of the given attribute >>> + * @kobj: Pointer to the driver object. >>> + * @kobj_attribute: Pointer to the attribute calling this function. >>> + * @buf: The buffer to write to. >>> + * @tunable_attr: The attribute to be read. >>> + * >>> + * This function is intended to be generic so it can be called from any "_show" >>> + * attribute which works only with integers. >>> + * >>> + * If the WMI is success, then the sysfs attribute is notified. >>> + * >>> + * Returns: Either count, or an error. >>> + */ >>> +ssize_t attr_current_value_show(struct kobject *kobj, >>> + struct kobj_attribute *attr, char *buf, >>> + struct tunable_attr_01 *tunable_attr) >>> +{ >>> + struct lenovo_wmi_om_priv *priv; >>> + int mode = SMARTFAN_MODE_CUSTOM; >>> + int retval; >>> + int err; >>> + >>> + struct lenovo_wmi_attr_id attr_id = { mode << 8, >>> + tunable_attr->feature_id, >>> + tunable_attr->device_id }; >>> + >>> + priv = get_first_wmi_priv(); >>> + if (!priv) >>> + return -ENODEV; >>> + >>> + guard(mutex)(&call_mutex); >>> + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, >>> + WMI_METHOD_ID_VALUE_GET, >>> + *(int *)&attr_id, &retval); >>> + >>> + if (err) { >>> + pr_err("Error getting attribute: %u\n", err); >>> + return err; >>> + } >>> + >>> + return sysfs_emit(buf, "%u\n", retval); >>> +} >>> + >>> +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", >>> + "Set the CPU sustained power limit"); >>> +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", >>> + "Set the CPU slow package power tracking limit"); >>> +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", >>> + "Set the CPU fast package power tracking limit"); >>> + >>> +static const struct capdata01_attr_group capdata01_attr_groups[] = { >>> + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, >>> + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, >>> + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, >>> + {}, >>> +}; >>> + >>> +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) >>> +{ >>> + int err, i; >>> + >>> + err = fw_attributes_class_get(&fw_attr_class); >>> + if (err) { >>> + pr_err("Failed to get firmware_attributes_class: %u\n", err); >>> + return err; >>> + } >>> + >>> + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), >>> + NULL, "%s", FW_ATTR_FOLDER); >>> + if (IS_ERR(priv->fw_attr_dev)) { >>> + err = PTR_ERR(priv->fw_attr_dev); >>> + pr_err("Failed to create firmware_attributes_class device: %u\n", >>> + err); >>> + goto fail_class_get; >>> + } >>> + >>> + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, >>> + &priv->fw_attr_dev->kobj); >>> + if (!priv->fw_attr_kset) { >>> + err = -ENOMEM; >>> + pr_err("Failed to create firmware_attributes_class kset: %u\n", >>> + err); >>> + goto err_destroy_classdev; >>> + } >>> + >>> + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { >>> + err = attr_capdata01_setup( >>> + capdata01_attr_groups[i].tunable_attr); >>> + if (err) { >>> + pr_err("Failed to populate capability data for %s: %u\n", >>> + capdata01_attr_groups[i].attr_group->name, err); >>> + continue; >>> + } >>> + >>> + err = sysfs_create_group(&priv->fw_attr_kset->kobj, >>> + capdata01_attr_groups[i].attr_group); >> AFAIK there exists sysfs_create_groups(). Together with the *_is_visible callbacks this >> should simplify this part of your code a lot. >> > Part of this code is caching the capability data so it doesn't need to > be called after probe. If I can get the cached list working in that > driver I can drop storing it here and use _is_visible as a macro > component. OK. >>> + if (err) { >>> + pr_err("Failed to create sysfs-group for %s: %u\n", >>> + capdata01_attr_groups[i].attr_group->name, err); >>> + goto err_remove_groups; >>> + } >>> + } >>> + >>> + return 0; >>> + >>> +err_remove_groups: >>> + while (i-- > 0) { >>> + sysfs_remove_group(&priv->fw_attr_kset->kobj, >>> + capdata01_attr_groups[i].attr_group); >>> + } >>> + >>> + return err; >> Please remove this return statement, since the other resources need to be cleaned up too. > Agree. Ilpo noted these as well. > >> Also where do you clean up the kset? > I'll add it here. > >>> + >>> +err_destroy_classdev: >>> + device_destroy(fw_attr_class, MKDEV(0, 0)); >> Please use device_unregister() instead. >> > Ack > >>> + >>> + return err; >> ... >>> + >>> +fail_class_get: >>> + fw_attributes_class_put(); >>> + >>> + return err; >>> +} >>> + >>> +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context) >>> +{ >>> + struct lenovo_wmi_om_priv *priv; >>> + >>> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + priv->wdev = wdev; >> Missing dev_set_drvdata(). >> > Ack > >>> + >>> + guard(mutex)(&om_list_mutex); >>> + list_add_tail(&priv->list, &om_wmi_list); >>> + >>> + return other_method_fw_attr_add(priv); >>> +} >>> + >>> +static void lenovo_wmi_other_remove(struct wmi_device *wdev) >>> +{ >>> + struct lenovo_wmi_om_priv *priv = dev_get_drvdata(&wdev->dev); >>> + >>> + guard(mutex)(&om_list_mutex); >>> + list_del(&priv->list); >>> + kset_unregister(priv->fw_attr_kset); >>> + device_destroy(fw_attr_class, MKDEV(0, 0)); >>> + fw_attributes_class_put(); >>> +} >>> + >>> +static struct wmi_driver lenovo_wmi_other_driver = { >>> + .driver = { .name = "lenovo_wmi_other" }, >> .probe_type = PROBE_PREFER_ASYNCHRONOUS >> > Ack > >>> + .id_table = lenovo_wmi_other_id_table, >>> + .probe = lenovo_wmi_other_probe, >>> + .remove = lenovo_wmi_other_remove, >> .no_singleton = true >> > Ack > >> In this case please make sure that the name of the firmware-attributes class device is unique. >> You can use an IDA (https://docs.kernel.org/core-api/idr.html) for this. > Will do, thanks. > >>> +}; >>> + >>> +module_wmi_driver(lenovo_wmi_other_driver); >>> + >>> +MODULE_IMPORT_NS("CAPDATA_WMI"); >>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table); >>> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>"); >>> +MODULE_DESCRIPTION("Lenovo Other Method WMI Driver"); >>> +MODULE_LICENSE("GPL"); >>> diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h >>> index 53cea84a956b..1c8358551ba6 100644 >>> --- a/drivers/platform/x86/lenovo-wmi.h >>> +++ b/drivers/platform/x86/lenovo-wmi.h >>> @@ -42,6 +42,14 @@ struct lenovo_wmi_attr_id { >>> u32 device_id : 8; /* CPU/GPU/... */ >>> } __packed; >>> >>> +enum attribute_property { >>> + DEFAULT_VAL, >>> + MAX_VAL, >>> + MIN_VAL, >>> + STEP_VAL, >>> + SUPPORTED, >>> +}; >>> + >>> /* Data struct for LENOVO_CAPABILITY_DATA_01 */ >>> struct capability_data_01 { >>> u32 id; >>> @@ -52,6 +60,14 @@ struct capability_data_01 { >>> u32 max_value; >>> }; >>> >>> +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */ >>> +struct tunable_attr_01 { >>> + struct capability_data_01 capdata; >>> + u32 device_id; >>> + u32 feature_id; >>> + u32 store_value; >>> +}; >>> + >>> /* General Use functions */ >>> static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, >>> u32 method_id, struct acpi_buffer *in, >>> @@ -122,4 +138,104 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance, >>> int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id, >>> struct capability_data_01 *cap_data); >>> >>> +/* Other Method attribute functions */ >>> +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, >>> + char *buf, struct tunable_attr_01 *tunable_attr, >>> + enum attribute_property prop); >>> + >>> +ssize_t attr_current_value_store(struct kobject *kobj, >>> + struct kobj_attribute *attr, const char *buf, >>> + size_t count, >>> + struct tunable_attr_01 *tunable_attr); >>> + >>> +ssize_t attr_current_value_show(struct kobject *kobj, >>> + struct kobj_attribute *attr, char *buf, >>> + struct tunable_attr_01 *tunable_attr); >>> + >>> +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, >>> + char *buf); >>> + >>> +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, >>> + char *buf) >>> +{ >>> + return sysfs_emit(buf, "integer\n"); >>> +} >>> + >>> +/* Other Method attribute macros */ >>> +#define __LL_ATTR_RO(_func, _name) \ >>> + { \ >>> + .attr = { .name = __stringify(_name), .mode = 0444 }, \ >>> + .show = _func##_##_name##_show, \ >>> + } >>> + >>> +#define __LL_ATTR_RO_AS(_name, _show) \ >>> + { \ >>> + .attr = { .name = __stringify(_name), .mode = 0444 }, \ >>> + .show = _show, \ >>> + } >>> + >>> +#define __LL_ATTR_RW(_func, _name) \ >>> + __ATTR(_name, 0644, _func##_##_name##_show, _func##_##_name##_store) >>> + >>> +/* Shows a formatted static variable */ >>> +#define __ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val) \ >>> + static ssize_t _attrname##_##_prop##_show( \ >>> + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ >>> + { \ >>> + return sysfs_emit(buf, _fmt, _val); \ >>> + } \ >>> + static struct kobj_attribute attr_##_attrname##_##_prop = \ >>> + __LL_ATTR_RO(_attrname, _prop) >>> + >>> +/* Attribute current_value show/store */ >>> +#define __LL_TUNABLE_RW_CAP01(_attrname) \ >>> + static ssize_t _attrname##_current_value_store( \ >>> + struct kobject *kobj, struct kobj_attribute *attr, \ >>> + const char *buf, size_t count) \ >>> + { \ >>> + return attr_current_value_store(kobj, attr, buf, count, \ >>> + &_attrname); \ >>> + } \ >>> + static ssize_t _attrname##_current_value_show( \ >>> + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ >>> + { \ >>> + return attr_current_value_show(kobj, attr, buf, &_attrname); \ >>> + } \ >>> + static struct kobj_attribute attr_##_attrname##_current_value = \ >>> + __LL_ATTR_RW(_attrname, current_value) >>> + >>> +/* Attribute property show only */ >>> +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \ >>> + static ssize_t _attrname##_##_prop##_show( \ >>> + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ >>> + { \ >>> + return attr_capdata01_show(kobj, attr, buf, &_attrname, \ >>> + _prop_type); \ >>> + } \ >>> + static struct kobj_attribute attr_##_attrname##_##_prop = \ >>> + __LL_ATTR_RO(_attrname, _prop) >>> + >>> +#define ATTR_GROUP_LL_TUNABLE_CAP01(_attrname, _fsname, _dispname) \ >>> + __LL_TUNABLE_RW_CAP01(_attrname); \ >>> + __LL_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL); \ >>> + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \ >>> + __LL_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL); \ >>> + __LL_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL); \ >>> + __LL_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL); \ >>> + static struct kobj_attribute attr_##_attrname##_type = \ >>> + __LL_ATTR_RO_AS(type, int_type_show); \ >>> + static struct attribute *_attrname##_attrs[] = { \ >>> + &attr_##_attrname##_current_value.attr, \ >>> + &attr_##_attrname##_default_value.attr, \ >>> + &attr_##_attrname##_display_name.attr, \ >>> + &attr_##_attrname##_max_value.attr, \ >>> + &attr_##_attrname##_min_value.attr, \ >>> + &attr_##_attrname##_scalar_increment.attr, \ >>> + &attr_##_attrname##_type.attr, \ >>> + NULL, \ >>> + }; \ >>> + static const struct attribute_group _attrname##_attr_group = { \ >>> + .name = _fsname, .attrs = _attrname##_attrs \ >>> + } >> Is there a reason why this needs to be put inside the header? If no then please put this >> inside the driver. > To clarify, you mean the macros? I was under the impression they > belonged in headers but I can move them. I will move some of the > enums/structs as well which are referenced here and the driver only. I mean both the macros and the show functions. They are only used inside lenovo-wmi-other, so there is no reason to expose them inside the public header. Thanks, Armin Wolf > >> Thanks, >> Armin Wolf >> >>> + >>> #endif /* !_LENOVO_WMI_H_ */
On Fri, Jan 10, 2025 at 4:10 PM Armin Wolf <W_Armin@gmx.de> wrote: > >> Would it make sense to do something similar with each attribute, so that each attribute > >> can use container_of() to access lenovo_wmi_om_priv without having to use a list lookup? > >> > >> This would of course mean that each attribute as to be allocated dynamically. > >> > >> Heep in mind that we are currently working on a new API for registering firmware-atrtibute class > >> devices which should fix this. > >> > > I'm not sure I understand what you mean exactly. I think what you're > > saying is, instead of an attr_group, allocate each attribute as a > > struct in priv? > > Kind of. I envisioned something like this (pseudo code): > > struct firmware_attribute { > struct kobj_attribute attr; > struct lenovo_wmi_om_priv; > } > > This would allow you to use container_of() to access priv, but would force you to allocate each attribute separately. Ah, I see. Since we have the kobj in the functions we're accessing the list in, we could get the firmware_attribute struct instead which gives the pointer to priv. This will take a bit of refactoring for the probe & macro sections but I agree that it would be worth it. > Maybe you can wait with the lenovo-wmi-other driver until the improved firmware-attribute class device API has landed. > Meanwhile we can focus on the lenovo-wmi-gamezone driver. I'm not opposed to that. The API update seems to be progressing quickly and with the multiple other changes I need to figure out v3 might come after that is in for_next anyway. I'll play it by ear and work on the gamezone changes first. > >> Is there a reason why this needs to be put inside the header? If no then please put this > >> inside the driver. > > To clarify, you mean the macros? I was under the impression they > > belonged in headers but I can move them. I will move some of the > > enums/structs as well which are referenced here and the driver only. > > I mean both the macros and the show functions. They are only used inside lenovo-wmi-other, so there > is no reason to expose them inside the public header. > > Thanks, > Armin Wolf Make sense. I'll move those as well. Thanks again Armin, Derek > > > >> Thanks, > >> Armin Wolf > >> > >>> + > >>> #endif /* !_LENOVO_WMI_H_ */
diff --git a/MAINTAINERS b/MAINTAINERS index c9374c395905..318e1e517eed 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13040,6 +13040,7 @@ L: platform-driver-x86@vger.kernel.org S: Maintained F: drivers/platform/x86/lenovo-wmi-capdata01.c F: drivers/platform/x86/lenovo-wmi-gamezone.c +F: drivers/platform/x86/lenovo-wmi-other.c F: drivers/platform/x86/lenovo-wmi.h LETSKETCH HID TABLET DRIVER diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index a2c1ab47ad9e..e2285ab987c5 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -481,6 +481,18 @@ config LENOVO_WMI_DATA01 To compile this driver as a module, choose M here: the module will be called lenovo_wmi_capdata01. +config LENOVO_WMI_TUNING + tristate "Lenovo Other Method WMI Driver" + depends on LENOVO_WMI_DATA01 + select FW_ATTR_CLASS + help + Say Y here if you have a WMI aware Lenovo Legion device and would like to use the + firmware_attributes API to control various tunable settings typically exposed by + Lenovo software in Windows. + + To compile this driver as a module, choose M here: the module will + be called lenovo_wmi_other. + config IDEAPAD_LAPTOP tristate "Lenovo IdeaPad Laptop Extras" depends on ACPI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 6c96cc3f3855..3e059b3c3647 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -70,6 +70,7 @@ 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 +obj-$(CONFIG_LENOVO_WMI_TUNING) += lenovo-wmi-other.o # Intel obj-y += intel/ diff --git a/drivers/platform/x86/lenovo-wmi-other.c b/drivers/platform/x86/lenovo-wmi-other.c new file mode 100644 index 000000000000..2392faa74973 --- /dev/null +++ b/drivers/platform/x86/lenovo-wmi-other.c @@ -0,0 +1,385 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Lenovo Other Method WMI interface driver. This driver uses the fw_attributes + * class to expose the various WMI functions provided by the "Other Method" WMI + * interface. This enables CPU and GPU power limit as well as various other + * attributes for devices that fall under the "Gaming Series" of Lenovo laptop + * devices. Each attribute exposed by the "Other Method"" interface has a + * corresponding LENOVO_CAPABILITY_DATA_01 struct that allows the driver to + * probe details about the attribute such as set/get support, step, min, max, + * and default value. Each attibute has multiple pages, one for each of the + * fan profiles managed by the GameZone interface, so it must be probed prior + * to returning the current_value. + * + * These attributes typically don't fit anywhere else in the sysfs and are set + * in Windows using one of Lenovo's multiple user applications. + * + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@gmail.com> + */ + +#include <linux/list.h> +#include "lenovo-wmi.h" +#include "firmware_attributes_class.h" + +#define FW_ATTR_FOLDER "lenovo-wmi-other" +#define LENOVO_OTHER_METHOD_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" + +/* Device IDs */ +#define WMI_DEVICE_ID_CPU 0x01 + +/* WMI_DEVICE_ID_CPU feature IDs */ +#define WMI_FEATURE_ID_CPU_SPPT 0x01 /* Short Term Power Limit */ +#define WMI_FEATURE_ID_CPU_FPPT 0x03 /* Long Term Power Limit */ +#define WMI_FEATURE_ID_CPU_SPL 0x02 /* Peak Power Limit */ +#define WMI_FEATURE_ID_CPU_FPPT_BAD 0x03 /* Long Term Power Limit */ + +/* Method IDs */ +#define WMI_METHOD_ID_VALUE_GET 17 /* Other Method Getter */ +#define WMI_METHOD_ID_VALUE_SET 18 /* Other Method Setter */ + +static DEFINE_MUTEX(call_mutex); +static DEFINE_MUTEX(om_list_mutex); +static LIST_HEAD(om_wmi_list); + +struct lenovo_wmi_om_priv { + struct wmi_device *wdev; + struct device *fw_attr_dev; + struct kset *fw_attr_kset; + struct list_head list; +}; + +static inline struct lenovo_wmi_om_priv *get_first_wmi_priv(void) +{ + guard(mutex)(&om_list_mutex); + return list_first_entry_or_null(&om_wmi_list, struct lenovo_wmi_om_priv, + list); +} + +static const struct wmi_device_id lenovo_wmi_other_id_table[] = { + { LENOVO_OTHER_METHOD_GUID, NULL }, + {} +}; + +/* Tunable Attributes */ +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU, + .feature_id = WMI_FEATURE_ID_CPU_SPL }; +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU, + .feature_id = WMI_FEATURE_ID_CPU_SPPT }; +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU, + .feature_id = WMI_FEATURE_ID_CPU_FPPT }; + +struct capdata01_attr_group { + const struct attribute_group *attr_group; + struct tunable_attr_01 *tunable_attr; +}; + +static const struct class *fw_attr_class; + +/** + * attr_capdata01_setup() - Get the data of the specified attribute + * from LENOVO_CAPABILITY_DATA_01 and store it. + * @tunable_attr: The attribute to be populated. + * + * Returns: Either 0 or an error. + */ +static int attr_capdata01_setup(struct tunable_attr_01 *tunable_attr) +{ + struct capability_data_01 cap_data; + int mode = SMARTFAN_MODE_CUSTOM; + int err; + + struct lenovo_wmi_attr_id attr_id = { mode << 8, + tunable_attr->feature_id, + tunable_attr->device_id }; + + err = lenovo_wmi_capdata01_get(attr_id, &cap_data); + if (err) { + pr_err("Failed to get capability data: %u\n", err); + return err; + } + + if (cap_data.supported < 1) + return -EOPNOTSUPP; + + tunable_attr->capdata = cap_data; + + return 0; +} + +/** + * attr_capdata01_show() - Get the value of the specified attribute property + * from LENOVO_CAPABILITY_DATA_01. + * @kobj: Pointer to the driver object. + * @kobj_attribute: Pointer to the attribute calling this function. + * @buf: The buffer to write to. + * @tunable_attr: The attribute to be read. + * @prop: The property of this attribute to be read. + * + * This function is intended to be generic so it can be called from any "_show" + * attribute which works only with integers. + * + * If the WMI is success, then the sysfs attribute is notified. + * + * Returns: Either count, or an error. + */ +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf, struct tunable_attr_01 *tunable_attr, + enum attribute_property prop) +{ + struct capability_data_01 cap_data; + int retval; + + cap_data = tunable_attr->capdata; + + switch (prop) { + case DEFAULT_VAL: + retval = cap_data.default_value; + break; + case MAX_VAL: + retval = cap_data.max_value; + break; + case MIN_VAL: + retval = cap_data.min_value; + break; + case STEP_VAL: + retval = cap_data.step; + break; + default: + return -EINVAL; + } + return sysfs_emit(buf, "%u\n", retval); +} + +/* Simple attribute creation */ + +/* + * att_current_value_store() - Set the current value of the given attribute + * @kobj: Pointer to the driver object. + * @kobj_attribute: Pointer to the attribute calling this function. + * @buf: The buffer to read from, this is parsed to `int` type. + * @count: Required by sysfs attribute macros, pass in from the callee attr. + * @tunable_attr: The attribute to be stored. + * + * This function is intended to be generic so it can be called from any + * attribute's "current_value_store" which works only with integers. The + * integer to be sent to the WMI method is range checked and an error returned + * if out of range. + * + * If the value is valid and WMI is success, then the sysfs attribute is + * notified. + * + * Returns: Either count, or an error. + */ +ssize_t attr_current_value_store(struct kobject *kobj, + struct kobj_attribute *attr, const char *buf, + size_t count, + struct tunable_attr_01 *tunable_attr) +{ + struct capability_data_01 cap_data; + struct lenovo_wmi_om_priv *priv; + int mode = SMARTFAN_MODE_CUSTOM; + u32 value; + int err; + + struct lenovo_wmi_attr_id attr_id = { mode << 8, + tunable_attr->feature_id, + tunable_attr->device_id }; + + err = kstrtouint(buf, 10, &value); + if (err) { + pr_err("Error converting value to int: %u\n", err); + return err; + } + + cap_data = tunable_attr->capdata; + + if (value < cap_data.min_value || value > cap_data.max_value) + return -EINVAL; + + priv = get_first_wmi_priv(); + if (!priv) + return -ENODEV; + + guard(mutex)(&call_mutex); + err = lenovo_wmidev_evaluate_method_2(priv->wdev, 0x0, + WMI_METHOD_ID_VALUE_SET, + *(int *)&attr_id, value, NULL); + + if (err) { + pr_err("Error setting attribute: %u\n", err); + return err; + } + + tunable_attr->store_value = value; + + sysfs_notify(kobj, NULL, attr->attr.name); + + return count; +}; + +/* + * attr_current_value_show() - Get the current value of the given attribute + * @kobj: Pointer to the driver object. + * @kobj_attribute: Pointer to the attribute calling this function. + * @buf: The buffer to write to. + * @tunable_attr: The attribute to be read. + * + * This function is intended to be generic so it can be called from any "_show" + * attribute which works only with integers. + * + * If the WMI is success, then the sysfs attribute is notified. + * + * Returns: Either count, or an error. + */ +ssize_t attr_current_value_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf, + struct tunable_attr_01 *tunable_attr) +{ + struct lenovo_wmi_om_priv *priv; + int mode = SMARTFAN_MODE_CUSTOM; + int retval; + int err; + + struct lenovo_wmi_attr_id attr_id = { mode << 8, + tunable_attr->feature_id, + tunable_attr->device_id }; + + priv = get_first_wmi_priv(); + if (!priv) + return -ENODEV; + + guard(mutex)(&call_mutex); + err = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0, + WMI_METHOD_ID_VALUE_GET, + *(int *)&attr_id, &retval); + + if (err) { + pr_err("Error getting attribute: %u\n", err); + return err; + } + + return sysfs_emit(buf, "%u\n", retval); +} + +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl", + "Set the CPU sustained power limit"); +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt", + "Set the CPU slow package power tracking limit"); +ATTR_GROUP_LL_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt", + "Set the CPU fast package power tracking limit"); + +static const struct capdata01_attr_group capdata01_attr_groups[] = { + { &ppt_pl1_spl_attr_group, &ppt_pl1_spl }, + { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt }, + { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt }, + {}, +}; + +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv) +{ + int err, i; + + err = fw_attributes_class_get(&fw_attr_class); + if (err) { + pr_err("Failed to get firmware_attributes_class: %u\n", err); + return err; + } + + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), + NULL, "%s", FW_ATTR_FOLDER); + if (IS_ERR(priv->fw_attr_dev)) { + err = PTR_ERR(priv->fw_attr_dev); + pr_err("Failed to create firmware_attributes_class device: %u\n", + err); + goto fail_class_get; + } + + priv->fw_attr_kset = kset_create_and_add("attributes", NULL, + &priv->fw_attr_dev->kobj); + if (!priv->fw_attr_kset) { + err = -ENOMEM; + pr_err("Failed to create firmware_attributes_class kset: %u\n", + err); + goto err_destroy_classdev; + } + + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) { + err = attr_capdata01_setup( + capdata01_attr_groups[i].tunable_attr); + if (err) { + pr_err("Failed to populate capability data for %s: %u\n", + capdata01_attr_groups[i].attr_group->name, err); + continue; + } + + err = sysfs_create_group(&priv->fw_attr_kset->kobj, + capdata01_attr_groups[i].attr_group); + if (err) { + pr_err("Failed to create sysfs-group for %s: %u\n", + capdata01_attr_groups[i].attr_group->name, err); + goto err_remove_groups; + } + } + + return 0; + +err_remove_groups: + while (i-- > 0) { + sysfs_remove_group(&priv->fw_attr_kset->kobj, + capdata01_attr_groups[i].attr_group); + } + + return err; + +err_destroy_classdev: + device_destroy(fw_attr_class, MKDEV(0, 0)); + + return err; + +fail_class_get: + fw_attributes_class_put(); + + return err; +} + +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context) +{ + struct lenovo_wmi_om_priv *priv; + + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->wdev = wdev; + + guard(mutex)(&om_list_mutex); + list_add_tail(&priv->list, &om_wmi_list); + + return other_method_fw_attr_add(priv); +} + +static void lenovo_wmi_other_remove(struct wmi_device *wdev) +{ + struct lenovo_wmi_om_priv *priv = dev_get_drvdata(&wdev->dev); + + guard(mutex)(&om_list_mutex); + list_del(&priv->list); + kset_unregister(priv->fw_attr_kset); + device_destroy(fw_attr_class, MKDEV(0, 0)); + fw_attributes_class_put(); +} + +static struct wmi_driver lenovo_wmi_other_driver = { + .driver = { .name = "lenovo_wmi_other" }, + .id_table = lenovo_wmi_other_id_table, + .probe = lenovo_wmi_other_probe, + .remove = lenovo_wmi_other_remove, +}; + +module_wmi_driver(lenovo_wmi_other_driver); + +MODULE_IMPORT_NS("CAPDATA_WMI"); +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table); +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>"); +MODULE_DESCRIPTION("Lenovo Other Method WMI Driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h index 53cea84a956b..1c8358551ba6 100644 --- a/drivers/platform/x86/lenovo-wmi.h +++ b/drivers/platform/x86/lenovo-wmi.h @@ -42,6 +42,14 @@ struct lenovo_wmi_attr_id { u32 device_id : 8; /* CPU/GPU/... */ } __packed; +enum attribute_property { + DEFAULT_VAL, + MAX_VAL, + MIN_VAL, + STEP_VAL, + SUPPORTED, +}; + /* Data struct for LENOVO_CAPABILITY_DATA_01 */ struct capability_data_01 { u32 id; @@ -52,6 +60,14 @@ struct capability_data_01 { u32 max_value; }; +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */ +struct tunable_attr_01 { + struct capability_data_01 capdata; + u32 device_id; + u32 feature_id; + u32 store_value; +}; + /* General Use functions */ static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, u32 method_id, struct acpi_buffer *in, @@ -122,4 +138,104 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance, int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id, struct capability_data_01 *cap_data); +/* Other Method attribute functions */ +ssize_t attr_capdata01_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf, struct tunable_attr_01 *tunable_attr, + enum attribute_property prop); + +ssize_t attr_current_value_store(struct kobject *kobj, + struct kobj_attribute *attr, const char *buf, + size_t count, + struct tunable_attr_01 *tunable_attr); + +ssize_t attr_current_value_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf, + struct tunable_attr_01 *tunable_attr); + +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf); + +ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "integer\n"); +} + +/* Other Method attribute macros */ +#define __LL_ATTR_RO(_func, _name) \ + { \ + .attr = { .name = __stringify(_name), .mode = 0444 }, \ + .show = _func##_##_name##_show, \ + } + +#define __LL_ATTR_RO_AS(_name, _show) \ + { \ + .attr = { .name = __stringify(_name), .mode = 0444 }, \ + .show = _show, \ + } + +#define __LL_ATTR_RW(_func, _name) \ + __ATTR(_name, 0644, _func##_##_name##_show, _func##_##_name##_store) + +/* Shows a formatted static variable */ +#define __ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val) \ + static ssize_t _attrname##_##_prop##_show( \ + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ + { \ + return sysfs_emit(buf, _fmt, _val); \ + } \ + static struct kobj_attribute attr_##_attrname##_##_prop = \ + __LL_ATTR_RO(_attrname, _prop) + +/* Attribute current_value show/store */ +#define __LL_TUNABLE_RW_CAP01(_attrname) \ + static ssize_t _attrname##_current_value_store( \ + struct kobject *kobj, struct kobj_attribute *attr, \ + const char *buf, size_t count) \ + { \ + return attr_current_value_store(kobj, attr, buf, count, \ + &_attrname); \ + } \ + static ssize_t _attrname##_current_value_show( \ + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ + { \ + return attr_current_value_show(kobj, attr, buf, &_attrname); \ + } \ + static struct kobj_attribute attr_##_attrname##_current_value = \ + __LL_ATTR_RW(_attrname, current_value) + +/* Attribute property show only */ +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \ + static ssize_t _attrname##_##_prop##_show( \ + struct kobject *kobj, struct kobj_attribute *attr, char *buf) \ + { \ + return attr_capdata01_show(kobj, attr, buf, &_attrname, \ + _prop_type); \ + } \ + static struct kobj_attribute attr_##_attrname##_##_prop = \ + __LL_ATTR_RO(_attrname, _prop) + +#define ATTR_GROUP_LL_TUNABLE_CAP01(_attrname, _fsname, _dispname) \ + __LL_TUNABLE_RW_CAP01(_attrname); \ + __LL_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL); \ + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \ + __LL_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL); \ + __LL_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL); \ + __LL_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL); \ + static struct kobj_attribute attr_##_attrname##_type = \ + __LL_ATTR_RO_AS(type, int_type_show); \ + static struct attribute *_attrname##_attrs[] = { \ + &attr_##_attrname##_current_value.attr, \ + &attr_##_attrname##_default_value.attr, \ + &attr_##_attrname##_display_name.attr, \ + &attr_##_attrname##_max_value.attr, \ + &attr_##_attrname##_min_value.attr, \ + &attr_##_attrname##_scalar_increment.attr, \ + &attr_##_attrname##_type.attr, \ + NULL, \ + }; \ + static const struct attribute_group _attrname##_attr_group = { \ + .name = _fsname, .attrs = _attrname##_attrs \ + } + #endif /* !_LENOVO_WMI_H_ */
Adds lenovo-wmi-other.c which provides a driver for the Lenovo "Other Mode" WMI interface that comes on some Lenovo "Gaming Series" hardware. Provides a firmware-attributes class which enables the use of tunable knobs for SPL, SPPT, and FPPT. 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv. - Use list to get the lenovo_wmi_om_priv instance in some macro called functions as the data provided by the macros that use it doesn't pass a member of the struct for use in container_of. - Do not rely on GameZone interface to grab the current fan mode. Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com> --- MAINTAINERS | 1 + drivers/platform/x86/Kconfig | 12 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++++++ drivers/platform/x86/lenovo-wmi.h | 116 +++++++ 5 files changed, 515 insertions(+) create mode 100644 drivers/platform/x86/lenovo-wmi-other.c