Re: [PATCH v10 06/16] platform/x86: lenovo-wmi-other: Limit adding attributes to supported devices

[Derek J. Clark]

On May 5, 2026 2:48:08 AM PDT, "Ilpo Järvinen" <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>On Thu, 30 Apr 2026, Derek J. Clark wrote:
>> On April 30, 2026 7:01:55 AM PDT, "Ilpo Järvinen" <ilpo.jarvinen@xxxxxxxxxxxxxxx> 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 <kuurtb@xxxxxxxxx>
>> >> Closes: https://lore.kernel.org/platform-driver-x86/DG60P3SHXR8H.3NSEHMZ6J7XRC@xxxxxxxxx/
>> >> Cc: stable@xxxxxxxxxxxxxxx
>> >> Reviewed-by: Rong Zhang <i@xxxxxxxx>
>> >> Tested-by: Rong Zhang <i@xxxxxxxx>
>> >> Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>> >> Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
>> >> ---
>> >> 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?

- Derek

>> 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.
>