linux-kernel - Re: [PATCH] platform/x86: asus-armoury: do not abort probe on unexpected CPU cores count

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8b999b96-f1bf-4231-b2f8-5c4a55e21d5f@amd.com>
Date: Fri, 14 Nov 2025 14:35:01 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Denis Benato <denis.benato@...ux.dev>, linux-kernel@...r.kernel.org
Cc: platform-driver-x86@...r.kernel.org, Hans de Goede <hansg@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 "Luke D . Jones" <luke@...nes.dev>, Alok Tiwari <alok.a.tiwari@...cle.com>,
 Derek John Clark <derekjohn.clark@...il.com>,
 Mateusz Schyboll <dragonn@...pl>, porfet828@...il.com,
 Denis Benato <benato.denis96@...il.com>
Subject: Re: [PATCH] platform/x86: asus-armoury: do not abort probe on
 unexpected CPU cores count

On 11/14/2025 12:53 PM, Denis Benato wrote:
> Until now the CPU cores count was only available for
> Intel hardware, however a few weeks ago an AMD hardware
> that provides aforementioned interface appeared on the
> market and data read from the interface doesn't
> follow the expected format and the driver fails to probe.

As a general statement; I don't like this feature at all.  You've said 
yourself that it's bricked systems.  Now it's not working on a bunch of 
systems due to mismatched expectations.

We already have core parking in Linux at runtime (you can trivially 
write 'offline' to any core and the kernel will put it in the right 
power state and stop using it).

So if it was me I would say axe the feature all together, or make it 
experimental and opt-in via a module parameter.

But nonetheless if you decide to keep it; code review for the patch is 
below.

> 
> Avoid failing on invalid cores count and print out debug information.

You seem to be printing it out all at err level not debug level.

> 
> Signed-off-by: Denis Benato <denis.benato@...ux.dev>
> ---
>   drivers/platform/x86/asus-armoury.c | 34 ++++++++++++++++++++++++-----
>   1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
> index 9f67218ecd14..6355ec3e253f 100644
> --- a/drivers/platform/x86/asus-armoury.c
> +++ b/drivers/platform/x86/asus-armoury.c
> @@ -818,10 +818,23 @@ static struct cpu_cores *init_cpu_cores_ctrl(void)
>   	cores_p->min_power_cores = CPU_POWR_CORE_COUNT_MIN;
>   	cores_p->min_perf_cores = CPU_PERF_CORE_COUNT_MIN;
>   
> +	if (cores_p->min_perf_cores > cores_p->max_perf_cores) {
> +		pr_err("Invalid CPU performance cores count detected: min: %u, max: %u, current: %u\n",
> +		       cores_p->min_perf_cores,
> +		       cores_p->max_perf_cores,
> +		       cores_p->cur_perf_cores
> +		);

Should this be debug level?

> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	if ((cores_p->min_perf_cores > cores_p->max_perf_cores) ||
>   	    (cores_p->min_power_cores > cores_p->max_power_cores)
>   	) {
> -		pr_err("Invalid CPU cores count detected: interface is not safe to be used.\n");
> +		pr_err("Invalid CPU efficiency cores count detected: min: %u, max: %u, current: %u\n",
> +		       cores_p->min_power_cores,
> +		       cores_p->max_power_cores,
> +		       cores_p->cur_power_cores
> +		);

Should this be debug level?

>   		return ERR_PTR(-EINVAL);
>   	}
>   
> @@ -841,6 +854,11 @@ static ssize_t cores_value_show(struct kobject *kobj, struct kobj_attribute *att
>   {
>   	u32 cpu_core_value;
>   
> +	if (asus_armoury.cpu_cores == NULL) {
> +		pr_err("CPU core control interface was not initialized.\n");
> +		return -ENODEV;
> +	}
> +

I think you should control the visibility of the attribute instead. 
There is no point making an attribute that will always show an error.

>   	switch (core_value) {
>   	case CPU_CORE_DEFAULT:
>   	case CPU_CORE_MAX:
> @@ -875,6 +893,11 @@ static ssize_t cores_current_value_store(struct kobject *kobj, struct kobj_attri
>   	if (result)
>   		return result;
>   
> +	if (asus_armoury.cpu_cores == NULL) {
> +		pr_err("CPU core control interface was not initialized.\n");
> +		return -ENODEV;
> +	}
> +
>   	scoped_guard(mutex, &asus_armoury.cpu_core_mutex) {
>   		if (!asus_armoury.cpu_cores_changeable) {
>   			pr_warn("CPU core count change not allowed until reboot\n");
> @@ -1389,16 +1412,17 @@ static int __init asus_fw_init(void)
>   		return -ENODEV;
>   
>   	asus_armoury.cpu_cores_changeable = false;
> +	asus_armoury.cpu_cores = NULL;
>   	if (armoury_has_devstate(ASUS_WMI_DEVID_CORES_MAX)) {
>   		cpu_cores_ctrl = init_cpu_cores_ctrl();
>   		if (IS_ERR(cpu_cores_ctrl)) {
>   			err = PTR_ERR(cpu_cores_ctrl);
>   			pr_err("Could not initialise CPU core control: %d\n", err);

AFAICT you don't need the err variable anymore for this function.

This can just be:

if (IS_ERR(cpu_cores_ctrl))
	pr_err("Could not initialise CPU core control: %d\n", err);
else {
...
}

> -			return err;
> +		} else {
> +			pr_debug("CPU cores control available.\n");
> +			asus_armoury.cpu_cores = cpu_cores_ctrl;
> +			asus_armoury.cpu_cores_changeable = true;
>   		}
> -
> -		asus_armoury.cpu_cores = cpu_cores_ctrl;
> -		asus_armoury.cpu_cores_changeable = true;
>   	}
>   
>   	init_rog_tunables();