| 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: <d925730a3c11c9b1b6a76c9be9f61287c64fa329.camel@rong.moe>
Date: Mon, 27 Oct 2025 20:15:33 +0800
From: Rong Zhang <i@...g.moe>
To: Armin Wolf <W_Armin@....de>, Derek John Clark
<derekjohn.clark@...il.com>
Cc: Mark Pearson <mpearson-lenovo@...ebb.ca>, Hans de Goede
<hansg@...nel.org>, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com>, Guenter Roeck <linux@...ck-us.net>,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 4/6] platform/x86: lenovo-wmi-other: Add HWMON for fan
speed RPM
Hi Armin and Derek,
On Mon, 2025-10-27 at 00:04 +0100, Armin Wolf wrote:
> Am 26.10.25 um 20:42 schrieb Rong Zhang:
>
> > Hi Derek,
> >
> > On Sat, 2025-10-25 at 22:23 -0700, Derek John Clark wrote:
> > > On Sun, Oct 19, 2025 at 2:05 PM Rong Zhang <i@...g.moe> wrote:
> > > > Register an HWMON device for fan spped RPM according to Capability Data
> > > > 00 provided by lenovo-wmi-capdata. The corresponding HWMON nodes are:
> > > >
> > > > - fanX_enable: enable/disable the fan (tunable)
> > > > - fanX_input: current RPM
> > > > - fanX_target: target RPM (tunable)
> > > >
> > > > Signed-off-by: Rong Zhang <i@...g.moe>
> > > > ---
> > > > .../wmi/devices/lenovo-wmi-other.rst | 5 +
> > > > drivers/platform/x86/lenovo/Kconfig | 1 +
> > > > drivers/platform/x86/lenovo/wmi-other.c | 324 +++++++++++++++++-
> > > > 3 files changed, 317 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > > index adbd7943c6756..cb6a9bfe5a79e 100644
> > > > --- a/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > > +++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > > @@ -31,6 +31,11 @@ under the following path:
> > > >
> > > > /sys/class/firmware-attributes/lenovo-wmi-other/attributes/<attribute>/
> > > >
> > > > +Besides, this driver also exports fan speed RPM to HWMON:
> > > > + - fanX_enable: enable/disable the fan (tunable)
> > > > + - fanX_input: current RPM
> > > > + - fanX_target: target RPM (tunable)
> > > > +
> > > > LENOVO_CAPABILITY_DATA_00
> > > > -------------------------
> > > >
> > > > diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
> > > > index fb96a0f908f03..be9af04511462 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 20c6ff0be37a1..f8771ed3c6642 100644
> > > > --- a/drivers/platform/x86/lenovo/wmi-other.c
> > > > +++ b/drivers/platform/x86/lenovo/wmi-other.c
> > > > @@ -14,7 +14,15 @@
> > > > * These attributes typically don't fit anywhere else in the sysfs and are set
> > > > * in Windows using one of Lenovo's multiple user applications.
> > > > *
> > > > + * Besides, this driver also exports tunable fan speed RPM to HWMON.
> > > > + *
> > > > * 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
> > > > */
> > > >
> > > > #include <linux/acpi.h>
> > > > @@ -25,6 +33,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>
> > > > @@ -43,12 +52,20 @@
> > > >
> > > > #define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B"
> > > >
> > > > +#define LWMI_SUPP_VALID BIT(0)
> > > > +#define LWMI_SUPP_MAY_GET (LWMI_SUPP_VALID | BIT(1))
> > > > +#define LWMI_SUPP_MAY_SET (LWMI_SUPP_VALID | BIT(2))
> > > > +
> > > > #define LWMI_DEVICE_ID_CPU 0x01
> > > >
> > > > #define LWMI_FEATURE_ID_CPU_SPPT 0x01
> > > > #define LWMI_FEATURE_ID_CPU_SPL 0x02
> > > > #define LWMI_FEATURE_ID_CPU_FPPT 0x03
> > > >
> > > > +#define LWMI_DEVICE_ID_FAN 0x04
> > > > +
> > > > +#define LWMI_FEATURE_ID_FAN_RPM 0x03
> > > > +
> > > > #define LWMI_TYPE_ID_NONE 0x00
> > > >
> > > > #define LWMI_FEATURE_VALUE_GET 17
> > > > @@ -59,7 +76,18 @@
> > > > #define LWMI_ATTR_MODE_ID_MASK GENMASK(15, 8)
> > > > #define LWMI_ATTR_TYPE_ID_MASK GENMASK(7, 0)
> > > >
> > > > +/* Only fan1 and fan2 are present on supported devices. */
> > > > +#define LWMI_FAN_ID_BASE 1
> > > > +#define LWMI_FAN_NR 2
> > > > +#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_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);
> > > > @@ -76,15 +104,256 @@ 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;
> > > > + long target;
> > > > + } fan_info[LWMI_FAN_NR];
> > > > };
[...snip...]
> > > There is another method in capdata00 that could be useful here
> > >
> > > Fan Test For Diagnostic Software
> > > uint32 IDs //0x04050000
> > > uint32 Capability //9:by project
> > > bit 3: 0: not support LENOVO_FAN_TEST_DATA, 1 support LENOVO_FAN_TEST_DATA
> > > bit 2: 0: not support SetFeatureValue(), 1: support SetFeatureValue()
> > > bit 1: 0: not support GetFeatureValue(), 1: support GetFeatureValue()
> > > bit 0: 0: not support fan test for diagnostic software, 1: support an
> > > test for diagnostic software
> > The information is useful, thanks for that!
> >
> > A quick look at the decompiled ASL code of my device's ACPI tables:
> >
> > Package (0x03)
> > {
> > 0x04050000,
> > 0x07,
> > One
> > },
> >
> > I've confirmed that the corresponding ACPI method didn't modify the
> > return value of 0x04050000.
> >
> > 0x07 means my device supports this interface, GetFeatureValue() and
> > SetFeatureValue(); but does not support LENOVO_FAN_TEST_DATA. Is BIT(3)
> > only defined in some models (but not on my device)? The data returned
> > by LENOVO_FAN_TEST_DATA seems correct and is probably the min/max auto
> > points.
>
> Can you please use this information instead of wmi_has_guid() when matching the
> components? I would prefer if we can phase out wmi_has_guid() eventually.
Doing so leads to a chicken-or-the-egg paradox as long as we use the
component helper:
- Calling lwmi_cd00_get_data() from wmi-other requires wmi-capdata
being bound to get a reference to cd00_list. Binding wmi-capdata again
to get a reference to cd_fan_list implies that the HWMON device can
only be registered in the driver probe callback instead of the master
bind callback, but the unbind callback still needs to check and
unregister it, which is really ugly.
- Calling lwmi_cd00_get_data() from wmi-capdata requires variables in
the module-level global scope to store references to cd_list. Doing so
completely makes the component helper meaningless for our use case.
Meanwhile, if we did't use the component helper at all, we would need
neither this information nor wmi_has_guid().
This information itself may also be inconsistent, e.g., it says my
device does not support LENOVO_FAN_TEST_DATA, but the GUID actually
exists and its data makes sense.
Moreover, capdata00 and capdata01 are irrelevant to each other. My
implementation is capable to work properly on devices with only one of
them (I am not sure if such devices exist, though). This again requires
wmi_has_guid() as it's the only way to determine their existence.
Do you think it's a good idea to drop the component approach? If so, I
will implement this in v2 (or v3, if I finish and send out v2 soon).
Thanks,
Rong
> Thanks,
> Armin Wolf
>
> >
> > My device didn't implement {Get,Set}FeatureValue(0x04050000). What will
> > it do when it's implemented?
[...snip...]
> > > >
> > > > +/* ======== Self (master: lenovo-wmi-other) ======== */
> > > > +
> > > > /**
> > > > * lwmi_om_master_bind() - Bind all components of the other mode driver
> > > > * @dev: The lenovo-wmi-other driver basic device.
> > > > *
> > > > - * Call component_bind_all to bind the lenovo-wmi-capdata01 driver to the
> > > > - * lenovo-wmi-other master driver. On success, assign the capability data 01
> > > > - * list pointer to the driver data struct for later access. This pointer
> > > > - * is only valid while the capdata01 interface exists. Finally, register all
> > > > - * firmware attribute groups.
> > > > + * Call component_bind_all to bind the lenovo-wmi-capdata devices to the
> > > > + * lenovo-wmi-other master driver. On success, assign the capability data
> > > > + * list pointers to the driver data struct for later access. These pointers
> > > > + * are only valid while the capdata interfaces exist. Finally, register the
> > > > + * HWMON device and all firmware attribute groups.
> > > > *
> > > > * Return: 0 on success, or an error code.
> > > > */
> > > > @@ -586,26 +857,47 @@ static int lwmi_om_master_bind(struct device *dev)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - priv->cd01_list = binder.cd01_list;
> > > > - if (!priv->cd01_list)
> > > > + if (!binder.cd00_list && !binder.cd01_list)
> > > > return -ENODEV;
> > > >
> > > > - return lwmi_om_fw_attr_add(priv);
> > > > + priv->cd00_list = binder.cd00_list;
> > > > + if (priv->cd00_list) {
> > > > + ret = lwmi_om_hwmon_add(priv);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + priv->cd01_list = binder.cd01_list;
> > > > + if (priv->cd01_list) {
> > > > + ret = lwmi_om_fw_attr_add(priv);
> > > > + if (ret) {
> > > > + if (priv->cd00_list)
> > > > + lwmi_om_hwmon_remove(priv);
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > + return 0;
> > > > }
> > > >
> > > > /**
> > > > * lwmi_om_master_unbind() - Unbind all components of the other mode driver
> > > > * @dev: The lenovo-wmi-other driver basic device
> > > > *
> > > > - * Unregister all capability data attribute groups. Then call
> > > > - * component_unbind_all to unbind the lenovo-wmi-capdata01 driver from the
> > > > - * lenovo-wmi-other master driver. Finally, free the IDA for this device.
> > > > + * Unregister the HWMON device and all capability data attribute groups. Then
> > > > + * call component_unbind_all to unbind the lenovo-wmi-capdata driver from the
> > > > + * lenovo-wmi-other master driver.
> > > > */
> > > > static void lwmi_om_master_unbind(struct device *dev)
> > > > {
> > > > struct lwmi_om_priv *priv = dev_get_drvdata(dev);
> > > >
> > > > - lwmi_om_fw_attr_remove(priv);
> > > > + if (priv->cd00_list)
> > > > + lwmi_om_hwmon_remove(priv);
> > > > +
> > > > + if (priv->cd01_list)
> > > > + lwmi_om_fw_attr_remove(priv);
> > > > +
> > > > component_unbind_all(dev, NULL);
> > > > }
> > > >
> > > > @@ -624,6 +916,9 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
> > > > if (!priv)
> > > > return -ENOMEM;
> > > >
> > > > + /* Sentinel for on-demand ida_free(). */
> > > > + priv->ida_id = -EIDRM;
> > > > +
> > > > priv->wdev = wdev;
> > > > dev_set_drvdata(&wdev->dev, priv);
> > > >
> > > > @@ -654,7 +949,9 @@ static void lwmi_other_remove(struct wmi_device *wdev)
> > > > struct lwmi_om_priv *priv = dev_get_drvdata(&wdev->dev);
> > > >
> > > > component_master_del(&wdev->dev, &lwmi_om_master_ops);
> > > > - ida_free(&lwmi_om_ida, priv->ida_id);
> > > > +
> > > > + if (priv->ida_id >= 0)
> > > > + ida_free(&lwmi_om_ida, priv->ida_id);
> > > > }
> > > >
> > > > static const struct wmi_device_id lwmi_other_id_table[] = {
> > > > @@ -679,5 +976,6 @@ MODULE_IMPORT_NS("LENOVO_WMI_CD");
> > > > MODULE_IMPORT_NS("LENOVO_WMI_HELPERS");
> > > > MODULE_DEVICE_TABLE(wmi, lwmi_other_id_table);
> > > > MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@...il.com>");
> > > > +MODULE_AUTHOR("Rong Zhang <i@...g.moe>");
> > > > MODULE_DESCRIPTION("Lenovo Other Mode WMI Driver");
> > > > MODULE_LICENSE("GPL");
> > > > --
> > > > 2.51.0
> > > >
Powered by blists - more mailing lists