feat(asusd): add PPD conflict detection warning - #217
Conversation
a51193c to
c7ee38e
Compare
|
Reporter of #205 here. Thanks for the quick turnaround on this @scardracs, and thanks @Ghoul4500 for weighing in on scope. One note on testing first:
|
c7ee38e to
7fac256
Compare
b243050 to
3175cc1
Compare
|
Thanks for splitting it @scardracs — this is now exactly what I was asking for in #205, and I'd be happy to see it merged as-is. I'm building the branch now and will report the warning output. Two things the split left behind, both in the docs rather than the code: 1. Unterminated code fence in
|
|
Tested as promised. Built
Detection works, fires first thing, correctly identifies the name owner. Nothing was mutated by either run (config file md5 unchanged, One observation from the test, which ties into my previous comment. My machine is deliberately configured the other way round — PPD owns the hardware and asusd is configured not to write EPP: In that configuration there is no contention — asusd never writes EPP, so nothing races — yet the warning fires anyway, on every single Cheap fix if you think it's worth it: gate the warning on asusd actually being configured to write, i.e. only warn when PPD is present and ( Not a blocker either way — I'd still merge this as-is. |
3175cc1 to
e27a541
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe documentation states that ChangesPower profile management documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
e27a541 to
3ee8fe3
Compare
Ghoul4500
left a comment
There was a problem hiding this comment.
I'm mostly happy with this. but fold it into just 2 commits instead of these 3 one for readme and one for warnings, and then improve the wording to say any power profiles daemon is running, not necessarily ppd. There is one part that does this somewhat well. Consider disabling all EPP related features if any such daemon is running, since this would currently warn and proceed to conflict
3ee8fe3 to
741f915
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 241-250: Update check_and_set_epp’s D-Bus ownership probe so every
failure to connect, create DBusProxy, parse the PowerProfiles BusName, or query
name_has_owner is logged and causes an immediate return. Fail closed by
proceeding to the EPP write only when ownership is successfully confirmed
absent; remove the unwrap_or(false) fallback.
- Around line 241-250: Update update_policy_ac_or_bat to check whether
net.hadess.PowerProfiles owns its D-Bus name before calling
set_platform_profile, and return before both platform_profile and EPP writes
when ownership is detected. Ensure check_and_set_epp does not bypass this
protection when change_epp is false; reuse the existing ownership-detection
logic rather than checking only after the platform profile write.
- Around line 241-250: The synchronous D-Bus owner check inside
check_and_set_epp must not block the async executor. Move the PowerProfiles
ownership probe into an async helper using zbus::fdo::DBusProxy with
self.connection, or run the existing blocking probe via spawn_blocking with an
explicit timeout, then preserve the early return when the daemon owns
net.hadess.PowerProfiles.
In `@asusd/src/daemon.rs`:
- Around line 176-183: Update the ppd_owned warning condition to reflect whether
asusd’s profile-related controls are actually enabled, including the
platform-profile path, rather than relying only on D-Bus ownership. Identify the
owning provider through D-Bus when possible, and make the remediation
provider-specific or provider-neutral instead of always naming
power-profiles-daemon.service.
In `@MANUAL.md`:
- Around line 80-83: Insert a blank quoted Markdown line between the explanatory
paragraph and the opening fenced command in the asusd platform-profiles
documentation section, resolving MD031 while preserving the existing command and
closing fence.
- Around line 74-83: Update the Power Profiles Daemons compatibility section in
MANUAL.md to document the alternative ownership mode: users may keep PPD active
while disabling asusd’s platform-profile-linked EPP and AC/battery
profile-change settings, allowing PPD to own those controls. Preserve the
existing instructions for users who want asusd to manage them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14cb48e8-f7f5-4e4b-838b-ee33d3011a96
📒 Files selected for processing (3)
MANUAL.mdasusd/src/ctrl_platform.rsasusd/src/daemon.rs
📜 Review details
🧰 Additional context used
🪛 markdownlint-cli2 (0.23.1)
MANUAL.md
[warning] 81-81: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
741f915 to
81b566b
Compare
81b566b to
8703d66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asusd/src/ctrl_platform.rs (1)
539-566: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlatform-profile writes bypass the PPD ownership guard in two zbus-exposed methods.
check_and_set_epp's internalis_power_profiles_daemon_active()check only gates the EPP write it performs. It does not stop the caller's separateself.platform.set_platform_profile(...)call, so both methods still writeplatform_profilewhile PPD is active.set_platform_profile_on_acandset_platform_profile_on_batterycallset_platform_profileand inherit this gap.
asusd/src/ctrl_platform.rs#L539-L566: Innext_platform_profile, callis_power_profiles_daemon_active()before Line 552 and return without changes (for example, return anFdoErr::Faileddescribing the conflict) when it istrue, before reachingself.platform.set_platform_profile(policy.into())at Line 553.asusd/src/ctrl_platform.rs#L579-L614: Inset_platform_profile, callis_power_profiles_daemon_active()before Line 589 and return the same way beforeself.platform.set_platform_profile(policy.into())at Lines 598-599.🐛 Proposed fix pattern (apply to both sites)
async fn next_platform_profile( &mut self, #[zbus(signal_context)] ctxt: SignalEmitter<'_>, ) -> Result<(), FdoErr> { let policy: PlatformProfile = platform_get_value!(self, platform_profile, "platform_profile").map(|n| n.into())?; let choices = platform_get_value!(self, platform_profile_choices, "platform_profile_choices")?; let policy = PlatformProfile::next(policy, &choices); if self.platform.has_platform_profile() { + if self.is_power_profiles_daemon_active().await { + return Err(FdoErr::Failed( + "Power profiles daemon is active; refusing to change platform_profile".to_owned(), + )); + } let change_epp = self.config.lock().await.platform_profile_linked_epp; let epp = self.get_config_epp_for_throttle(policy).await; self.check_and_set_epp(epp, change_epp).await; self.platform .set_platform_profile(policy.into())🤖 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 539 - 566, Platform-profile writes bypass the PPD ownership guard in both zbus methods. In asusd/src/ctrl_platform.rs lines 539-566, update next_platform_profile to call is_power_profiles_daemon_active() before self.platform.set_platform_profile and return the same conflict FdoErr without changing state when active; apply the same guard before the write in set_platform_profile at lines 579-614.
🤖 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/daemon.rs`:
- Around line 186-228: The provider name lookup in the warning flow should read
`/proc/{pid}/exe` instead of `/proc/{pid}/comm`. Update the
`std::fs::read_to_string` call near `provider_name` to resolve the executable
path and derive the executable filename, preserving the existing trimming and
service-name construction so `power-profiles-daemon` produces the correct unit.
---
Outside diff comments:
In `@asusd/src/ctrl_platform.rs`:
- Around line 539-566: Platform-profile writes bypass the PPD ownership guard in
both zbus methods. In asusd/src/ctrl_platform.rs lines 539-566, update
next_platform_profile to call is_power_profiles_daemon_active() before
self.platform.set_platform_profile and return the same conflict FdoErr without
changing state when active; apply the same guard before the write in
set_platform_profile at lines 579-614.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a649fea6-6176-43be-b59b-560533a9bac9
📒 Files selected for processing (3)
MANUAL.mdasusd/src/ctrl_platform.rsasusd/src/daemon.rs
📜 Review details
🔇 Additional comments (8)
asusd/src/daemon.rs (3)
129-139: LGTM!
165-184: LGTM!
200-215: LGTM!Also applies to: 229-234
MANUAL.md (1)
74-94: LGTM!asusd/src/ctrl_platform.rs (4)
236-270: LGTM!
272-303: LGTM!
400-428: LGTM!
707-751: LGTM!Also applies to: 869-930, 962-1158
8703d66 to
655999c
Compare
I think I'm to blame on this one for being a bit vague. I meant in rogcc and asusctl CLI we can disable changing EPP settings if PPD is detected, and give some feedback to user side. But at the same time, it does feel like more work than it's worth. You can consider it but if it's a big change, I would rather just keep the warning in readme and drop all code change for now and think of a better permanent solution in time. I need to take a look at the EPP changes mario has been working on for that |
2752df5 to
8cedd35
Compare
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 `@MANUAL.md`:
- Around line 82-94: Update the daemon ownership instructions in the “Allow an
external daemon” option so they refer to the user’s selected external daemon,
either PPD or Tuned, rather than always instructing users to keep
power-profiles-daemon active. Keep the existing power-profiles-daemon disable
command under the asusd option and label it as an example for PPD users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2081bc60-6178-4c29-88a3-8da35a4985f5
📒 Files selected for processing (1)
MANUAL.md
📜 Review details
🔇 Additional comments (1)
MANUAL.md (1)
74-81: LGTM!
8cedd35 to
29e5143
Compare
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 `@README.md`:
- Around line 26-29: Update the “Power Profiles” section in README.md to
accurately state that asusd or an external power-profile daemon manages power
profiles, replacing the ambiguous “ppds” wording. Retain and clarify the Manual
link as the source for ownership options and warnings about conflicting daemons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6b60813-d180-4b45-8573-2205510f132c
📒 Files selected for processing (2)
MANUAL.mdREADME.md
📜 Review details
🔇 Additional comments (2)
MANUAL.md (2)
88-88: Refer to the selected external daemon.Line 88 still labels the option
PPD/Tunedbut tells users to keeppower-profiles-daemonactive. If the user selectstuned, this instruction is incorrect. Refer to the selected daemon, and keep the PPD command only under theasusdownership option.
74-87: LGTM!Also applies to: 90-94
29e5143 to
7dbf3fe
Compare
… profiles daemon warning
7dbf3fe to
ab4d9e0
Compare
Ghoul4500
left a comment
There was a problem hiding this comment.
This looks good. Thank you for the contribution
Summary
This Pull Request resolves power profile contention between
power-profiles-daemon(PPD) andasusdwhile providing seamless native compatibility for KDE Plasma and GNOME desktop power widgets.Problem
When
power-profiles-daemonandasusdrun concurrently, PPD reacts toplatform_profilechanges made byasusdand silently resets CPU EPP to driver defaults (balance_performance), overriding configured EPP settings (profile_balanced_epp).Solution
PPD Conflict Warning:
asusd,rog-control-center, andasusctlcheck at startup ifnet.hadess.PowerProfilesis already owned on system D-Bus. If active, a prominent warning is logged instructing the user to disablepower-profiles-daemon.service.Detailed Changes
rog-control-center/src/main.rs&asusctl/src/main.rs: Added startup PPD conflict warning checks.MANUAL.md&distro-packaging/asusctl.install: Updated manual documentation and installation scripts.Verification & Testing
cargo check --all-targets: Clean compilation across workspace crates.cargo test --all: All unit tests passing (100% pass rate).cargo clippy --all -- -D warnings: 0 warnings.cargo cranky: 0 warnings.cargo fmt --all -- --check: Formatting verified.Closes #205