Re: [PATCH v2 1/4] platform/x86: acer-wmi: Fix setting of fan behavior

[Armin Wolf <W_Armin@gmx.de>]

Am 24.09.25 um 13:00 schrieb Ilpo Järvinen:

> On Tue, 23 Sep 2025, Armin Wolf wrote:
>
>> After studying the linuwu_sense driver
>> (https://github.com/0x7375646F/Linuwu-Sense) i was able to understand
>> the meaning of the SetGamingFanBehavior() WMI method:
>>
>> - the first 16-bit are a bitmap of all fans affected by a fan behavior
>>    change request.
>>
>> - the next 8 bits contain four fan mode fields (2-bit), each being
>>    associated with a bit inside the fan bitmap.
>>
>> There are three fan modes: auto, turbo and custom.
>>
>> Use this newfound knowledge to fix the turbo fan handling by setting
>> the correct bits before calling SetGamingFanBehavior(). Also check
>> the result of the WMI method call and return an error should the ACPI
>> firmware signal failure.
>>
>> Reviewed-by: Kurt Borja <kuurtb@gmail.com>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/acer-wmi.c | 76 +++++++++++++++++++++++----------
>>   1 file changed, 53 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> index 69336bd778ee..a41555ee8589 100644
>> --- a/drivers/platform/x86/acer-wmi.c
>> +++ b/drivers/platform/x86/acer-wmi.c
>> @@ -68,10 +68,19 @@ MODULE_LICENSE("GPL");
>>   #define ACER_WMID_SET_GAMING_LED_METHODID 2
>>   #define ACER_WMID_GET_GAMING_LED_METHODID 4
>>   #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
>> -#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
>> +#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
>>   #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>>   #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>>   
>> +#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
>> +#define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
>> +
>> +#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
>> +#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
>> +
>> +#define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
>> +#define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
>> +
>>   #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
>>   #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
>>   #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
>> @@ -121,6 +130,12 @@ enum acer_wmi_predator_v4_sensor_id {
>>   	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
>>   };
>>   
>> +enum acer_wmi_gaming_fan_mode {
>> +	ACER_WMID_FAN_MODE_AUTO		= 0x01,
>> +	ACER_WMID_FAN_MODE_TURBO	= 0x02,
>> +	ACER_WMID_FAN_MODE_CUSTOM	= 0x03,
>> +};
>> +
>>   enum acer_wmi_predator_v4_oc {
>>   	ACER_WMID_OC_NORMAL			= 0x0000,
>>   	ACER_WMID_OC_TURBO			= 0x0002,
>> @@ -1565,9 +1580,6 @@ static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
>>   	case ACER_CAP_TURBO_LED:
>>   		method_id = ACER_WMID_SET_GAMING_LED_METHODID;
>>   		break;
>> -	case ACER_CAP_TURBO_FAN:
>> -		method_id = ACER_WMID_SET_GAMING_FAN_BEHAVIOR;
>> -		break;
>>   	default:
>>   		return AE_BAD_PARAMETER;
>>   	}
>> @@ -1618,25 +1630,43 @@ static int WMID_gaming_get_sys_info(u32 command, u64 *out)
>>   	return 0;
>>   }
>>   
>> +static int WMID_gaming_set_fan_behavior(u16 fan_bitmap, u8 mode_bitmap)
> Should the name be in plural as this sets all fans?

I don't think so, as this functions can also be used to control each fan
individually.

> Please also consider what I noted to patch 2 about this nesting of
> FIELD_PREP()s. Getting rid of that effectively means the caller ORs all
> bits together but I don't think that would make the code harder to read.

Alright, i will try to get rid of the nesting.

Thanks,
Armin Wolf

>> +{
>> +	acpi_status status;
>> +	u64 input = 0;
>> +	u64 result;
>> +
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK, mode_bitmap);
>> +
>> +	status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID, input,
>> +					&result);
>> +	if (ACPI_FAILURE(status))
>> +		return -EIO;
>> +
>> +	/* The return status must be zero for the operation to have succeeded */
>> +	if (FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK, result))
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>>   static void WMID_gaming_set_fan_mode(u8 fan_mode)
>>   {
>> -	/* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
>> -	u64 gpu_fan_config1 = 0, gpu_fan_config2 = 0;
>> -	int i;
>> -
>> -	if (quirks->cpu_fans > 0)
>> -		gpu_fan_config2 |= 1;
>> -	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
>> -		gpu_fan_config2 |= 1 << (i + 1);
>> -	for (i = 0; i < quirks->gpu_fans; ++i)
>> -		gpu_fan_config2 |= 1 << (i + 3);
>> -	if (quirks->cpu_fans > 0)
>> -		gpu_fan_config1 |= fan_mode;
>> -	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
>> -		gpu_fan_config1 |= fan_mode << (2 * i + 2);
>> -	for (i = 0; i < quirks->gpu_fans; ++i)
>> -		gpu_fan_config1 |= fan_mode << (2 * i + 6);
>> -	WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
>> +	u16 mode_bitmap = 0;
>> +	u16 fan_bitmap = 0;
>> +
>> +	if (quirks->cpu_fans > 0) {
>> +		fan_bitmap |= ACER_GAMING_FAN_BEHAVIOR_CPU;
>> +		mode_bitmap |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK, fan_mode);
>> +	}
>> +
>> +	if (quirks->gpu_fans > 0) {
>> +		fan_bitmap |= ACER_GAMING_FAN_BEHAVIOR_GPU;
>> +		mode_bitmap |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK, fan_mode);
>> +	}
>> +
>> +	WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
>>   }
>>   
>>   static int WMID_gaming_set_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 value)
>> @@ -1923,7 +1953,7 @@ static int acer_toggle_turbo(void)
>>   		WMID_gaming_set_u64(0x1, ACER_CAP_TURBO_LED);
>>   
>>   		/* Set FAN mode to auto */
>> -		WMID_gaming_set_fan_mode(0x1);
>> +		WMID_gaming_set_fan_mode(ACER_WMID_FAN_MODE_AUTO);
>>   
>>   		/* Set OC to normal */
>>   		if (has_cap(ACER_CAP_TURBO_OC)) {
>> @@ -1937,7 +1967,7 @@ static int acer_toggle_turbo(void)
>>   		WMID_gaming_set_u64(0x10001, ACER_CAP_TURBO_LED);
>>   
>>   		/* Set FAN mode to turbo */
>> -		WMID_gaming_set_fan_mode(0x2);
>> +		WMID_gaming_set_fan_mode(ACER_WMID_FAN_MODE_TURBO);
>>   
>>   		/* Set OC to turbo mode */
>>   		if (has_cap(ACER_CAP_TURBO_OC)) {
>>