On Tue, 5 May 2026, Derek J. Clark wrote: > On May 5, 2026 2:48:08 AM PDT, "Ilpo Järvinen" wrote: > >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. > > > > That's simple enough. I assume that can be added to that same patch > instead of an additional one? Yes, adding both of them in the same patch is fine. Otherwise you'd just be changing the same spots again. > >> 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.