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

[Rong Zhang]

Hi Derek,

On Wed, 2026-04-01 at 12:37 -0700, Derek J. Clark wrote:
> 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. 

Thanks. I will get in touch with you off-list when I finishes the PoC.
Probably in this week or next if I am not going to be very busy :)

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

I understand your difficulties, as that's mostly my preference too. But
on the other hand, bug reports can't be ignored if it's legit and
reproducible.

I tried my best to make my fix patches tiny and easy to review. I hope
this is helpful.

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

I also hope the dust settles soon. Thanks a lot for your great efforts
and patience -- they are priceless.

Thanks,
Rong

> 
> 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