On Fri, 1 May 2026, Rong Zhang wrote: > On Thu, 2026-04-30 at 07:56 -0700, Derek J. Clark wrote: > > On April 30, 2026 7:01:55 AM PDT, "Ilpo Järvinen" 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 > > > > Closes: https://lore.kernel.org/platform-driver-x86/DG60P3SHXR8H.3NSEHMZ6J7XRC@gmail.com/ > > > > Cc: stable@vger.kernel.org > > > > Reviewed-by: Rong Zhang > > > > Tested-by: Rong Zhang > > > > Reviewed-by: Mark Pearson > > > > Signed-off-by: Derek J. Clark > > > > --- > > > > 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. > > Moving it back is OK for me, too. > > I think exposing non-fully-functioning fw-attrs on stable/LTS kernels > should be acceptable as long as reading/writing these attributes doesn't > break anything, which is exactly the case now (i.e., without this > patch). How would refactoring the code into a helper result in changing stable interface? Or did you perhaps move to talk about something entirely else (and I ended up losing the context)? > Thanks a lot for your hard work in this series, -- i.