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

[Rong Zhang <i@rong.moe>]

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@gmail.com>

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.

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.


- 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

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@gmail.com/
>   - 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@gmail.com/
>   - 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@gmail.com/
>   - 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@gmail.com/
>   - 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@gmail.com/
> 
> 
> 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