Message-ID: <3da451d6-d57b-4b6b-8d24-9fd77924e777@amd.com>
Date: Mon, 17 Nov 2025 15:58:37 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: luke@...nes.dev, Denis Benato <denis.benato@...ux.dev>
Cc: linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 Hans de Goede <hansg@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 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: remove unsafe CPU cores
 unsafe interface



On 11/17/2025 3:50 PM, luke@...nes.dev wrote:
> 
>> On 18 Nov 2025, at 07:47, Denis Benato <denis.benato@...ux.dev> wrote:
>>
>>
>> On 11/17/25 05:27, luke@...nes.dev wrote:
>>>> On 16 Nov 2025, at 03:51, Denis Benato <denis.benato@...ux.dev> wrote:
>>>>
>>>> CPU cores unsafe interface is known to be problematic and recently new
>>>> hardware that is using it in an incompatible way was released, therefore
>>>> remove this interface to avoid any present and future risks.
>>>>
>>>> Link: https://lore.kernel.org/all/018ce202-420c-40c9-a089-b3509c7ee4bd@gmail.com
>>> I’ll copy/paste my response to other thread here:
>>>
>>> I spent a long time debating this myself. Never liked it tbh (the implementation or the actual setting).
>>> 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.
>> I haven't considered the mixed usage of windows/linux,
>> perhaps we can rely on the BIOS to change the core count, but
>> thinking more about this considering:
>> - the user-facing interface is correct
>> - this interface is unsafe only when writing
>> - it must not prevent the rest of the attributes to function
> 
> As I no-longer have horse in this race the outcome doesn’t actually affect me.
> I raise the point merely because I am still familiar with many parts of the driver
> and the reversing of much of the WMI interfaces plus the interaction between
> Windows and Linux.
> 
> In general, if a WMI interface setting requires reboot to change (which means
> It is stored across boots), or is generally stored and retained across boots then
> I very very strongly suggest those be exposed in Linux driver so they can be
> tracked in userspace. I’ve had more than a few users complain about something
> That was changed in Windows which then caused unexpected effect in Linux
> and then because it was not visible in Linux anywhere it was rather difficult to
> determine what changed.
> 
>>> 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.
>>> If users dual boot at all and use the armoury crate to control this, then that app will use the WMI methods. Again leading to confusion in Linux environment. This is actually the main reason I implemented it first time around and it was initially a read-only attribute.
>>>
>>> Do what is best for users and maintenance and safety. At least make it RO at a minimum.
>> I think I will propose to make the _store compile-time disabled behind a config that can't be enabled.
>>
>> This way the code is kept and I can work on it and users will get the RO version
>> until we are very happy with the attribute write.
> 
> I would strongly suggest RO, permanently. The write method could be
> documented in a comment block for reference with a note about why RO
> is default (I mean how it’s written, not the function itself).
> 
> Perhaps if the read core count is differing to the maximum a kernel warning
> could be emitted so it gets logged and potential future bug reports about
> cores catch it early.
> 
> This *is* going to rely on BIOS continuing to expose the core selection or
> dual boot so Windows can change it. Do also talk to Sergei over at ghelper
> about this - he gets a *much* wider range of users and testing due to being
> Windows focused.
> 
>>
>> This also has the benefit of not removing the code that was just merged
>> hopefully making things easier for everybody.
> 
> You can remove the write parts fine in a new patch version if agreed on.
> 
>>
>> To proceed I will fuse [1] after integrating Mario's suggestions to the
>> making it RO, unless someone isn't opposed for some reason.
>>
> 
> Mario I do hope you agree with my assessment on this. Making this attribute RO
> would at least prevent a great deal of confusion thanks to ASUS quirks and also
> allow the stripping out of a chunk of now redundant code. This way it defers to
> the preferred AMD methods, but exposes if the WMI method was used in a dual
> boot scenario.
> 


I don't see a problem with it being read-only, but please make sure it's 
not critical path for driver init if it's read-only.

IE:
* If any part of it (reading interfaces, counts not matching, etc) fail 
then show a debug message, not an error/warning.
* Hide attributes unless the interface for reading works.

Then you shouldn't need to qualify anything against any system.

We also should keep in mind that Armin has plans for a WMI MOF parser in 
the future in the kernel.  I anticipate this is going to expose this 
interface again as writable.

It sounds like it's a year or two out, but WHEN that comes in we should 
make sure that the design at that time prevents people from using this 
for a 🦶 🔫 too.

Powered by blists - more mailing lists