From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BA4AB344025 for ; Wed, 12 Nov 2025 19:21:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762975262; cv=none; b=E2P3CtfhYLFlGdP6QtVbAHc2xummpDpqbZwSkRg4/OjjZ1ixpWiiG6bv/8iEpnHapkFssCAzMJzb8Dsys5wd7QcqP2/vlypRg6ETTElXCJgJdG2+W2caEtp0fYW1vrXMSNoyRVFR9AlSUSGYwh/F53y4gprd4yDuoQEnzsSHRxE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762975262; c=relaxed/simple; bh=hvTuPUTaEd+9GT62kJKEwVWuFc5yaWBToPCo951hLPg=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=aIAm1eRY8v+IYHSUqdr78467K56neXSN8a9vXesACR2sIqFSiC419yhDdoeIYAFOpjYnZt4W2sMsxcY/dtUq6Zi/4hWpvSZ78Imc/ZUj2RbfiSVeKbnvI20btRHlJzxzp3esw9EeFD+6uvWTf6ucCBQvEe3VBSwUPc2feuYJQo8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=jDSDzdBf; arc=none smtp.client-ip=209.85.128.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="jDSDzdBf" Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-477770019e4so492995e9.3 for ; Wed, 12 Nov 2025 11:21:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1762975259; x=1763580059; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=a8c+QV8wEsjqNtYb7plJT/6wsLDC2aEj2aThOWqHbM0=; b=jDSDzdBfwMSx19r+/UF3n4jreMnQKSUFomNozsAbD86S5Y0X6OTYVOKbFsbqzlxHT3 UtQtikLqL8ho6PwWQUqRPT+9y0CFYAqNTGL0D+aMWs7TSvgJhcPyRb6hIs8kqJ2cBrsT E3x4UJ4ueBN1b2Ulhjq4UHCeviCuuM+R0gjHM60Xtx5/bZyKgRWUv5jXz6/W2E3pirnD nFZKf3r7DfT3zAkXyT9xRVDiEhT2rzmx8mNEqFzgujAse1+r9erFZMhPs+awQYNIsvT3 /3o6AQBt8jYPOWtSMI3J0MwpN1+mofQYcvxIzXSGhgT5AtF+RS7D0e7ZD1R0u1+3gcB4 o67A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1762975259; x=1763580059; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=a8c+QV8wEsjqNtYb7plJT/6wsLDC2aEj2aThOWqHbM0=; b=vwlCBM0axWcoo5zrFXhdNXlbTu152Xv8/EiaUnhzxltBlphQvJCTTrwfY/efYdGdw1 VZtf9HaKBfrxpMFl9iCL7dqMdJxjYBnNH9GsqKaNshuHu6F7HqZPBYvPdl1fUCmdgaXw Cd2EneSvLPgT97Tep/8TUnLS9snuUiwDOasFk9vTmsYOZsBoao9UffuMNQlpvqr68l7k u4FIrgID1UHTC8PepGuBWed/BlCVHNneQevKPi2AJuFbnaJ1iV2zJ9y5EMJNrYJ3vfR9 XeVMnZ1PUlJX/ZySZUf4qz7FvNwZLf3zJqz31V3tDOhHe4s04Z2nCFVOywFFUvCbyKrK LlTg== X-Forwarded-Encrypted: i=1; AJvYcCVFGfWHBXEeH4POnr+t2vlxQNLxGfP2pXr1gB7uqTeYy3T9485KC7Zgo9zjQYPFeerQIsz5yk16d1yVx5U=@vger.kernel.org X-Gm-Message-State: AOJu0Yz0d6MMD01+o94Se3vHitMJzLCE1oafjDEsdI6RCGLV91nwj7Rq z2B5Fz0RFgYfD2NmUyXq5THCWP8WJe6UM5fWo85/VrZ1qy7dlBhglPK0 X-Gm-Gg: ASbGncvkg8sQKvSQcLoHXQ3/VHuIROJeY0ud8Xjc/prDTZSi3BOL3hdEWOhwVsVa0mX 7YY1tXS5kUmHdZjjZVW7IVvZoTLHipE97x2cFZoFiCkT5P5WIRBLQpJ9Qes+nSp+l+gqSYbRkYu SKA3jP622hxGR9IcfL/xENwW+S6hcDGGT2DQpv+q8cmCwQtj5F9KbN2l6cIdjxfze9XQ3n7IJ65 yzRvOlBKl6U8zjoXsGx0IPaG9Y8q1i99B1QaVy1cbE8QiPi7bUeaKyyWcmTrUPMI/eVQO5Whaj8 vMV1QFmmiV3r0CjtdD9SevCzt41+i4F5lNCjykVSZviuQPZMnhoN6sqNU+nHHk92acBKoyLdvhP TNHa1ASeMvpHuxMOPaAFVMyaeOqYenVx6PXmKoSwu08d3Mp7E3xWVAjWEYzlxOJti0DKKSpLCZi fBK1b+lAgZBVFT X-Google-Smtp-Source: AGHT+IHNH2NUnbh7UeXDxVzS6HVOtFRG1Vq39eAQH22UHfrB/4y5nvsPHY5jRG874fuoqFl2Ya5SoA== X-Received: by 2002:a05:600c:1c1c:b0:477:7b16:5f80 with SMTP id 5b1f17b1804b1-47787049f0bmr40180085e9.10.1762975258734; Wed, 12 Nov 2025 11:20:58 -0800 (PST) Received: from [192.168.1.121] ([176.206.83.235]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-42b326a950bsm25447653f8f.8.2025.11.12.11.20.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Nov 2025 11:20:58 -0800 (PST) Message-ID: Date: Wed, 12 Nov 2025 20:20:57 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v17 0/9] platform/x86: Add asus-armoury driver To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= Cc: Denis Benato , LKML , platform-driver-x86@vger.kernel.org, Hans de Goede , "Limonciello, Mario" , "Luke D . Jones" , Alok Tiwari , Derek John Clark , Mateusz Schyboll , porfet828@gmail.com References: <20251102215319.3126879-1-denis.benato@linux.dev> <6b5d7dab-1175-8096-64d0-fdf2cc693679@linux.intel.com> <78d35771-02b6-4163-88da-ceae3146afe7@linux.dev> Content-Language: en-US, it-IT, en-US-large From: Denis Benato In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 11/12/25 13:42, 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). Force of habit, that way if I ever find myself touching a pointer that is not supposed to change I get an error. I am fine removing that const, especially if blending in with the existing code is deemed more important than the reason above. > 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 :-)). > > Yeah I always do that but the patch number 2 already had a warning due to reasons tied to the MAINTAINER file, so I did not gave it much attentions. Sorry. I ended up spotting it when I was going to prepare v18 adding that other laptop.