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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7685 bytes --]

On Tue, 5 May 2026, Derek J. Clark wrote:

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