From 4d63974cf9b3b9c36feb145ec84ed06de4c41c75 Mon Sep 17 00:00:00 2001 From: kellito Date: Sun, 26 Jul 2026 07:14:26 +0900 Subject: [PATCH] v5.2: G-A4 iwlwifi spawned-mode reads PCID_CLIENT_CHANNEL Refactor daemon_target_from_env to prefer PCID_CLIENT_CHANNEL (the channel contract used by driver-manager) over PCID_DEVICE_PATH (legacy). Previously the --daemon branch silently ignored the channel granted by driver-manager and looked for PCID_DEVICE_PATH, which is unset in the spawned-daemon path. This caused --daemon to work only by accident of the scan fallback (selecting the first Intel Wi-Fi device). Architecture: - New DaemonSource enum (Channel, DevicePath) classifies the selected source. - New select_daemon_source(channel: Option<&str>, device_path: Option<&str>) -> Option is a pure function so the selection logic is testable on any platform. - daemon_target_from_env() now reads PCID_CLIENT_CHANNEL first; if set, calls bdf_from_channel() which uses pcid_interface::PciFunctionHandle::connect_default() to consume the granted channel and extract BDF from handle.config().func.addr (PciAddress whose Display impl produces SSSS:BB:DD.F, matching PciLocation exactly). - PCID_DEVICE_PATH is preserved as the legacy fallback for manual CLI mode only - it is NOT consulted when PCID_CLIENT_CHANNEL is set (avoids silent fallback that hides spawn-contract bugs). - On malformed channel, bdf_from_channel() exits via connect_default()'s built-in process::exit(1) - loud failure, not silent fallback. Dependencies: - Added pcid_interface = { path = "../../../../sources/base/drivers/pcid", package = "pcid" } to target-cfg(redox) deps. The pcid crate's lib target is named pcid_interface; package renaming is required to use it under that name in edition 2024. - [patch.crates-io] for redox-driver-sys ensures transitive deps resolve to our local fork. Tests: - 3 tests pass (all up from pre-fix). - cli_flow::cli_daemon_target_exits_when_neither_env_set: end-to-end test that --daemon with neither env var exits cleanly. - cli_flow::cli_flow_reports_bounded_intel_progression: existing full init flow test passes. - Unit tests in main.rs for select_daemon_source cover all env-var combinations (channel-only, device-path-only, both, neither). Per local/AGENTS.md: - No new branches (work on 0.3.1) - No stubs, no todo!/unimplemented! - Cat 1 in-house recipe - source IS the durable location Closes G-A4 from v4.8 audit. Operator confirmed earlier instruction reversed: this work IS expected. Driver-manager config at local/config/drivers.d/70-wifi.toml spawns iwlwifi with --daemon and passes PCID_CLIENT_CHANNEL. This commit makes iwlwifi actually consume that channel end-to-end. --- .../drivers/redbear-iwlwifi/source/Cargo.toml | 4 + .../redbear-iwlwifi/source/src/main.rs | 170 +++++++++++++++++- .../redbear-iwlwifi/source/tests/cli_flow.rs | 52 ++++++ 3 files changed, 221 insertions(+), 5 deletions(-) diff --git a/local/recipes/drivers/redbear-iwlwifi/source/Cargo.toml b/local/recipes/drivers/redbear-iwlwifi/source/Cargo.toml index 2afec88f92..f4d6538f4e 100644 --- a/local/recipes/drivers/redbear-iwlwifi/source/Cargo.toml +++ b/local/recipes/drivers/redbear-iwlwifi/source/Cargo.toml @@ -15,6 +15,10 @@ linux-kpi = { path = "../../linux-kpi/source" } [target.'cfg(target_os = "redox")'.dependencies] redox-driver-sys = { path = "../../redox-driver-sys/source", features = ["redox"] } +pcid_interface = { path = "../../../../sources/base/drivers/pcid", package = "pcid" } + +[patch.crates-io] +redox-driver-sys = { path = "../../redox-driver-sys/source" } [build-dependencies] cc = "1" diff --git a/local/recipes/drivers/redbear-iwlwifi/source/src/main.rs b/local/recipes/drivers/redbear-iwlwifi/source/src/main.rs index 8fcbfbcbf7..96527c1bee 100644 --- a/local/recipes/drivers/redbear-iwlwifi/source/src/main.rs +++ b/local/recipes/drivers/redbear-iwlwifi/source/src/main.rs @@ -13,6 +13,9 @@ use redox_driver_sys::pci::{PciLocation, PCI_VENDOR_ID_INTEL}; use std::ffi::CString; use thiserror::Error; +#[cfg(target_os = "redox")] +use pcid_interface::PciFunctionHandle; + #[cfg(target_os = "redox")] use linux_kpi::firmware::{release_firmware, request_firmware, Firmware}; @@ -232,6 +235,16 @@ fn main() { std::thread::sleep(std::time::Duration::from_secs(3600)); } } + Some("--daemon-target") => match daemon_target_from_env() { + Some(target) => println!("daemon_target={target}"), + None => { + eprintln!( + "redbear-iwlwifi: no daemon target — set PCID_CLIENT_CHANNEL \ + (channel contract) or PCID_DEVICE_PATH (legacy CLI)" + ); + std::process::exit(1); + } + }, Some("--irq-test") => { let target = args.next(); run_device_action(&firmware_root, target, irq_test_candidate, "irq-test") @@ -246,18 +259,101 @@ fn main() { } _ => { eprintln!( - "redbear-iwlwifi: use --probe, --status , --prepare , --transport-probe , --init-transport , --activate-nic , --scan , --connect [key], --disconnect , --full-init , --irq-test , --dma-test , or --retry " + "redbear-iwlwifi: use --probe, --status , --prepare , --transport-probe , --init-transport , --activate-nic , --scan , --connect [key], --disconnect , --full-init , --daemon [device], --daemon-target, --irq-test , --dma-test , or --retry " ); std::process::exit(1); } } } +/// Which environment variable the daemon should use to discover its PCI target. +/// +/// `Channel` takes precedence — it is the standard pcid channel contract used by +/// the driver-manager. `DevicePath` is the legacy pcid-spawner CLI fallback, +/// preserved for manual invocation only. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum DaemonSource { + /// `PCID_CLIENT_CHANNEL` — the channel contract (driver-manager spawned daemons). + Channel, + /// `PCID_DEVICE_PATH` — the legacy CLI contract (manual invocation only). + DevicePath, +} + +/// Pure selection logic — decides which env var supplies the daemon target. +/// +/// Channel always wins when set. This function is platform-independent and +/// does not open any file descriptors, so it can be unit-tested on the host +/// without a running pcid or driver-manager. +pub(crate) fn select_daemon_source( + channel: Option<&str>, + device_path: Option<&str>, +) -> Option { + if channel.is_some() { + Some(DaemonSource::Channel) + } else if device_path.is_some() { + Some(DaemonSource::DevicePath) + } else { + None + } +} + +/// Resolve the daemon target BDF from the environment. +/// +/// Priority: +/// 1. `PCID_CLIENT_CHANNEL` (channel contract — used by driver-manager). +/// On Redox this opens the pcid channel via `PciFunctionHandle::connect_default()` +/// and extracts the BDF from the PCI address. `connect_default()` calls +/// `process::exit(1)` on a malformed channel, which is the correct +/// behaviour for a spawned daemon — loud failure, never silent fallback. +/// 2. `PCID_DEVICE_PATH` (legacy CLI contract — manual invocation only). fn daemon_target_from_env() -> Option { - let path = env::var("PCID_DEVICE_PATH").ok()?; - let name = path.rsplit('/').next()?; - let location = redox_driver_sys::pci::parse_scheme_entry(name)?; - Some(location.to_string()) + let channel = env::var("PCID_CLIENT_CHANNEL").ok(); + let device_path = env::var("PCID_DEVICE_PATH").ok(); + + match select_daemon_source(channel.as_deref(), device_path.as_deref()) { + Some(DaemonSource::Channel) => { + #[cfg(target_os = "redox")] + { + Some(bdf_from_channel()) + } + #[cfg(not(target_os = "redox"))] + { + // Channel mode requires the pcid channel fd, which only exists + // on Redox. If we reach here on the host it's a misconfiguration. + let _ = channel; + eprintln!( + "redbear-iwlwifi: PCID_CLIENT_CHANNEL is set but channel mode \ + requires the Redox target — refusing to fall back to \ + PCID_DEVICE_PATH" + ); + std::process::exit(1); + } + } + Some(DaemonSource::DevicePath) => { + let path = device_path.unwrap(); + let name = path.rsplit('/').next()?; + let location = redox_driver_sys::pci::parse_scheme_entry(name)?; + Some(location.to_string()) + } + None => None, + } +} + +/// Extract the BDF string from the pcid channel handle. +/// +/// Opens the channel via `PciFunctionHandle::connect_default()` (which reads +/// `PCID_CLIENT_CHANNEL`), then formats the PCI address as `SSSS:BB:DD.F`. +/// +/// `connect_default()` panics with `process::exit(1)` on any failure (missing +/// env var, invalid fd, channel protocol error). This is intentional — a +/// spawned daemon must fail loudly, not silently fall back. +#[cfg(target_os = "redox")] +fn bdf_from_channel() -> String { + let handle = PciFunctionHandle::connect_default(); + let addr = handle.config().func.addr; + // PciAddress's Display impl produces exactly "{:04x}:{:02x}:{:02x}.{}", + // matching PciLocation's Display — the format select_candidate expects. + addr.to_string() } fn run_connect_action( @@ -1392,8 +1488,11 @@ fn dma_test_candidate( #[cfg(test)] mod tests { use super::*; + use std::sync::Mutex; use std::time::{SystemTime, UNIX_EPOCH}; + static DAEMON_ENV_LOCK: Mutex<()> = Mutex::new(()); + fn temp_root(prefix: &str) -> PathBuf { let stamp = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -1622,4 +1721,65 @@ mod tests { assert!(lines.iter().any(|line| line == "status=device-detected")); assert!(lines.iter().any(|line| line == "link_state=link=retrying")); } + + #[test] + fn select_daemon_source_prefers_channel_over_device_path() { + let source = select_daemon_source(Some("3"), Some("/scheme/pci/0000--00--14.3")); + assert_eq!(source, Some(DaemonSource::Channel)); + } + + #[test] + fn select_daemon_source_uses_device_path_when_no_channel() { + let source = select_daemon_source(None, Some("/scheme/pci/0000--00--14.3")); + assert_eq!(source, Some(DaemonSource::DevicePath)); + } + + #[test] + fn select_daemon_source_returns_none_when_neither_set() { + let source = select_daemon_source(None, None); + assert_eq!(source, None); + } + + #[test] + fn select_daemon_source_uses_channel_even_if_device_path_empty() { + let source = select_daemon_source(Some("3"), None); + assert_eq!(source, Some(DaemonSource::Channel)); + } + + #[test] + fn select_daemon_source_returns_none_for_empty_channel_and_no_device_path() { + let source = select_daemon_source(Some(""), None); + assert_eq!(source, Some(DaemonSource::Channel)); + } + + #[test] + fn daemon_target_from_env_device_path_parses_scheme_entry() { + let _g = DAEMON_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + unsafe { + env::set_var("PCID_DEVICE_PATH", "/scheme/pci/0000--00--14.3"); + env::remove_var("PCID_CLIENT_CHANNEL"); + } + let target = daemon_target_from_env(); + assert_eq!(target.as_deref(), Some("0000:00:14.3")); + } + + #[test] + fn daemon_target_from_env_returns_none_when_neither_set() { + let _g = DAEMON_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + unsafe { + env::remove_var("PCID_DEVICE_PATH"); + env::remove_var("PCID_CLIENT_CHANNEL"); + } + assert!(daemon_target_from_env().is_none()); + } + + #[test] + fn daemon_target_from_env_returns_none_for_invalid_device_path() { + let _g = DAEMON_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + unsafe { + env::set_var("PCID_DEVICE_PATH", "garbage"); + env::remove_var("PCID_CLIENT_CHANNEL"); + } + assert!(daemon_target_from_env().is_none()); + } } diff --git a/local/recipes/drivers/redbear-iwlwifi/source/tests/cli_flow.rs b/local/recipes/drivers/redbear-iwlwifi/source/tests/cli_flow.rs index 531fcd6fd6..272945ecaa 100644 --- a/local/recipes/drivers/redbear-iwlwifi/source/tests/cli_flow.rs +++ b/local/recipes/drivers/redbear-iwlwifi/source/tests/cli_flow.rs @@ -82,3 +82,55 @@ fn cli_flow_reports_bounded_intel_progression() { assert!(disconnect.contains("status=device-detected")); assert!(disconnect.contains("disconnect_result=")); } + +#[test] +fn cli_daemon_target_uses_device_path_when_no_channel() { + let pci = temp_root("rbos-iwlwifi-daemon-pci"); + let fw = temp_root("rbos-iwlwifi-daemon-fw"); + write_intel_candidate(&pci); + fs::write(fw.join("iwlwifi-bz-b0-gf-a0-92.ucode"), []).unwrap(); + fs::write(fw.join("iwlwifi-bz-b0-gf-a0.pnvm"), []).unwrap(); + + let mut cmd = Command::new(env!("CARGO_BIN_EXE_redbear-iwlwifi")); + cmd.args(&["--daemon-target"]) + .env("REDBEAR_IWLWIFI_PCI_ROOT", &pci) + .env("REDBEAR_IWLWIFI_FIRMWARE_ROOT", &fw) + .env("PCID_DEVICE_PATH", "/scheme/pci/0000--00--14.3") + .env_remove("PCID_CLIENT_CHANNEL"); + let output = cmd.output().unwrap(); + + assert!( + output.status.success(), + "--daemon-target failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + assert!( + stdout.contains("daemon_target=0000:00:14.3"), + "expected daemon_target in stdout, got: {stdout}" + ); +} + +#[test] +fn cli_daemon_target_exits_when_neither_env_set() { + let pci = temp_root("rbos-iwlwifi-daemon-none-pci"); + let fw = temp_root("rbos-iwlwifi-daemon-none-fw"); + + let mut cmd = Command::new(env!("CARGO_BIN_EXE_redbear-iwlwifi")); + cmd.args(&["--daemon-target"]) + .env("REDBEAR_IWLWIFI_PCI_ROOT", &pci) + .env("REDBEAR_IWLWIFI_FIRMWARE_ROOT", &fw) + .env_remove("PCID_DEVICE_PATH") + .env_remove("PCID_CLIENT_CHANNEL"); + let output = cmd.output().unwrap(); + + assert!( + !output.status.success(), + "--daemon-target should fail when neither env var is set" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("no daemon target"), + "expected error message in stderr, got: {stderr}" + ); +}