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

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

[Denis Benato]

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.

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

Signed-off-by: Denis Benato <denis.benato@linux.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
+		);
+		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
+		);
 		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;
+	}
+
 	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);
-			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();
-- 
2.51.2

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

[Mario Limonciello]

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@linux.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();

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

[Denis Benato]

On 11/14/25 21:35, Mario Limonciello wrote:
>
>
> 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. 
>
These past days I reflected much on this feature and these are the major factors
that contributed to the current position and future steps I think are appropriate:

- I didn't want to drop work already done since I promised to fulfill it

- I tried to satisfy all involved parties

- This interface is very unsafe

- This interface appears to exist to solve a windows problem that
doesn't exist in linux (disabling specific cores already work)

- How the code is written is not liked by a maintainer

- Reworking this interface is difficult for all the wrong reasons

- This interface requires a reboot unlike the linux-specific one

And since a few days, as discovered here:
https://gitlab.com/asus-linux/asusctl/-/issues/695

- This interface has already become problematic and it's just
a few days from its introduction.

Considering the fact that running today's code in future
laptops might very well increase the risk of rendering hardware
unusable (what if on some future CPUs the number of minimum
core becomes 8? What if new CPUs shows up with a third type of
core that must not be disabled and the interface does always
set it to zero?) I want to apply my own judgment here and say:
"let's drop this interface and ensure it's not used in its current state".

Please Luke forgive my decision, but for now I don't feel like
putting more work on it makes anyone happy and my time will be
better spent in other more important areas.

I will soon send a patch to exclude the core control interface:
please Ilpo, unless you can think of a good reason to keep it
(such as Intel CPUs having a different power drawn depending
on how cores are disabled) or you know a better way of removing it,
accept my upcoming patch to remove this interface from the kernel.

Sorry for the trouble,
Denis

>>
>> 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@linux.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();
>

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

[luke]

> On 16 Nov 2025, at 02:38, Denis Benato <benato.denis96@gmail.com> wrote:
> 
> 
> On 11/14/25 21:35, Mario Limonciello wrote:
>> 
>> 
>> 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. 
>> 
> These past days I reflected much on this feature and these are the major factors
> that contributed to the current position and future steps I think are appropriate:
> 
> - I didn't want to drop work already done since I promised to fulfill it
> 
> - I tried to satisfy all involved parties
> 
> - This interface is very unsafe
> 
> - This interface appears to exist to solve a windows problem that
> doesn't exist in linux (disabling specific cores already work)
> 
> - How the code is written is not liked by a maintainer
> 
> - Reworking this interface is difficult for all the wrong reasons
> 
> - This interface requires a reboot unlike the linux-specific one
> 
> And since a few days, as discovered here:
> https://gitlab.com/asus-linux/asusctl/-/issues/695
> 
> - This interface has already become problematic and it's just
> a few days from its introduction.
> 
> Considering the fact that running today's code in future
> laptops might very well increase the risk of rendering hardware
> unusable (what if on some future CPUs the number of minimum
> core becomes 8? What if new CPUs shows up with a third type of
> core that must not be disabled and the interface does always
> set it to zero?) I want to apply my own judgment here and say:
> "let's drop this interface and ensure it's not used in its current state".
> 
> Please Luke forgive my decision, but for now I don't feel like
> putting more work on it makes anyone happy and my time will be
> better spent in other more important areas.

I spent a long time debating this myself. Never liked it tbh.
It seems that at least making it RO so that if a user changes the settings using the BIOS controls for it, it can at least be tracked in userspace and not lead to confusion might be the best option. It only needs to show the values then - no min/max or other parts required.
This setting is independent to the AMD version and if BIOS changes it, it may lead to a lot of confusion between this plus the official AMD setting.
Intel is a different story.

Do what is best for users and maintenance and safety. At least make it RO at a minimum.

Cheers,
Luke.

> 
> I will soon send a patch to exclude the core control interface:
> please Ilpo, unless you can think of a good reason to keep it
> (such as Intel CPUs having a different power drawn depending
> on how cores are disabled) or you know a better way of removing it,
> accept my upcoming patch to remove this interface from the kernel.
> 
> Sorry for the trouble,
> Denis
> 
>>> 
>>> 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@linux.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();