Message-ID: <362f2353-8529-47a0-9cfb-d54015e7f9f3@kernel.org>
Date: Wed, 12 Nov 2025 13:24:50 -0600
From: Mario Limonciello <superm1@...nel.org>
To: Denis Benato <benato.denis96@...il.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Denis Benato <denis.benato@...ux.dev>, LKML
 <linux-kernel@...r.kernel.org>, platform-driver-x86@...r.kernel.org,
 Hans de Goede <hansg@...nel.org>,
 "Limonciello, Mario" <mario.limonciello@....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 v17 0/9] platform/x86: Add asus-armoury driver

On 11/12/25 1:23 PM, Denis Benato wrote:
> 
> On 11/12/25 14:44, Mario Limonciello wrote:
>>
>>
>> On 11/12/25 6:42 AM, Ilpo Järvinen wrote:
>>> On Tue, 11 Nov 2025, Denis Benato wrote:
>>>> On 11/11/25 11:38, Ilpo Järvinen wrote:
>>>>> On Mon, 10 Nov 2025, Denis Benato wrote:
>>>>>> On 11/10/25 16:17, Ilpo Järvinen wrote:
>>>>>>> On Sun, 2 Nov 2025, Denis Benato wrote:
>>>>>>>
>>>>>>>> the TL;DR:
>>>>>>>> 1. Introduce new module to contain bios attributes, using fw_attributes_class
>>>>>>>> 2. Deprecate all possible attributes from asus-wmi that were added ad-hoc
>>>>>>>> 3. Remove those in the next LTS cycle
>>>>>>>>
>>>>>>>> The idea for this originates from a conversation with Mario Limonciello
>>>>>>>> https://lore.kernel.org/platform-driver-x86/371d4109-a3bb-4c3b-802f-4ec27a945c99@amd.com/
>>>>>>>>
>>>>>>>> It is without a doubt much cleaner to use, easier to discover, and the
>>>>>>>> API is well defined as opposed to the random clutter of attributes I had
>>>>>>>> been placing in the platform sysfs. Given that Derek is also working on a
>>>>>>>> similar approach to Lenovo in part based on my initial work I'd like to think
>>>>>>>> that the overall approach is good and may become standardised for these types
>>>>>>>> of things.
>>>>>>>>
>>>>>>>> Regarding PPT: it is intended to add support for "custom" platform profile
>>>>>>>> soon. If it's a blocker for this patch series being accepted I will drop the
>>>>>>>> platform-x86-asus-armoury-add-ppt_-and-nv_-tuning.patch and get that done
>>>>>>>> separately to avoid holding the bulk of the series up. Ideally I would like
>>>>>>>> to get the safe limits in so users don't fully lose functionality or continue
>>>>>>>> to be exposed to potential instability from setting too low, or be mislead
>>>>>>>> in to thinking they can set limits higher than actual limit.
>>>>>>>>
>>>>>>>> The bulk of the PPT patch is data, the actual functional part is relatively
>>>>>>>> small and similar to the last version.
>>>>>>>>
>>>>>>>> Unfortunately I've been rather busy over the months and may not cover
>>>>>>>> everything in the v7 changelog but I've tried to be as comprehensive as I can.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Luke
>>>>>>>>
>>>>>>>> Changelog:
>>>>>>>> - v1
>>>>>>>>     - Initial submission
>>>>>>>> - v2
>>>>>>>>     - Too many changes to list, but all concerns raised in previous submission addressed.
>>>>>>>>     - History: https://lore.kernel.org/platform-driver-x86/20240716051612.64842-1-luke@ljones.dev/
>>>>>>>> - v3
>>>>>>>>     - All concerns addressed.
>>>>>>>>     - History: https://lore.kernel.org/platform-driver-x86/20240806020747.365042-1-luke@ljones.dev/
>>>>>>>> - v4
>>>>>>>>     - Use EXPORT_SYMBOL_NS_GPL() for the symbols required in this patch series
>>>>>>>>     - Add patch for hid-asus due to the use of EXPORT_SYMBOL_NS_GPL()
>>>>>>>>     - Split the PPT knobs out to a separate patch
>>>>>>>>     - Split the hd_panel setting out to a new patch
>>>>>>>>     - Clarify some of APU MEM configuration and convert int to hex
>>>>>>>>     - Rename deprecated Kconfig option to ASUS_WMI_DEPRECATED_ATTRS
>>>>>>>>     - Fixup cyclic dependency in Kconfig
>>>>>>>> - v5
>>>>>>>>     - deprecate patch: cleanup ``#if`, ``#endif` statements, edit kconfig detail, edit commit msg
>>>>>>>>     - cleanup ppt* tuning patch
>>>>>>>>     - proper error handling in module init, plus pr_err()
>>>>>>>>     - ppt tunables have a notice if there is no match to get defaults
>>>>>>>>     - better error handling in cpu core handling
>>>>>>>>       - don't continue if failure
>>>>>>>>     - use the mutex to gate WMI writes
>>>>>>>> - V6
>>>>>>>>     - correctly cleanup/unwind if module init fails
>>>>>>>> - V7
>>>>>>>>     - Remove review tags where the code changed significantly
>>>>>>>>     - Add auto_screen_brightness WMI attribute support
>>>>>>>>     - Move PPT patch to end
>>>>>>>>     - Add support min/max PPT values for 36 laptops (and two handhelds)
>>>>>>>>     - reword commit for "asus-wmi: export symbols used for read/write WMI"
>>>>>>>>     - asus-armoury: move existing tunings to asus-armoury
>>>>>>>>       - Correction to license header
>>>>>>>>       - Remove the (initial) mutex use (added for core count only in that patch)
>>>>>>>>       - Clarify some doc comments (attr_int_store)
>>>>>>>>       - Cleanup pr_warn in dgpu/egpu/mux functions
>>>>>>>>       - Restructure logic in asus_fw_attr_add()
>>>>>>>>       - Check gpu_mux_dev_id and mini_led_dev_id before remove attrs
>>>>>>>>     - asus-armoury: add core count control:
>>>>>>>>       - add mutex to prevent possible concurrent write to the core
>>>>>>>>         count WMI due to separated bit/little attributes
>>>>>>>>     - asus-armoury: add ppt_* and nv_* tuning knobs:
>>>>>>>>       - Move to end of series
>>>>>>>>       - Refactor to use a table of allowed min/max values to
>>>>>>>>         ensure safe settings
>>>>>>>>       - General code cleanup
>>>>>>>>     - Ensure checkpatch.pl returns clean for all
>>>>>>>> - V8
>>>>>>>>     - asus-armoury: move existing tunings to asus-armoury module
>>>>>>>>       - Further cleanup: https://lore.kernel.org/platform-driver-x86/20250316230724.100165-2-luke@ljones.dev/T/#m72e203f64a5a28c9c21672406b2e9f554a8a8e38
>>>>>>>>     - asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - Address concerns in https://lore.kernel.org/platform-driver-x86/20250316230724.100165-2-luke@ljones.dev/T/#m77971b5c1e7f018954c16354e623fc06522c5e41
>>>>>>>>       - Refactor struct asus_armoury_priv to record both AC and DC settings
>>>>>>>>       - Tidy macros and functions affected by the above to be clearer as a result
>>>>>>>>       - Move repeated strings such as "ppt_pl1_spl" to #defines
>>>>>>>>       - Split should_create_tunable_attr() in to two functions to better clarify:
>>>>>>>>         - is_power_tunable_attr()
>>>>>>>>         - has_valid_limit()
>>>>>>>>       - Restructure init_rog_tunables() to initialise AC and DC in a
>>>>>>>>         way that makes more sense.
>>>>>>>>       - Ensure that if DC setting table is not available then attributes
>>>>>>>>         return -ENODEV only if on DC mode.
>>>>>>>> - V9
>>>>>>>>     - asus-armoury: move existing tunings to asus-armoury module
>>>>>>>>       - return -EBUSY when eGPU/dGPU cannot be deactivated
>>>>>>>>     - asus-armoury: add apu-mem control support
>>>>>>>>       - discard the WMI presence bit fixing the functionality
>>>>>>>>     - asus-armoury: add core count control
>>>>>>>>       - replace mutex lock/unlock with guard
>>>>>>>>       - move core count alloc for initialization in init_max_cpu_cores()
>>>>>>>> - v10
>>>>>>>>     - platform/x86: asus-wmi: export symbols used for read/write WMI
>>>>>>>>       - fix error with redefinition of asus_wmi_set_devstate
>>>>>>>>     - asus-armoury: move existing tunings to asus-armoury module
>>>>>>>>       - hwmon or other -> hwmon or others
>>>>>>>>       - fix wrong function name in documentation (attr_uint_store)
>>>>>>>>       - use kstrtouint where appropriate
>>>>>>>>       - (*) fix unreachable code warning: the fix turned out to be partial
>>>>>>>>       - improve return values in case of error in egpu_enable_current_value_store
>>>>>>>>     - asus-armoury: asus-armoury: add screen auto-brightness toggle
>>>>>>>>       - actually register screen_auto_brightness attribute
>>>>>>>> - v11
>>>>>>>>     - cover-letter:
>>>>>>>>       - reorganize the changelog of v10
>>>>>>>>     - asus-armoury: move existing tunings to asus-armoury module
>>>>>>>>       - move the DMIs list in its own include, fixing (*) for good
>>>>>>>>     - asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - fix warning about redefinition of ppt_pl2_sppt_def for GV601R
>>>>>>>> - v12
>>>>>>>>     - asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - add min/max values for FA608WI and FX507VI
>>>>>>>> - v13
>>>>>>>>     - asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - fix a typo in a comment about _def attributes
>>>>>>>>       - add min/max values for GU605CW and G713PV
>>>>>>>>     - asus-armoury: add apu-mem control support
>>>>>>>>       - fix a possible out-of-bounds read in apu_mem_current_value_store
>>>>>>>> - v14
>>>>>>>>     - platform/x86: asus-wmi: rename ASUS_WMI_DEVID_PPT_FPPT
>>>>>>>>       - added patch to rename the symbol for consistency
>>>>>>>>     - platform/x86: asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - remove the unchecked usage of dmi_get_system_info while
>>>>>>>>         also increasing consistency with other messages
>>>>>>>> - v15
>>>>>>>>     - platform/x86: asus-wmi: export symbols used for read/write WMI
>>>>>>>>       - fix kernel doc
>>>>>>>>     - platform/x86: asus-armoury: move existing tunings to asus-armoury module
>>>>>>>>       - avoid direct calls to asus-wmi and provide helpers instead
>>>>>>>>       - rework xg mobile activation logic
>>>>>>>>       - add helper for enum allowed attributes
>>>>>>>>       - improve mini_led_mode_current_value_store
>>>>>>>>       - improved usage of kstrtouint, kstrtou32 and kstrtobool
>>>>>>>>       - unload attributes in reverse order of loading
>>>>>>>>     - platform/x86: asus-armoury: add apu-mem control support
>>>>>>>>       - fix return value in apu_mem_current_value_show
>>>>>>>>     - platform/x86: asus-armoury: add core count control
>>>>>>>>       - put more safeguards in place against possible bricking of laptops
>>>>>>>>       - improve loading logic
>>>>>>>>     - platform/x86: asus-wmi: deprecate bios features
>>>>>>>>       - modified deprecation message
>>>>>>>>     - platform/x86: asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - make _store(s) to interfaces unusable in DC to fail,
>>>>>>>>         instead of accepting 0 as a value (0 is also invalid)
>>>>>>>>       - make it easier to understand AC vs DC logic
>>>>>>>>       - improved init_rog_tunables() logic
>>>>>>>>       - commas after every field in the table for consistency
>>>>>>>>       - add support for RC73 handheld
>>>>>>>> -v16
>>>>>>>>     - platform/x86: asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - add support for GU605CX
>>>>>>>> -v17
>>>>>>>>     - platform/x86: asus-armoury: add ppt_* and nv_* tuning knobs
>>>>>>>>       - fix RC73 -> RC73AX as another RC73 exists
>>>>>>>>     - platform/x86: asus-armoury: add core count control
>>>>>>>>       - be more tolerant on out-or-range current CPU cores count
>>>>>>>>     - platform/x86: asus-armoury: move existing tunings to asus-armoury module
>>>>>>>>       - fix usage of undeclared static functions in macros
>>>>>>> I've applied this to the review-ilpo-next branch. I'm still not entirely
>>>>>>> happy with how the cpu cores change does store values without arrays but
>>>>>>> it's not an end of the world (and could be fixed in tree).
>>>>>> Hello and thanks.
>>>>>>
>>>>>> You would make me very happy applying things as Luke wrote them
>>>>>> so that successive modifications are more easily compared to
>>>>>> what those were doing before I changed them...
>>>>> I just took them as they were so you should be "happy" now :-)
>>>>>
>>>>> ...Even if I didn't like having all those as separate variables requiring
>>>>> if statements here and there, which could be avoided if core type would be
>>>>> an array index so one could simply do:
>>>>>
>>>>>      ...
>>>>>      case CPU_CORE_MAX:
>>>>>          cpu_core_value = asus_armoury.cpu_cores[core_type]->max;
>>>>>          break;
>>>>>      ...
>>>>>
>>>>> Doing that transformation incrementally looks simple enough it should be
>>>>> low risk after a careful review.
>>>>>
>>>>>
>>>> Apparently one of the two new handhelds from asus reports
>>>> weird numbers for core count so that area requires a bit of work
>>>> anyway. I will soon move to investigate that hardware.
>>>>
>>>>>> Also if you have some more hints on how I could change that
>>>>>> interface (while avoiding bad surprises due to index mismatch)
>>>>>> I will try my best... without destroying any laptop...
>>>>>> perhaps... Hopefully? Wish me luck.
>>>>>>
>>>>>>> I had to reorder a few includes to make the order alphabetical which
>>>>>>> luckily worked out without causing conflicts within the subsequent
>>>>>>> patches (and a need to respin the series). Please try to remember to
>>>>>>> keep those in the alphabetical order.
>>>>>> I have noticed a pair of warnings in this v17 I would like to solve:
>>>>>> one line is too long, I should break it and one macro has an
>>>>>> unused parameter.
>>>>>>
>>>>>> No semantic changes.
>>>>>>
>>>>>> I have seen one of those unordered includes in asus-armoury.h...
>>>>>> That branch is public in your git tree: this means I can respin
>>>>>> a v18 from a git format-path, correct?
>>>>>
>>>>> While I could replace the previous series with a new version, it would
>>>>> probably just be better to send incremental patches and I can see myself
>>>>> if I fold them into the existing patches or not.
>>>>
>>>> Ah forgive me, I am not used to the process and understood
>>>> something totally different. All good, patches sent, thanks!
>>>>
>>>> Would you also want to break the long assignment line?
>>>> Is it better if it's just one long line for clarity?
>>>>
>>>> ```
>>>> const struct rog_tunables *const ac_rog_runables = ....
>>>> ```
>>>
>>> I'm not sure what that second const gains us here so I'd prefer removing
>>> it.
>>>
>>> It's a local variable so it doesn't look like protecting the variable
>>> itself with const is that important (in contrast to things which are in
>>> global scope where const for the variable too is good).
>>>
>>> Are you aware of scripts/checkpatch.pl? I think it would have caught this
>>> (if one remembers to run it before sending patches, which is the hardest
>>> part with that tool :-)).
>>>
>> In case you aren't aware, two other quality of life improvements I want to share to look into:
>>
>> 1) There is a vscode extension for checkpatch
>>
>> It can be configured to automatically run when you save the file and then will underline all the failures with blue squiggly lines.
>>
>> https://marketplace.visualstudio.com/items?itemName=idanp.checkpatch
>>
> It has been in place since the first time you recommended it to me,
> but apparently it's only highlighting errors? In blue color?
> 
> Why isn't it telling me about warnings? :(

Make sure that you set the path for checkpatch.pl properly in the 
extension settings and that you set it to run on every save.

>> 2) There is a pre-commit hook for checkpatch
>>
>> This will help make sure that it's "automatically" run on every code commit you do at the right time you make the patch (ie on a per-patch basis).
>>
>> https://github.com/dloidolt/pre-commit-checkpatch
> I am going to check this as well, thanks!

Powered by blists - more mailing lists