From d2905ba7bb7f08ef6a85b142a7b4480f0d187413 Mon Sep 17 00:00:00 2001 From: MattJackson <1085847+MattJackson@users.noreply.github.com> Date: Sun, 26 Apr 2026 09:51:46 -0700 Subject: [PATCH] =?UTF-8?q?v0.13.20=20=E2=80=94=20sync=20blocking=20SG=5FI?= =?UTF-8?q?O=20+=20cross-platform=20parity=20strip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - scsi/linux.rs: full rewrite from async write/poll/read+1.5s timeout+ close-on-timeout to one synchronous ioctl(fd, SG_IO, &hdr). Kernel honors hdr.timeout and runs its own ABORT/RESET escalation. Errors check host_status and driver_status (both 0xFF-synthesised) plus status. Sense-key parser handles descriptor (0x72/0x73) + fixed (0x70/0x71) formats. Deleted fd_recovery, bg close+open thread, fd swap dance. -331/+155 lines. - scsi/macos.rs: try_recover() removed (userspace handle-recovery on task failure was the same anti-pattern stripped from Linux). bsd_name field deleted. Errors bubble up directly. - scsi/windows.rs: try_recover() removed, wide_path field deleted, INVALID_HANDLE guard removed. - scsi/mod.rs: parse_sense_key() helper extracted (used by all three platforms now — single canonical sense-key parse rather than three inlined copies). +10 unit tests covering descriptor format, fixed format, truncated buffers, unknown response codes. - drive/mod.rs: Drive::reset() deleted (escalating eject + STOP/START + reinit recovery — per audit, kernel handles its own escalation; userspace shouldn't). pub fn find_drives() -> Vec deleted (opened N drives just to throw most away). find_drive() now uses discover_drives() directly. wait_ready() simplified — drops the reset path on sense_key=5, just keeps polling TUR for 60 iterations. - lib.rs: find_drives re-export removed. - benches/sgio_read.rs: switched to find_drive() (no longer iterates a drive list). Net: 9 files changed, 226 insertions(+), 473 deletions(-). 329 tests pass, clippy -D warnings clean. No consumer breakage (CLI, autorip, bdemu compile + test green). Architecture decision documented in (internal)/docs/audits/2026-04-26-scsi-architecture-research.md (primary-source survey of MakeMKV, sg_dd, ddrescue, and the kernel mid-layer's own scsi_eh.rst escalation ladder). --- CHANGELOG.md | 63 ++++++++ Cargo.toml | 2 +- benches/sgio_read.rs | 7 +- src/drive/mod.rs | 127 ++--------------- src/lib.rs | 2 +- src/scsi/linux.rs | 331 ++++++++++++------------------------------- src/scsi/macos.rs | 66 ++------- src/scsi/mod.rs | 136 ++++++++++++++++++ src/scsi/windows.rs | 71 +++------- 9 files changed, 332 insertions(+), 473 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fe001c..508ce0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,68 @@ # Changelog +## 0.13.20 (2026-04-26) + +### Architecture: SCSI transport — sync blocking SG_IO + +`scsi/linux.rs` rewritten from async `write/poll/read + 1.5 s timeout ++ close-on-timeout in bg thread` to a single synchronous blocking +`ioctl(fd, SG_IO, &hdr)`. The old pattern abandoned slow-but-alive +commands faster than the drive could drain its internal queue, +deepening the BU40N wedge. Per the audit at +`(internal)/docs/audits/2026-04-26-scsi-architecture-research.md`, +no reference project (MakeMKV / sg_dd / ddrescue) does what we did — +all use sync blocking SG_IO with 8-60 s timeouts and let the kernel's +mid-layer (`scsi_eh.rst`) run ABORT TASK / LUN RESET / BUS RESET / +HOST RESET escalation internally. + +What changed: +- `SgIoTransport::execute()` is one syscall now. Caller-supplied + `timeout_ms` is honored by the kernel, which does its own + ABORT/RESET escalation if the device times out. +- Errors check `host_status` and `driver_status` (both 0xFF-synthesised + for the caller) in addition to `status` — transport-level failures + no longer slip through as Ok. +- Sense-key parser handles both descriptor format (0x72/0x73, key at + byte 1) and fixed format (0x70/0x71, key at byte 2). +- Deleted the `fd_recovery: Arc` field, the bg close+open + thread, and the stale-fd swap dance. `scsi/linux.rs` shrank from + ~720 to ~520 lines. +- Module doc rewritten to reflect the new architecture. + +### Architecture: parity strip on macOS + Windows + +`scsi/macos.rs` and `scsi/windows.rs` had `try_recover()` — +userspace handle-recovery on task failure. Same anti-pattern as the +Linux fd-recovery dance, removed for the same reason: the kernel +mid-layer already runs its own escalation. Errors bubble up directly. + +Cleanups: +- `MacScsiTransport`: `try_recover()` deleted, `bsd_name` field + deleted (was only used by try_recover), fail-fast device_iface guard + deleted (no longer null'd mid-session). +- `SptiTransport`: `try_recover()` deleted, `wide_path` field deleted, + INVALID_HANDLE guard deleted. + +### API cleanup: drop `Drive::reset` and `find_drives` + +Two duplicates removed from the public surface: + +- `Drive::reset()` — escalating recovery (STOP/START unit + eject + + reinit). Per the audit, userspace shouldn't escalate; the kernel + already does. Only one internal caller (`wait_ready` line 195), + which now just keeps polling TUR for 60 iterations. No external + consumer used it. +- `pub fn find_drives() -> Vec` — opened N drives just to throw + most away. Only caller was `find_drive()` itself, which now uses + `discover_drives()` directly. No external consumer used it. For + lightweight enumeration (UI sidebar etc.) use `scsi::list_drives()`. + +`lib.rs` re-export of `find_drives` removed. + +## 0.13.19 (2026-04-26 — held, never released) + +Held in development; folded into 0.13.20. + ## 0.13.18 (2026-04-26) ### Sync release — no functional changes diff --git a/Cargo.toml b/Cargo.toml index 7aff0c4..0de3442 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "libfreemkv" -version = "0.13.18" +version = "0.13.20" edition = "2024" rust-version = "1.86" license = "AGPL-3.0-only" diff --git a/benches/sgio_read.rs b/benches/sgio_read.rs index 2ff4d15..adaf9c0 100644 --- a/benches/sgio_read.rs +++ b/benches/sgio_read.rs @@ -8,13 +8,12 @@ fn main() { let device = std::env::args() .skip(1) .find(|a| !a.starts_with('-')) - .unwrap_or_else(|| { - let drives = libfreemkv::find_drives(); - if drives.is_empty() { + .unwrap_or_else(|| match libfreemkv::find_drive() { + Some(d) => d.device_path().to_string(), + None => { eprintln!("No drives found"); std::process::exit(1); } - drives[0].device_path().to_string() }); let mut drive = Drive::open(Path::new(&device)).unwrap_or_else(|e| { diff --git a/src/drive/mod.rs b/src/drive/mod.rs index a2df95c..5b6a7f7 100644 --- a/src/drive/mod.rs +++ b/src/drive/mod.rs @@ -177,31 +177,16 @@ impl Drive { pub fn wait_ready(&mut self) -> Result<()> { let tur = [SCSI_TEST_UNIT_READY, 0x00, 0x00, 0x00, 0x00, 0x00]; - let mut tried_reset = false; for _ in 0..60 { let mut buf = [0u8; 0]; - match self.scsi.as_mut().execute( - &tur, - crate::scsi::DataDirection::None, - &mut buf, - 5_000, - ) { - Ok(_) => return Ok(()), - Err(Error::ScsiError { sense_key: 5, .. }) if !tried_reset => { - // Illegal Request on TUR — drive may be stuck from a previous session. - // Try reset() which attempts multiple recovery approaches. - tried_reset = true; - if self.reset().is_ok() { - return Ok(()); - } - // If reset failed but disc is present, proceed anyway — - // the scan path will handle errors individually. - if self.drive_status() == DriveStatus::DiscPresent { - return Ok(()); - } - } - Err(_) => {} + if self + .scsi + .as_mut() + .execute(&tur, crate::scsi::DataDirection::None, &mut buf, 5_000) + .is_ok() + { + return Ok(()); } std::thread::sleep(std::time::Duration::from_millis(500)); } @@ -264,86 +249,6 @@ impl Drive { } } - /// Attempt to reset the drive to a clean state. - /// - /// Escalates through increasingly aggressive recovery: - /// 1. Unlock tray + stop/start — handles normal stuck states - /// 2. Eject cycle — clears LibreDrive firmware stuck state (proven on BU40N) - /// 3. Re-init — firmware re-upload if profile available - /// - /// Note: step 2 physically ejects the tray. On slimline drives the user - /// must push it back in manually. Returns Ok(()) if TUR succeeds after - /// any step, even if the drive reports "tray open" (that's a valid state). - pub fn reset(&mut self) -> Result<()> { - let mut buf = [0u8; 0]; - let tur = [SCSI_TEST_UNIT_READY, 0x00, 0x00, 0x00, 0x00, 0x00]; - - // 1. Unlock + stop/start - self.unlock_tray(); - let stop = [SCSI_START_STOP_UNIT, 0x00, 0x00, 0x00, 0x00, 0x00]; - let _ = - self.scsi - .as_mut() - .execute(&stop, crate::scsi::DataDirection::None, &mut buf, 5_000); - std::thread::sleep(std::time::Duration::from_millis(500)); - let start = [SCSI_START_STOP_UNIT, 0x00, 0x00, 0x00, 0x01, 0x00]; - let _ = - self.scsi - .as_mut() - .execute(&start, crate::scsi::DataDirection::None, &mut buf, 5_000); - std::thread::sleep(std::time::Duration::from_millis(2000)); - - if self - .scsi - .as_mut() - .execute(&tur, crate::scsi::DataDirection::None, &mut buf, 5_000) - .is_ok() - { - return Ok(()); - } - - // 2. Eject cycle — clears MT1959 LibreDrive stuck state. - // After eject, TUR returning "Not Ready — tray open" (sense key 2) - // counts as success: the drive is functional, just needs disc reinserted. - self.unlock_tray(); - let eject = [SCSI_START_STOP_UNIT, 0x00, 0x00, 0x00, 0x02, 0x00]; - let _ = - self.scsi - .as_mut() - .execute(&eject, crate::scsi::DataDirection::None, &mut buf, 30_000); - std::thread::sleep(std::time::Duration::from_millis(2000)); - - match self - .scsi - .as_mut() - .execute(&tur, crate::scsi::DataDirection::None, &mut buf, 5_000) - { - Ok(_) => return Ok(()), - Err(Error::ScsiError { sense_key: 2, .. }) => return Ok(()), // tray open = valid - _ => {} - } - - // 3. If still stuck and we have a profile, try re-init - if self.driver.is_some() { - self.init()?; - std::thread::sleep(std::time::Duration::from_millis(1000)); - match self.scsi.as_mut().execute( - &tur, - crate::scsi::DataDirection::None, - &mut buf, - 5_000, - ) { - Ok(_) => return Ok(()), - Err(Error::ScsiError { sense_key: 2, .. }) => return Ok(()), - _ => {} - } - } - - Err(Error::DeviceResetFailed { - path: self.device_path.clone(), - }) - } - pub fn platform_name(&self) -> &str { match self.platform { Some(ref p) => p.name(), @@ -662,19 +567,15 @@ impl SectorReader for Drive { } } -/// Find all optical drives connected to this system. -/// Returns opened Drive objects ready for use. -pub fn find_drives() -> Vec { +/// Find the first optical drive on this system and open it. +/// +/// For just listing drives without opening (e.g. UI sidebar), use +/// `scsi::list_drives()` — that returns `DriveInfo` (path + identity) +/// without the cost of running every drive's profile + identity probe. +pub fn find_drive() -> Option { discover_drives() .into_iter() - .filter_map(|(path, _)| Drive::open(std::path::Path::new(&path)).ok()) - .collect() -} - -/// Find the first optical drive. -/// Returns an opened Drive ready for use. -pub fn find_drive() -> Option { - find_drives().into_iter().next() + .find_map(|(path, _)| Drive::open(std::path::Path::new(&path)).ok()) } /// Halt-aware sleep primitive — wakes within ~100 ms of `halt` flipping diff --git a/src/lib.rs b/src/lib.rs index aadecfd..57dbd50 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -103,7 +103,7 @@ pub mod verify; pub use drive::capture::{ CapturedFeature, DriveCapture, capture_drive_data, mask_bytes, mask_string, }; -pub use drive::{Drive, DriveStatus, find_drive, find_drives}; +pub use drive::{Drive, DriveStatus, find_drive}; // ─── Errors ───────────────────────────────────────────────────────────────── // diff --git a/src/scsi/linux.rs b/src/scsi/linux.rs index 011e8f9..5d598be 100644 --- a/src/scsi/linux.rs +++ b/src/scsi/linux.rs @@ -1,11 +1,23 @@ -//! Linux SCSI transport via async sg write/poll/read. +//! Linux SCSI transport via synchronous blocking SG_IO ioctl. //! -//! Uses the sg driver's asynchronous interface instead of the blocking -//! SG_IO ioctl. Commands are submitted via write(), waited on via -//! poll() with a hard timeout, and completed via read(). If poll() -//! times out, the fd is abandoned (closed in a background thread) and -//! a fresh fd is opened. This gives us true user-controlled timeouts -//! that the kernel's USB error recovery cannot override. +//! `execute()` is one syscall: `ioctl(fd, SG_IO, &hdr)` blocks until the +//! kernel completes the command (success, error, or its own timeout). +//! No userspace abort, no fd close+reopen, no SG_SCSI_RESET escalation — +//! the kernel SCSI mid-layer's `scsi_eh.rst` ladder +//! (ABORT TASK → LUN RESET → BUS RESET → HOST RESET) runs internally +//! when `hdr.timeout` expires, and by the time the ioctl returns the +//! kernel has already done what it can. +//! +//! This matches what every reference project does: MakeMKV (8 s sync +//! ioctl), sg_dd (60 s sync ioctl), the kernel default for SCSI block +//! devices (30 s `/sys/.../timeout`). See +//! `(internal)/docs/audits/2026-04-26-scsi-architecture-research.md` +//! for the full primary-source audit. +//! +//! Pre-0.13.20 we ran an async `write() + poll(1.5s) + close-on-timeout + +//! bg reopen` pattern. That abandoned slow-but-alive commands faster than +//! the drive could drain its internal queue, deepening the wedge +//! pattern on the LG BU40N. Reverted in 0.13.20. use super::{DataDirection, ScsiResult, ScsiTransport}; use crate::error::{Error, Result}; @@ -45,7 +57,7 @@ struct sg_io_hdr { } // Compile-time validation: sg_io_hdr must match the kernel's layout. -// 64 bytes on 64-bit, 44 bytes on 32-bit (pointer-size dependent). +// 88 bytes on 64-bit, 64 bytes on 32-bit (pointer-size dependent). #[cfg(target_pointer_width = "64")] const _: () = assert!(std::mem::size_of::() == 88); #[cfg(target_pointer_width = "32")] @@ -54,21 +66,12 @@ const _: () = assert!(std::mem::size_of::() == 64); pub struct SgIoTransport { fd: i32, device_path: std::path::PathBuf, - /// Background-recovered fd. After a poll timeout `execute()` spawns a - /// thread that closes `self.fd` and opens a fresh fd; the new fd is - /// stored here. The next call to `execute()` swaps it into `self.fd`. - /// `-1` means no recovery is ready (or the recovery open failed). See - /// RIP_DESIGN.md §7 for the design rationale. - fd_recovery: std::sync::Arc, } -// SgIoTransport's contained types (i32, PathBuf, Arc) are all -// Send; the auto-derived Send is intentional. Sync is NOT — callers must -// hold &mut for execute(), which the trait object dispatch enforces. - impl SgIoTransport { - /// Open a SCSI device for use. Resets the drive first to ensure - /// a known good state, then opens a fresh fd for commands. + /// Open a SCSI device for use. Software-clean the kernel SG queue + /// (`reset()`) before opening so a previous killed process's queued + /// commands don't bleed into ours. pub fn open(device: &Path) -> Result { let device = Self::resolve_to_sg(device); Self::reset(&device)?; @@ -85,7 +88,6 @@ impl SgIoTransport { Ok(SgIoTransport { fd, device_path: device, - fd_recovery: std::sync::Arc::new(std::sync::atomic::AtomicI32::new(-1)), }) } @@ -105,7 +107,7 @@ impl SgIoTransport { /// STOP+START UNIT. Both escalations were tried in 0.13.0–0.13.5 /// against the LG BU40N (Initio USB-SATA bridge); both failed to /// recover wedged drives and made the wedge worse — see - /// (internal)/postmortems/2026-04-25-bu40n-wedge-recovery.md. + /// `(internal)/postmortems/2026-04-25-bu40n-wedge-recovery.md`. /// If the drive is genuinely unresponsive, the next workload command /// fails naturally and the caller surfaces a "physical reconnect /// required" prompt. Software has no path back from a wedged Initio @@ -161,8 +163,7 @@ impl SgIoTransport { } /// Send a raw SCSI command on an fd. Used by reset() before the - /// transport is constructed. Uses synchronous SG_IO — fine for - /// short commands (TUR, PREVENT MEDIUM REMOVAL, START/STOP). + /// transport is constructed. fn raw_command(fd: i32, cdb: &[u8], timeout_ms: u32) -> std::result::Result<(), ()> { let mut sense = [0u8; 32]; let mut hdr: sg_io_hdr = unsafe { std::mem::zeroed() }; @@ -178,7 +179,7 @@ impl SgIoTransport { hdr.flags = SG_FLAG_Q_AT_HEAD; let ret = unsafe { libc::ioctl(fd, SG_IO as _, &mut hdr as *mut sg_io_hdr) }; - if ret < 0 || hdr.status != 0 { + if ret < 0 || hdr.status != 0 || hdr.host_status != 0 || hdr.driver_status != 0 { Err(()) } else { Ok(()) @@ -223,36 +224,32 @@ impl SgIoTransport { impl Drop for SgIoTransport { fn drop(&mut self) { if self.fd >= 0 { - // Unlock tray before closing — don't leave it locked + // Unlock tray before closing — don't leave it locked. let _ = Self::raw_command(self.fd, &[0x1E, 0, 0, 0, 0, 0], 3_000); unsafe { libc::close(self.fd) }; } - // Drain any background-recovered fd so it doesn't leak. - let recovered = self - .fd_recovery - .swap(-1, std::sync::atomic::Ordering::Acquire); - if recovered >= 0 { - unsafe { libc::close(recovered) }; - } } } impl ScsiTransport for SgIoTransport { - /// Execute a SCSI command with an enforceable timeout. + /// Execute a SCSI command via synchronous blocking SG_IO. /// - /// Uses the sg driver's async write/poll/read interface: - /// 1. write() submits the command — returns immediately - /// 2. poll() waits for completion — respects our timeout exactly - /// 3. read() retrieves the result — copies data to caller's buffer + /// One syscall: `ioctl(fd, SG_IO, &hdr)`. The kernel honors + /// `hdr.timeout` and runs its own ABORT TASK → LUN RESET → BUS + /// RESET → HOST RESET escalation if the device times out (per + /// `Documentation/scsi/scsi_eh.rst`). By the time this returns, + /// the kernel has done its recovery work. /// - /// If poll() times out, the pending command is abandoned: the old fd - /// is closed in a background thread (may block while kernel finishes - /// the USB transfer) and a fresh fd is opened. The caller sees a - /// normal SCSI error and can retry. + /// Errors we surface to caller (any of these = command failed): /// - /// Without SG_FLAG_DIRECT_IO, the kernel uses internal buffers for - /// DMA and copies to userspace during read(). On timeout (no read), - /// the caller's buffer is untouched — safe to return immediately. + /// - ioctl returned -1 → `Error::IoError` (kernel-level failure) + /// - `hdr.host_status` != 0 → `Error::ScsiError` with status=0xFF + /// (transport-level: timeout, bridge wedge, etc.) + /// - `hdr.driver_status` != 0 → `Error::ScsiError` with status=0xFF + /// - `hdr.status` != 0 → `Error::ScsiError` with parsed sense key + /// + /// Caller's `data` buffer is mutated only on success; partial + /// transfers are reported via `bytes_transferred = data.len() - resid`. fn execute( &mut self, cdb: &[u8], @@ -272,43 +269,6 @@ impl ScsiTransport for SgIoTransport { "SgIoTransport::execute" ); - // Recover from a prior timeout: if a background reopen produced a - // fresh fd, swap it in. If recovery is still pending (-1), the - // background thread hasn't finished — return DeviceNotFound and let - // the caller's retry loop come back later. - if self.fd < 0 { - let recovered = self - .fd_recovery - .swap(-1, std::sync::atomic::Ordering::Acquire); - if recovered >= 0 { - tracing::trace!( - target: "freemkv::scsi", - phase = "recovery_swap_ok", - new_fd = recovered, - "fd_recovery delivered fresh fd" - ); - self.fd = recovered; - } else { - tracing::trace!( - target: "freemkv::scsi", - phase = "recovery_pending", - elapsed_us = exec_t0.elapsed().as_micros() as u64, - "fd_recovery still pending → DeviceNotFound" - ); - return Err(Error::DeviceNotFound { - path: self.device_path.display().to_string(), - }); - } - } - - let mut sense = [0u8; 32]; - - let dxfer_direction = match direction { - DataDirection::None => SG_DXFER_NONE, - DataDirection::FromDevice => SG_DXFER_FROM_DEV, - DataDirection::ToDevice => SG_DXFER_TO_DEV, - }; - if data.len() > u32::MAX as usize { return Err(Error::ScsiError { opcode: cdb[0], @@ -317,8 +277,14 @@ impl ScsiTransport for SgIoTransport { }); } + let dxfer_direction = match direction { + DataDirection::None => SG_DXFER_NONE, + DataDirection::FromDevice => SG_DXFER_FROM_DEV, + DataDirection::ToDevice => SG_DXFER_TO_DEV, + }; let cmd_len = cdb.len().min(16) as u8; + let mut sense = [0u8; 32]; let mut hdr: sg_io_hdr = unsafe { std::mem::zeroed() }; hdr.interface_id = b'S' as i32; hdr.dxfer_direction = dxfer_direction; @@ -331,125 +297,43 @@ impl ScsiTransport for SgIoTransport { hdr.timeout = timeout_ms; hdr.flags = SG_FLAG_Q_AT_HEAD; - // Submit command asynchronously via write() - let hdr_size = std::mem::size_of::(); - let write_t0 = std::time::Instant::now(); - let wr = unsafe { - libc::write( - self.fd, - &hdr as *const sg_io_hdr as *const libc::c_void, - hdr_size, - ) - }; - let write_elapsed_us = write_t0.elapsed().as_micros() as u64; - if wr < 0 { + // The single blocking syscall. Returns when the device responds, + // when the kernel's timeout fires, or when the kernel's error + // recovery completes its escalation. On a healthy read this is + // <100 ms; on a slow-recovery bad sector it can be tens of + // seconds; on a hung drive it returns at `timeout_ms` with + // `host_status` flagged. + let ret = unsafe { libc::ioctl(self.fd, SG_IO as _, &mut hdr as *mut sg_io_hdr) }; + let exec_elapsed_ms = exec_t0.elapsed().as_millis() as u64; + + if ret < 0 { let errno = std::io::Error::last_os_error(); tracing::trace!( target: "freemkv::scsi", - phase = "write_err", + phase = "ioctl_err", opcode = opcode, errno = errno.raw_os_error().unwrap_or(0), - write_elapsed_us, - "sg write() returned <0" + exec_elapsed_ms, + "ioctl(SG_IO) returned <0" ); return Err(Error::IoError { source: errno }); } - tracing::trace!( - target: "freemkv::scsi", - phase = "write_ok", - opcode = opcode, - wr, - write_elapsed_us, - "sg write() submitted" - ); - // Wait for completion with enforceable timeout. - // Retry on EINTR (signal interrupted poll) with remaining time. - let poll_t0 = std::time::Instant::now(); - let deadline = poll_t0 + std::time::Duration::from_millis(timeout_ms as u64); - let pr = loop { - let remaining = deadline - .saturating_duration_since(std::time::Instant::now()) - .as_millis() as i32; - if remaining <= 0 { - break 0; // expired - } - let mut pfd = libc::pollfd { - fd: self.fd, - events: libc::POLLIN, - revents: 0, - }; - let ret = unsafe { libc::poll(&mut pfd, 1, remaining) }; - if ret >= 0 || std::io::Error::last_os_error().kind() != std::io::ErrorKind::Interrupted - { - break ret; - } - }; - let poll_elapsed_ms = poll_t0.elapsed().as_millis() as u64; - tracing::trace!( - target: "freemkv::scsi", - phase = "poll_done", - opcode = opcode, - pr, - poll_elapsed_ms, - timeout_ms, - "poll() returned" - ); - - if pr <= 0 { - // Timeout (0) or fatal poll error (-1). Command is still pending - // in the kernel. Per RIP_DESIGN.md §4(b)/§7: close + reopen run - // in a background thread so the main thread is never blocked - // beyond the poll() budget. The recovered fd is published to - // `fd_recovery`; the next call to execute() picks it up. - let old_fd = self.fd; - self.fd = -1; - let c_path = Self::to_c_path(&self.device_path); - let recovery = self.fd_recovery.clone(); + // Transport-level failure (kernel timeout, USB bridge wedge, + // bus error). `hdr.status` may still be zero — the SCSI device + // never got to send a status byte. Surface as 0xFF so callers + // (e.g. `drive_has_disc`) can detect the wedge signature. + if hdr.host_status != 0 || hdr.driver_status != 0 { tracing::trace!( target: "freemkv::scsi", - phase = "timeout_spawn_recovery", + phase = "transport_err", opcode = opcode, - old_fd, - exec_elapsed_ms = exec_t0.elapsed().as_millis() as u64, - "poll timeout — spawning bg close+open" + host_status = hdr.host_status, + driver_status = hdr.driver_status, + status = hdr.status, + exec_elapsed_ms, + "transport-level failure (timeout / bridge wedge)" ); - std::thread::spawn(move || { - // Close blocks until the kernel finishes/aborts the - // abandoned command. Then we open a fresh fd. Both happen - // off the main thread. - let close_t0 = std::time::Instant::now(); - unsafe { libc::close(old_fd) }; - let close_ms = close_t0.elapsed().as_millis() as u64; - let open_t0 = std::time::Instant::now(); - let new_fd = unsafe { - libc::open( - c_path.as_ptr() as *const libc::c_char, - libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC, - ) - }; - let open_ms = open_t0.elapsed().as_millis() as u64; - tracing::trace!( - target: "freemkv::scsi", - phase = "bg_recovery_done", - old_fd, - new_fd, - close_ms, - open_ms, - "bg recovery thread completed close+open" - ); - if new_fd >= 0 { - let prev = recovery.swap(new_fd, std::sync::atomic::Ordering::Release); - if prev >= 0 { - // Stale recovery fd from a prior unclaimed attempt; - // close it so it doesn't leak. - unsafe { libc::close(prev) }; - } - } else { - recovery.store(-1, std::sync::atomic::Ordering::Release); - } - }); - return Err(Error::ScsiError { opcode: cdb[0], status: 0xFF, @@ -457,52 +341,17 @@ impl ScsiTransport for SgIoTransport { }); } - // Read response — copies data from kernel buffer to caller's buffer - let read_t0 = std::time::Instant::now(); - let rd = unsafe { - libc::read( - self.fd, - &mut hdr as *mut sg_io_hdr as *mut libc::c_void, - hdr_size, - ) - }; - let read_elapsed_us = read_t0.elapsed().as_micros() as u64; - if rd < 0 { - tracing::trace!( - target: "freemkv::scsi", - phase = "read_err", - opcode = opcode, - read_elapsed_us, - exec_elapsed_ms = exec_t0.elapsed().as_millis() as u64, - "sg read() returned <0" - ); - return Err(Error::IoError { - source: std::io::Error::last_os_error(), - }); - } - - let bytes_transferred = (data.len() as i32).saturating_sub(hdr.resid).max(0) as usize; - + // SCSI-level failure: device responded, returned non-zero status. + // Parse sense key for the caller. if hdr.status != 0 { - let sense_key = if hdr.sb_len_wr >= 3 { - let response_code = sense[0] & 0x7F; - if response_code == 0x72 || response_code == 0x73 { - // Descriptor format sense: sense key at byte 1 - sense[1] & 0x0F - } else { - // Fixed format sense (0x70/0x71): sense key at byte 2 - sense[2] & 0x0F - } - } else { - 0 - }; + let sense_key = super::parse_sense_key(&sense, hdr.sb_len_wr); tracing::trace!( target: "freemkv::scsi", phase = "scsi_err", opcode = opcode, status = hdr.status, sense_key, - exec_elapsed_ms = exec_t0.elapsed().as_millis() as u64, + exec_elapsed_ms, "SCSI status non-zero" ); return Err(Error::ScsiError { @@ -512,12 +361,13 @@ impl ScsiTransport for SgIoTransport { }); } + let bytes_transferred = (data.len() as i32).saturating_sub(hdr.resid).max(0) as usize; tracing::trace!( target: "freemkv::scsi", phase = "ok", opcode = opcode, bytes_transferred, - exec_elapsed_ms = exec_t0.elapsed().as_millis() as u64, + exec_elapsed_ms, "execute() success" ); Ok(ScsiResult { @@ -535,10 +385,10 @@ impl ScsiTransport for SgIoTransport { // `/dev/sg0..15` probe when sysfs is unreadable (minimal containers). // // `drive_has_disc` issues a single TEST UNIT READY. On the wedge signature -// (kernel returns status `0xff` with no sense) it escalates: SCSI bus reset -// → if still wedged → USB device reset (`USBDEVFS_RESET`) → retry TUR. -// Callers never see the escalation; if it fails too, surface -// `DeviceResetFailed` so the caller can back off. +// (kernel returns status `0xff` with no sense — synthesised by `execute()` +// from a non-zero `host_status`) the error bubbles directly to the caller; +// no in-library reset escalation. See the rationale block on +// `drive_has_disc` below. /// SCSI peripheral type 5 = "CD-ROM device" (covers DVD, BD-ROM, BD-RE, etc.). /// Stored in `/sys/class/scsi_generic/sgN/device/type` as ASCII decimal. @@ -554,12 +404,6 @@ const SENSE_KEY_NOT_READY: u8 = 2; /// realistic homelab (typical PERC + USB optical = ≤8 nodes). const SG_FALLBACK_MAX: u8 = 16; -// SCSI INQUIRY field-offset constants previously lived here. They were -// used by an in-process SCSI INQUIRY parse path that 0.13.6 retired in -// favour of reading the kernel-cached sysfs identity (vendor/model/rev -// under /sys/class/scsi_generic/sgN/device/). Removed to keep clippy -// -D warnings clean. - pub(super) fn list_drives() -> Vec { let mut out = Vec::new(); let names = enumerate_sg_names(); @@ -579,8 +423,7 @@ pub(super) fn list_drives() -> Vec { // INQUIRY-only probe — open transport, run INQUIRY, drop. No // identify, no init, no firmware reset preamble's secondary - // commands beyond what `SgIoTransport::open` already does (one - // SCSI bus reset on the kernel SG fd, ~2 s). + // commands beyond what `SgIoTransport::open` already does. let info = match SgIoTransport::open(std::path::Path::new(&path)) { Ok(mut transport) => match super::inquiry(&mut transport) { Ok(r) => super::DriveInfo { @@ -664,13 +507,13 @@ fn enumerate_sg_names() -> Vec { names } -/// `drive_has_disc` = single TEST UNIT READY. Any error (including our -/// synthesised wedge signature, `ScsiError { status: 0xFF }`, when -/// `execute()` times out) bubbles straight up to the caller. +/// `drive_has_disc` = single TEST UNIT READY. Any error (including the +/// wedge signature `ScsiError { status: 0xFF }` synthesised by `execute()` +/// when `host_status` is set) bubbles straight up to the caller. /// /// ## No in-library wedge recovery — and why /// -/// Versions 0.13.1 – 0.13.3 layered `scsi::reset()` + `scsi::usb_reset()` +/// Versions 0.13.1–0.13.3 layered `scsi::reset()` + `scsi::usb_reset()` /// (`USBDEVFS_RESET`) escalation inside `drive_has_disc`. Production /// testing on the LG BU40N USB BD-RE showed all three userspace recovery /// ladders succeed at the USB transport level (the kernel logs diff --git a/src/scsi/macos.rs b/src/scsi/macos.rs index 8f2818d..8eda238 100644 --- a/src/scsi/macos.rs +++ b/src/scsi/macos.rs @@ -175,9 +175,6 @@ const VTIDX_EXECUTE_SYNC: usize = 15; pub struct MacScsiTransport { device_iface: ComRef, exclusive: bool, - /// BSD name (e.g. "disk2") retained for `try_recover()` after a - /// task-level failure. Without it we can't re-call `find_scsi_service`. - bsd_name: String, } // IOKit COM interface pointers are Mach port references — safe to send between threads. @@ -203,13 +200,11 @@ impl MacScsiTransport { Ok(MacScsiTransport { device_iface, exclusive: true, - bsd_name: bsd_name.to_string(), }) } /// Resolve the BSD name → IOKit SCSITaskDeviceInterface with exclusive - /// access. Shared between `open()` and `try_recover()`. Returns the - /// COM ref the caller must release. + /// access. Returns the COM ref the caller must release. fn acquire_device_iface(bsd_name: &str) -> Result { let service = find_scsi_service(bsd_name)?; @@ -270,42 +265,11 @@ impl MacScsiTransport { Ok(device_iface) } - /// Recover the IOKit interface after a task-level failure. Releases - /// the current device_iface and re-acquires fresh state via - /// `acquire_device_iface`. Same observable contract as the Linux fd - /// recovery: after `try_recover()`, the next `execute()` either uses a - /// fresh interface or returns `DeviceNotFound` if recovery failed. - /// - /// Synchronous because IOKit `RELEASE_EXCLUSIVE` + `com_release` don't - /// block on in-flight CDBs the way Linux SG_IO `close` does. - fn try_recover(&mut self) { - if !self.device_iface.is_null() { - if self.exclusive { - unsafe { - type Fn = unsafe extern "C" fn(ComRef) -> IOReturn; - let f: Fn = vtable_fn(self.device_iface, VTIDX_RELEASE_EXCLUSIVE); - f(self.device_iface); - } - self.exclusive = false; - } - com_release(self.device_iface); - self.device_iface = std::ptr::null_mut(); - } - match Self::acquire_device_iface(&self.bsd_name) { - Ok(new_iface) => { - self.device_iface = new_iface; - self.exclusive = true; - } - Err(_) => { - // Leave device_iface null; next execute() returns - // DeviceNotFound. Caller's retry path will reopen Drive. - self.device_iface = std::ptr::null_mut(); - self.exclusive = false; - } - } - } - // `reset()` removed in 0.13.6 — see scsi/mod.rs for rationale. + // `try_recover()` removed in 0.13.20 — userspace handle-recovery on + // task failure was the same anti-pattern stripped from Linux SG_IO + // (see (internal)/docs/audits/2026-04-26-scsi-architecture-research.md). + // Errors bubble up; caller decides whether to reopen the Drive. } /// Enumerate optical drives on macOS. Mirrors `drive::macos::find_drives` @@ -404,15 +368,6 @@ impl ScsiTransport for MacScsiTransport { data: &mut [u8], timeout_ms: u32, ) -> Result { - // Per RIP_DESIGN.md §15.1: parity with Linux/Windows recovery - // contract. If a prior execute() invalidated the interface and - // try_recover() also failed, fail fast. - if self.device_iface.is_null() { - return Err(Error::DeviceNotFound { - path: self.bsd_name.clone(), - }); - } - // Create a SCSI task let task: ComRef = unsafe { type Fn = unsafe extern "C" fn(ComRef) -> ComRef; @@ -420,7 +375,6 @@ impl ScsiTransport for MacScsiTransport { f(self.device_iface) }; if task.is_null() { - self.try_recover(); return Err(Error::ScsiError { opcode: cdb[0], status: 0xFF, @@ -491,9 +445,8 @@ impl ScsiTransport for MacScsiTransport { com_release(task); if kr != K_IO_RETURN_SUCCESS { - // Task-level failure (timeout / IOKit error). Recover the - // interface so the caller's retry path can resume. - self.try_recover(); + // Task-level failure (timeout / IOKit error). Bubble it up; + // the kernel mid-layer has already done what it can. return Err(Error::ScsiError { opcode: cdb[0], status: 0xFF, @@ -502,7 +455,10 @@ impl ScsiTransport for MacScsiTransport { } if task_status != K_SCSI_TASK_STATUS_GOOD as u32 { - let sense_key = if sense[2] != 0 { sense[2] & 0x0F } else { 0 }; + // IOKit doesn't surface a "bytes written into sense buffer" + // count the way SG_IO does — pass the buffer's full length + // and let parse_sense_key inspect byte 0's response code. + let sense_key = super::parse_sense_key(&sense, sense.len() as u8); return Err(Error::ScsiError { opcode: cdb[0], status: task_status as u8, diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index 415f089..7ac30c9 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -43,6 +43,36 @@ pub const AACS_KEY_CLASS: u8 = 0x02; /// a poll-loop tick. pub(crate) const TUR_TIMEOUT_MS: u32 = 5_000; +// ── Sense-key parsing ─────────────────────────────────────────────────────── + +/// Extract the SPC-4 sense key from a sense buffer. +/// +/// Handles both response-code formats: +/// - Descriptor format (0x72 / 0x73): sense key in the low nibble of byte 1. +/// - Fixed format (0x70 / 0x71): sense key in the low nibble of byte 2. +/// +/// `sb_len_wr` is the number of bytes the transport actually wrote into +/// `sense`. When < 3 (or `sense.len() < 3`) we can't safely read either +/// the format byte or the key byte — return 0 (NO SENSE) per SPC-4 §4.5.3. +/// +/// Pure function. Same parse runs on every platform backend so +/// callers don't have to special-case Linux SG_IO vs macOS IOKit vs +/// Windows SPTI sense layouts. +pub(crate) fn parse_sense_key(sense: &[u8], sb_len_wr: u8) -> u8 { + if (sb_len_wr as usize) < 3 || sense.len() < 3 { + return 0; + } + let response_code = sense[0] & 0x7F; + if response_code == 0x72 || response_code == 0x73 { + sense[1] & 0x0F + } else { + // Fixed format (0x70/0x71) and any unknown code fall through here; + // SPC-4 says implementations MUST tolerate unknown response codes + // and treat them as fixed — matches what reference projects do. + sense[2] & 0x0F + } +} + // ── Types ─────────────────────────────────────────────────────────────────── #[derive(Debug, Clone, Copy, PartialEq)] @@ -326,3 +356,109 @@ pub fn build_read10_raw(lba: u32, count: u16) -> [u8; 10] { 0x00, ] } + +#[cfg(test)] +mod parse_sense_tests { + //! Unit tests for `parse_sense_key`. Covers both SPC-4 sense data + //! formats (descriptor / fixed) and the short-buffer fallback. The + //! same helper runs on every platform backend so a regression here + //! would silently miscategorize SCSI errors on Linux, macOS, and + //! Windows simultaneously. + use super::parse_sense_key; + + /// Helper: build a 32-byte sense buffer whose first three bytes are + /// the given prefix; the rest are zeroes (sense data area). + fn buf(b0: u8, b1: u8, b2: u8) -> [u8; 32] { + let mut s = [0u8; 32]; + s[0] = b0; + s[1] = b1; + s[2] = b2; + s + } + + #[test] + fn descriptor_format_72_picks_byte_1() { + // Response code 0x72 (current, descriptor): sense key is the + // low nibble of byte 1. Byte 2 here is 0x77 to prove it is NOT + // the byte the parser reads. + let s = buf(0x72, 0x05, 0x77); // ILLEGAL REQUEST + assert_eq!(parse_sense_key(&s, 8), 5); + } + + #[test] + fn descriptor_format_73_picks_byte_1() { + // Response code 0x73 (deferred, descriptor): same parse rule + // as 0x72. + let s = buf(0x73, 0x06, 0xFF); // UNIT ATTENTION + assert_eq!(parse_sense_key(&s, 8), 6); + } + + #[test] + fn fixed_format_70_picks_byte_2() { + // Response code 0x70 (current, fixed): sense key is the low + // nibble of byte 2. Byte 1 is 0x77 to prove it is NOT read. + let s = buf(0x70, 0x77, 0x05); // ILLEGAL REQUEST + assert_eq!(parse_sense_key(&s, 18), 5); + } + + #[test] + fn fixed_format_71_picks_byte_2() { + // Response code 0x71 (deferred, fixed): same parse as 0x70. + let s = buf(0x71, 0x77, 0x02); // NOT READY + assert_eq!(parse_sense_key(&s, 18), 2); + } + + #[test] + fn high_bit_in_byte_0_is_masked() { + // SPC-4 sets the top bit of byte 0 ("INFORMATION VALID" / "VALID") + // independently of the response code. parse_sense_key must mask + // it off before classifying the format. + let s = buf(0xF2, 0x05, 0x77); + assert_eq!(parse_sense_key(&s, 8), 5, "VALID-bit must not leak into format detection"); + let s = buf(0xF0, 0x77, 0x02); + assert_eq!(parse_sense_key(&s, 18), 2); + } + + #[test] + fn high_nibble_in_key_byte_is_masked() { + // Sense key is byte_n & 0x0F (low nibble). Top nibble holds + // FILEMARK / EOM / ILI / SDAT_OVFL flags, which must not bleed + // into the key value. + let s = buf(0x70, 0x00, 0xE5); // 0xE0 flags + key 5 + assert_eq!(parse_sense_key(&s, 18), 5); + } + + #[test] + fn sb_len_wr_zero_returns_no_sense() { + // Transport set status non-zero but wrote zero sense bytes — + // SPC-4 §4.5.3 says treat as NO SENSE (key 0). + let s = buf(0x72, 0x05, 0x05); + assert_eq!(parse_sense_key(&s, 0), 0); + } + + #[test] + fn sb_len_wr_below_three_returns_no_sense() { + // Less than three bytes in the buffer means we can't safely + // read either format byte 0 or key byte 2 — return 0. + let s = buf(0x72, 0x05, 0x05); + assert_eq!(parse_sense_key(&s, 1), 0); + assert_eq!(parse_sense_key(&s, 2), 0); + } + + #[test] + fn slice_below_three_returns_no_sense() { + // Defense-in-depth: even if a caller passes a too-short slice + // with a falsely-large sb_len_wr, we don't panic and we return 0. + let s = [0x72u8, 0x05]; + assert_eq!(parse_sense_key(&s, 8), 0); + } + + #[test] + fn unknown_response_code_falls_through_to_fixed() { + // SPC-4 mandates implementations tolerate unknown response + // codes and treat them as fixed format. Vendor-specific codes + // in the 0x74..0x7E range surface here. + let s = buf(0x7A, 0x77, 0x03); // MEDIUM ERROR via "fixed" + assert_eq!(parse_sense_key(&s, 18), 3); + } +} diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index d7f27b9..2ed5d4f 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -85,15 +85,11 @@ unsafe extern "system" { pub struct SptiTransport { handle: isize, - /// Wide-encoded device path used by `try_recover()` to reopen the - /// handle after a failed DeviceIoControl. Saved from `open()` so we - /// don't have to re-resolve the device path on recovery. - wide_path: Vec, } -// SptiTransport contains an isize HANDLE and a Vec; both Send. The -// auto-derived Send is intentional. Sync is NOT — handle mutation in -// execute() requires &mut, enforced by the trait object dispatch. +// SptiTransport's only field is the isize HANDLE — Send is auto-derived +// and intentional. Sync is NOT: handle mutation in execute() requires +// &mut, enforced by the trait object dispatch. /// Normalize a device path to Windows \\.\X: format. /// @@ -151,34 +147,7 @@ impl SptiTransport { }); } - Ok(SptiTransport { - handle, - wide_path: wide, - }) - } - - /// Recover the handle after a failed DeviceIoControl. Closes the bad - /// handle and opens a fresh one synchronously (CloseHandle/CreateFileW - /// are fast on Windows — no in-flight CDB to drain like Linux SG_IO). - /// On success, `self.handle` is replaced and the next `execute()` call - /// uses the new handle. On failure, `self.handle` is set to - /// INVALID_HANDLE_VALUE and subsequent calls return `DeviceNotFound`. - fn try_recover(&mut self) { - if self.handle != INVALID_HANDLE_VALUE { - unsafe { CloseHandle(self.handle) }; - } - let new_handle = unsafe { - CreateFileW( - self.wide_path.as_ptr(), - GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, - std::ptr::null(), - OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, - std::ptr::null(), - ) - }; - self.handle = new_handle; + Ok(SptiTransport { handle }) } /// Reset the drive to a known good state. @@ -282,15 +251,6 @@ impl ScsiTransport for SptiTransport { data: &mut [u8], timeout_ms: u32, ) -> Result { - // Per RIP_DESIGN.md §15.1: parity with Linux's recovery contract. - // If a prior call invalidated the handle and try_recover() also - // failed, fail fast. - if self.handle == INVALID_HANDLE_VALUE { - return Err(Error::DeviceNotFound { - path: String::new(), - }); - } - // Zero the data buffer for reads to prevent returning uninitialized data // if the driver doesn't fully update DataTransferLength. if direction == DataDirection::FromDevice { @@ -339,12 +299,13 @@ impl ScsiTransport for SptiTransport { }; if ok == 0 { - // Driver-level failure (timeout, handle gone, etc.). Recover - // the handle so the caller's retry loop can resume — same - // observable contract as Linux's async fd recovery, but - // synchronous because Windows's CloseHandle/CreateFileW don't - // block on in-flight CDBs. - self.try_recover(); + // Driver-level failure (timeout, handle gone, etc.). Bubble + // up; in-library handle recovery was removed in 0.13.20 along + // with Linux's async fd-recovery and macOS's `try_recover` — + // the kernel mid-layer already did its escalation by the time + // DeviceIoControl returned, and re-issuing reset/reopen here + // is at best redundant and at worst deepens the wedge. Caller + // surfaces the failure to UX. return Err(Error::ScsiError { opcode: cdb[0], status: 0xFF, @@ -353,11 +314,11 @@ impl ScsiTransport for SptiTransport { } if sptwb.spt.ScsiStatus != 0 { - let sense_key = if sptwb.sense[2] != 0 { - sptwb.sense[2] & 0x0F - } else { - 0 - }; + // SPTI doesn't surface a "bytes written into sense buffer" + // count separate from SenseInfoLength (input). Pass the full + // K_SENSE_SIZE; parse_sense_key keys off byte 0's response + // code to handle descriptor (0x72/0x73) vs fixed (0x70/0x71). + let sense_key = super::parse_sense_key(&sptwb.sense, K_SENSE_SIZE as u8); return Err(Error::ScsiError { opcode: cdb[0], status: sptwb.spt.ScsiStatus,