| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFqHKTm56JbApnPxRo=0Aq9V_vTH1UVtLdsDWt9oE0_4uF42EQ@mail.gmail.com>
Date: Wed, 26 Feb 2025 16:56:33 -0800
From: Derek John Clark <derekjohn.clark@...il.com>
To: Mario Limonciello <superm1@...nel.org>
Cc: Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Armin Wolf <W_Armin@....de>, Jonathan Corbet <corbet@....net>, Luke Jones <luke@...nes.dev>,
Xino Ni <nijs1@...ovo.com>, Zhixin Zhang <zhangzx36@...ovo.com>, Mia Shao <shaohz1@...ovo.com>,
Mark Pearson <mpearson-lenovo@...ebb.ca>,
"Pierre-Loup A . Griffais" <pgriffais@...vesoftware.com>, "Cody T . -H . Chiu" <codyit@...il.com>,
John Martens <johnfanv2@...il.com>, platform-driver-x86@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/4] platform/x86: Add Lenovo Other Mode WMI Driver
On Tue, Feb 25, 2025 at 10:47 PM Mario Limonciello <superm1@...nel.org> wrote:
>
> On 2/25/2025 13:59, 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.
> >
> > v3:
> > - Add notifier block and store result for getting the Gamezone interface
> > profile changes.
> > - Add driver as master component of capdata01 driver.
> > - Use FIELD_PREP where appropriate.
> > - Move macros and associated functions out of lemovo-wmi.h that are only
> > used by this driver.
> > v2:
> > - Use devm_kmalloc 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@...il.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/platform/x86/Kconfig | 14 +
> > drivers/platform/x86/Makefile | 1 +
> > drivers/platform/x86/lenovo-wmi-other.c | 549 ++++++++++++++++++++++++
> > drivers/platform/x86/lenovo-wmi.h | 13 +
> > 5 files changed, 578 insertions(+)
> > create mode 100644 drivers/platform/x86/lenovo-wmi-other.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f6d3e79e50ce..f6e16b2346a2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13159,6 +13159,7 @@ F: Documentation/wmi/devices/lenovo-wmi-gamezone.rst
> > F: Documentation/wmi/devices/lenovo-wmi-other.rst
> > 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.c
> > F: drivers/platform/x86/lenovo-wmi.h
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 56336dc3c2d0..017ecdfad8ce 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -480,6 +480,20 @@ config LENOVO_WMI_DATA01
> > depends on ACPI_WMI
> > select LENOVO_WMI
> >
> > +config LENOVO_WMI_TUNING
> > + tristate "Lenovo Other Mode WMI Driver"
> > + depends on ACPI_WMI
> > + select FW_ATTR_CLASS
> > + select LENOVO_WMI
> > + select LENOVO_WMI_DATA01
> > + 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 be9031bea090..28ce39631a6d 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -71,6 +71,7 @@ obj-$(CONFIG_LENOVO_WMI) += lenovo-wmi.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..cd04ead94ba3
> > --- /dev/null
> > +++ b/drivers/platform/x86/lenovo-wmi-other.c
> > @@ -0,0 +1,549 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Lenovo Other Mode WMI interface driver. This driver uses the fw_attributes
> > + * class to expose the various WMI functions provided by the "Other Mode" 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 Mode"" 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.
> > + *
> > + * 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@...il.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/component.h>
> > +#include <linux/container_of.h>
> > +#include <linux/device.h>
> > +#include <linux/kobject.h>
> > +#include <linux/notifier.h>
> > +#include <linux/types.h>
> > +#include <linux/wmi.h>
> > +#include "lenovo-wmi.h"
> > +#include "firmware_attributes_class.h"
> > +
> > +/* Interface GUIDs */
> > +#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 */
> > +
> > +/* Type IDs*/
> > +#define WMI_TYPE_ID_NONE 0x00
> > +
> > +/* Method IDs */
> > +#define WMI_FAN_TABLE_GET 5 /* Other Mode FAN_METHOD Getter */
> > +#define WMI_FAN_TABLE_SET 6 /* Other Mode FAN_METHOD Setter */
> > +#define WMI_FEATURE_VALUE_GET 17 /* Other Mode Getter */
> > +#define WMI_FEATURE_VALUE_SET 18 /* Other Mode Setter */
> > +
> > +/* Attribute ID bitmasks */
> > +#define ATTR_DEV_ID_MASK GENMASK(31, 24)
> > +#define ATTR_FEAT_ID_MASK GENMASK(23, 16)
> > +#define ATTR_MODE_ID_MASK GENMASK(15, 8)
> > +#define ATTR_TYPE_ID_MASK GENMASK(7, 0)
> > +
> > +enum attribute_property {
> > + DEFAULT_VAL,
> > + MAX_VAL,
> > + MIN_VAL,
> > + STEP_VAL,
> > + SUPPORTED,
> > +};
> > +
> > +/* Tunable attribute that uses LENOVO_CAPABILITY_DATA_01 */
> > +struct tunable_attr_01 {
> > + u32 type_id;
> > + u32 device_id;
> > + u32 feature_id;
> > + u32 store_value;
> > + struct device *dev;
> > + struct capdata01 *capdata;
> > +};
> > +
> > +/* Tunable Attributes */
> > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = WMI_DEVICE_ID_CPU,
> > + .feature_id = WMI_FEATURE_ID_CPU_SPL,
> > + .type_id = WMI_TYPE_ID_NONE };
> > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = WMI_DEVICE_ID_CPU,
> > + .feature_id = WMI_FEATURE_ID_CPU_SPPT,
> > + .type_id = WMI_TYPE_ID_NONE };
> > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = WMI_DEVICE_ID_CPU,
> > + .feature_id = WMI_FEATURE_ID_CPU_FPPT,
> > + .type_id = WMI_TYPE_ID_NONE };
> > +
> > +struct capdata01_attr_group {
> > + const struct attribute_group *attr_group;
> > + struct tunable_attr_01 *tunable_attr;
> > +};
> > +
> > +#define FW_ATTR_FOLDER "lenovo-wmi-other"
> > +
> > +/**
> > + * int_type_show() - Emit the data type for an integer attribute
> > + * @kobj: Pointer to the driver object.
> > + * @kobj_attribute: Pointer to the attribute calling this function.
> > + * @buf: The buffer to write to.
> > + *
> > + * Returns: Number of characters written to buf.
> > + */
> > +static ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *kattr,
> > + char *buf)
> > +{
> > + return sysfs_emit(buf, "integer\n");
> > +}
> > +
> > +/** .
> > + * attr_capdata01_get - Get the data of the specified attribute
> > + * from lenovo_wmi_om->cd01.
> > + * @tunable_attr: The attribute to be populated.
> > + *
> > + * Returns: Either 0 or an error.
> > + */
> > +static struct capdata01 *
> > +attr_capdata01_get_data(struct lenovo_wmi_om *om,
> > + struct tunable_attr_01 *tunable_attr,
> > + enum thermal_mode mode)
> > +{
> > + u32 attribute_id =
> > + FIELD_PREP(ATTR_DEV_ID_MASK, tunable_attr->device_id) |
> > + FIELD_PREP(ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
> > + FIELD_PREP(ATTR_MODE_ID_MASK, mode) |
> > + FIELD_PREP(ATTR_TYPE_ID_MASK, tunable_attr->type_id);
> > + int idx;
> > +
> > + if (!om->cd01)
> > + return NULL;
> > +
> > + for (idx = 0; idx < om->cd01->instance_count; idx++) {
> > + if (om->cd01->capdata[idx]->id != attribute_id)
> > + continue;
> > + return om->cd01->capdata[idx];
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/**
> > + * 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 number of characters written to buf, or an error.
> > + */
> > +static ssize_t attr_capdata01_show(struct kobject *kobj,
> > + struct kobj_attribute *kattr, char *buf,
> > + struct tunable_attr_01 *tunable_attr,
> > + enum attribute_property prop)
> > +{
> > + struct lenovo_wmi_om *om = dev_get_drvdata(tunable_attr->dev);
> > + struct capdata01 *capdata;
> > + int value;
> > +
> > + if (!om)
> > + return -ENODEV;
> > +
> > + capdata = attr_capdata01_get_data(om, tunable_attr,
> > + SMARTFAN_MODE_CUSTOM);
> > +
> > + if (!capdata)
> > + return -ENODEV;
> > +
> > + switch (prop) {
> > + case DEFAULT_VAL:
> > + value = capdata->default_value;
> > + break;
> > + case MAX_VAL:
> > + value = capdata->max_value;
> > + break;
> > + case MIN_VAL:
> > + value = capdata->min_value;
> > + break;
> > + case STEP_VAL:
> > + value = capdata->step;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return sysfs_emit(buf, "%d\n", value);
> > +}
> > +
> > +/* 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.
> > + */
> > +static ssize_t attr_current_value_store(struct kobject *kobj,
> > + struct kobj_attribute *kattr,
> > + const char *buf, size_t count,
> > + struct tunable_attr_01 *tunable_attr)
> > +{
> > + struct lenovo_wmi_om *om = dev_get_drvdata(tunable_attr->dev);
> > + struct capdata01 *capdata;
> > + u32 attribute_id;
> > + u32 value;
> > + int err;
> > +
> > + if (!om)
> > + return -ENODEV;
> > +
> > + capdata = attr_capdata01_get_data(om, tunable_attr,
> > + SMARTFAN_MODE_CUSTOM);
> > +
> > + if (!capdata)
> > + return -ENODEV;
> > +
> > + attribute_id = FIELD_PREP(ATTR_DEV_ID_MASK, tunable_attr->device_id) |
> > + FIELD_PREP(ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
> > + FIELD_PREP(ATTR_MODE_ID_MASK, SMARTFAN_MODE_CUSTOM) |
> > + FIELD_PREP(ATTR_TYPE_ID_MASK, tunable_attr->type_id);
> > +
> > + err = kstrtouint(buf, 10, &value);
> > + if (err)
> > + return err;
> > +
> > + if (value < capdata->min_value || value > capdata->max_value)
> > + return -EINVAL;
> > +
> > + err = lenovo_wmidev_evaluate_method_2(om->wdev, 0x0,
> > + WMI_FEATURE_VALUE_SET,
> > + attribute_id, value, NULL);
> > +
> > + if (err)
> > + return err;
> > +
> > + tunable_attr->store_value = value;
> > + 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 number of characters written to buf, or an error.
> > + */
> > +static ssize_t attr_current_value_show(struct kobject *kobj,
> > + struct kobj_attribute *kattr, char *buf,
> > + struct tunable_attr_01 *tunable_attr)
> > +{
> > + struct lenovo_wmi_om *om = dev_get_drvdata(tunable_attr->dev);
> > + u32 attribute_id;
> > + int retval;
> > + int err;
> > +
> > + if (!om)
> > + return -ENODEV;
> > +
> > + attribute_id = FIELD_PREP(ATTR_DEV_ID_MASK, tunable_attr->device_id) |
> > + FIELD_PREP(ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
> > + FIELD_PREP(ATTR_MODE_ID_MASK, om->mode) |
> > + FIELD_PREP(ATTR_TYPE_ID_MASK, tunable_attr->type_id);
> > +
> > + err = lenovo_wmidev_evaluate_method_1(om->wdev, 0x0, WMI_FEATURE_VALUE_GET,
> > + attribute_id, &retval);
> > +
> > + if (err)
> > + return err;
> > +
> > + return sysfs_emit(buf, "%d\n", retval);
> > +}
> > +
> > +/* 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 *kattr, char *buf) \
> > + { \
> > + return sysfs_emit(buf, _fmt, _val); \
> > + } \
> > + static struct kobj_attribute attr_##_attrname##_##_prop = \
> > + __LL_ATTR_RO(_attrname, _prop)
> > +
> > +/* Attribute current value read/write */
> > +#define __LL_TUNABLE_CURRENT_VALUE_CAP01(_attrname) \
> > + static ssize_t _attrname##_current_value_store( \
> > + struct kobject *kobj, struct kobj_attribute *kattr, \
> > + const char *buf, size_t count) \
> > + { \
> > + return attr_current_value_store(kobj, kattr, buf, count, \
> > + &_attrname); \
> > + } \
> > + static ssize_t _attrname##_current_value_show( \
> > + struct kobject *kobj, struct kobj_attribute *kattr, char *buf) \
> > + { \
> > + return attr_current_value_show(kobj, kattr, buf, &_attrname); \
> > + } \
> > + static struct kobj_attribute attr_##_attrname##_current_value = \
> > + __LL_ATTR_RW(_attrname, current_value)
> > +
> > +/* Attribute property read only */
> > +#define __LL_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type) \
> > + static ssize_t _attrname##_##_prop##_show( \
> > + struct kobject *kobj, struct kobj_attribute *kattr, char *buf) \
> > + { \
> > + return attr_capdata01_show(kobj, kattr, 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_CURRENT_VALUE_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 \
> > + }
> > +
> > +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 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 lenovo_wmi_om_fw_attr_add(struct lenovo_wmi_om *om)
> > +{
> > + int err, i;
> > +
> > + om->fw_attr_dev = device_create(&firmware_attributes_class, NULL,
> > + MKDEV(0, 0), NULL, "%s",
> > + FW_ATTR_FOLDER);
> > + if (IS_ERR(om->fw_attr_dev)) {
> > + err = PTR_ERR(om->fw_attr_dev);
> > + return err;
>
> I'd say just combine these two in this case and
>
> return PTR_ERR();
Acked.
> > + }
> > +
> > + om->fw_attr_kset =
> > + kset_create_and_add("attributes", NULL, &om->fw_attr_dev->kobj);
> > + if (!om->fw_attr_kset) {
> > + err = -ENOMEM;
> > + goto err_destroy_classdev;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) {
> > + err = sysfs_create_group(&om->fw_attr_kset->kobj,
> > + capdata01_attr_groups[i].attr_group);
> > + if (err) {
> > + pr_debug("Failed to create sysfs-group for %s: %d\n",
> > + capdata01_attr_groups[i].attr_group->name,
> > + err);
> > + goto err_remove_groups;
> > + }
> > + capdata01_attr_groups[i].tunable_attr->dev = &om->wdev->dev;
> > + }
> > + return 0;
> > +
> > +err_remove_groups:
> > + while (i-- > 0) {
>
> Are these boundaries right for cleanup?
>
> Let's say that sysfs_create_group() fails on the second iteration (i = 1).
>
> i-- will make it zero and then you never clear the group from the first
> iteration.
>
If sysfs_create_group() fails I'm not sure we would want to call
sysfs_remove_group() on it. Couldn't that be a null pointer? I'm open
to being wrong here, but I'm just not sure what the expectation of the
create function is in a failure.
> > + sysfs_remove_group(&om->fw_attr_kset->kobj,
> > + capdata01_attr_groups[i].attr_group);
> > + }
> > + kset_unregister(om->fw_attr_kset);
> > +
> > +err_destroy_classdev:
> > + device_unregister(om->fw_attr_dev);
> > + return err;
> > +}
> > +
> > +static int lenovo_wmi_om_notifier(struct notifier_block *nb, unsigned long cmd,
> > + void *data)
> > +{
> > + struct lenovo_wmi_om *om = container_of(nb, struct lenovo_wmi_om, nb);
> > +
> > + if (!om)
> > + NOTIFY_BAD;
>
> Presumably you meant:
>
> return NOTIFY_BAD;
>
You are correct. Same for the next one. Acked
Thanks,
- Derek
> > +
> > + if (cmd != THERMAL_MODE_EVENT)
> > + NOTIFY_OK;
>
> Presumably you meant:
>
> return NOTIFY_OK;
>
> > +
> > + om->mode = *((enum thermal_mode *)data);
> > +
> > + return NOTIFY_OK;
> > +}
> > +
> > +static int lenovo_wmi_om_master_bind(struct device *dev)
> > +{
> > + struct lenovo_wmi_om *om = dev_get_drvdata(dev);
> > +
> > + int ret;
> > +
> > + ret = component_bind_all(dev, om);
> > + if (ret)
> > + return ret;
> > +
> > + return lenovo_wmi_om_fw_attr_add(om);
> > +}
> > +
> > +static void lenovo_wmi_om_master_unbind(struct device *dev)
> > +{
> > + component_unbind_all(dev, NULL);
> > +}
> > +
> > +static const struct component_master_ops lenovo_wmi_om_master_ops = {
> > + .bind = lenovo_wmi_om_master_bind,
> > + .unbind = lenovo_wmi_om_master_unbind,
> > +};
> > +
> > +static int lenovo_wmi_other_probe(struct wmi_device *wdev, const void *context)
> > +{
> > + struct notifier_block lenovo_wmi_om_notifier_block = {
> > + .notifier_call = lenovo_wmi_om_notifier,
> > + };
> > + struct component_match *master_match = NULL;
> > + struct lenovo_wmi_om *om;
> > + int ret;
> > +
> > + om = devm_kzalloc(&wdev->dev, sizeof(*om), GFP_KERNEL);
> > + if (!om) {
> > + ret = -ENOMEM;
> > + goto err_exit;
> > + }
> > +
> > + om->wdev = wdev;
> > + om->nb = lenovo_wmi_om_notifier_block;
> > + om->mode = SMARTFAN_MODE_CUSTOM; /* fallback */
> > +
> > + dev_set_drvdata(&wdev->dev, om);
> > +
> > + ret = devm_lenovo_wmi_gz_register_notifier(&wdev->dev, &om->nb);
> > + if (ret) {
> > + pr_err("Failed to register notifier_block\n");
> > + goto err_exit;
> > + }
> > +
> > + component_match_add(&wdev->dev, &master_match, lenovo_wmi_cd01_match,
> > + NULL);
> > + if (IS_ERR_OR_NULL(master_match)) {
> > + ret = -ENOMEM;
> > + goto err_exit;
> > + }
> > +
> > + ret = component_master_add_with_match(&wdev->dev,
> > + &lenovo_wmi_om_master_ops,
> > + master_match);
> > + if (ret < 0) {
> > + dev_err(&wdev->dev, "Master comp add failed %d\n", ret);
> > + goto err_exit;
> > + }
> > +
> > + return 0;
> > +err_exit:
> > + kfree(om);
> > + return ret;
> > +}
> > +
> > +static void lenovo_wmi_other_remove(struct wmi_device *wdev)
> > +{
> > + struct lenovo_wmi_om *om = dev_get_drvdata(&wdev->dev);
> > +
> > + kset_unregister(om->fw_attr_kset);
> > + device_destroy(&firmware_attributes_class, MKDEV(0, 0));
> > + component_master_del(&wdev->dev, &lenovo_wmi_om_master_ops);
> > +}
> > +
> > +static const struct wmi_device_id lenovo_wmi_other_id_table[] = {
> > + { LENOVO_OTHER_METHOD_GUID, NULL },
> > + {}
> > +};
> > +
> > +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,
> > +};
> > +
> > +module_wmi_driver(lenovo_wmi_other_driver);
> > +
> > +MODULE_IMPORT_NS("CAPDATA_WMI");
> > +MODULE_IMPORT_NS("GZ_WMI");
> > +MODULE_IMPORT_NS("LENOVO_WMI");
> > +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_other_id_table);
> > +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@...il.com>");
> > +MODULE_DESCRIPTION("Lenovo Other Mode WMI Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h
> > index 07fa67ed89d6..40b6418fbf02 100644
> > --- a/drivers/platform/x86/lenovo-wmi.h
> > +++ b/drivers/platform/x86/lenovo-wmi.h
> > @@ -61,6 +61,19 @@ struct capdata01 {
> > u32 max_value;
> > };
> >
> > +/* other method structs */
> > +struct lenovo_wmi_om {
> > + struct component_master_ops *ops;
> > + struct lenovo_wmi_cd01 *cd01;
> > + struct capdata01 **capdata;
> > + struct device *fw_attr_dev;
> > + struct kset *fw_attr_kset;
> > + struct notifier_block nb;
> > + struct wmi_device *wdev;
> > + enum thermal_mode mode;
> > + int instance_count;
> > +};
> > +
> > /* wmidev_evaluate_method helper functions */
> > int lenovo_wmidev_evaluate_method_2(struct wmi_device *wdev, u8 instance,
> > u32 method_id, u32 arg0, u32 arg1,
>
Powered by blists - more mailing lists