Message-ID: <f3a69b7e-a698-2dd5-731a-f7db0eabd8f3@linux.intel.com>
Date: Mon, 20 Oct 2025 20:15:10 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Denis Benato <benato.denis96@...il.com>
cc: Mario Limonciello <superm1@...nel.org>, 
    Hans de Goede <hdegoede@...hat.com>, 
    Mark Pearson <mpearson-lenovo@...ebb.ca>, 
    "Luke D . Jones" <luke@...nes.dev>, 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 Sat, 18 Oct 2025, Denis Benato wrote:
> On 10/17/25 14:48, Ilpo Järvinen wrote:
> > 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

> >> +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.
>
> Please note that this interface has the potential to brick in an irreversible
> way laptops and it has happened.

This function only prints values held by kernel in its internal storage? 
How can that brick something?

> Of all the code CPU core handling it the most delicate code of all since
> a wrong value here means permanent irreversible damage.
> 
> I am more than happy making changes that can be easily verified,
> but others more complex changes will put (at least) my own hardware
> at risk.

Understood.

> If you think benefit outweighs risks I will try my best.
> >> +	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.
>
> Given what I have written above I am thinking more along the lines of allowing
> only one change at the time, giving absolute priority to the ability to demonstrate
> not all P-cores can be disabled all at once, or after a reboot hardware will be
> irreversibly lost. It cannot be recovered the same way as I did with APU mem.
> 
> To summarize I am more inclined in allowing only small changes,
> or to postpone this patch entirely while we think to a better, safer solution.

Fair enough, please mention this as in the changelog as a justification 
and to warn other messing with the code.

> > 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.
> >
> Enabling eGPU on a laptop with a dGPU that is active makes the PCI-e
> spam PCI AER uncorrectable errors and renders both GPUs unusable.
> 
> I have gone with storing the value at driver load time and treating it
> as the boot value for 2/9 (eGPU), but I am very open to suggestions!

I'm not entirely sure what you're trying to say here. My point is that 
there's no way for user to know what value something will be changed to 
(the "to-be-current" value) except through rebooting the system (when the 
"to-be-current" value becomes the "current").

-- 
 i.

Powered by blists - more mailing lists