Re: [PATCH 4/7] platform/x86: asus-wmi: read the firmware fan floor tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] platform/x86: asus-wmi: unhandled DSTS return in fan_curve_load_floors()
--

commit 2c4e7fb8cca0f513f1d86b6eff6061e09cba0940
Author: Bartu Alev <bartualev@gmail.com>

platform/x86: asus-wmi: read the firmware fan floor tables

This commit reads the firmware fan floor tables via DSTS methods at probe
and refreshes them when power profiles change or custom fan curves are
reset. The cached floor values are validated and protected by a mutex for
sysfs access.

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index c55f469cb03dc..b6541183d4568 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c

[ ... ]

> @@ -3651,7 +3664,136 @@ static int fan_curve_get_factory_default(struct asus_wmi *asus, u32 fan_dev)
>  	return 0;
>  }
>  
> -/* Check if capability exists, and populate defaults */
> +/*
> + * Read the firmware fan floor table. The EC enforces a minimum duty per
> + * curve point on top of the custom curve. Reading is best effort: at
> + * probe time a failed read leaves the table zeroed and invalid, and a
> + * failed refresh keeps the last table that was read.
> + */
> +static int fan_curve_load_floors(struct asus_wmi *asus, u32 fan_dev)
> +{
> +	struct fan_curve_data *curves;
> +	u8 buf[FAN_CURVE_BUF_LEN] = {};
> +	size_t len;
> +	int err, fan_idx, i;
> +
> +	switch (fan_dev) {
> +	case ASUS_WMI_DEVID_CPU_FAN_CURVE:
> +		fan_idx = FAN_CURVE_DEV_CPU;
> +		fan_dev = ASUS_WMI_DEVID_CPU_FAN_FLOOR;
> +		break;
> +	case ASUS_WMI_DEVID_GPU_FAN_CURVE:
> +		fan_idx = FAN_CURVE_DEV_GPU;
> +		fan_dev = ASUS_WMI_DEVID_GPU_FAN_FLOOR;
> +		break;
> +	case ASUS_WMI_DEVID_MID_FAN_CURVE:
> +		fan_idx = FAN_CURVE_DEV_MID;
> +		fan_dev = ASUS_WMI_DEVID_MID_FAN_FLOOR;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	curves = &asus->custom_fan_curves[fan_idx];
> +	if (curves->floors_unsupported)
> +		return -ENODEV;
> +
> +	err = asus_wmi_evaluate_method_buf(asus->dsts_id, fan_dev, 0, buf,
> +					   FAN_CURVE_BUF_LEN, &len);
> +	if (err) {
> +		pr_debug("%s (0x%08x) failed: %d\n", __func__, fan_dev, err);
> +		/* 0xFFFFFFFE means the firmware has no such method */
> +		if (err == -ENODEV)
> +			curves->floors_unsupported = true;
> +		return err;
> +	}

[ ... ]

> @@ -3668,6 +3810,16 @@ static int fan_curve_check_present(struct asus_wmi *asus, bool *available,
>  	}
>  
>  	*available = true;
> +
> +	err = fan_curve_load_floors(asus, fan_dev);
> +	if (err == -ENODEV)
> +		dev_dbg(&asus->platform_device->dev,
> +			"fan curve (0x%08x): no floor table\n", fan_dev);
> +	else if (err)
> +		dev_warn(&asus->platform_device->dev,
> +			 "fan curve (0x%08x): unusable floor table (%d)\n",
> +			 fan_dev, err);
> +
>  	return 0;
>  }

[Severity: Medium]
What happens in asus_wmi_evaluate_method_buf() and fan_curve_load_floors()
on laptops with custom fan curves whose firmware does not return
ASUS_WMI_UNSUPPORTED_METHOD (0xFFFFFFFE) for the new floor device IDs?

First, in drivers/platform/x86/asus-wmi.c, asus_wmi_evaluate_method_buf()
does not check whether output.pointer is NULL before dereferencing
obj->type:

drivers/platform/x86/asus-wmi.c:asus_wmi_evaluate_method_buf() {
	...
	status = asus_wmi_evaluate_method_locked(method_id, &input, &output);
	if (ACPI_FAILURE(status))
		return -EIO;

	obj = (union acpi_object *)output.pointer;

	switch (obj->type) {
	...
}

Unlike asus_wmi_evaluate_method3(), asus_wmi_evaluate_method5(),
show_dsts(), and show_call() in the same file (which all check if (!obj)),
if firmware returns AE_OK with no return object for an unknown method ID
(as noted in asus_wmi_platform_init() for some TUF series laptops), could
calling asus_wmi_evaluate_method_buf() via
asus_wmi_add() -> asus_wmi_custom_fan_curve_init() ->
fan_curve_check_present() -> fan_curve_load_floors() trigger a NULL pointer
dereference?

Second, when DSTS evaluation for an unimplemented floor method returns an
ACPI_TYPE_INTEGER other than 0xFFFFFFFE, asus_wmi_evaluate_method_buf()
only translates ASUS_WMI_UNSUPPORTED_METHOD to -ENODEV:

	case ACPI_TYPE_INTEGER:
		err = (u32)obj->integer.value;

		if (err == ASUS_WMI_UNSUPPORTED_METHOD)
			err = -ENODEV;
		/*
		 * At least one method returns a 0 with no buffer if no arg
		 * is provided, such as ASUS_WMI_DEVID_CPU_FAN_CURVE
		 */
		if (err == 0)
			err = -ENODATA;
		break;

On firmwares that return 0 (mapped to -ENODATA), 0xFFFFFFFF (which
asus_wmi_get_devstate() and show_dsts() treat as -ENODEV, but here becomes
-1), or ASUS_WMI_DSTS_UNKNOWN_BIT (0x2, returned as +2) for unsupported
DSTS device IDs, fan_curve_load_floors() will not set
curves->floors_unsupported = true, and fan_curve_check_present() will log a
false dev_warn() at boot and continue evaluating the unsupported WMI
methods on every fan_curve_refresh_floors() call.

Should asus_wmi_evaluate_method_buf() guard against !obj, and should
integer returns from DSTS in fan_curve_load_floors() also be treated as
unsupported?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-asus-fan-floors-v4-v1-0-b8d1c31a99a9@gmail.com?part=4