Re: [PATCH v11 09/15] platform/x86: lenovo-wmi-other: Limit adding attributes to supported devices

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 7 May 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>
> ---
> v11:
>   - Also use cd_mode_id in attr_capdata_show.
> 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-other.c | 86 +++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> index e69bea72e6d3..e3cdcd0f4331 100644
> --- a/drivers/platform/x86/lenovo/wmi-other.c
> +++ b/drivers/platform/x86/lenovo/wmi-other.c
> @@ -542,6 +542,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 */
>  };
>  
>  /**
> @@ -623,7 +625,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
>  	u32 attribute_id;
>  	int value, ret;
>  
> -	attribute_id = tunable_attr_01_id(tunable_attr, LWMI_GZ_THERMAL_MODE_CUSTOM);
> +	attribute_id = tunable_attr_01_id(tunable_attr, tunable_attr->cd_mode_id);
>  
>  	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
>  	if (ret)
> @@ -688,7 +690,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>  	if (mode != LWMI_GZ_THERMAL_MODE_CUSTOM)
>  		return -EBUSY;
>  
> -	args.arg0 = tunable_attr_01_id(tunable_attr, mode);
> +	args.arg0 = tunable_attr_01_id(tunable_attr, tunable_attr->cd_mode_id);
>  
>  	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
>  	if (ret)
> @@ -701,6 +703,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>  	if (value < capdata.min_value || value > capdata.max_value)
>  		return -EINVAL;
>  
> +	args.arg0 = tunable_attr_01_id(tunable_attr, tunable_attr->cv_mode_id);
>  	args.arg1 = value;
>  
>  	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,
> @@ -741,6 +744,10 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
>  	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 = tunable_attr_01_id(tunable_attr, mode);
>  
>  	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
> @@ -752,6 +759,75 @@ 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.

Can you try to split this into 2-3 paragraphs. It has the long wall of 
text feel as is.

> + *
> + * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
> + * reference.
> + *
> + * Return: bool.

Describe what is returned, not its type (that is know to kerneldoc from 
syntax).

> + */
> +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*/

Missing space.

> +	for (i = 0; i < ARRAY_SIZE(modes); i++) {
> +		args.arg0 = tunable_attr_01_id(tunable_attr, modes[i]);
> +
> +		ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
> +		if (ret || !capdata.supported)
> +			continue;

I'd add empty lines here to help.

> +		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*/

Missing space.

> +	for (i = 0; i < ARRAY_SIZE(modes); i++) {
> +		args.arg0 = tunable_attr_01_id(tunable_attr, modes[i]);
> +
> +		ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
> +					    (unsigned char *)&args, sizeof(args),
> +					    &retval);
> +		if (ret || !retval)
> +			continue;

Empty line here as well.

> +		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;

This is enough when the function returns bool:

return capdata.supported > 0;

> +}
> +
>  /* Lenovo WMI Other Mode Attribute macros */
>  #define __LWMI_ATTR_RO(_func, _name)                                  \
>  	{                                                             \
> @@ -875,12 +951,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;
>  
> 

-- 
 i.