Re: [PATCH v5 3/8] platform/x86: lenovo-wmi-other: Add lwmi_attr_id() function
From: Rong Zhang
Date: Wed Mar 25 2026 - 14:02:23 EST
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.
Thanks,
Rong
>
> ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
> (unsigned char *)&args, sizeof(args),