| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <04969cb2-3b59-4232-bada-a69d138005fa@gmx.de>
Date: Thu, 2 Oct 2025 21:16:29 +0200
From: Armin Wolf <W_Armin@....de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: jlee@...e.com, basak.sb2006@...il.com, rayanmargham4@...il.com,
kuurtb@...il.com, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/4] platform/x86: acer-wmi: Fix setting of fan
behavior
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@...il.com>
>> Signed-off-by: Armin Wolf <W_Armin@....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)) {
>>
Powered by blists - more mailing lists