Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions crates/fbuild-cli/src/cli/port_doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,114 @@ pub fn render_report(diagnoses: &[PortDiagnosis], problems: &[UsbProblemDevice])
out
}

/// The exact `powercfg` argv `--fix` would run, AC and DC.
///
/// Pure so the change set can be shown by `--dry-run` and asserted in tests
/// without touching the host. Both indices matter: disabling only the AC side
/// leaves a laptop suspending ports the moment it is unplugged.
pub fn suspend_fix_commands() -> Vec<Vec<String>> {
["-setacvalueindex", "-setdcvalueindex"]
.iter()
.map(|verb| {
vec![
"powercfg".to_string(),
(*verb).to_string(),
"SCHEME_CURRENT".to_string(),
USB_SUBGROUP_GUID.to_string(),
SELECTIVE_SUSPEND_GUID.to_string(),
"0".to_string(),
]
})
.collect()
}

/// Human-readable plan for `--fix`, used by `--dry-run` and before elevating.
pub fn render_fix_plan(commands: &[Vec<String>], already_disabled: bool) -> String {
if already_disabled {
// Idempotent: nothing to do, and in particular no UAC prompt for a
// change that would be a no-op.
return "nothing to do — USB selective suspend is already disabled\n".to_string();
}
let mut out = String::from("would run, elevated:\n");
for cmd in commands {
out.push_str(" ");
out.push_str(&cmd.join(" "));
out.push('\n');
}
out.push_str(
"this changes a host-wide power setting; revert with the same commands and a \
trailing 1 instead of 0\n",
);
out
}

/// Apply the safe subset of remedies: disable USB selective suspend.
///
/// Deliberately narrow. It does **not** disable/enable devnodes or restart
/// hubs: a bus reset is not a VBUS cycle and does not recover a
/// descriptor-failed device, while a root-hub restart disrupts every other
/// device on that hub. A fix that looks helpful and is not is worse than none.
pub fn run_fix(dry_run: bool, assume_yes: bool, no_elevate: bool) -> Result<()> {
let current = query_selective_suspend();
let already_disabled = current == Some(false);
let commands = suspend_fix_commands();
crate::output::result(render_fix_plan(&commands, already_disabled).trim_end_matches('\n'));

if already_disabled || dry_run {
return Ok(());
}
if !cfg!(windows) {
crate::output::result("not applicable on this platform");
return Ok(());
}
if no_elevate {
return Err(FbuildError::SerialError(
"--no-elevate was passed but this change needs administrator rights; \
re-run without it, or run the commands above from an elevated shell"
.to_string(),
));
}
if !assume_yes {
// A host-wide power-policy change should never be a side effect of a
// diagnostic. Require the caller to say so.
return Err(FbuildError::SerialError(
"this changes a host-wide power setting; re-run with --yes to apply, \
or --dry-run to see the plan only"
.to_string(),
));
}

// One elevation for the whole change set, not one per command.
let joined = commands
.iter()
.map(|c| c.join(" "))
.collect::<Vec<_>>()
.join("; ");
let script =
format!("Start-Process -Verb RunAs -Wait -FilePath cmd -ArgumentList '/c {joined}'");
let out = fbuild_core::subprocess::run_command_blocking(
&[
"powershell",
"-NoProfile",
"-NonInteractive",
"-Command",
&script,
],
None,
None,
Some(std::time::Duration::from_secs(120)),
)
.map_err(|e| FbuildError::SerialError(format!("elevation failed: {e}")))?;
if !out.success() {
return Err(FbuildError::SerialError(format!(
"elevated powercfg failed: {}",
out.stderr.trim()
)));
}
crate::output::result("USB selective suspend disabled; re-run `fbuild port doctor` to confirm");
Ok(())
}

/// `fbuild port doctor` entry point.
pub fn run(only_port: Option<&str>) -> Result<()> {
let ports = fbuild_serial::ports::available_ports()
Expand Down Expand Up @@ -405,4 +513,37 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced)
assert!(render_suspend_section(None).is_empty());
assert!(render_suspend_section(Some(false)).contains("disabled"));
}

/// Both indices matter: disabling only AC leaves a laptop suspending
/// ports the moment it is unplugged.
#[test]
fn fix_covers_both_ac_and_dc() {
let cmds = suspend_fix_commands();
assert_eq!(cmds.len(), 2);
assert!(cmds.iter().any(|c| c.contains(&"-setacvalueindex".into())));
assert!(cmds.iter().any(|c| c.contains(&"-setdcvalueindex".into())));
for c in &cmds {
assert_eq!(c.last().unwrap(), "0", "must disable, not enable");
assert!(c.contains(&USB_SUBGROUP_GUID.to_string()));
assert!(c.contains(&SELECTIVE_SUSPEND_GUID.to_string()));
}
}

/// Idempotence: an already-disabled host must produce no plan, so `--fix`
/// never raises a UAC prompt for a no-op.
#[test]
fn fix_plan_is_empty_when_already_disabled() {
let plan = render_fix_plan(&suspend_fix_commands(), true);
assert!(plan.contains("nothing to do"), "got: {plan}");
assert!(!plan.contains("would run"), "got: {plan}");
}

/// A host-wide change must show exactly what it will run, and how to undo it.
#[test]
fn fix_plan_shows_commands_and_how_to_revert() {
let plan = render_fix_plan(&suspend_fix_commands(), false);
assert!(plan.contains("would run, elevated"), "got: {plan}");
assert!(plan.contains("-setacvalueindex"), "got: {plan}");
assert!(plan.contains("revert"), "got: {plan}");
}
}
27 changes: 26 additions & 1 deletion crates/fbuild-cli/src/cli/port_scan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,39 @@ pub enum PortAction {
/// Diagnose a single port (e.g. `COM17`). Defaults to every port.
#[arg(long)]
port: Option<String>,
/// Apply the safe remedies (currently: disable USB selective suspend).
/// Needs administrator rights and `--yes`.
#[arg(long)]
fix: bool,
/// Print exactly what `--fix` would run and change nothing. Never elevates.
#[arg(long)]
dry_run: bool,
/// Confirm a host-wide change. Required by `--fix`.
#[arg(long)]
yes: bool,
/// Fail rather than raising a UAC prompt. For CI and headless runs.
#[arg(long)]
no_elevate: bool,
},
}

/// Top-level entry — dispatcher calls this.
pub fn run_port(action: PortAction) -> Result<()> {
match action {
PortAction::Scan { offline } => run_scan(offline),
PortAction::Doctor { port } => super::port_doctor::run(port.as_deref()),
PortAction::Doctor {
port,
fix,
dry_run,
yes,
no_elevate,
} => {
if fix || dry_run {
super::port_doctor::run_fix(dry_run, yes, no_elevate)
} else {
super::port_doctor::run(port.as_deref())
}
}
}
}

Expand Down
Loading