Re: [PATCH v4 07/12] platform/x86: lenovo-wmi-capdata: Adopt new WMI API

[Rong Zhang <i@rong.moe>]

Hi Ilpo,

Thanks for your review.

On Mon, 2026-10-19 at 09:46 +0300, Ilpo Järvinen wrote:
> On Sun, 18 Oct 2026, Rong Zhang wrote:
> 
> > The new WMI API supports multiple ACPI types by converting them into a
> > unified buffer that satisfies alignment and size requirements.
> > 
> > Adopt it to make our life easier.
> > 
> > Note that the new WMI API only accepts a few ACPI types to conform to
> > the behavior of the Windows WMI-ACPI driver. By adopting the new API, we
> > intentionally rejects improper ACPI types instead of silently ignoring
> > them.
> > 
> > Meanwhile, considering that `struct_size(block, data, count * 3)' may
> > overflow when calculating `count * 3', ignore Fan Test Data with count >
> > U8_MAX instead of capping `count'.
> > 
> > Reviewed-by: Armin Wolf <W_Armin@gmx.de>
> > Tested-by: Navon John Lukose <navonjohnlukose@gmail.com>
> > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> > Changes in v4:
> > - Fix typo (thanks Mark Pearson)
> > 
> > Changes in v3:
> > - Leave struct cd_fan_block's __packed attribute as is (thanks Armin
> >   Wolf)
> > ---
> >  drivers/platform/x86/lenovo/wmi-capdata.c | 70 ++++++++++++++-----------------
> >  1 file changed, 31 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> > index 3c09bb9e3d46..abf5ffda7163 100644
> > --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> > +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> > @@ -626,17 +626,19 @@ static int __lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >  	}
> >  
> >  	for (idx = 0; idx < priv->list->count; idx++, p += size) {
> > -		union acpi_object *ret_obj __free(kfree) = NULL;
> > +		struct wmi_buffer wbuf;
> > +		int ret;
> >  
> > -		ret_obj = wmidev_block_query(priv->wdev, idx);
> > -		if (!ret_obj)
> > -			return -ENODEV;
> > -
> > -		if (ret_obj->type != ACPI_TYPE_BUFFER ||
> > -		    ret_obj->buffer.length < size)
> > +		ret = wmidev_query_block(priv->wdev, idx, &wbuf, size);
> > +		if (ret == -ENODATA) /* The block is too short, probably stubbed. */
> >  			continue;
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Capdata 01 is an extension to capdata 00. */
> > +		struct capdata00 *capdata __free(kfree) = wbuf.data;
> >  
> > -		memcpy(p, ret_obj->buffer.pointer, size);
> > +		memcpy(p, capdata, size);
> >  	}
> >  
> >  	return 0;
> > @@ -678,43 +680,34 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >   */
> >  static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv)
> >  {
> > +	struct wmi_buffer wbuf;
> >  	struct cd_list *list;
> > -	size_t size;
> > +	int ret, idx;
> >  	u32 count;
> > -	int idx;
> >  
> > -	/* Emit unaligned access to u8 buffer with __packed. */
> >  	struct cd_fan_block {
> >  		u32 nr;
> >  		u32 data[]; /* id[nr], max_rpm[nr], min_rpm[nr] */
> > -	} __packed * block;
> > +	} __packed;
> 
> Could this just be moved outside of the function?

Makes sense. Will submit a v5 soon.

Thanks,
Rong

> 
> > -	union acpi_object *ret_obj __free(kfree) = wmidev_block_query(priv->wdev, 0);
> > -	if (!ret_obj)
> > -		return -ENODEV;
> > +	ret = wmidev_query_block(priv->wdev, 0, &wbuf, sizeof(struct cd_fan_block));
> > +	if (ret == -ENODATA) /* The block is too short, probably stubbed. */
> > +		return 0;
> > +	if (ret)
> > +		return ret;
> >  
> > -	if (ret_obj->type == ACPI_TYPE_BUFFER) {
> > -		block = (struct cd_fan_block *)ret_obj->buffer.pointer;
> > -		size = ret_obj->buffer.length;
> > +	struct cd_fan_block *block __free(kfree) = wbuf.data;
> >  
> > -		count = size >= sizeof(*block) ? block->nr : 0;
> > -		if (size < struct_size(block, data, count * 3)) {
> > -			dev_warn(&priv->wdev->dev,
> > -				 "incomplete fan test data block: %zu < %zu, ignoring\n",
> > -				 size, struct_size(block, data, count * 3));
> > -			count = 0;
> > -		} else if (count > U8_MAX) {
> > -			dev_warn(&priv->wdev->dev,
> > -				 "too many fans reported: %u > %u, truncating\n",
> > -				 count, U8_MAX);
> > -			count = U8_MAX;
> > -		}
> > -	} else {
> > -		/*
> > -		 * This is usually caused by a dummy ACPI method. Do not return an error
> > -		 * as failing to probe this device will result in sub-master device being
> > -		 * unbound. This behavior aligns with lwmi_cd_cache().
> > -		 */
> > +	count = block->nr;
> > +
> > +	if (count > U8_MAX) {
> > +		dev_warn(&priv->wdev->dev,
> > +			 "too many fans reported: %u > %u, ignoring\n", count, U8_MAX);
> > +		count = 0;
> > +	} else if (wbuf.length < struct_size(block, data, count * 3)) {
> > +		dev_warn(&priv->wdev->dev,
> > +			 "incomplete fan test data block: %zu < %zu (%u fans), ignoring\n",
> > +			 wbuf.length, struct_size(block, data, count * 3), count);
> >  		count = 0;
> >  	}
> >  
> > @@ -729,11 +722,10 @@ static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv)
> >  	priv->list = list;
> >  
> >  	for (idx = 0; idx < count; idx++) {
> > -		/* Do not calculate array index using count, as it may be truncated. */
> >  		list->cd_fan[idx] = (struct capdata_fan) {
> >  			.id      = block->data[idx],
> > -			.max_rpm = block->data[idx + block->nr],
> > -			.min_rpm = block->data[idx + (2 * block->nr)],
> > +			.max_rpm = block->data[idx + count],
> > +			.min_rpm = block->data[idx + (2 * count)],
> >  		};
> >  	}
> >  
> > 
> >