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

[Ilpo Järvinen]

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