On Thu, 30 Apr 2026, 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) It still leaves some boilerplate having to deref all those tunable_attr fields so perhaps it would be better to do nested helpers, one taking tunable_attr and that calls the more flexible helper. IMO that would be the best approach here. > 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. I see. This patch would be cleaner though if we have the helper already in place but I'm not insisting if you two prefer the current order of the patches. -- i.