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

[Rong Zhang]

Hi Armin,

Thanks for your review.


于 2026年10月10日 GMT+08:00 09:16:48，Armin Wolf <W_Armin@xxxxxx> 写道：
> Am 09.10.26 um 14:53 schrieb Rong Zhang:
> 
> > 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 caping `count'.
> > 
> > Signed-off-by: Rong Zhang <i@xxxxxxxx>
> > ---
> >   drivers/platform/x86/lenovo/wmi-capdata.c | 71 ++++++++++++++-----------------
> >   1 file changed, 32 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> > index de8044ef68b8..d70fe4504fc5 100644
> > --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> > +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> > @@ -628,17 +628,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;
> > @@ -680,43 +682,35 @@ 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;
> > +	};
> 
> Hi,
> 
> i suggest you keep the __packed here. With this being addressed:

Hmm, may I ask why?

The data itself is never packed, the only use of __packed was to emit
unaligned access.

WMI driver development guide says:

    Since the WMI driver core guarantees that all buffers received from
    a WMI device are aligned on an 8-byte boundary, WMI drivers can
    simply perform a cast.

Thus, the buffer has a better alignment than sizeof(u32).

Thanks,
Rong

> 
> Reviewed-by: Armin Wolf <W_Armin@xxxxxx>
> 
> > +
> > +	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;
> >   -	union acpi_object *ret_obj __free(kfree) = wmidev_block_query(priv->wdev, 0);
> > -	if (!ret_obj)
> > -		return -ENODEV;
> > +	struct cd_fan_block *block __free(kfree) = wbuf.data;
> >   -	if (ret_obj->type == ACPI_TYPE_BUFFER) {
> > -		block = (struct cd_fan_block *)ret_obj->buffer.pointer;
> > -		size = ret_obj->buffer.length;
> > +	count = block->nr;
> >   -		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().
> > -		 */
> > +	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;
> >   	}
> >   @@ -731,11 +725,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)],
> >   		};
> >   	}
> >