diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index c5691b6e..920ba3b0 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -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> { + ["-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], 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::>() + .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() @@ -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}"); + } } diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index 2b257cef..92279ccc 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -36,6 +36,19 @@ pub enum PortAction { /// Diagnose a single port (e.g. `COM17`). Defaults to every port. #[arg(long)] port: Option, + /// 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, }, } @@ -43,7 +56,19 @@ pub enum PortAction { 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()) + } + } } }