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

[Armin Wolf <W_Armin@gmx.de>]

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@rong.moe>
> ---
>   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:

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> +
> +	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)],
>   		};
>   	}
>   
>