| 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: <79934289-cdb4-385d-8042-e96ec37fdb55@linux.intel.com>
Date: Wed, 26 Nov 2025 09:44:45 +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 5/7] platform/x86: lenovo-wmi-capdata: Add support
for Fan Test Data
On Tue, 25 Nov 2025, Rong Zhang wrote:
> On Mon, 2025-11-24 at 18:45 +0200, Ilpo Järvinen wrote:
> > On Sun, 23 Nov 2025, Rong Zhang wrote:
> >
> > > Add support for LENOVO_FAN_TEST_DATA WMI data block. Provides an
> > > interface for querying the min/max fan speed RPM (reference data) of a
> > > given fan ID.
> > >
> > > This interface is optional. Hence, it does not bind to lenovo-wmi-other
> > > and is not registered as a component for the moment. Appropriate binding
> > > will be implemented in the subsequent patch.
> > >
> > > Signed-off-by: Rong Zhang <i@...g.moe>
> > > Reviewed-by: Derek J. Clark <derekjohn.clark@...il.com>
> > > Tested-by: Derek J. Clark <derekjohn.clark@...il.com>
> > > ---
> > > Changes in v4:
> > > - Rebase on top of changes made to [PATCH v4 3/7]
> > > - Do not register it as a component until [PATCH v4 6/7]
> > >
> > > Changes in v2:
> > > - Reword documentation
> > > ---
> > > .../wmi/devices/lenovo-wmi-other.rst | 17 +++
> > > drivers/platform/x86/lenovo/wmi-capdata.c | 102 ++++++++++++++++++
> > > drivers/platform/x86/lenovo/wmi-capdata.h | 7 ++
> > > 3 files changed, 126 insertions(+)
> > >
> > > diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > index fcad595d49af..821282e07d93 100644
> > > --- a/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > +++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
> > > @@ -62,6 +62,13 @@ The following firmware-attributes are implemented:
> > > - ppt_pl2_sppt: Platform Profile Tracking Slow Package Power Tracking
> > > - ppt_pl3_fppt: Platform Profile Tracking Fast Package Power Tracking
> > >
> > > +LENOVO_FAN_TEST_DATA
> > > +-------------------------
> > > +
> > > +WMI GUID ``B642801B-3D21-45DE-90AE-6E86F164FB21``
> > > +
> > > +The LENOVO_FAN_TEST_DATA interface provides reference data for self-test of
> > > +cooling fans.
> > >
> > > WMI interface description
> > > =========================
> > > @@ -115,3 +122,13 @@ data using the `bmfdec <https://github.com/pali/bmfdec>`_ utility:
> > > [WmiDataId(3), read, Description("Data Size.")] uint32 DataSize;
> > > [WmiDataId(4), read, Description("Default Value"), WmiSizeIs("DataSize")] uint8 DefaultValue[];
> > > };
> > > +
> > > + [WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"), Description("Definition of Fan Test Data"), guid("{B642801B-3D21-45DE-90AE-6E86F164FB21}")]
> > > + class LENOVO_FAN_TEST_DATA {
> > > + [key, read] string InstanceName;
> > > + [read] boolean Active;
> > > + [WmiDataId(1), read, Description("Mode.")] uint32 NumOfFans;
> > > + [WmiDataId(2), read, Description("Fan ID."), WmiSizeIs("NumOfFans")] uint32 FanId[];
> > > + [WmiDataId(3), read, Description("Maximum Fan Speed."), WmiSizeIs("NumOfFans")] uint32 FanMaxSpeed[];
> > > + [WmiDataId(4), read, Description("Minumum Fan Speed."), WmiSizeIs("NumOfFans")] uint32 FanMinSpeed[];
> > > + };
> > > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> > > index 29267c373ab3..e6392357395c 100644
> > > --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> > > +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> > > @@ -13,6 +13,10 @@
> > > * attribute has multiple pages, one for each of the thermal modes managed by
> > > * the Gamezone interface.
> > > *
> > > + * Fan Test Data includes the max/min fan speed RPM for each fan. This is
> > > + * reference data for self-test. If the fan is in good condition, it is capable
> > > + * to spin faster than max RPM or slower than min RPM.
> > > + *
> > > * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@...il.com>
> > > * - Initial implementation (formerly named lenovo-wmi-capdata01)
> > > *
> > > @@ -41,6 +45,7 @@
> > >
> > > #define LENOVO_CAPABILITY_DATA_00_GUID "362A3AFE-3D96-4665-8530-96DAD5BB300E"
> > > #define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
> > > +#define LENOVO_FAN_TEST_DATA_GUID "B642801B-3D21-45DE-90AE-6E86F164FB21"
> > >
> > > #define ACPI_AC_CLASS "ac_adapter"
> > > #define ACPI_AC_NOTIFY_STATUS 0x80
> > > @@ -48,6 +53,7 @@
> > > enum lwmi_cd_type {
> > > LENOVO_CAPABILITY_DATA_00,
> > > LENOVO_CAPABILITY_DATA_01,
> > > + LENOVO_FAN_TEST_DATA,
> > > };
> > >
> > > #define LWMI_CD_TABLE_ITEM(_type) \
> > > @@ -62,6 +68,7 @@ static const struct lwmi_cd_info {
> > > } lwmi_cd_table[] = {
> > > LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_00),
> > > LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
> > > + LWMI_CD_TABLE_ITEM(LENOVO_FAN_TEST_DATA),
> > > };
> > >
> > > struct lwmi_cd_priv {
> > > @@ -78,6 +85,7 @@ struct cd_list {
> > > union {
> > > DECLARE_FLEX_ARRAY(struct capdata00, cd00);
> > > DECLARE_FLEX_ARRAY(struct capdata01, cd01);
> > > + DECLARE_FLEX_ARRAY(struct capdata_fan, cd_fan);
> > > };
> > > };
> > >
> > > @@ -117,6 +125,10 @@ void lwmi_cd_match_add_all(struct device *master, struct component_match **match
> > > return;
> > >
> > > for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
> > > + /* Skip optional interfaces. */
> > > + if (lwmi_cd_table[i].type == LENOVO_FAN_TEST_DATA)
> > > + continue;
> > > +
> > > component_match_add(master, matchptr, lwmi_cd_match,
> > > (void *)&lwmi_cd_table[i].type);
> > > if (IS_ERR(*matchptr))
> > > @@ -194,6 +206,9 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd00_get_data, "LENOVO_WMI_CD");
> > > DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
> > > EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
> > >
> > > +DEF_LWMI_CDXX_GET_DATA(cd_fan, LENOVO_FAN_TEST_DATA, struct capdata_fan);
> > > +EXPORT_SYMBOL_NS_GPL(lwmi_cd_fan_get_data, "LENOVO_WMI_CD");
> > > +
> > > /**
> > > * lwmi_cd_cache() - Cache all WMI data block information
> > > * @priv: lenovo-wmi-capdata driver data.
> > > @@ -217,6 +232,9 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> > > p = &priv->list->cd01[0];
> > > size = sizeof(priv->list->cd01[0]);
> > > break;
> > > + case LENOVO_FAN_TEST_DATA:
> > > + /* Done by lwmi_cd_alloc() => lwmi_cd_fan_list_alloc_cache(). */
> > > + return 0;
> > > default:
> > > return -EINVAL;
> > > }
> > > @@ -239,6 +257,78 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> > > return 0;
> > > }
> > >
> > > +/**
> > > + * lwmi_cd_fan_list_alloc_cache() - Alloc and cache Fan Test Data list
> > > + * @priv: lenovo-wmi-capdata driver data.
> > > + * @listptr: Pointer to returned cd_list pointer.
> > > + *
> > > + * Return: count of fans found, or an error.
> > > + */
> > > +static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv, struct cd_list **listptr)
> > > +{
> > > + u32 count, *fan_ids, *fan_min_rpms, *fan_max_rpms;
> > > + union acpi_object *ret_obj __free(kfree) = NULL;
> >
> > Since you're using __free(), please move this to where you assign the
> > value. This is to create a pattern with cleanup helpers. The cleanup
> > order depends on the order the variables are introduced which in some
> > other cases may be significant.
>
> Make sense. Will move it to the last declaration lines with its
> assignment. Thanks.
>
> > > + struct block { u32 nr; u32 data[]; } *block;
> >
> > This is the first time I see this style anywhere in the kernel's context,
> > has there been some general discussion about this style somewhere?
> >
> > At least it seems immediately obvious to me that this style will have a
> > negative impact on documentability due to (too) concise use of space.
>
> Make sense. Will break it into multiple lines. Thanks.
>
> > > + struct cd_list *list;
> > > + size_t size;
> > > + int idx;
> > > +
> > > + ret_obj = wmidev_block_query(priv->wdev, 0);
> > > + if (!ret_obj)
> > > + return -ENODEV;
> > > +
> > > + /*
> > > + * This is usually caused by a dummy ACPI method. Do not return an error
> > > + * as failing to probe this device will result in master driver being
> > > + * unbound - this behavior aligns with lwmi_cd_cache().
> > > + */
> > > + if (ret_obj->type != ACPI_TYPE_BUFFER) {
> > > + count = 0;
> > > + goto alloc;
> > > + }
> > > +
> > > + size = ret_obj->buffer.length;
> > > + block = (struct block *)ret_obj->buffer.pointer;
> >
> > void * can be cast implicitly.
>
> `ret_obj->buffer.pointer' is a `u8 *' pointer so the cast is mandatory.
Ah, right. I think I even tried to check it but probably picked up
the void *pointer from struct acpi_buffer instead of the correct one.
> Hmmm, this reminds me that `struct block' probably needs a `__packed'
> to generate unaligned access to it.
In all u32 struct, I don't think __packed is required. You can use pahole
tool to check the layout if you want, but I don't think compiler adds
any gaps with only u32s.
--
i.
Powered by blists - more mailing lists