Re: [PATCH v6 5/7] platform/x86: lenovo-wmi-capdata: Add support for Fan Test Data

[Rong Zhang <i@rong.moe>]

Hi Ilpo,

On Wed, 2025-11-26 at 09:44 +0200, Ilpo Järvinen wrote:
> 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@rong.moe>
> > > > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > > > Tested-by: Derek J. Clark <derekjohn.clark@gmail.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@gmail.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.

__packed is not used to compact the structure layout here, but to tell
the compiler to emit[1] unaligned access to the ACPI buffer. See also
the change to [PATCH v3 1/6] suggested by Armin:
https://lore.kernel.org/r/a0b6d30b-3cba-4760-81dc-099e8fada7c0@gmx.de/

[1]: If CONFIG_UBSAN_ALIGNMENT=n, the assembly code should be the same
on x86. Otherwise, __packed effectively suppresses the alignment check
as unaligned access is expected.

Thanks,
Rong