| 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: <25bd0c90-2de0-ef66-c18d-661180b71fd4@linux.intel.com>
Date: Fri, 17 Oct 2025 15:48:46 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Mario Limonciello <superm1@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Denis Benato <benato.denis96@...il.com>,
Mark Pearson <mpearson-lenovo@...ebb.ca>,
"Luke D . Jones" <luke@...nes.dev>
cc: LKML <linux-kernel@...r.kernel.org>, platform-driver-x86@...r.kernel.org,
Alok Tiwari <alok.a.tiwari@...cle.com>,
Derek John Clark <derekjohn.clark@...il.com>,
Mateusz Schyboll <dragonn@...pl>, porfet828@...il.com
Subject: Re: [PATCH v14 5/9] platform/x86: asus-armoury: add core count
control
On Wed, 15 Oct 2025, Denis Benato wrote:
> From: "Luke D. Jones" <luke@...nes.dev>
>
> Implement Intel core enablement under the asus-armoury module using the
> fw_attributes class.
>
> This allows users to enable or disable preformance or efficiency cores
> depending on their requirements. After change a reboot is required.
>
> Signed-off-by: Denis Benato <benato.denis96@...il.com>
> Signed-off-by: Luke D. Jones <luke@...nes.dev>
> ---
> drivers/platform/x86/asus-armoury.c | 258 ++++++++++++++++++++-
> drivers/platform/x86/asus-armoury.h | 28 +++
> include/linux/platform_data/x86/asus-wmi.h | 5 +
> 3 files changed, 290 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
> index 3b49a27e397d..3d963025d84e 100644
> --- a/drivers/platform/x86/asus-armoury.c
> +++ b/drivers/platform/x86/asus-armoury.c
> @@ -45,13 +45,49 @@
> #define ASUS_MINI_LED_2024_STRONG 0x01
> #define ASUS_MINI_LED_2024_OFF 0x02
>
> +#define ASUS_POWER_CORE_MASK GENMASK(15, 8)
> +#define ASUS_PERF_CORE_MASK GENMASK(7, 0)
> +
> +enum cpu_core_type {
> + CPU_CORE_PERF = 0,
> + CPU_CORE_POWER,
> +};
> +
> +enum cpu_core_value {
> + CPU_CORE_DEFAULT = 0,
This could be mapped in the sysfs _show function as there's no real
backing value for it.
> + CPU_CORE_MIN,
> + CPU_CORE_MAX,
> + CPU_CORE_CURRENT,
> +};
> +
> +#define CPU_PERF_CORE_COUNT_MIN 4
> +#define CPU_POWR_CORE_COUNT_MIN 0
> +
> +/* Tunables provided by ASUS for gaming laptops */
> +struct cpu_cores {
> + u32 cur_perf_cores;
> + u32 min_perf_cores;
> + u32 max_perf_cores;
> + u32 cur_power_cores;
> + u32 min_power_cores;
> + u32 max_power_cores;
> +};
> +
> static struct asus_armoury_priv {
> struct device *fw_attr_dev;
> struct kset *fw_attr_kset;
>
> + struct cpu_cores *cpu_cores;
> u32 mini_led_dev_id;
> u32 gpu_mux_dev_id;
> -} asus_armoury;
> + /*
> + * Mutex to prevent big/little core count changes writing to same
> + * endpoint at the same time. Must lock during attr store.
> + */
> + struct mutex cpu_core_mutex;
> +} asus_armoury = {
> + .cpu_core_mutex = __MUTEX_INITIALIZER(asus_armoury.cpu_core_mutex)
> +};
>
> struct fw_attrs_group {
> bool pending_reboot;
> @@ -93,6 +129,8 @@ static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
> static bool asus_bios_requires_reboot(struct kobj_attribute *attr)
> {
> return !strcmp(attr->attr.name, "gpu_mux_mode") ||
> + !strcmp(attr->attr.name, "cores_performance") ||
> + !strcmp(attr->attr.name, "cores_efficiency") ||
> !strcmp(attr->attr.name, "panel_hd_mode");
> }
>
> @@ -171,6 +209,12 @@ static ssize_t enum_type_show(struct kobject *kobj, struct kobj_attribute *attr,
> return sysfs_emit(buf, "enumeration\n");
> }
>
> +static ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "integer\n");
> +}
> +
> /* Mini-LED mode **************************************************************/
> static ssize_t mini_led_mode_current_value_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> @@ -474,6 +518,207 @@ static ssize_t apu_mem_possible_values_show(struct kobject *kobj, struct kobj_at
> }
> ATTR_GROUP_ENUM_CUSTOM(apu_mem, "apu_mem", "Set available system RAM (in GB) for the APU to use");
>
> +static int init_max_cpu_cores(void)
> +{
> + u32 cores;
> + int err;
> +
> + asus_armoury.cpu_cores = kzalloc(sizeof(struct cpu_cores), GFP_KERNEL);
> + if (!asus_armoury.cpu_cores)
> + return -ENOMEM;
> +
> + err = asus_wmi_get_devstate_dsts(ASUS_WMI_DEVID_CORES_MAX, &cores);
> + if (err)
> + return err;
> +
> + if ((cores & ASUS_WMI_DSTS_PRESENCE_BIT) == 0) {
> + pr_err("ACPI does not support CPU core count control\n");
> + err = -ENODEV;
> + goto init_max_cpu_cores_err;
Please use __free() and return immediately.
Only assign from local variable to asus_armoury.cpu_cores with
no_free_ptr() at the end.
> + }
> +
> + asus_armoury.cpu_cores->max_power_cores = FIELD_GET(ASUS_POWER_CORE_MASK, cores);
> + asus_armoury.cpu_cores->max_perf_cores = FIELD_GET(ASUS_PERF_CORE_MASK, cores);
> +
> + err = asus_wmi_get_devstate_dsts(ASUS_WMI_DEVID_CORES, &cores);
> + if (err) {
> + pr_err("Could not get CPU core count: error %d\n", err);
> + goto init_max_cpu_cores_err;
> + }
> +
> + asus_armoury.cpu_cores->cur_perf_cores = FIELD_GET(ASUS_PERF_CORE_MASK, cores);
> + asus_armoury.cpu_cores->cur_power_cores = FIELD_GET(ASUS_POWER_CORE_MASK, cores);
> +
> + asus_armoury.cpu_cores->min_perf_cores = CPU_PERF_CORE_COUNT_MIN;
> + asus_armoury.cpu_cores->min_power_cores = CPU_POWR_CORE_COUNT_MIN;
Should these be bounds checked with max?
> + return 0;
> +
> +init_max_cpu_cores_err:
> + kfree(asus_armoury.cpu_cores);
> + return err;
> +}
> +
> +static ssize_t cores_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf,
> + enum cpu_core_type core_type, enum cpu_core_value core_value)
> +{
> + u32 cores;
> +
> + switch (core_value) {
> + case CPU_CORE_DEFAULT:
> + case CPU_CORE_MAX:
> + if (core_type == CPU_CORE_PERF)
> + return sysfs_emit(buf, "%u\n",
> + asus_armoury.cpu_cores->max_perf_cores);
> + else
> + return sysfs_emit(buf, "%u\n",
> + asus_armoury.cpu_cores->max_power_cores);
> + case CPU_CORE_MIN:
> + if (core_type == CPU_CORE_PERF)
> + return sysfs_emit(buf, "%u\n",
> + asus_armoury.cpu_cores->min_perf_cores);
> + else
> + return sysfs_emit(buf, "%u\n",
> + asus_armoury.cpu_cores->min_power_cores);
> + default:
> + break;
> + }
> +
> + if (core_type == CPU_CORE_PERF)
> + cores = asus_armoury.cpu_cores->cur_perf_cores;
> + else
> + cores = asus_armoury.cpu_cores->cur_power_cores;
Why isn't this inside the switch?? The logic in this function looks very
mixed up.
If I'd be you, I'd consider converting the asus_armoury.cpu_cores to a
multi-dimensional array. It would make this just bounds checks and one
line to get the data.
> + return sysfs_emit(buf, "%u\n", cores);
> +}
> +
> +static ssize_t cores_current_value_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, enum cpu_core_type core_type)
> +{
> + u32 new_cores, perf_cores, power_cores, out_val, min, max;
> + int result, err;
> +
> + result = kstrtou32(buf, 10, &new_cores);
> + if (result)
> + return result;
> +
> + scoped_guard(mutex, &asus_armoury.cpu_core_mutex) {
> + if (core_type == CPU_CORE_PERF) {
> + perf_cores = new_cores;
> + power_cores = asus_armoury.cpu_cores->cur_power_cores;
> + min = asus_armoury.cpu_cores->min_perf_cores;
> + max = asus_armoury.cpu_cores->max_perf_cores;
> + } else {
> + perf_cores = asus_armoury.cpu_cores->cur_perf_cores;
> + power_cores = new_cores;
> + min = asus_armoury.cpu_cores->min_power_cores;
> + max = asus_armoury.cpu_cores->max_power_cores;
> + }
> +
> + if (new_cores < min || new_cores > max)
> + return -EINVAL;
> +
> + out_val = FIELD_PREP(ASUS_PERF_CORE_MASK, perf_cores) |
> + FIELD_PREP(ASUS_POWER_CORE_MASK, power_cores);
> +
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CORES, out_val, &result);
> + if (err) {
> + pr_warn("Failed to set CPU core count: %d\n", err);
> + return err;
> + }
> +
> + if (result > 1) {
> + pr_warn("Failed to set CPU core count (result): 0x%x\n", result);
> + return -EIO;
> + }
> + }
> +
> + pr_info("CPU core count changed, reboot required\n");
This interface has a problematic behavior. If user wants to adjust both
core counts one after another (without reboot in between), the new value
of the first core count will be overwritten on the second store.
You might have to store also the value that will be used after the next
boot to solve it but how the divergence should be presented to user is
another question to which I don't have a good answer.
This seems a more general problem, that is, how to represent values which
are only enacted after booting (current vs to-be-current) as it doesn't
fit to the current, min, max, possible_values, type model.
--
i.
> + sysfs_notify(kobj, NULL, attr->attr.name);
> + asus_set_reboot_and_signal_event();
> +
> + return 0;
> +}
> +
> +static ssize_t cores_performance_min_value_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_PERF, CPU_CORE_MIN);
> +}
> +
> +static ssize_t cores_performance_max_value_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_PERF, CPU_CORE_MAX);
> +}
> +
> +static ssize_t cores_performance_default_value_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_PERF, CPU_CORE_DEFAULT);
> +}
> +
> +static ssize_t cores_performance_current_value_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_PERF, CPU_CORE_CURRENT);
> +}
> +
> +static ssize_t cores_performance_current_value_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int err;
> +
> + err = cores_current_value_store(kobj, attr, buf, CPU_CORE_PERF);
> + if (err)
> + return err;
> +
> + return count;
> +}
> +ATTR_GROUP_CORES_RW(cores_performance, "cores_performance",
> + "Set the max available performance cores");
> +
> +static ssize_t cores_efficiency_min_value_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_POWER, CPU_CORE_MIN);
> +}
> +
> +static ssize_t cores_efficiency_max_value_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_POWER, CPU_CORE_MAX);
> +}
> +
> +static ssize_t cores_efficiency_default_value_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_POWER, CPU_CORE_DEFAULT);
> +}
> +
> +static ssize_t cores_efficiency_current_value_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return cores_value_show(kobj, attr, buf, CPU_CORE_POWER, CPU_CORE_CURRENT);
> +}
> +
> +static ssize_t cores_efficiency_current_value_store(struct kobject *kobj,
> + struct kobj_attribute *attr, const char *buf,
> + size_t count)
> +{
> + int err;
> +
> + err = cores_current_value_store(kobj, attr, buf, CPU_CORE_POWER);
> + if (err)
> + return err;
> +
> + return count;
> +}
> +ATTR_GROUP_CORES_RW(cores_efficiency, "cores_efficiency",
> + "Set the max available efficiency cores");
> +
> /* Simple attribute creation */
> ATTR_GROUP_ENUM_INT_RO(charge_mode, "charge_mode", ASUS_WMI_DEVID_CHARGE_MODE, "0;1;2",
> "Show the current mode of charging");
> @@ -495,6 +740,8 @@ static const struct asus_attr_group armoury_attr_groups[] = {
> { &egpu_enable_attr_group, ASUS_WMI_DEVID_EGPU },
> { &dgpu_disable_attr_group, ASUS_WMI_DEVID_DGPU },
> { &apu_mem_attr_group, ASUS_WMI_DEVID_APU_MEM },
> + { &cores_efficiency_attr_group, ASUS_WMI_DEVID_CORES_MAX },
> + { &cores_performance_attr_group, ASUS_WMI_DEVID_CORES_MAX },
>
> { &charge_mode_attr_group, ASUS_WMI_DEVID_CHARGE_MODE },
> { &boot_sound_attr_group, ASUS_WMI_DEVID_BOOT_SOUND },
> @@ -598,6 +845,7 @@ static int asus_fw_attr_add(void)
> static int __init asus_fw_init(void)
> {
> char *wmi_uid;
> + int err;
>
> wmi_uid = wmi_get_acpi_device_uid(ASUS_WMI_MGMT_GUID);
> if (!wmi_uid)
> @@ -610,6 +858,14 @@ static int __init asus_fw_init(void)
> if (!strcmp(wmi_uid, ASUS_ACPI_UID_ASUSWMI))
> return -ENODEV;
>
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_CORES_MAX)) {
> + err = init_max_cpu_cores();
> + if (err) {
> + pr_err("Could not initialise CPU core control %d\n", err);
> + return err;
> + }
> + }
> +
> return asus_fw_attr_add();
> }
>
> diff --git a/drivers/platform/x86/asus-armoury.h b/drivers/platform/x86/asus-armoury.h
> index 61675e7b5a60..a6c4caefdef9 100644
> --- a/drivers/platform/x86/asus-armoury.h
> +++ b/drivers/platform/x86/asus-armoury.h
> @@ -161,4 +161,32 @@
> .name = _fsname, .attrs = _attrname##_attrs \
> }
>
> +/* CPU core attributes need a little different in setup */
> +#define ATTR_GROUP_CORES_RW(_attrname, _fsname, _dispname) \
> + __ATTR_SHOW_FMT(scalar_increment, _attrname, "%d\n", 1); \
> + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \
> + static struct kobj_attribute attr_##_attrname##_current_value = \
> + __ASUS_ATTR_RW(_attrname, current_value); \
> + static struct kobj_attribute attr_##_attrname##_default_value = \
> + __ASUS_ATTR_RO(_attrname, default_value); \
> + static struct kobj_attribute attr_##_attrname##_min_value = \
> + __ASUS_ATTR_RO(_attrname, min_value); \
> + static struct kobj_attribute attr_##_attrname##_max_value = \
> + __ASUS_ATTR_RO(_attrname, max_value); \
> + static struct kobj_attribute attr_##_attrname##_type = \
> + __ASUS_ATTR_RO_AS(type, int_type_show); \
> + static struct attribute *_attrname##_attrs[] = { \
> + &attr_##_attrname##_current_value.attr, \
> + &attr_##_attrname##_default_value.attr, \
> + &attr_##_attrname##_min_value.attr, \
> + &attr_##_attrname##_max_value.attr, \
> + &attr_##_attrname##_scalar_increment.attr, \
> + &attr_##_attrname##_display_name.attr, \
> + &attr_##_attrname##_type.attr, \
> + NULL \
> + }; \
> + static const struct attribute_group _attrname##_attr_group = { \
> + .name = _fsname, .attrs = _attrname##_attrs \
> + }
> +
> #endif /* _ASUS_ARMOURY_H_ */
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index a4f6bab93a6f..9a79dae97adf 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -137,6 +137,11 @@
> /* dgpu on/off */
> #define ASUS_WMI_DEVID_DGPU 0x00090020
>
> +/* Intel E-core and P-core configuration in a format 0x0[E]0[P] */
> +#define ASUS_WMI_DEVID_CORES 0x001200D2
> + /* Maximum Intel E-core and P-core availability */
> +#define ASUS_WMI_DEVID_CORES_MAX 0x001200D3
> +
> #define ASUS_WMI_DEVID_APU_MEM 0x000600C1
>
> /* gpu mux switch, 0 = dGPU, 1 = Optimus */
>
Powered by blists - more mailing lists