| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6f732364-9839-7845-b408-fad6fd2464d8@linux.intel.com>
Date: Wed, 26 Nov 2025 10:04:39 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Rong Zhang <i@...g.moe>
cc: Mark Pearson <mpearson-lenovo@...ebb.ca>,
"Derek J. Clark" <derekjohn.clark@...il.com>, Armin Wolf <W_Armin@....de>,
Hans de Goede <hansg@...nel.org>, Guenter Roeck <linux@...ck-us.net>,
platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v6 7/7] platform/x86: lenovo-wmi-other: Add HWMON for
fan reporting/tuning
On Tue, 25 Nov 2025, Rong Zhang wrote:
> Hi Ilpo,
>
> Thanks for your review.
>
> On Mon, 2025-11-24 at 18:58 +0200, Ilpo Järvinen wrote:
> > On Sun, 23 Nov 2025, Rong Zhang wrote:
> >
> > > Register an HWMON device for fan reporting/tuning according to
> > > Capability Data 00 (capdata00) and Fan Test Data (capdata_fan) provided
> > > by lenovo-wmi-capdata. The corresponding HWMON nodes are:
> > >
> > > - fanX_enable: enable/disable the fan (tunable)
> > > - fanX_input: current RPM
> > > - fanX_max: maximum RPM
> > > - fanX_min: minimum RPM
> > > - fanX_target: target RPM (tunable)
> > >
> > > Information from capdata00 and capdata_fan are used to control the
> > > visibility and constraints of HWMON attributes. Fan info from capdata00
> > > is collected on bind, while fan info from capdata_fan is collected in a
> > > callback. Once all fan info is collected, register the HWMON device.
> > >
> > > Signed-off-by: Rong Zhang <i@...g.moe>
> > > ---
> > > Changes in v4:
> > > - Rework HWMON registration due to the introduction of [PATCH v4 6/7]
> > > - 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 presense of
> > > capdata_fan
> > > - Drop Reviewed-by & Tested-by due to the changes, please review & test
> > >
> > > Changes in v3:
> > > - Reword documentation (thanks Derek J. Clark)
> > >
> > > Changes in v2:
> > > - Define 4 fan channels instead of 2 (thanks Derek J. Clark)
> > > - Squash min/max reporting patch into this 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)
> > > - Reword documentation (ditto)
> > > - Do not register HWMON device if no fan can be exposed
> > > - fanX_target: Return -EBUSY instead of raw target value when fan stops
> > > ---
> > > .../wmi/devices/lenovo-wmi-other.rst | 11 +
> > > drivers/platform/x86/lenovo/Kconfig | 1 +
> > > drivers/platform/x86/lenovo/wmi-other.c | 485 +++++++++++++++++-
> > > 3 files changed, 487 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > index 821282e07d93..bd1d733ff286 100644
> > > --- a/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > +++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > @@ -31,6 +31,8 @@ under the following path:
> > >
> > > /sys/class/firmware-attributes/lenovo-wmi-other/attributes/<attribute>/
> > >
> > > +Additionally, this driver also exports attributes to HWMON.
> > > +
> > > LENOVO_CAPABILITY_DATA_00
> > > -------------------------
> > >
> > > @@ -39,6 +41,11 @@ WMI GUID ``362A3AFE-3D96-4665-8530-96DAD5BB300E``
> > > The LENOVO_CAPABILITY_DATA_00 interface provides various information that
> > > does not rely on the gamezone thermal mode.
> > >
> > > +The following HWMON attributes are implemented:
> > > + - fanX_enable: enable/disable the fan (tunable)
> > > + - fanX_input: current RPM
> > > + - fanX_target: target RPM (tunable)
> > > +
> > > LENOVO_CAPABILITY_DATA_01
> > > -------------------------
> > >
> > > @@ -70,6 +77,10 @@ WMI GUID ``B642801B-3D21-45DE-90AE-6E86F164FB21``
> > > The LENOVO_FAN_TEST_DATA interface provides reference data for self-test of
> > > cooling fans.
> > >
> > > +The following HWMON attributes are implemented:
> > > + - fanX_max: maximum RPM
> > > + - fanX_min: minimum RPM
> > > +
> > > WMI interface description
> > > =========================
> > >
> > > diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
> > > index fb96a0f908f0..be9af0451146 100644
> > > --- a/drivers/platform/x86/lenovo/Kconfig
> > > +++ b/drivers/platform/x86/lenovo/Kconfig
> > > @@ -263,6 +263,7 @@ config LENOVO_WMI_GAMEZONE
> > > config LENOVO_WMI_TUNING
> > > tristate "Lenovo Other Mode WMI Driver"
> > > depends on ACPI_WMI
> > > + select HWMON
> > > select FW_ATTR_CLASS
> > > select LENOVO_WMI_DATA
> > > select LENOVO_WMI_EVENTS
> > > diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> > > index b3adcc2804fa..ce1ca13db4b5 100644
> > > --- a/drivers/platform/x86/lenovo/wmi-other.c
> > > +++ b/drivers/platform/x86/lenovo/wmi-other.c
> > > @@ -14,7 +14,16 @@
> > > * These attributes typically don't fit anywhere else in the sysfs and are set
> > > * in Windows using one of Lenovo's multiple user applications.
> > > *
> > > + * Additionally, this driver also exports tunable fan speed RPM to HWMON.
> > > + * Min/max RPM are also provided for reference.
> > > + *
> > > * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@...il.com>
> > > + * - fw_attributes
> > > + * - binding to Capability Data 01
> > > + *
> > > + * Copyright (C) 2025 Rong Zhang <i@...g.moe>
> > > + * - HWMON
> > > + * - binding to Capability Data 00 and Fan
> > > */
> > >
> > > #include <linux/acpi.h>
> > > @@ -25,6 +34,7 @@
> > > #include <linux/device.h>
> > > #include <linux/export.h>
> > > #include <linux/gfp_types.h>
> > > +#include <linux/hwmon.h>
> > > #include <linux/idr.h>
> > > #include <linux/kdev_t.h>
> > > #include <linux/kobject.h>
> > > @@ -49,12 +59,26 @@
> > > #define LWMI_FEATURE_ID_CPU_SPL 0x02
> > > #define LWMI_FEATURE_ID_CPU_FPPT 0x03
> > >
> > > +#define LWMI_FEATURE_ID_FAN_RPM 0x03
> > > +
> > > #define LWMI_TYPE_ID_NONE 0x00
> > >
> > > #define LWMI_FEATURE_VALUE_GET 17
> > > #define LWMI_FEATURE_VALUE_SET 18
> > >
> > > +#define LWMI_FAN_ID_BASE 1
> > > +#define LWMI_FAN_NR 4
> > > +#define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
> > > +
> > > +#define LWMI_ATTR_ID_FAN_RPM(x) \
> > > + (FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) | \
> > > + FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_RPM) | \
> > > + FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_FAN_ID(x)))
> > > +
> > > +#define LWMI_FAN_STOP_RPM 1
> > > +
> > > #define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
> > > +#define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
> > >
> > > static BLOCKING_NOTIFIER_HEAD(om_chain_head);
> > > static DEFINE_IDA(lwmi_om_ida);
> > > @@ -71,15 +95,439 @@ struct lwmi_om_priv {
> > > struct component_master_ops *ops;
> > >
> > > /* only valid after capdata bind */
> > > + struct cd_list *cd00_list;
> > > struct cd_list *cd01_list;
> > >
> > > + struct device *hwmon_dev;
> > > struct device *fw_attr_dev;
> > > struct kset *fw_attr_kset;
> > > struct notifier_block nb;
> > > struct wmi_device *wdev;
> > > int ida_id;
> > > +
> > > + struct fan_info {
> > > + u32 supported;
> > > + u32 last_target;
> > > + long min_rpm;
> > > + long max_rpm;
> > > + } fan_info[LWMI_FAN_NR];
> >
> > I personally don't like this style at all because it makes finding the
> > type of the variable harder with grep.
>
> Make sense. Will add a `lwmi_' prefix to the struct name. Thanks.
While having prefix is good too, I was more referring to naming it at the
closing brace line, I'd prefer to have it on a separate line (which shows
type, staticness easily with grep):
struct lwmi_fan_info fan_info[LWMI_FAN_NR];
I see you placed it into priv in v7 which is better anyway.
--
i.
Powered by blists - more mailing lists