| 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: <f50d9ced-a2c3-7d46-01a0-794b31ee73a2@redhat.com>
Date: Mon, 2 Aug 2021 13:39:27 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Jafar Akhondali <jafar.akhoondali@...il.com>, jlee@...e.com,
mgross@...ux.intel.com, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] platform/x86: acer-wmi: Add Turbo Mode support for
Acer PH315-53
Hi,
On 7/12/21 12:25 AM, Jafar Akhondali wrote:
> Looks like the previous patch got some indentation problems, here is
> the fixed version.
Thank you for the new version, this is much better.
>
> The Acer Predator Helios series (usually denoted by PHxxx-yy) features
> a special key above the keyboard named "TURBO". The turbo key does 3
> things:
> 1. Set all fan's speeds to TURBO mode
> 2. Overclocks the CPU and GPU in the safe range
> 3. Turn on an LED just below the turbo button
>
> All of these actions are done by WMI function calls, and there is no
> custom OC level for turbo. It acts as a flag for enabling turbo
> mode instead of telling processors to use 1.3x of power.
>
> I've run some benchmark tests and it worked fine:
>
> GpuTest 0.7.0
> http://www.geeks3d.com
>
> Module: FurMark
> Normal mode Score: 7289 points (FPS: 121)
> Turbo mode Score: 7675 points (FPS: 127)
> Settings:
> - 1920x1080 fullscreen
> - antialiasing: Off
> - duration: 60000 ms
>
> Renderer:
> - GeForce RTX 2060/PCIe/SSE2
> - OpenGL: 4.6.0 NVIDIA 460.32.03
>
> This feature is presented by Acer officially and should not harm
> hardware in any case.
>
> A challenging part of implementing this feature is that calling overclocking
> the function requires knowing the exact count of fans for CPU and GPU
> for each model, which to the best of my knowledge is not available in
> the kernel.
>
> So after checking the official PredatorSense application methods, it
> turned out they have provided the software the list of fans in each model.
> I have access to the mentioned list, and all similar PH-iii-jj can be
> added easily by matching "DMI_PRODUCT_NAME".
>
> Creating a separate file for the gaming interface was not possible because
> the current WMI event GUID is needed for the turbo button, and it's not possible
> to register multiple functions on the same event GUID.
>
>
> Some small indent problems have been also fixed.
>
> Signed-off-by: JafarAkhondali <jafar.akhoondali@...il.com>
> ---
> drivers/platform/x86/acer-wmi.c | 243 ++++++++++++++++++++++++++++++--
> 1 file changed, 231 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 85db9403cc14..06b25ddf04ee 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -30,10 +30,12 @@
> #include <linux/input/sparse-keymap.h>
> #include <acpi/video.h>
>
> +
> MODULE_AUTHOR("Carlos Corbacho");
> MODULE_DESCRIPTION("Acer Laptop WMI Extras Driver");
> MODULE_LICENSE("GPL");
>
> +
> /*
> * Magic Number
> * Meaning is unknown - this number is required for writing to ACPI for AMW0
These 2 whitespace changes are unrelated to the turbo feature, please drop.
> @@ -59,7 +61,10 @@ MODULE_LICENSE("GPL");
> #define ACER_WMID_SET_BRIGHTNESS_METHODID 6
> #define ACER_WMID_GET_THREEG_METHODID 10
> #define ACER_WMID_SET_THREEG_METHODID 11
> -
Please keep the empty line here.
> +#define ACER_WMID_SET_GAMING_LED_METHODID 2
> +#define ACER_WMID_GET_GAMING_LED_METHODID 4
> +#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
> +#define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
And add an extra empty line here.
> /*
> * Acer ACPI method GUIDs
> */
> @@ -68,6 +73,7 @@ MODULE_LICENSE("GPL");
> #define WMID_GUID1 "6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3"
> #define WMID_GUID2 "95764E09-FB56-4E83-B31A-37761F60994A"
> #define WMID_GUID3 "61EF69EA-865C-4BC3-A502-A0DEBA0CB531"
> +#define WMID_GUID4 "7A4DDFE7-5B5D-40B4-8595-4408E0CC7F56"
>
> /*
> * Acer ACPI event GUIDs
> @@ -81,6 +87,7 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
> enum acer_wmi_event_ids {
> WMID_HOTKEY_EVENT = 0x1,
> WMID_ACCEL_OR_KBD_DOCK_EVENT = 0x5,
> + WMID_GAMING_TURBO_KEY_EVENT = 0x7,
> };
>
> static const struct key_entry acer_wmi_keymap[] __initconst = {
> @@ -144,6 +151,7 @@ struct event_return_value {
>
> #define ACER_WMID3_GDS_TOUCHPAD (1<<1) /* Touchpad */
>
> +
> /* Hotkey Customized Setting and Acer Application Status.
> * Set Device Default Value and Report Acer Application Status.
> * When Acer Application starts, it will run this method to inform
Unrelated whitespace change, please drop.
> @@ -215,6 +223,9 @@ struct hotkey_function_type_aa {
> #define ACER_CAP_THREEG BIT(4)
> #define ACER_CAP_SET_FUNCTION_MODE BIT(5)
> #define ACER_CAP_KBD_DOCK BIT(6)
> +#define ACER_CAP_TURBO_OC BIT(7)
> +#define ACER_CAP_TURBO_LED BIT(8)
> +#define ACER_CAP_TURBO_FAN BIT(9)
>
> /*
> * Interface type flags
> @@ -224,6 +235,7 @@ enum interface_flags {
> ACER_AMW0_V2,
> ACER_WMID,
> ACER_WMID_v2,
> + ACER_WMID_GAMING,
> };
>
> #define ACER_DEFAULT_WIRELESS 0
> @@ -290,6 +302,9 @@ struct wmi_interface {
> /* The static interface pointer, points to the currently detected interface */
> static struct wmi_interface *interface;
>
> +/* The static gaming interface pointer, points to the currently
> detected gaming interface */
> +static struct wmi_interface *gaming_interface;
> +
The interface pointer stuff is intended to point to one of the
4 types of main Acer WMI interfaces, since there is only one
gamig interface, there is no need for this separate
gaming_interface pointer, please drop it.
> /*
> * Embedded Controller quirks
> * Some laptops require us to directly access the EC to either enable or query
> @@ -301,6 +316,9 @@ struct quirk_entry {
> u8 mailled;
> s8 brightness;
> u8 bluetooth;
> + u8 turbo;
> + u8 cpu_fans;
> + u8 gpu_fans;
> };
>
> static struct quirk_entry *quirks;
> @@ -312,6 +330,10 @@ static void __init set_quirks(void)
>
> if (quirks->brightness)
> interface->capability |= ACER_CAP_BRIGHTNESS;
> +
> + if (quirks->turbo)
> + gaming_interface->capability |= ACER_CAP_TURBO_OC | ACER_CAP_TURBO_LED
> + | ACER_CAP_TURBO_FAN;
Just use interface->capability here.
> }
>
> static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -340,6 +362,12 @@ static struct quirk_entry quirk_acer_travelmate_2490 = {
> .mailled = 1,
> };
>
> +static struct quirk_entry quirk_acer_predator_ph315_53 = {
> + .turbo = 1,
> + .cpu_fans = 1,
> + .gpu_fans = 1,
> +};
> +
> /* This AMW0 laptop has no bluetooth */
> static struct quirk_entry quirk_medion_md_98300 = {
> .wireless = 1,
> @@ -507,6 +535,15 @@ static const struct dmi_system_id acer_quirks[]
> __initconst = {
> },
> .driver_data = &quirk_acer_travelmate_2490,
> },
> + {
> + .callback = dmi_matched,
> + .ident = "Acer Predator PH315-53",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Predator PH315-53"),
> + },
> + .driver_data = &quirk_acer_predator_ph315_53,
> + },
> {
> .callback = set_force_caps,
> .ident = "Acer Aspire Switch 10E SW3-016",
> @@ -903,7 +940,7 @@ static acpi_status __init AMW0_set_capabilities(void)
> */
> if (wmi_has_guid(AMW0_GUID2)) {
> if ((quirks != &quirk_unknown) ||
> - !AMW0_set_cap_acpi_check_device())
> + !AMW0_set_cap_acpi_check_device())
> interface->capability |= ACER_CAP_WIRELESS;
> return AE_OK;
> }
Unrelated whitespace change, please drop.
> @@ -1344,6 +1381,93 @@ static struct wmi_interface wmid_v2_interface = {
> .type = ACER_WMID_v2,
> };
>
> +
> +/*
> + * WMID Gaming interface
> + */
> +
> +static struct wmi_interface wmid_gaming_interface = {
> + .type = ACER_WMID_GAMING
> +};
Not necessary, please drop.
> +
> +static acpi_status
> +WMI_gaming_execute_u64(u32 method_id, u64 in, u64 *out)
> +{
> + struct acpi_buffer input = { (acpi_size) sizeof(u64), (void *)(&in) };
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + u32 tmp = 0;
> + acpi_status status;
> +
> + status = wmi_evaluate_method(WMID_GUID4, 0, method_id, &input, &result);
> +
> + if (ACPI_FAILURE(status))
> + return status;
> + obj = (union acpi_object *) result.pointer;
> +
> + if (obj) {
> + if (obj->type == ACPI_TYPE_BUFFER &&
> + (obj->buffer.length == sizeof(u32) ||
> + obj->buffer.length == sizeof(u64))) {
> + tmp = *((u64 *) obj->buffer.pointer);
You need to have a separate case for length == sizeof(u32), in that
case you must use:
tmp = *((u32 *) obj->buffer.pointer);
So that you don't read beyond the end of the buffer.
> + } else if (obj->type == ACPI_TYPE_INTEGER) {
> + tmp = (u64) obj->integer.value;
> + }
> + }
> +
> + if (out)
> + *out = tmp;
> +
> + kfree(result.pointer);
> +
> + return status;
> +}
> +
> +static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
> +{
> + u32 method_id = 0;
> +
> + switch (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;
> + case ACER_CAP_TURBO_OC:
> + method_id = ACER_WMID_SET_GAMING_MISC_SETTING_METHODID;
> + break;
> + default:
> + return AE_ERROR;
AE_BAD_PARAMETER ?
> + }
> + return WMI_gaming_execute_u64(method_id, value, NULL);
> +}
> +
> +static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
> +{
> + acpi_status status;
> + u64 result;
> + u64 input;
> + u32 method_id;
> +
> +
> + switch (cap) {
> + case ACER_CAP_TURBO_LED:
> + method_id = ACER_WMID_GET_GAMING_LED_METHODID;
> + input = 0x1;
> + break;
> + default:
> + return AE_ERROR;
AE_BAD_PARAMETER ?
> + }
> +
> + status = WMI_gaming_execute_u64(method_id, input, &result);
> + if (ACPI_SUCCESS(status))
> + *value = (u64) result;
> +
> + return status;
> +}
> +
> +
> /*
> * Generic Device (interface-independent)
> */
> @@ -1374,7 +1498,6 @@ static acpi_status get_u32(u32 *value, u32 cap)
> status = WMID_get_u32(value, cap);
> break;
> }
> -
> return status;
> }
>
Unrelated whitespace change, please drop.
> @@ -1422,6 +1545,34 @@ static acpi_status set_u32(u32 value, u32 cap)
> return AE_BAD_PARAMETER;
> }
>
> +static acpi_status set_u64(u64 value, u32 cap)
> +{
> + if (gaming_interface->capability & cap) {
Just use interface->capability here and move this
check into WMID_gaming_set_u64() and drop this
(set_u64) function and instead use WMID_gaming_set_u64()
directly.
If the WMID_GUID4 GUID does not exist then wmi_evaluate_method()
will catch this.
> + switch (gaming_interface->type) {
> + case ACER_WMID_GAMING:
> + return WMID_gaming_set_u64(value, cap);
> + default:
> + return AE_BAD_PARAMETER;
> + }
> + }
> + return AE_BAD_PARAMETER;
> +}
> +
> +
> +static acpi_status get_u64(u64 *value, u32 cap)
> +{
> + acpi_status status = AE_ERROR;
> +
> + if (gaming_interface->capability & cap) {
Same here.
> + switch (gaming_interface->type) {
> + case ACER_WMID_GAMING:
> + status = WMID_gaming_get_u64(value, cap);
> + break;
> + }
> + }
> + return status;
> +}
> +
> static void __init acer_commandline_init(void)
> {
> /*
> @@ -1501,7 +1652,7 @@ static int acer_backlight_init(struct device *dev)
> props.type = BACKLIGHT_PLATFORM;
> props.max_brightness = max_brightness;
> bd = backlight_device_register("acer-wmi", dev, NULL, &acer_bl_ops,
> - &props);
> + &props);
> if (IS_ERR(bd)) {
> pr_err("Could not register Acer backlight device\n");
> acer_backlight_device = NULL;
Unrelated whitespace change, please drop.
> @@ -1575,6 +1726,67 @@ static int acer_gsensor_event(void)
> return 0;
> }
>
> +/*
> + * Predator series turbo button
> + */
> +static int acer_toggle_turbo(void)
> +{
> + /* Get current state from turbo button */
> + u64 turbo_led_state, gpu_fan_config1, gpu_fan_config2;
> + u8 i;
> +
> + if (ACPI_FAILURE(get_u64(&turbo_led_state, ACER_CAP_TURBO_LED)))
> + return -1;
> +
> + if (turbo_led_state) {
> + // turns off turbo led
> + set_u64(0x1, ACER_CAP_TURBO_LED);
> +
> + // set FAN mode to auto
> + 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 |= 1;
> + for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> + gpu_fan_config1 |= 1 << (2 * i + 2);
> + for (i = 0; i < quirks->gpu_fans; ++i)
> + gpu_fan_config1 |= 1 << (2 * i + 6);
> + set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
> +
> + // set OC to normal
> + set_u64(0x5, ACER_CAP_TURBO_OC);
> + set_u64(0x7, ACER_CAP_TURBO_OC);
> + } else {
> + // turn on turbo led
> + set_u64(0x10001, ACER_CAP_TURBO_LED);
> +
> + // set FAN to turbo mode
> + 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 |= 2;
> + for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> + gpu_fan_config1 |= 2 << (2 * i + 2);
> + for (i = 0; i < quirks->gpu_fans; ++i)
> + gpu_fan_config1 |= 2 << (2 * i + 6);
> + set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
> +
> + // set OC to turbo mode
> + set_u64(0x205, ACER_CAP_TURBO_OC);
> + set_u64(0x207, ACER_CAP_TURBO_OC);
> + }
> + return turbo_led_state;
> +}
> +
> +
> /*
> * Switch series keyboard dock status
> */
> @@ -1605,7 +1817,7 @@ static void acer_kbd_dock_get_initial_state(void)
> status = wmi_evaluate_method(WMID_GUID3, 0, 0x2, &input_buf, &output_buf);
> if (ACPI_FAILURE(status)) {
> pr_err("Error getting keyboard-dock initial status: %s\n",
> - acpi_format_exception(status));
> + acpi_format_exception(status));
> return;
> }
>
Unrelated whitespace change, please drop.
> @@ -1618,7 +1830,7 @@ static void acer_kbd_dock_get_initial_state(void)
> output = obj->buffer.pointer;
> if (output[0] != 0x00 || (output[3] != 0x05 && output[3] != 0x45)) {
> pr_err("Unexpected output [0]=0x%02x [3]=0x%02x getting
> keyboard-dock initial status\n",
> - output[0], output[3]);
> + output[0], output[3]);
> goto out_free_obj;
> }
>
Unrelated whitespace change, please drop.
> @@ -1759,7 +1971,7 @@ static int acer_rfkill_init(struct device *dev)
> rfkill_inited = true;
>
> if ((ec_raw_mode || !wmi_has_guid(ACERWMID_EVENT_GUID)) &&
> - has_cap(ACER_CAP_WIRELESS | ACER_CAP_BLUETOOTH | ACER_CAP_THREEG))
> + has_cap(ACER_CAP_WIRELESS | ACER_CAP_BLUETOOTH | ACER_CAP_THREEG))
> schedule_delayed_work(&acer_rfkill_work,
> round_jiffies_relative(HZ));
>
Unrelated whitespace change, please drop.
> @@ -1782,7 +1994,7 @@ static int acer_rfkill_init(struct device *dev)
> static void acer_rfkill_exit(void)
> {
> if ((ec_raw_mode || !wmi_has_guid(ACERWMID_EVENT_GUID)) &&
> - has_cap(ACER_CAP_WIRELESS | ACER_CAP_BLUETOOTH | ACER_CAP_THREEG))
> + has_cap(ACER_CAP_WIRELESS | ACER_CAP_BLUETOOTH | ACER_CAP_THREEG))
> cancel_delayed_work_sync(&acer_rfkill_work);
>
> if (has_cap(ACER_CAP_WIRELESS)) {
Unrelated whitespace change, please drop.
> @@ -1872,6 +2084,10 @@ static void acer_wmi_notify(u32 value, void *context)
> acer_gsensor_event();
> acer_kbd_dock_event(&return_value);
> break;
> + case WMID_GAMING_TURBO_KEY_EVENT:
> + if (return_value.key_num == 0x4)
> + acer_toggle_turbo();
> + break;
> default:
> pr_warn("Unknown function number - %d - %d\n",
> return_value.function, return_value.key_num);
> @@ -2251,8 +2467,8 @@ static int __init acer_wmi_init(void)
> * in the past quirk list.
> */
> if (wmi_has_guid(AMW0_GUID1) &&
> - !dmi_check_system(amw0_whitelist) &&
> - quirks == &quirk_unknown) {
> + !dmi_check_system(amw0_whitelist) &&
> + quirks == &quirk_unknown) {
> pr_debug("Unsupported machine has AMW0_GUID1, unable to load\n");
> return -ENODEV;
> }
Unrelated whitespace change, please drop.
> @@ -2266,8 +2482,11 @@ static int __init acer_wmi_init(void)
> if (!wmi_has_guid(AMW0_GUID1) && wmi_has_guid(WMID_GUID1))
> interface = &wmid_interface;
>
> - if (wmi_has_guid(WMID_GUID3))
> + if (wmi_has_guid(WMID_GUID3)) {
> interface = &wmid_v2_interface;
> + if (wmi_has_guid(WMID_GUID4))
> + gaming_interface = &wmid_gaming_interface;
> + }
>
Since we don't need the gaming_interface this entire chunk
can be dropped too.
> if (interface)
> dmi_walk(type_aa_dmi_decode, NULL);
> @@ -2316,7 +2535,7 @@ static int __init acer_wmi_init(void)
> interface->capability = force_caps;
>
> if (wmi_has_guid(WMID_GUID3) &&
> - (interface->capability & ACER_CAP_SET_FUNCTION_MODE)) {
> + (interface->capability & ACER_CAP_SET_FUNCTION_MODE)) {
> if (ACPI_FAILURE(acer_wmi_enable_rf_button()))
> pr_warn("Cannot enable RF Button Driver\n");
>
Unrelated whitespace change, please drop.
Regards,
Hans
Powered by blists - more mailing lists