Re: [PATCH v3 2/6] platform/x86: lenovo-wmi-other: Limit adding attributes to supported devices

Derek J. Clark
2026-02-25T18:09:07Z

On February 25, 2026 9:14:12 AM PST, Rong Zhang <i@rong.moe> wrote:
>Hi Derek,
>
>On Tue, 2026-02-24 at 04:31 +0000, Derek J. Clark wrote:
>> Adds lwmi_is_attr_01_supported, and only creates the attribute subfolder
>> if the attribute is supported by the hardware. Due to some poorly
>> implemented BIOS, this is a multi-step sequence of events. This is
>> because:
>> - Some BIOS support getting the capability data from custom mode (0xff),
>> while others only support it in no-mode (0x00).
>> - Similarly, some BIOS support get/set for the current value from custom
>> mode (0xff), while others only support it in no-mode (0x00).
>> - Some BIOS report capability data for a method that is not fully
>> implemented.
>> - Some BIOS have methods fully implemented, but no complimentary
>> capability data.
>>
>> To ensure we only expose fully implemented methods with corresponding
>> capability data, we check each outcome before reporting that an
>> attribute can be supported.
>>
>> Checking for lwmi_is_attr_01_supported during remove is not done to
>> ensure that we don't attempt to call cd01 or send WMI events if one of
>> the interfaces being removed was the cause of the driver unloading.
>>
>> While adding members to tunable_attr_01, remove unused capdata pointer
>> and limit size of all ID's to the appropriate size.
>>
>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Reported-by: Kurt Borja <kuurtb@gmail.com>
>> Closes: https://lore.kernel.org/platform-driver-x86/DG60P3SHXR8H.3NSEHMZ6J7XRC@gmail.com/
>> Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
>> ---
>> drivers/platform/x86/lenovo/wmi-other.c | 117 +++++++++++++++++++++---
>> 1 file changed, 102 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
>> index 95886df39c8d..f3f12303e379 100644
>> --- a/drivers/platform/x86/lenovo/wmi-other.c
>> +++ b/drivers/platform/x86/lenovo/wmi-other.c
>> @@ -545,11 +545,12 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
>> /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
>>
>> struct tunable_attr_01 {
>> - struct capdata01 *capdata;
>> struct device *dev;
>> - u32 feature_id;
>> - u32 device_id;
>> - u32 type_id;
>> + u8 feature_id;
>> + u8 device_id;
>> + u8 type_id;
>> + u8 cd_mode_id; /* mode arg for searching capdata */
>> + u8 cv_mode_id; /* mode arg for set/get current_value */
>> };
>>
>> static struct tunable_attr_01 ppt_pl1_spl = {
>> @@ -716,7 +717,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
>> int value, ret;
>>
>> attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
>> - LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
>> + tunable_attr->cd_mode_id, tunable_attr->type_id);
>>
>> ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
>> if (ret)
>> @@ -782,7 +783,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>> return -EBUSY;
>>
>> args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
>> - mode, tunable_attr->type_id);
>> + tunable_attr->cd_mode_id, tunable_attr->type_id);
>>
>> ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
>> if (ret)
>> @@ -795,6 +796,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>> if (value < capdata.min_value || value > capdata.max_value)
>> return -EINVAL;
>>
>> + args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
>> + tunable_attr->cv_mode_id, tunable_attr->type_id);
>> args.arg1 = value;
>>
>> ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,
>> @@ -828,13 +831,16 @@ 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;
>> - int retval;
>> - int ret;
>> + int retval, ret;
>>
>> ret = lwmi_om_notifier_call(&mode);
>> if (ret)
>> return ret;
>>
>> + /* If "no-mode" is the supported mode, ensure we never send current mode */
>> + if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
>> + mode = tunable_attr->cv_mode_id;
>> +
>> args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
>> mode, tunable_attr->type_id);
>>
>> @@ -847,6 +853,85 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
>> return sysfs_emit(buf, "%d\n", retval);
>> }
>>
>> +/**
>> + * lwmi_attr_01_is_supported() - Determine if the given attribute is supported.
>> + * @tunable_attr: The attribute to verify.
>> + *
>> + * First check if the attribute has a corresponding capdata01 table in the cd01
>> + * module under the "custom" mode (0xff). If that is not present then check if
>> + * there is a corresponding "no-mode" (0x00) entry. If either of those passes,
>> + * check capdata->supported for values > 0. If capdata is available, attempt to
>> + * determine the set/get mode for the current value property using a similar
>> + * pattern. If the value returned by either custom or no-mode is 0, or we get
>> + * an error, we assume that mode is not supported. If any of the above checks
>> + * fail then the attribute is not fully supported.
>> + *
>> + * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
>> + * reference.
>> + *
>> + * Return: Support level, or an error code.
>> + */
>> +static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
>> +{
>> + struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
>> + u8 mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
>> + struct wmi_method_args_32 args;
>> + struct capdata01 capdata;
>> + int retval, ret;
>> +
>> + /* Determine tunable_attr->cd_mode_id */
>> +no_mode_fallback_1:
>> + 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, args.arg0, &capdata);
>> + if (ret && mode) {
>> + dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
>> + mode = LWMI_GZ_THERMAL_MODE_NONE;
>> + goto no_mode_fallback_1;
>> + }
>> + if (ret)
>> + goto not_supported;
>> + if (!capdata.supported) {
>> + ret = -EOPNOTSUPP;
>> + goto not_supported;
>> + }
>> +
>> + tunable_attr->cd_mode_id = mode;
>> +
>> + /* Determine tunable_attr->cv_mode_id */
>> + mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
>> +no_mode_fallback_2:
>> + args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
>> + mode, tunable_attr->type_id);
>> +
>> + ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
>> + (unsigned char *)&args, sizeof(args),
>> + &retval);
>> + if ((ret && mode) || (!retval && mode)) {
>> + dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
>> + mode = LWMI_GZ_THERMAL_MODE_NONE;
>> + goto no_mode_fallback_2;
>> + }
>> + if (ret)
>> + goto not_supported;
>> + if (retval == 0) {
>> + ret = -EOPNOTSUPP;
>> + goto not_supported;
>> + }
>> +
>
>I agree with Ilpo that backward gotos are hard to read and should be
>avoided if possible.
>
>> + tunable_attr->cv_mode_id = mode;
>> + dev_dbg(tunable_attr->dev, "cd_mode_id: %02x%02x%02x%02x, cv_mode_id: %#08x attribute support level: %x\n",
>
>It's a long line. Wrap at the first comma?
>
>> + tunable_attr->device_id, tunable_attr->feature_id, tunable_attr->cd_mode_id,
>> + tunable_attr->type_id, args.arg0, capdata.supported);
>> +
>> + return capdata.supported;
>> +
>> +not_supported:
>> + dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
>> + return ret;
>> +}
>> +
>> /* Lenovo WMI Other Mode Attribute macros */
>> #define __LWMI_ATTR_RO(_func, _name) \
>> { \
>> @@ -970,19 +1055,21 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
>> }
>>
>> for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
>> - err = sysfs_create_group(&priv->fw_attr_kset->kobj,
>> - cd01_attr_groups[i].attr_group);
>> - if (err)
>> - goto err_remove_groups;
>> -
>> cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
>> + if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0) {
>> + err = sysfs_create_group(&priv->fw_attr_kset->kobj,
>> + cd01_attr_groups[i].attr_group);
>> + if (err)
>> + goto err_remove_groups;
>> + }
>> }
>> return 0;
>>
>> err_remove_groups:
>> while (i--)
>> - sysfs_remove_group(&priv->fw_attr_kset->kobj,
>> - cd01_attr_groups[i].attr_group);
>> + if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0)
>
>I don't see lwmi_om_fw_attr_remove() doing the same. If we care about
>symmetric cleanup, I would imagine a bitmap or so recording what have
>been created is used there.
>
>Also it's doubtful if the firmware always return the same result on the
>same parameters. If a broken firmware somehow returns different results
>here, some attributes may be left uncleaned.
>
>That being said, I am not sure if we really need symmetric cleanup.
>IIUC, sysfs_remove_group() is a no-op if we pass an attr_group that has
>not been created.

Hi Rong,
We don't want to query the interface if it was removed, and the no-op is harmless. It doesn't seem worth it to me to retain that in memory to achieve the same results.

>> + sysfs_remove_group(&priv->fw_attr_kset->kobj,
>> + cd01_attr_groups[i].attr_group);
>>
>> kset_unregister(priv->fw_attr_kset);
>>
>
>Thanks,
>Rong