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

["Derek J. Clark" <derekjohn.clark@gmail.com>]

On May 5, 2026 10:20:15 AM PDT, Rong Zhang <i@rong.moe> wrote:
>Hi Ilpo,
>
>于 2026年5月5日 GMT+08:00 18:25:34，"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> 写道：
>>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@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)
>>> > 
>>> > 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)?
>
>I meant the *current* LTS/stable state is acceptable for me as reading/writing these attributes doesn't break anything. Moving it back would be OK for me even if it made this patch unable to be backported (though unlikely, since moving it back makes the patch clearer as you've said). In other words, it doesn't matter for me whether the patch is backported or not.
>
>Some context: I didn't ask Derek to add Fixes: tag to this patch or move it. I asked him to rearrange other patches to make them backportable.
>
>Sorry for causing misunderstandings.
>
>Thanks,
>Rong
>

I preferred it earlier myself since the rest of the series was cleaner that way. I'll move it again as that's fairly simple. IMO stable should be able to take it as part of the fixes it affects since there are no functional changes and it will be a prerequisite for some much needed fixes on stable, but I suppose that's ultimately up to them.

I'll submit in a couple of days as I'm currently waiting on some feedback from Lenovo regarding some clarification of the charge limiting features after getting some unexpected results in a test.

Thanks,
Derek
>>
>>> Thanks a lot for your hard work in this series,
>>
>>