Re: [PATCH v6 00/13] platform-x86: lenovo-wmi: Add fixes and enhancement

[Derek J. Clark]

On April 1, 2026 11:44:51 AM PDT, Rong Zhang <i@xxxxxxxx> wrote:
>Hi Derek,
>
>On Tue, 2026-03-31 at 18:11 +0000, Derek J. Clark wrote:
>> This series adds many much needed features and fixes to the lenovo-wmi
>> drivers.
>> 
>> Patch 1 moves LWMI_FAN_DIV to be next to the rest of the fan attribute
>> defines in preparation for adding additional attrbiute macros. This is
>> so the attribute macros can all be in the same place in the file.
>> 
>> Patch 2 cleans up tunable_attr_01 by removing an unused pointer and
>> correctly assigning the members as u8 isntead of u32.
>> 
>> Patch 3 fixes a bug when sending 32 bit arguments via WMI where the
>> second value in the args struct was uninitialized.
>> 
>> Patch 4 moves all gamezone enums from the gamezone header into the
>> helpers header in preparation for the rest of the series.
>> 
>> Patch 5 adds a function to make assigning attribute ID's for capdata
>> cleaner and easier.
>> 
>> Patch 6 addresses bugs where devices that don't support exposed
>> attributes would still create the attribute, and also attempts to
>> identify the correct capdata and set/get methods since some legacy
>> interfaces don't use the custom mode in the method or capdata ID.
>> 
>> Patch 7 adds the remaining CPU attributes that weren't previously
>> exposed.
>> 
>> Patch 8 adds GPU attributes.
>> 
>> Patch 9 renames a name constant in preparation for patch 6.
>> 
>> Patch 10 adds battery charge-type limiting when supported only by WMI, or
>> when a module parameter to skip compatibility checks is set. The
>> MODULE_PARM_DESC macro creates one check and two warnings in checkpatch.
>> I reviewed other examples from the kernel and I am following the same
>> convention, so I left it as is.
>> 
>> Patch 11 fixes a bug where the 'gamezone' and the 'other' drivers were
>> incorrectly coupled in the Kconfig, leading to side effects under
>> certain kernel configurations.
>> 
>> Patch 12 adds a debugfs directory.
>> 
>> Patch 13 adds a debugfs file for dumping capdata.
>> 
>> Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
>
>The series LGTM except for some tiny issues. See my replies to the
>corresponding patches.
>
>Sashiko.dev reports some potential issues in the series as well as some
>existing bugs.
>
>https://sashiko.dev/#/patchset/20260331181208.421552-1-derekjohn.clark%40gmail.com
>
>I've mentioned some in my reply to the corresponding patches. Besides, I
>am going to talk about existing bugs here:
>
>
>- tunable_attr_01 is a statically shared structure, but .dev is
>overwritten each time when the master is bound, which violates
>`.no_singleton = true'
>
>AFAIK no device in wild has multiple wmi-other instances so it should be
>safe for the time being.
>
>Fixing it will need numerous fundamental refactorings to the attributes
>and conflict with patch 6 where .cd_mode_id and .cv_mode_id is added to
>tunable_attr_01.
>
>I don't really want to make this series too large and miss this cycle.
>Hence, I am going to fix it in the next cycle with some extra
>refactorings to get rid of attribute-specific show/store callbacks.

Might be good to collaborate on this. I wanted to add some callbacks & macros for BOOL type attributes and rename the current ATTR macro/callbacks to specify U8 in the names. 

I also wanted to add fan curve settings with pwm_auto attributes for devices that support it soon, but that might be a big lift with such a large refactor going on. I personally prefer to alternate between bug fixing and feature adding per series/cycle if I can, this one just became a mix of both.

>For this series, I'd consider violating `.no_singleton = true' a very
>minor concern. It should be fine to keep it as is. If we really care
>about it, we could set it to false temporarily.
>

I'll leave it for now unless Ilpo or Armin specifically want it changed. 

>- Rebinding wmi-other leaks IDA
>
>- Failure in lwmi_om_fw_attr_add() followed by lwmi_other_remove()
>double frees the same IDA
>
>- Calling lwmi_dev_evaluate_int() with `retval == NULL' leaks memory
>
>- Unbalanced component bind and unbind in the error path of
>lwmi_om_master_bind()
>
>Legit concerns. I will reply with a series fixing them. Could you
>incorporate it into your series?
>
>Fix patches should be the very first patches in the series so that
>backporting is less painful. I'd suggest rearranging the series like:
>
>patches in my fix series
>patch 3
>patch 4
>patch 11
>the rest patches
>

Sounds good. I'll try to get those changes incorporated and sent out today. This series is getting out of hand as it is (from 5 to 16 patches now) so putting it to bed is high on my priority list.

Thanks, 

Derek
>Thanks,
>Rong
>
>> ---
>> v6:
>>   - Incorporate Rong Zhang's debugfs and decoupling patches into the
>>     series.
>>   - Add a patch to clean up too many cross-references to wmi-gamezone.h
>>   - Make lwmi_attr_id a static inline in wmi-capdata.h
>>   - Added a patch  to fix a bug where ares.arg1 is uninitialized when it
>>     is sent to the firmware.
>>   - Add supported checks before adding battery extenstion, and ensure
>>     both the new checks and the is_writable checks are not casting u32
>>     to i32.
>>   - Misc formating changes.
>> v5: https://lore.kernel.org/platform-driver-x86/20260324221032.1333636-1-derekjohn.clark@xxxxxxxxx/
>>   - Remove cv/cd_mode_id references that occured before patch 4.
>>   - Move lwmi_attr_id to capdata.c with a namespace export.
>>   - Fix mixing include.
>>   - Make lwmi_attr_is_supported return bool.
>>   - Use switch instead of if for setting/getting charge type state.
>>   - Various formatting fixes.
>> v4: https://lore.kernel.org/platform-driver-x86/20260312031032.3467565-1-derekjohn.clark@xxxxxxxxx/
>>   - Use loop instead of back gotos for identifying the working attribute
>>     ID.
>>   - Use function instead of macro to assign attribute_id, preserving
>>     types.
>>   - Removed unused defines and enum values.
>>   - Rename charging defines to clarify thier purpose.
>>   - Fixed various formatting issues from v3.
>>   - Added module param to skip ACPI check when loading the driver for
>>     the power supply extension.
>>   - Don't abort adding power supply extension if the ACPI handle from
>>     ideapad is not present.
>>   - Don't worry about symmetric cleanup when cleaning up attributes in
>>     an error state.
>>   - Reword Patch 8 commit message to be more concise.
>>   - Fix wording in Patch 7 to match the changes.
>> v3: https://lore.kernel.org/platform-driver-x86/20260224043200.2680384-1-derekjohn.clark@xxxxxxxxx/
>>   - Re-add HWMON name const and just rename LWMI_OM_FW_ATTR_BASE_PATH
>>   - Fix linker warnings by moving acpi/battery include to the end of the
>>     list.
>>   - Remove CPU/GPU OC features. These attributes are BOOL type and will
>>     need a new constructor that I'll add later.
>> v2: https://lore.kernel.org/platform-driver-x86/20260215061339.2842486-1-derekjohn.clark@xxxxxxxxx/
>>   - Fix gpu_mode misisng from attributes list.
>>   - Fix prototypes for power suppy patch.
>>   - Reorganize CPU and GPU attributes alphabetically.
>>   - Break out the patch consolidating the driver name cost.
>>   - Move some of the refactoring of attribute_id back to into patch 1
>>     where it belongs.
>>   - Fix some additional typos in function prototypes.
>> v1: https://lore.kernel.org/platform-driver-x86/20260213081243.794288-1-derekjohn.clark@xxxxxxxxx/
>> 
>> 
>> Derek J. Clark (10):
>>   platform/x86: lenovo-wmi-other: Move LWMI_FAN_DIV
>>   platform/x86: lenovo-wmi-other: Fix tunable_attr_01 struct members
>>   platform/x86: lenovo-wmi-other: Zero initialize WMI arguments
>>   platform/x86: lenovo-wmi-helpers: Move gamezone enums to wmi-helpers
>>   platform/x86: lenovo-wmi-other: Add lwmi_attr_id() function
>>   platform/x86: lenovo-wmi-other: Limit adding attributes to supported
>>     devices
>>   platform/x86: lenovo-wmi-other: Add missing CPU tunable attributes
>>   platform/x86: lenovo-wmi-other: Add GPU tunable attributes
>>   platform/x86: lenovo-wmi-other: Rename LWMI_OM_FW_ATTR_BASE_PATH
>>   platform/x86: lenovo-wmi-other: Add WMI battery charge limiting
>> 
>> Rong Zhang (3):
>>   platform/x86: lenovo: Decouple lenovo-wmi-gamezone and
>>     lenovo-wmi-other
>>   platform/x86: lenovo-wmi-helpers: Add helper for creating per-device
>>     debugfs dir
>>   platform/x86: lenovo-wmi-capdata: Add debugfs file for dumping capdata
>> 
>>  .../wmi/devices/lenovo-wmi-other.rst          |  19 +
>>  drivers/platform/x86/lenovo/Kconfig           |   3 +-
>>  drivers/platform/x86/lenovo/wmi-capdata.c     | 128 +++-
>>  drivers/platform/x86/lenovo/wmi-capdata.h     |  31 +-
>>  drivers/platform/x86/lenovo/wmi-events.c      |   2 +-
>>  drivers/platform/x86/lenovo/wmi-gamezone.c    |   5 +-
>>  drivers/platform/x86/lenovo/wmi-gamezone.h    |  20 -
>>  drivers/platform/x86/lenovo/wmi-helpers.c     | 136 ++++
>>  drivers/platform/x86/lenovo/wmi-helpers.h     |  23 +
>>  drivers/platform/x86/lenovo/wmi-other.c       | 689 ++++++++++++++----
>>  drivers/platform/x86/lenovo/wmi-other.h       |  16 -
>>  11 files changed, 891 insertions(+), 181 deletions(-)
>>  delete mode 100644 drivers/platform/x86/lenovo/wmi-gamezone.h
>>  delete mode 100644 drivers/platform/x86/lenovo/wmi-other.h