Re: [PATCH v4 4/8] platform/x86: lenovo-wmi-other: Limit adding attributes to supported devices
From: Rong Zhang
Date: Sun Mar 15 2026 - 00:10:39 EST
Hi Derek,
On Sun, 2026-03-15 at 09:26 +0800, Rong Zhang wrote:
> Hi Derek,
>
> On Thu, 2026-03-12 at 03:10 +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).
> > - 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.
> >
> > Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> > Reported-by: Kurt Borja <kuurtb@xxxxxxxxx>
> > Closes: https://lore.kernel.org/platform-driver-x86/DG60P3SHXR8H.3NSEHMZ6J7XRC@xxxxxxxxx/
> > Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
> > ---
> > 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 | 76 ++++++++++++++++++++++++-
> > 1 file changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> > index 9fff9c1f768c..55a26e5617d4 100644
> > --- a/drivers/platform/x86/lenovo/wmi-other.c
> > +++ b/drivers/platform/x86/lenovo/wmi-other.c
> > @@ -32,6 +32,7 @@
> > #include <linux/component.h>
> > #include <linux/container_of.h>
> > #include <linux/device.h>
> > +#include <linux/dev_printk.h>
> > #include <linux/export.h>
> > #include <linux/gfp_types.h>
> > #include <linux/hwmon.h>
> > @@ -871,6 +872,76 @@ 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)
> > +{
> > + 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++) {
>
> #include <linux/array_size.h>
>
> > + args.arg0 = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
> > + modes[i], 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 -EOPNOTSUPP;
> > +
> > + /* Determine tunable_attr->cv_mode_id, returns 1 if supported*/
> > + for (i = 0; i < ARRAY_SIZE(modes); i++) {
> > + args.arg0 = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
> > + modes[i], 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 -EOPNOTSUPP;
> > +
> > + 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);
> ^
>
> dev_dbg(tunable_attr->dev,
> "cd_mode_id: %#10x, cv_mode_id: %#10x, attribute support level: %#10x\n",
Sorry, they should be %#010x.
Thanks,
Rong
> lwmi_attr_id(...), args.arg0, capdata.supported);
>
> > +
> > + return capdata.supported;
>
> You are casting u32 to int. Return it in a pointer argument if you need
> it.
>
> > +}
> > +
> > /* Lenovo WMI Other Mode Attribute macros */
> > #define __LWMI_ATTR_RO(_func, _name) \
> > { \
> > @@ -994,12 +1065,13 @@ static int 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) <= 0)
>
> Extra whitespace.
>
> > + continue;
>
> Add an empty line in between.
>
> Thanks,
> Rong
>
> > 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 0;
> >