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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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?

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.

> +{
> +	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)) {
> 

-- 
 i.