fix: platform profile handling when quiet/low-power is missing - #280
Open
Boogie61 wants to merge 4 commits into
Open
fix: platform profile handling when quiet/low-power is missing#280Boogie61 wants to merge 4 commits into
Boogie61 wants to merge 4 commits into
Conversation
…ices
PlatformProfile::next() took `choices` but only consulted it to decide
between LowPower and Quiet. From Performance it therefore returned Quiet
whenever LowPower was absent - including on hardware that has neither.
The ROG Zephyrus G16 GU606AX exposes only balanced and performance:
$ cat /sys/firmware/acpi/platform_profile_choices
balanced performance
so cycling was a dead end. Once on Performance the user could not get
back to Balanced at all, by CLI or by hotkey:
$ asusctl profile next
Error: org.freedesktop.DBus.Error.NotSupported:
RogPlatform: platform_profile: (quiet) not supported
next() now filters a canonical cycle order by what the kernel actually
reports, so the cycle always stays inside the supported set. Behaviour on
hardware that does offer Quiet or LowPower is unchanged. If choices comes
back empty the previous logic is kept rather than inventing a profile.
Adds unit tests for the two/three-profile cases and for a current profile
that is not part of the cycle.
…cted
Both next_platform_profile() and set_platform_profile() wrote the CPU
energy-performance preference for the requested profile before trying to
apply the profile itself, then returned early on failure. The EPP change
was never rolled back, so a rejected profile left the machine in a state
that matches neither profile.
Measured on a GU606AX, which offers only balanced and performance. A
single `asusctl profile next` from Performance:
platform_profile performance (unchanged, the set failed)
EPP power (applied anyway - Quiet's EPP)
The CPU is left on the most power-saving preference while the profile
still reads Performance, with no error visible to the user beyond the
one-line D-Bus failure.
Reorder both paths so the profile is validated and applied first, and EPP
and the config write only happen once that succeeded. set_platform_profile
already validated against platform_profile_choices - that check just ran
too late to protect anything.
The apply-time normalization only handled the Quiet -> LowPower rename
from kernel 6.11. Its condition required LowPower to be present, so on
hardware offering neither Quiet nor LowPower the configured profile was
passed through unchanged and the write simply failed.
The default config ships platform_profile_on_battery: Quiet, so on a
GU606AX (balanced + performance only) a fresh install never switched
profile on battery at all. Measured by unplugging:
platform_profile performance -> performance (no change)
throttle_thermal_policy 1 -> 1 (no change)
The laptop stays at full performance on battery, silently. asusd sees the
AC/DC transition but the profile write is rejected every time.
Generalize the check: if the configured profile is not in
platform_profile_choices, fall back to the first available of LowPower,
Quiet, Balanced, Performance and persist that. Custom is excluded - it is
a userspace concept for custom fan curves and never appears in choices,
so normalizing it would clobber a deliberate user setting.
📝 WalkthroughSummary by CodeRabbit
WalkthroughPlatform profile cycling now uses hardware-reported choices. Unavailable profiles normalize to a supported fallback. Platform writes now complete before linked EPP updates and configuration persistence. ChangesPlatform profile handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProfileRotation
participant PlatformProfile_next
participant ProfileSetter
participant Platform
participant EPP_and_Configuration
ProfileRotation->>PlatformProfile_next: request next available profile
PlatformProfile_next-->>ProfileRotation: return supported profile
ProfileRotation->>ProfileSetter: apply selected profile
ProfileSetter->>Platform: write platform profile
Platform-->>ProfileSetter: return write result
ProfileSetter->>EPP_and_Configuration: update linked EPP after success
ProfileSetter->>EPP_and_Configuration: persist configuration after success
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
asusd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusd/src/ctrl_platform.rs`:
- Around line 283-315: The platform-profile flow currently persists fallback
choices before applying them and duplicates incomplete normalization in the
AC/battery setters. Consolidate fallback selection into a shared normalization
path used by set_platform_profile_on_battery, set_platform_profile_on_ac, and
set_platform_profile; have it return the available candidate without mutating
configuration, apply that candidate through update_policy_ac_or_bat, and only
after a successful platform write update and persist the source-specific
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2db393a-2298-4d2c-8ec7-67c72b12c0ec
📒 Files selected for processing (2)
asusd/src/ctrl_platform.rsrog-platform/src/platform.rs
📜 Review details
🔇 Additional comments (2)
rog-platform/src/platform.rs (1)
157-189: LGTM!Also applies to: 192-257
asusd/src/ctrl_platform.rs (1)
508-518: LGTM!Also applies to: 547-548, 563-569
Review feedback: select_power_profile_for_source() wrote the normalized profile to the config before returning it, but the caller can still fail to apply it. That records a profile the hardware never took - the same write-before-confirm mistake this series fixes for EPP. Split the logic: normalize_profile() only computes, and the config write moves to update_policy_ac_or_bat() after set_platform_profile() succeeds. The AC and battery D-Bus setters had their own narrower Quiet -> LowPower normalization that missed the case where neither exists, and also wrote the config before applying. Both now share normalize_profile() and the same store-after-apply helper.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusd/src/ctrl_platform.rs`:
- Around line 285-286: Prevent PlatformProfile::Custom from reaching the kernel
setter: update the AC and battery policy flows, including
update_policy_ac_or_bat and both power-source setters, to apply Custom entirely
in userspace or reject it before set_platform_profile. Preserve custom EPP
application for stored Custom policies, and add coverage for Custom handling on
both AC and battery sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 82607264-1360-4813-a4ad-52cc71498132
📒 Files selected for processing (1)
asusd/src/ctrl_platform.rs
📜 Review details
🔇 Additional comments (1)
asusd/src/ctrl_platform.rs (1)
404-405: LGTM!Also applies to: 529-539, 568-590
Comment on lines
+285
to
+286
| if configured == PlatformProfile::Custom { | ||
| return None; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Custom profile semantics and all profile-application call paths.
ast-grep outline rog-platform/src/platform.rs --items all
rg -n -C 5 'PlatformProfile::Custom|normalize_profile|set_platform_profile_on_(battery|ac)|update_policy_ac_or_bat|platform_profile_choices' \
asusd/src/ctrl_platform.rs rog-platform/src/platform.rsRepository: OpenGamingCollective/asusctl
Length of output: 19169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the complete policy update and profile setter implementations,
# plus configuration defaults and existing tests for Custom.
sed -n '250,460p' asusd/src/ctrl_platform.rs
sed -n '550,670p' asusd/src/ctrl_platform.rs
sed -n '850,915p' asusd/src/ctrl_platform.rs
rg -n -C 4 'profile_custom|platform_profile_on_(ac|battery)|Custom' asusd rog-platform | head -n 240Repository: OpenGamingCollective/asusctl
Length of output: 31603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace AC/battery tuning application and identify test coverage for the two
# power-source setters and update_policy_ac_or_bat.
rg -n -C 8 'async fn run_ac_or_bat_cmd|run_ac_or_bat_cmd|ac_profile_tunings|dc_profile_tunings|set_platform_profile_on_(ac|battery)|update_policy_ac_or_bat' \
asusd/src/ctrl_platform.rs asusd/src
rg -n --glob '*.{rs,toml}' 'ctrl_platform|platform_profile_on_ac|platform_profile_on_battery|update_policy_ac_or_bat|Custom' \
asusd rog-platform | head -n 260Repository: OpenGamingCollective/asusctl
Length of output: 34994
Do not pass PlatformProfile::Custom to the kernel profile setter.
normalize_profile preserves Custom, but both power-source setters pass it to set_platform_profile, which rejects it because it is absent from platform_profile_choices. A stored Custom AC or battery policy also fails during update_policy_ac_or_bat, so its custom EPP is not applied.
Add a userspace-only Custom application path, or reject Custom before it enters an AC or battery policy. Add tests for both power sources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/ctrl_platform.rs` around lines 285 - 286, Prevent
PlatformProfile::Custom from reaching the kernel setter: update the AC and
battery policy flows, including update_policy_ac_or_bat and both power-source
setters, to apply Custom entirely in userspace or reject it before
set_platform_profile. Preserve custom EPP application for stored Custom
policies, and add coverage for Custom handling on both AC and battery sources.
Contributor
Author
|
Fixed in 647e726: The |
Member
|
I believe this is a symptom of a bigger issue that asusd doesn't infer the available profiles from what the kernel allows and instead uses the hardcoded enum. If possible, could you make it so this reads the profiles and uses them instead of juggling choices? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
My laptop (ROG Zephyrus G16 GU606AX, kernel 7.1.6) only has two profiles:
Three things break because of that.
Cycling gets stuck.
PlatformProfile::next()only checkschoicesfor LowPower, so from Performance it returns Quiet even when Quiet doesn't exist:Once you're on Performance you can't get back to Balanced. Now it filters the cycle by what the kernel actually reports. Added tests for the two- and three-profile cases.
EPP changes even when the profile doesn't. Both
next_platform_profile()andset_platform_profile()set EPP first, then bail out if the profile write fails, leaving EPP where it shouldn't be. After oneasusctl profile next:So the CPU sits on the most power-saving preference while the profile still says Performance. Now the profile goes first and EPP only follows if it worked.
Battery switching never happens. The existing Quiet fallback needs LowPower to be present, which it isn't here, so the default
platform_profile_on_battery: Quietjust fails every time. Unplugged:The laptop stays at full performance on battery. Now any unavailable profile falls back to the closest one that exists. Custom is left alone since it never shows up in
choices.Tested on the GU606AX.
cargo test -p rog_platformpasses,cargo build -p asusdis clean.