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

Ilpo Järvinen
2026-02-26T09:20:45Z

[-- Attachment #1: Type: text/plain, Size: 10247 bytes --]

On Wed, 25 Feb 2026, Derek J. Clark wrote:

> On February 24, 2026 12:47:56 AM PST, "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> wrote:
> >On Tue, 24 Feb 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).
> >> - 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.
> >
> >Please don't mix changes like this. Create a seprate patch for it.
> >
> >> 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);
> >
> >Add include.
> >
> >> + mode = LWMI_GZ_THERMAL_MODE_NONE;
> >> + goto no_mode_fallback_1;
> >
> >Is it possible to make a helper so you don't need these back gotos?
> >
>
> Sure. How about I put both modes into an array and loop through them,
> passing the values to a function that will evaluate it. I can break if
> there is a match and throw an error if there's no match at the end.

It would probably work okay with a loop (which you've basically done now
in a custom way using that goto) and array.

> >> + }
> >> + 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;
> >
> >Same question here?
> >
> >> + }
> >> + if (ret)
> >> + goto not_supported;
> >> + if (retval == 0) {
> >> + ret = -EOPNOTSUPP;
> >> + goto not_supported;
> >> + }
> >> +
> >> + 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",
> >> + 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) {
> >
> >Reverse logic and use continue.
> >
> >> + 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)
> >
> >Reverse logic + continue.
> >
> >> + sysfs_remove_group(&priv->fw_attr_kset->kobj,
> >> + cd01_attr_groups[i].attr_group);
> >
> >You need to add braces for multiline constructs.
> >
> >>
> >> kset_unregister(priv->fw_attr_kset);
> >>
> >>
> >
>

--
i.