Re: [PATCH v7 0/7] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed

[Rong Zhang <i@rong.moe>]

Hi Derek,

On Tue, 2025-11-25 at 13:43 -0800, Derek John Clark wrote:
> On Tue, Nov 25, 2025 at 11:50 AM Rong Zhang <i@rong.moe> wrote:
> > 
> > Lenovo WMI Other Mode interface also supports querying or setting fan
> > speed RPM. This capability is described by LENOVO_CAPABILITY_DATA_00.
> > Besides, LENOVO_FAN_TEST_DATA provides reference data for self-test of
> > cooling fans, including minimum and maximum fan speed RPM.
> > 
> > This patchset turns lenovo-wmi-capdata01 into a unified driver (now
> > named lenovo-wmi-capdata) for LENOVO_CAPABILITY_DATA_{00,01} and
> > LENOVO_FAN_TEST_DATA; then adds HWMON support for lenovo-wmi-other:
> > 
> >  - fanX_enable: enable/disable the fan (tunable)
> >  - fanX_input: current RPM
> >  - fanX_max: maximum RPM
> >  - fanX_min: minimum RPM
> >  - fanX_target: target RPM (tunable)
> > 
> > LENOVO_CAPABILITY_DATA_{00,01} presents on all devices, so
> > both binds to lenovo-wmi-other. However, some device does not have
> > LENOVO_FAN_TEST_DATA and its presence is described by
> > LENOVO_CAPABILITY_DATA_00; hence, the former binds to the latter and a
> > callback is used to pass the data to lenovo-wmi-other.
> > 
> > Summarizing this scheme:
> > 
> >         lenovo-wmi-other <-> capdata00 <-> capdata_fan
> >         |- master            |- component
> >                              |- sub-master
> >                                            |- sub-component
> > 
> > The callback will be called once both the master and the sub-component
> > are bound to the sub-master (component).
> > 
> > This scheme is essential to solve these issues:
> > - The component framework only supports one aggregation per master
> > - A binding is only established until all components are found
> > - The Fan Test Data interface may be missing on some devices
> > - To get rid of queries for the presence of WMI GUIDs
> > - The notifier framework cannot cleanly connect capdata_fan to
> >   lenovo-wmi-other without introducing assumptions on probing sequence
> > 
> > capdata00 is registered as a component and a sub-master on probe,
> > instead of chaining the registrations in one's bind callback. This is
> > because calling (un)registration methods of the component framework
> > causes deadlock in (un)bind callbacks, i.e., it's impossible to register
> > capdata00 as a sub-master/component in its component/sub-master bind
> > callback, and vice versa.
> > 
> > The implementation does not rely on a specific binding sequence. This
> > has been fuzz-tested using:
> > 
> >         #!/bin/bash
> > 
> >         DRV_DIR=/sys/bus/wmi/drivers/lenovo_wmi_cd
> >         CAPDATA_GUIDS=(
> >                 $(find "$DRV_DIR"/ -name '*-*-*-*-*-*' -printf "%f ")
> >         )
> > 
> >         b() { sudo tee "$DRV_DIR"/bind <<<"$1"; }
> >         u() { sudo tee "$DRV_DIR"/unbind <<<"$1"; }
> > 
> >         for guid in "${CAPDATA_GUIDS[@]}"; do
> >                 u "$guid"
> >         done
> > 
> >         while read -rsa perm; do
> >                 for guid in "${perm[@]}"; do
> >                         b "$guid"
> >                 done
> >                 sensors | grep -A3 lenovo_wmi_other || true
> >                 for guid in "${perm[@]}"; do
> >                         u "$guid"
> >                 done
> >         done < <(python3 -c "
> >         from itertools import permutations
> >         ps = permutations('${CAPDATA_GUIDS[*]}'.split())
> >         for p in ps: print(' '.join(p))")
> > 
> >         for guid in "${CAPDATA_GUIDS[@]}"; do
> >                 b "$guid"
> >         done
> > 
> > Tested on ThinkBook 14 G7+ ASP.
> > 
> > Changes in v7:
> > - Fix missing #include (thanks Ilpo Järvinen)
> > - Fix formatting issues (ditto)
> > - dev_dbg() instead of dev_info() on probe success (ditto)
> > - Rearrange to drop some gotos (ditto)
> > - Move the declarations of __free()-managed variables to where they are
> >   assigned (ditto)
> > - Improve the readability of struct definition and error paths (ditto)
> > - Prevent back-and-forth changes (ditto)
> > - Fix mistakenly inverted boundary check
> > - Emit unaligned access to Fan Test Data's WMI block
> > - Properly calculate array index when we truncate Fan Test Data
> > - Fix typo
> > - Link to v6: https://lore.kernel.org/r/20251122184522.18677-1-i@rong.moe/
> > 
> > Changes in v6:
> > - Fix mistaken error paths
> > - Link to v5: https://lore.kernel.org/r/20251114175927.52533-1-i@rong.moe/
> > 
> > Changes in v5:
> > - Do not cast pointer to non-pointer or vice versa (thanks kernel test
> >   robot)
> > - Fix missing include (ditto)
> > - Link to v4: https://lore.kernel.org/r/20251113191152.96076-1-i@rong.moe/
> > 
> > Changes in v4:
> > - Get rid of wmi_has_guid() (thanks Armin Wolf's inspiration)
> >   - Add [PATCH v4 6/7], please review & test
> >     - Check 0x04050000.supported and bind capdata_fan to capdata00
> >   - Rework HWMON registration
> >     - Collect fan info from capdata00 and capdata_fan separately
> >     - Use a callback to collect fan info from capdata_fan
> >     - Trigger HWMON registration only if all fan info is collected
> >     - Do not check 0x04050000.supported, implied by the presence of
> >       capdata_fan
> > - Drop Reviewed-by & Tested-by from [PATCH v4 7/7] due to the changes,
> >   please review & test
> > - Link to v3: https://lore.kernel.org/r/20251031155349.24693-1-i@rong.moe/
> > 
> > Changes in v3:
> > - Fix grammar (thanks Derek J. Clark)
> > - Link to v2: https://lore.kernel.org/r/20251030193955.107148-1-i@rong.moe/
> > 
> > Changes in v2:
> > - Add a workaround for ACPI methods that return a 4B buffer for u32
> >   (thanks Armin Wolf)
> > - Fix function documentation (thanks kernel test bot)
> > - Reword documentation (thanks Derek J. Clark)
> > - Squash min/max reporting patch into the initial HWMON one (ditto)
> > - Query 0x04050000 for interface availability (ditto)
> >   - New parameter "expose_all_fans" to skip this check
> > - Enforce min/max RPM constraint on set (ditto)
> >   - New parameter "relax_fan_constraint" to disable this behavior
> >   - Drop parameter "ignore_fan_cap", superseded by the next one
> >   - New parameter "expose_all_fans" to expose fans w/o such data
> > - Assume auto mode on probe (ditto)
> > - Do not register HWMON device if no fan can be exposed
> > - fanX_target: Return -EBUSY instead of raw target value when fan stops
> > - Link to v1: https://lore.kernel.org/r/20251019210450.88830-1-i@rong.moe/
> > 
> > Rong Zhang (7):
> >   platform/x86: lenovo-wmi-helpers: Convert returned buffer into u32
> >   platform/x86: Rename lenovo-wmi-capdata01 to lenovo-wmi-capdata
> >   platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability
> >     Data
> >   platform/x86: lenovo-wmi-capdata: Add support for Capability Data 00
> >   platform/x86: lenovo-wmi-capdata: Add support for Fan Test Data
> >   platform/x86: lenovo-wmi-capdata: Wire up Fan Test Data
> >   platform/x86: lenovo-wmi-other: Add HWMON for fan reporting/tuning
> > 
> >  .../wmi/devices/lenovo-wmi-other.rst          |  43 +-
> >  drivers/platform/x86/lenovo/Kconfig           |   5 +-
> >  drivers/platform/x86/lenovo/Makefile          |   2 +-
> >  drivers/platform/x86/lenovo/wmi-capdata.c     | 812 ++++++++++++++++++
> >  drivers/platform/x86/lenovo/wmi-capdata.h     |  65 ++
> >  drivers/platform/x86/lenovo/wmi-capdata01.c   | 302 -------
> >  drivers/platform/x86/lenovo/wmi-capdata01.h   |  25 -
> >  drivers/platform/x86/lenovo/wmi-helpers.c     |  21 +-
> >  drivers/platform/x86/lenovo/wmi-other.c       | 515 ++++++++++-
> >  9 files changed, 1433 insertions(+), 357 deletions(-)
> >  create mode 100644 drivers/platform/x86/lenovo/wmi-capdata.c
> >  create mode 100644 drivers/platform/x86/lenovo/wmi-capdata.h
> >  delete mode 100644 drivers/platform/x86/lenovo/wmi-capdata01.c
> >  delete mode 100644 drivers/platform/x86/lenovo/wmi-capdata01.h
> > 
> > 
> > base-commit: ac3fd01e4c1efce8f2c054cdeb2ddd2fc0fb150d
> > --
> > 2.51.0
> > 
> 
> Rong,
> 
> Everything seems to be working as expected on unsupported devices.
> 
> [    4.398189] lenovo_wmi_other
> DC2A8805-3A8C-41BA-A6F7-092E0089CD3B-3: bound
> 362A3AFE-3D96-4665-8530-96DAD5BB300E-13 (ops lwmi_cd_component_ops
> [lenovo_wmi_capdata])
> [    4.398203] lenovo_wmi_other
> DC2A8805-3A8C-41BA-A6F7-092E0089CD3B-3: bound
> 7A8F5407-CB67-4D6E-B547-39B3BE018154-9 (ops lwmi_cd_component_ops
> [lenovo_wmi_capdata])
> [    4.398208] lenovo_wmi_other
> DC2A8805-3A8C-41BA-A6F7-092E0089CD3B-3: fan reporting/tuning is
> unsupported on this device
> 
> Obviously I can only test so far, but for where it makes sense on the
> series (through to 6/7) where it is missing you can add my tag
> Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
> 
> And for the entire series where it is missing:
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com

Thanks for your review and testing.

> As an aside, I'll likely need you to test a future patch series I'm
> working on for regressions after this is accepted as I will touch the
> hwmon interface a lot when adding fan curve auto points from the
> cd01/om interfaces.

Sure. Feel free to get in touch with me onlist or offlist when needed
;-)

> Thanks,
> - Derek

Thanks,
Rong