Message-ID: <018ce202-420c-40c9-a089-b3509c7ee4bd@gmail.com>
Date: Sat, 15 Nov 2025 14:38:43 +0100
From: Denis Benato <benato.denis96@...il.com>
To: Mario Limonciello <mario.limonciello@....com>,
 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
Subject: Re: [PATCH] platform/x86: asus-armoury: do not abort probe on
 unexpected CPU cores count


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

Powered by blists - more mailing lists