Re: [PATCH v10 06/16] platform/x86: lenovo-wmi-other: Limit adding attributes to supported devices

["Derek J. Clark" <derekjohn.clark@gmail.com>]

On April 30, 2026 7:01:55 AM PDT, "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> wrote:
>On Sun, 12 Apr 2026, 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).
>> - 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.
>> 
>> Fixes: edc4b183b794 ("platform/x86: Add Lenovo Other Mode WMI Driver")
>> Reported-by: Kurt Borja <kuurtb@gmail.com>
>> Closes: https://lore.kernel.org/platform-driver-x86/DG60P3SHXR8H.3NSEHMZ6J7XRC@gmail.com/
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Rong Zhang <i@rong.moe>
>> Tested-by: Rong Zhang <i@rong.moe>
>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
>> ---
>> v7:
>>   - Move earlier in the series. This required dropping the use of
>>     lwmi_attr_id as it will be added later.
>>   - Add missing switch between cd_mode_id and cv_mode_id in
>>     current_value_store.
>> v6:
>>   - Zero initialize args in lwmi_is_attr_01_supported.
>>   - Fix formatting.
>> v5:
>>   - Move cv/cd_mode_id refrences from path 3/4.
>>   - Add missing import for ARRAY_SIZE.
>>   - Make lwmi_is_attr_01_supported return bool instead of u32.
>>   - Various formatting fixes.
>> v4:
>>   - Use for loop instead of backtrace gotos for checking if an attribute
>>     is supported.
>>   - Add include for dev_printk.
>>   - Wrap dev_dbg in lwmi_is_attr_01_supported earlier.
>>   - Don't use symmetric cleanup of attributes in error states.
>> ---
>>  drivers/platform/x86/lenovo/wmi-gamezone.h |   1 +
>>  drivers/platform/x86/lenovo/wmi-other.c    | 114 ++++++++++++++++++---
>>  2 files changed, 98 insertions(+), 17 deletions(-)
>> 
>> 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 50a03f5fd6ab..29d062a1c6dc 100644
>> --- a/drivers/platform/x86/lenovo/wmi-other.c
>> +++ b/drivers/platform/x86/lenovo/wmi-other.c
>> @@ -550,6 +550,8 @@ struct tunable_attr_01 {
>>  	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 = {
>> @@ -775,7 +777,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 +787,12 @@ 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 = 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, tunable_attr->cd_mode_id) |
>> +		    FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, 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 +803,10 @@ 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.arg0 = 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, tunable_attr->cv_mode_id) |
>> +		    FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>
>It's already repeated a few times and you're adding more in this patch.
>
>We should have a helper function for this encoding as it seems to 
>repeat. That is, something that takes tunable_attr and mode as input
>(the conversion of existing entries should be in own patch preceeding 
>this fix patch).
>

Hi Ilpo,

A function for that is added in patch 10, though it is slightly modified from that to be more flexible is tunable_attr isn't used (such as with the fan test attributes)

Originally I had that patch preceding any additions, but after discussing with Rong we felt like it would be easier for stable backports if all the fixes were upfront. I can certainly move it back if you still prefer.

Thanks,
Derek


>>  	args.arg1 = value;
>>  
>>  	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,
>> @@ -837,7 +840,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 +847,14 @@ 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);
>> +	/* 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 = attribute_id;
>> +	args.arg0 = 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);
>>  
>>  	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
>>  				    (unsigned char *)&args, sizeof(args),
>> @@ -862,6 +865,81 @@ 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: bool.
>> + */
>> +static bool lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
>> +{
>> +	u8 modes[2] = { LWMI_GZ_THERMAL_MODE_CUSTOM, LWMI_GZ_THERMAL_MODE_NONE };
>> +	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
>> +	struct wmi_method_args_32 args = {};
>> +	bool cd_mode_found = false;
>> +	bool cv_mode_found = false;
>> +	struct capdata01 capdata;
>> +	int retval, ret, i;
>> +
>> +	/* Determine tunable_attr->cd_mode_id*/
>> +	for (i = 0; i < ARRAY_SIZE(modes); i++) {
>> +		args.arg0 = 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, modes[i]) |
>> +			    FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>> +
>> +		ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
>> +		if (ret || !capdata.supported)
>> +			continue;
>> +		tunable_attr->cd_mode_id = modes[i];
>> +		cd_mode_found = true;
>> +		break;
>> +	}
>> +
>> +	if (!cd_mode_found)
>> +		return cd_mode_found;
>> +
>> +	dev_dbg(tunable_attr->dev,
>> +		"cd_mode_id: %#010x\n", args.arg0);
>> +
>> +	/* Determine tunable_attr->cv_mode_id, returns 1 if supported*/
>> +	for (i = 0; i < ARRAY_SIZE(modes); i++) {
>> +		args.arg0 = 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, modes[i]) |
>> +			    FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>> +
>> +		ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
>> +					    (unsigned char *)&args, sizeof(args),
>> +					    &retval);
>> +		if (ret || !retval)
>> +			continue;
>> +		tunable_attr->cv_mode_id = modes[i];
>> +		cv_mode_found = true;
>> +		break;
>> +	}
>> +
>> +	if (!cv_mode_found)
>> +		return cv_mode_found;
>> +
>> +	dev_dbg(tunable_attr->dev, "cv_mode_id: %#010x, attribute support level: %#010x\n",
>> +		args.arg0, capdata.supported);
>> +
>> +	return capdata.supported > 0 ? true : false;
>> +}
>> +
>>  /* Lenovo WMI Other Mode Attribute macros */
>>  #define __LWMI_ATTR_RO(_func, _name)                                  \
>>  	{                                                             \
>> @@ -985,12 +1063,14 @@ static void lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
>>  	}
>>  
>>  	for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
>> +		cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
>> +		if (!lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr))
>> +			continue;
>> +
>>  		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;
>>  	}
>>  	return;
>>  
>> 
>