Re: [PATCH v5 3/8] platform/x86: lenovo-wmi-other: Add lwmi_attr_id() function
From: Derek J. Clark
Date: Wed Mar 25 2026 - 14:23:51 EST
On March 25, 2026 10:54:49 AM PDT, Rong Zhang <i@xxxxxxxx> wrote:
>Hi Derek,
>
>On Tue, 2026-03-24 at 22:10 +0000, Derek J. Clark wrote:
>> Adds lwmi_attr_id() function. In the same vein as LWMI_ATTR_ID_FAN_RPM(),
>> but as a generic, to de-duplicate attribute_id assignment biolerplate.
>>
>> Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>> Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
>> ---
>> v5:
>> - Move references to cv/cd_mode_id to patch 4/8.
>> - Move lwmi_attr_id to wmi-capdata.c and export with namespace.
>> v4:
>> - Switch from macro to static inline to preserve types.
>> ---
>> drivers/platform/x86/lenovo/wmi-capdata.c | 25 ++++++++++++--
>> drivers/platform/x86/lenovo/wmi-capdata.h | 3 ++
>> drivers/platform/x86/lenovo/wmi-gamezone.h | 1 +
>> drivers/platform/x86/lenovo/wmi-other.c | 39 ++++++----------------
>> 4 files changed, 36 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
>> index ee1fb02d8e31..6cb3665e9399 100644
>> --- a/drivers/platform/x86/lenovo/wmi-capdata.c
>> +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
>> @@ -48,6 +48,7 @@
>> #include <linux/wmi.h>
>>
>> #include "wmi-capdata.h"
>> +#include "wmi-gamezone.h"
>>
>> #define LENOVO_CAPABILITY_DATA_00_GUID "362A3AFE-3D96-4665-8530-96DAD5BB300E"
>> #define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
>> @@ -58,9 +59,27 @@
>>
>> #define LWMI_FEATURE_ID_FAN_TEST 0x05
>>
>> -#define LWMI_ATTR_ID_FAN_TEST \
>> - (FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) | \
>> - FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_TEST))
>> +/**
>> + * lwmi_attr_id() - Formats a capability data attribute ID
>> + * @dev_id: The u8 corresponding to the device ID.
>> + * @feat_id: The u8 corresponding to the feature ID on the device.
>> + * @mode_id: The u8 corresponding to the wmi-gamezone mode for set/get.
>> + * @type_id: The u8 corresponding to the sub-device.
>> + *
>> + * Return: u32.
>> + */
>> +u32 lwmi_attr_id(u8 dev_id, u8 feat_id, u8 mode_id, u8 type_id)
>> +{
>> + return (FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, dev_id) |
>> + FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, feat_id) |
>> + FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode_id) |
>> + FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, type_id));
>> +}
>> +EXPORT_SYMBOL_NS_GPL(lwmi_attr_id, "LENOVO_WMI_CAPDATA");
>> +
>> +#define LWMI_ATTR_ID_FAN_TEST \
>> + lwmi_attr_id(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_TEST, \
>> + LWMI_GZ_THERMAL_MODE_NONE, LWMI_TYPE_ID_NONE)
>
>I don't really love the idea of exporting a very simple function instead
>of providing a static inline equivalent in the corresponding header.
>
>Compilers are smart. For example, they are capable to convert
>LWMI_ATTR_ID_FAN_RPM(x) into `0x04030000 | (x + 1)', as long as the
>function body is accessible.
>
>By implementing and exporting it in wmi-capdata.c, wmi-other.c can no
>longer access the function body so no optimization can be done there
>(note that LTO has nothing to do when both are compiled as modules).
>Hence, the compiler has no choice but to emit a function call to
>lwmi_attr_id().
>
>Moreover, your following patches have a lot of
>lwmi_attr_id(tunable_attr->*_id, ...) patterns in wmi-other.c. Outlining
>a simple function usually has higher overhead than inlining it (hence
>worse performance). The calling convention also increases register
>pressure, resulting in a bloated code size:
>
>PATCH v5 (whole series):
>
> text data bss dec hex filename
> 6465 1832 0 8297 2069 lenovo-wmi-capdata.ko
> 43132 9153 3 52288 cc40 lenovo-wmi-other.ko
>
>Inlining lwmi_attr_id():
>
> text data bss dec hex filename
> 6325 1832 0 8157 1fdd lenovo-wmi-capdata.ko
> 41070 9153 3 50226 c432 lenovo-wmi-other.ko
>
>52288 - 50226 ~= 2KiB, which is quite a lot.
>
>So please move it to capdata.h and make it a static inline function.
>
>>
>> enum lwmi_cd_type {
>> LENOVO_CAPABILITY_DATA_00,
>> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
>> index 8c1df3efcc55..b5b6d0305b6a 100644
>> --- a/drivers/platform/x86/lenovo/wmi-capdata.h
>> +++ b/drivers/platform/x86/lenovo/wmi-capdata.h
>> @@ -19,6 +19,8 @@
>>
>> #define LWMI_DEVICE_ID_FAN 0x04
>>
>> +#define LWMI_TYPE_ID_NONE 0x00
>> +
>> struct component_match;
>> struct device;
>> struct cd_list;
>> @@ -57,6 +59,7 @@ struct lwmi_cd_binder {
>> cd_list_cb_t cd_fan_list_cb;
>> };
>>
>> +u32 lwmi_attr_id(u8 dev_id, u8 feat_id, u8 mode_id, u8 type_id);
>> void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
>> int lwmi_cd00_get_data(struct cd_list *list, u32 attribute_id, struct capdata00 *output);
>> int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
>> diff --git a/drivers/platform/x86/lenovo/wmi-gamezone.h b/drivers/platform/x86/lenovo/wmi-gamezone.h
>> index 6b163a5eeb95..ddb919cf6c36 100644
>> --- a/drivers/platform/x86/lenovo/wmi-gamezone.h
>> +++ b/drivers/platform/x86/lenovo/wmi-gamezone.h
>> @@ -10,6 +10,7 @@ enum gamezone_events_type {
>> };
>>
>> enum thermal_mode {
>> + LWMI_GZ_THERMAL_MODE_NONE = 0x00,
>> LWMI_GZ_THERMAL_MODE_QUIET = 0x01,
>> LWMI_GZ_THERMAL_MODE_BALANCED = 0x02,
>> LWMI_GZ_THERMAL_MODE_PERFORMANCE = 0x03,
>> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
>> index c1728c7c2957..7aa512ff1446 100644
>> --- a/drivers/platform/x86/lenovo/wmi-other.c
>> +++ b/drivers/platform/x86/lenovo/wmi-other.c
>> @@ -27,7 +27,6 @@
>> */
>>
>> #include <linux/acpi.h>
>> -#include <linux/bitfield.h>
>> #include <linux/cleanup.h>
>> #include <linux/component.h>
>> #include <linux/container_of.h>
>> @@ -62,8 +61,6 @@
>>
>> #define LWMI_FEATURE_ID_FAN_RPM 0x03
>>
>> -#define LWMI_TYPE_ID_NONE 0x00
>> -
>> #define LWMI_FEATURE_VALUE_GET 17
>> #define LWMI_FEATURE_VALUE_SET 18
>>
>> @@ -73,10 +70,9 @@
>>
>> #define LWMI_FAN_DIV 100
>>
>> -#define LWMI_ATTR_ID_FAN_RPM(x) \
>> - (FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) | \
>> - FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_RPM) | \
>> - FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_FAN_ID(x)))
>> +#define LWMI_ATTR_ID_FAN_RPM(x) \
>> + lwmi_attr_id(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
>> + LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
>>
>> #define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
>> #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
>> @@ -715,12 +711,8 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
>> u32 attribute_id;
>> int value, ret;
>>
>> - attribute_id =
>> - FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
>> - FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
>> - FIELD_PREP(LWMI_ATTR_MODE_ID_MASK,
>> - LWMI_GZ_THERMAL_MODE_CUSTOM) |
>> - FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>> + attribute_id = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
>> + LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
>>
>> ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
>> if (ret)
>> @@ -775,7 +767,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>> struct wmi_method_args_32 args;
>> struct capdata01 capdata;
>> enum thermal_mode mode;
>> - u32 attribute_id;
>> u32 value;
>> int ret;
>>
>> @@ -786,13 +777,10 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>> if (mode != LWMI_GZ_THERMAL_MODE_CUSTOM)
>> return -EBUSY;
>>
>> - attribute_id =
>> - FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
>> - FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
>> - FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
>> - FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>> + args.arg0 = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
>> + mode, tunable_attr->type_id);
>>
>> - ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
>> + ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
>> if (ret)
>> return ret;
>>
>> @@ -803,7 +791,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>> if (value < capdata.min_value || value > capdata.max_value)
>> return -EINVAL;
>>
>> - args.arg0 = attribute_id;
>> args.arg1 = value;
>>
>> ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,
>> @@ -837,7 +824,6 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
>> struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
>> struct wmi_method_args_32 args;
>> enum thermal_mode mode;
>> - u32 attribute_id;
>> int retval;
>> int ret;
>>
>> @@ -845,13 +831,8 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
>> if (ret)
>> return ret;
>>
>> - attribute_id =
>> - FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
>> - FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
>> - FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
>> - FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>> -
>> - args.arg0 = attribute_id;
>> + args.arg0 = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
>> + mode, tunable_attr->type_id);
>
>IIUC args.arg1 contains uninitialized data and will be passed to
>firmware. Since you are revising its assignment, let's zero-initialize
>`args' when declaring it too.
>
Hi Rong,
Sure, I can do those.
Derek
>Thanks,
>Rong
>
>>
>> ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
>> (unsigned char *)&args, sizeof(args),