| 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: <04bffbcf-0520-d1d1-d3a5-b173976ff132@linux.intel.com>
Date: Mon, 6 Oct 2025 11:11:59 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Armin Wolf <W_Armin@....de>
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 2/4] platform/x86: acer-wmi: Add fan control support
On Thu, 2 Oct 2025, Armin Wolf wrote:
> Am 24.09.25 um 12:52 schrieb Ilpo Järvinen:
>
> > On Tue, 23 Sep 2025, Armin Wolf wrote:
> >
> > > Add support for controlling the fan speed using the
> > > SetGamingFanSpeed() and GetGamingFanSpeed() WMI methods.
> > >
> > > This feature is only enabled if the machine has ACER_CAP_PWM enabled
> > > and depend on ACER_CAP_HWMON for detecting the number of available
> > > fans.
> > >
> > > Reviewed-by: Kurt Borja <kuurtb@...il.com>
> > > Signed-off-by: Armin Wolf <W_Armin@....de>
> > > ---
> > > drivers/platform/x86/acer-wmi.c | 221 +++++++++++++++++++++++++++++++-
> > > 1 file changed, 219 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/acer-wmi.c
> > > b/drivers/platform/x86/acer-wmi.c
> > > index a41555ee8589..345b6e24ae31 100644
> > > --- a/drivers/platform/x86/acer-wmi.c
> > > +++ b/drivers/platform/x86/acer-wmi.c
> > > @@ -12,10 +12,12 @@
> > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > #include <linux/kernel.h>
> > > +#include <linux/minmax.h>
> > > #include <linux/module.h>
> > > #include <linux/init.h>
> > > #include <linux/types.h>
> > > #include <linux/dmi.h>
> > > +#include <linux/fixp-arith.h>
> > > #include <linux/backlight.h>
> > > #include <linux/leds.h>
> > > #include <linux/platform_device.h>
> > > @@ -69,11 +71,16 @@ MODULE_LICENSE("GPL");
> > > #define ACER_WMID_GET_GAMING_LED_METHODID 4
> > > #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
> > > #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
> > > +#define ACER_WMID_GET_GAMING_FAN_BEHAVIOR_METHODID 15
> > > +#define ACER_WMID_SET_GAMING_FAN_SPEED_METHODID 16
> > > +#define ACER_WMID_GET_GAMING_FAN_SPEED_METHODID 17
> > > #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
> > > #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
> > > +#define ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK GENMASK_ULL(7, 0)
> > > #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_GET_MODE_MASK GENMASK_ULL(23, 8)
> > > #define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
> > > #define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
> > > @@ -81,6 +88,10 @@ MODULE_LICENSE("GPL");
> > > #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_FAN_SPEED_STATUS_MASK GENMASK_ULL(7, 0)
> > > +#define ACER_GAMING_FAN_SPEED_ID_MASK GENMASK_ULL(7, 0)
> > > +#define ACER_GAMING_FAN_SPEED_VALUE_MASK GENMASK_ULL(15, 8)
> > > +
> > > #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)
> > > @@ -130,6 +141,11 @@ enum acer_wmi_predator_v4_sensor_id {
> > > ACER_WMID_SENSOR_GPU_TEMPERATURE = 0x0A,
> > > };
> > > +enum acer_wmi_gaming_fan_id {
> > > + ACER_WMID_CPU_FAN = 0x01,
> > > + ACER_WMID_GPU_FAN = 0x04,
> > > +};
> > In patch 1 you added these:
> >
> > +#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
> > ...
> > +#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
> > +#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
> >
> > Are these numbers related (you even called the field "ID mask")? Could we
> > do with less defines for the same thing if they're related?
>
> I honestly don't know if those IDs are related. I would like to play it safe
> and keep them separate for now.
>
> > > enum acer_wmi_gaming_fan_mode {
> > > ACER_WMID_FAN_MODE_AUTO = 0x01,
> > > ACER_WMID_FAN_MODE_TURBO = 0x02,
> > > @@ -292,6 +308,7 @@ struct hotkey_function_type_aa {
> > > #define ACER_CAP_TURBO_FAN BIT(9)
> > > #define ACER_CAP_PLATFORM_PROFILE BIT(10)
> > > #define ACER_CAP_HWMON BIT(11)
> > > +#define ACER_CAP_PWM BIT(12)
> > > /*
> > > * Interface type flags
> > > @@ -386,6 +403,7 @@ struct quirk_entry {
> > > u8 cpu_fans;
> > > u8 gpu_fans;
> > > u8 predator_v4;
> > > + u8 pwm;
> > > };
> > > static struct quirk_entry *quirks;
> > > @@ -405,6 +423,9 @@ static void __init set_quirks(void)
> > > if (quirks->predator_v4)
> > > interface->capability |= ACER_CAP_PLATFORM_PROFILE |
> > > ACER_CAP_HWMON;
> > > +
> > > + if (quirks->pwm)
> > > + interface->capability |= ACER_CAP_PWM;
> > > }
> > > static int __init dmi_matched(const struct dmi_system_id *dmi)
> > > @@ -1651,6 +1672,27 @@ static int WMID_gaming_set_fan_behavior(u16
> > > fan_bitmap, u8 mode_bitmap)
> > > return 0;
> > > }
> > > +static int WMID_gaming_get_fan_behavior(u16 fan_bitmap, u8
> > > *mode_bitmap)
> > > +{
> > > + acpi_status status;
> > > + u32 input = 0;
> > > + u64 result;
> > > +
> > > + input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
> > > + status =
> > > WMI_gaming_execute_u32_u64(ACER_WMID_GET_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;
> > > +
> > > + *mode_bitmap = FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GET_MODE_MASK,
> > > result);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static void WMID_gaming_set_fan_mode(u8 fan_mode)
> > > {
> > > u16 mode_bitmap = 0;
> > > @@ -1669,6 +1711,55 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
> > > WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
> > > }
> > > +static int WMID_gaming_set_gaming_fan_speed(u8 fan, u8 speed)
> > > +{
> > > + acpi_status status;
> > > + u64 input = 0;
> > > + u64 result;
> > > +
> > > + if (speed > 100)
> > > + return -EINVAL;
> > > +
> > > + input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
> > > + input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_VALUE_MASK, speed);
> > > +
> > > + status =
> > > WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_SPEED_METHODID, input,
> > > &result);
> > > + if (ACPI_FAILURE(status))
> > > + return -EIO;
> > > +
> > > + switch (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result)) {
> > > + case 0x00:
> > > + return 0;
> > > + case 0x01:
> > > + return -ENODEV;
> > > + case 0x02:
> > > + return -EINVAL;
> > > + default:
> > > + return -ENXIO;
> > > + }
> > > +}
> > > +
> > > +static int WMID_gaming_get_gaming_fan_speed(u8 fan, u8 *speed)
> > > +{
> > > + acpi_status status;
> > > + u32 input = 0;
> > > + u64 result;
> > > +
> > > + input |= FIELD_PREP(ACER_GAMING_FAN_SPEED_ID_MASK, fan);
> > > +
> > > + status =
> > > WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_FAN_SPEED_METHODID, input,
> > > + &result);
> > > + if (ACPI_FAILURE(status))
> > > + return -EIO;
> > > +
> > > + if (FIELD_GET(ACER_GAMING_FAN_SPEED_STATUS_MASK, result))
> > > + return -ENODEV;
> > > +
> > > + *speed = FIELD_GET(ACER_GAMING_FAN_SPEED_VALUE_MASK, result);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int WMID_gaming_set_misc_setting(enum
> > > acer_wmi_gaming_misc_setting setting, u8 value)
> > > {
> > > acpi_status status;
> > > @@ -2840,6 +2931,16 @@ static const enum acer_wmi_predator_v4_sensor_id
> > > acer_wmi_fan_channel_to_sensor_
> > > [1] = ACER_WMID_SENSOR_GPU_FAN_SPEED,
> > > };
> > > +static const enum acer_wmi_gaming_fan_id
> > > acer_wmi_fan_channel_to_fan_id[] = {
> > > + [0] = ACER_WMID_CPU_FAN,
> > > + [1] = ACER_WMID_GPU_FAN,
> > > +};
> > > +
> > > +static const u16 acer_wmi_fan_channel_to_fan_bitmap[] = {
> > > + [0] = ACER_GAMING_FAN_BEHAVIOR_CPU,
> > > + [1] = ACER_GAMING_FAN_BEHAVIOR_GPU,
> > > +};
> > > +
> > > static umode_t acer_wmi_hwmon_is_visible(const void *data,
> > > enum hwmon_sensor_types type, u32
> > > attr,
> > > int channel)
> > > @@ -2851,6 +2952,11 @@ static umode_t acer_wmi_hwmon_is_visible(const void
> > > *data,
> > > case hwmon_temp:
> > > sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
> > > break;
> > > + case hwmon_pwm:
> > > + if (!has_cap(ACER_CAP_PWM))
> > > + return 0;
> > > +
> > > + fallthrough;
> > > case hwmon_fan:
> > > sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
> > > break;
> > > @@ -2858,8 +2964,12 @@ static umode_t acer_wmi_hwmon_is_visible(const void
> > > *data,
> > > return 0;
> > > }
> > > - if (*supported_sensors & BIT(sensor_id - 1))
> > > + if (*supported_sensors & BIT(sensor_id - 1)) {
> > > + if (type == hwmon_pwm)
> > > + return 0644;
> > > +
> > > return 0444;
> > > + }
> > > return 0;
> > > }
> > > @@ -2868,8 +2978,10 @@ static int acer_wmi_hwmon_read(struct device *dev,
> > > enum hwmon_sensor_types type,
> > > u32 attr, int channel, long *val)
> > > {
> > > u64 command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING;
> > > + u8 fan, speed, mode_bitmap;
> > > + u16 fan_bitmap;
> > > + int mode, ret;
> > > u64 result;
> > > - int ret;
> > > switch (type) {
> > > case hwmon_temp:
> > > @@ -2893,6 +3005,106 @@ static int acer_wmi_hwmon_read(struct device *dev,
> > > enum hwmon_sensor_types type,
> > > *val =
> > > FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
> > > return 0;
> > > + case hwmon_pwm:
> > > + switch (attr) {
> > > + case hwmon_pwm_input:
> > > + fan = acer_wmi_fan_channel_to_fan_id[channel];
> > > + ret = WMID_gaming_get_gaming_fan_speed(fan, &speed);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *val = fixp_linear_interpolate(0, 0, 100, U8_MAX,
> > > speed);
> > > + return 0;
> > > + case hwmon_pwm_enable:
> > > + fan_bitmap =
> > > acer_wmi_fan_channel_to_fan_bitmap[channel];
> > > + ret = WMID_gaming_get_fan_behavior(fan_bitmap,
> > > &mode_bitmap);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + switch (channel) {
> > > + case 0:
> > > + mode =
> > > FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
> > > + mode_bitmap);
> > > + break;
> > > + case 1:
> > > + mode =
> > > FIELD_GET(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
> > > + mode_bitmap);
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > You've managed to create an internal API that resembles the WMI method
> > input by working in terms of fan_bitmap and mode_bitmap which makes things
> > complicated as then you have to build logic on top of it to do all the
> > mapping in the caller...
> >
> > I suggest changing this so you make WMID_gaming_get_fan_behavior() to
> > input ACER_GAMING_FAN_BEHAVIOR_CPU / GPU (or ACER_WMID_CPU_FAN / GPU_FAN
> > depending on the question I asked above) and perform the output mapping
> > within that function. I think it would be much cleaner.
> >
> > I didn't exactly like the internal API in patch 1 either and I'd prefer
> > the field GENMASK()s to directly point to the correct bits instead of the
> > nested FIELD_GET/PREP()s.
>
> Ok, will do.
>
> > > + switch (mode) {
> > > + case ACER_WMID_FAN_MODE_AUTO:
> > > + *val = 2;
> > > + return 0;
> > > + case ACER_WMID_FAN_MODE_TURBO:
> > > + *val = 0;
> > > + return 0;
> > > + case ACER_WMID_FAN_MODE_CUSTOM:
> > > + *val = 1;
> > > + return 0;
> > > + default:
> > > + return -ENXIO;
> > > + }
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > +}
> > > +
> > > +static int acer_wmi_hwmon_write(struct device *dev, enum
> > > hwmon_sensor_types type,
> > > + u32 attr, int channel, long val)
> > > +{
> > > + u8 fan, speed, mode_bitmap;
> > > + u16 fan_bitmap;
> > > + int mode;
> > > +
> > > + switch (type) {
> > > + case hwmon_pwm:
> > > + switch (attr) {
> > > + case hwmon_pwm_input:
> > > + fan = acer_wmi_fan_channel_to_fan_id[channel];
> > > + speed = fixp_linear_interpolate(0, 0, U8_MAX, 100,
> > > + clamp_val(val, 0,
> > > U8_MAX));
> > > +
> > > + return WMID_gaming_set_gaming_fan_speed(fan, speed);
> > > + case hwmon_pwm_enable:
> > > + fan_bitmap =
> > > acer_wmi_fan_channel_to_fan_bitmap[channel];
> > > +
> > > + switch (val) {
> > > + case 0:
> > > + mode = ACER_WMID_FAN_MODE_TURBO;
> > > + break;
> > > + case 1:
> > > + mode = ACER_WMID_FAN_MODE_CUSTOM;
> > > + break;
> > > + case 2:
> > > + mode = ACER_WMID_FAN_MODE_AUTO;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + switch (channel) {
> > > + case 0:
> > > + mode_bitmap =
> > > FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK,
> > > + mode);
> > > + break;
> > > + case 1:
> > > + mode_bitmap =
> > > FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK,
> > > + mode);
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > These too look they could be mapped inside WMID_gaming_set_fan_behavior().
>
> I don't think so, as the function also needs to handle the setting of multiple
> fans
> at once.
Oh, right. How about making it two functions then, one which is called
from here to set one fan and which then internally calls into the
function that can set any fans?
--
i.
Powered by blists - more mailing lists