diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 6099303..eddf0c5 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -3893,6 +3893,7 @@ impl Disc { total_errors = pass_sum.total_errors, zones_entered = pass_sum.zones_entered, jumps_taken = pass_sum.jumps_taken, + marginal_recovered = pass_sum.marginal_recovered, bytes_good = stats.bytes_good, bytes_pending = stats.bytes_pending, copy_elapsed_ms = copy_t0.elapsed().as_millis() as u64, diff --git a/src/disc/read_error.rs b/src/disc/read_error.rs index 4efcdd1..90b3fff 100644 --- a/src/disc/read_error.rs +++ b/src/disc/read_error.rs @@ -114,6 +114,12 @@ pub struct ReadCtx { /// good reads after the last error in the cluster." Used to count /// zone entries and to bound zone_reads accurately. pub in_damage_zone: bool, + /// Count of RECOVERED ERROR (marginal) reads the drive reported this pass + /// (surfaced by the PER=1 mode-select at drive-prep). Each is distrusted and + /// marked NonTrimmed for a Pass N re-read; the count is reported in the + /// pass summary so an operator can see how much of a "clean" rip was actually + /// marginal. + pub marginal_recovered: u64, } /// Coarse classification of a SCSI sense key for diagnostic logging. @@ -187,6 +193,7 @@ impl ReadCtx { zones_entered: 0, jumps_taken: 0, in_damage_zone: false, + marginal_recovered: 0, } } @@ -226,6 +233,7 @@ impl ReadCtx { zones_entered: 0, jumps_taken: 0, in_damage_zone: false, + marginal_recovered: 0, } } @@ -283,6 +291,7 @@ impl ReadCtx { total_errors: self.total_errors, zones_entered: self.zones_entered, jumps_taken: self.jumps_taken, + marginal_recovered: self.marginal_recovered, } } } @@ -296,6 +305,8 @@ pub struct PassSummary { pub total_errors: u64, pub zones_entered: u64, pub jumps_taken: u64, + /// RECOVERED ERROR (marginal) reads distrusted and re-queued for Pass N. + pub marginal_recovered: u64, } /// What the caller should do after a read failure. The caller owns the @@ -639,6 +650,36 @@ pub fn handle_read_error(err: &Error, ctx: &mut ReadCtx) -> ReadAction { }; } + // 4b. RECOVERED ERROR — the drive returned this sector but had to fight for + // it (ECC worked hard / retried). We enable reporting of these via MODE + // SELECT PER=1 at drive-prep precisely so they surface: on marginal/dirty + // media the drive's best-effort correction can be silently WRONG (a rip + // that "passed clean" but decoded with errors — the Bourne case). We + // distrust it: mark THIS block NonTrimmed and let Pass N re-read it with + // proper recovery (FUA cache-bypass) — a clean re-read wins, a persistent + // marginal becomes an honest concealed gap. + // + // Critically this is a per-block SkipBlock, NOT the damage-jump path: a + // recovered error is a single marginal sector, not a clustered hard-damage + // zone, so it must NOT trigger the 64 MB JumpAhead (which would nuke huge + // swaths of good data on a lightly-smudged disc). It also does NOT touch + // the damage window / multiplier — a recovered read is not damage-zone + // signal. Returns before the fast-jump logic below. + if sense_key == scsi::SENSE_KEY_RECOVERED_ERROR { + ctx.marginal_recovered += 1; + tracing::warn!( + target: "freemkv::disc", + phase = "recovered_error", + marginal_recovered = ctx.marginal_recovered, + asc = err.scsi_sense().map(|s| s.asc), + ascq = err.scsi_sense().map(|s| s.ascq), + "drive reported a recovered (marginal) read — distrusting; marking NonTrimmed for Pass N re-read" + ); + return ReadAction::SkipBlock { + pause_secs: FAIL_PAUSE_SECS, + }; + } + // 5. Marginal media (MEDIUM_ERROR / ABORTED_COMMAND) on a multi- // sector batch: the drive can often read the same sectors // individually. Bisect into single-sector reads (gentler on the @@ -803,6 +844,62 @@ mod tests { } } + fn recovered_err() -> Error { + Error::DiscRead { + sector: 100, + status: Some(2), + sense: Some(ScsiSense { + sense_key: scsi::SENSE_KEY_RECOVERED_ERROR, + asc: 0x17, + ascq: 0x01, + }), + } + } + + #[test] + fn recovered_error_skips_block_not_jump_pass_1() { + // A recovered (marginal) read on Pass 1 must NOT trigger the 64 MB + // damage-jump — that would nuke huge good regions on a lightly-smudged + // disc. It marks just this block NonTrimmed (SkipBlock) for a Pass N + // re-read, and is counted as marginal_recovered. + let mut ctx = ReadCtx::for_sweep(32); + let action = handle_read_error(&recovered_err(), &mut ctx); + assert!( + matches!(action, ReadAction::SkipBlock { .. }), + "recovered error must SkipBlock, not JumpAhead; got {action:?}" + ); + assert_eq!(ctx.marginal_recovered, 1); + // It must not have inflated the damage-jump multiplier (not damage signal). + assert_eq!(ctx.jump_multiplier, 1); + assert_eq!(ctx.jumps_taken, 0); + } + + #[test] + fn recovered_error_skips_block_pass_n_too() { + // Pass N sees the same: a recovered read is distrusted → SkipBlock (the + // outer patch loop re-reads the range with FUA). + let mut ctx = ReadCtx::for_patch(1); + let action = handle_read_error(&recovered_err(), &mut ctx); + assert!( + matches!(action, ReadAction::SkipBlock { .. }), + "got {action:?}" + ); + assert_eq!(ctx.marginal_recovered, 1); + } + + #[test] + fn many_recovered_errors_never_jump() { + // Even a run of recovered errors must never escalate to a damage-jump — + // they're marginal reads, not a hard-damage cluster. + let mut ctx = ReadCtx::for_sweep(32); + for _ in 0..40 { + let a = handle_read_error(&recovered_err(), &mut ctx); + assert!(matches!(a, ReadAction::SkipBlock { .. }), "got {a:?}"); + } + assert_eq!(ctx.jumps_taken, 0, "recovered errors never jump"); + assert_eq!(ctx.marginal_recovered, 40); + } + #[test] fn pass_n_marginal_with_batch_gt_1_bisects() { let mut ctx = ReadCtx::for_patch(32); diff --git a/src/drive/mod.rs b/src/drive/mod.rs index 52f1622..76a1c5c 100644 --- a/src/drive/mod.rs +++ b/src/drive/mod.rs @@ -61,8 +61,28 @@ const SPIN_UP_SETTLE_SECS: u64 = 10; const SCSI_PREVENT_ALLOW_MEDIUM_REMOVAL: u8 = 0x1E; const SCSI_GET_EVENT_STATUS: u8 = 0x4A; const SCSI_MODE_SENSE: u8 = 0x5A; +const SCSI_MODE_SELECT: u8 = 0x55; const SCSI_REPORT_KEY: u8 = 0xA4; +/// SBC/MMC Read-Write Error Recovery mode page (page code 0x01). We flip the +/// `PER` bit to make the drive REPORT a recovered read (via CHECK CONDITION + +/// sense key RECOVERED ERROR) instead of silently returning best-effort data as +/// GOOD status. On marginal/dirty media that silent-GOOD data can be +/// mis-corrected — a rip that "passed clean" but decoded with errors. With PER +/// on, freemkv sees the marginal read and re-reads it in Pass N (a loud miss, +/// never a silent commit). See `build_error_recovery_select_payload`. +const MODE_PAGE_ERROR_RECOVERY: u8 = 0x01; +/// Bit masks in the Read-Write Error Recovery flags byte (page byte 2). +const ERP_FLAG_TB: u8 = 0x20; // Transfer Block: still deliver the recovered data +const ERP_FLAG_PER: u8 = 0x04; // Post Error: report recovered errors +const ERP_FLAG_DTE: u8 = 0x02; // Data Terminate on Error: MUST be off (we want the data) +/// `Parameters Saveable` bit in a mode page's byte 0 — valid only on MODE SENSE; +/// must be cleared before echoing the page back in a MODE SELECT. +const MODE_PAGE_PS_BIT: u8 = 0x80; +/// MODE SENSE(10) parameter header length (bytes), preceding any block +/// descriptors and the mode pages. +const MODE10_HEADER_LEN: usize = 8; + /// Optical disc drive session -- open, identify, unlock, and read. pub struct Drive { scsi: Box, @@ -464,6 +484,12 @@ impl Drive { // wants max speed. Best-effort: a failure here must NOT fail the rip. if r.is_ok() { self.set_speed(crate::speed::DriveSpeed::Max.to_kbps()); + // Ask the drive to REPORT recovered/marginal reads rather than + // silently commit best-effort data as GOOD (the dirty-disc + // "passed-clean-but-decodes-with-errors" trap). Best-effort: a drive + // that doesn't honor it just keeps its defaults — no regression, and + // on a clean disc it changes nothing. + self.enable_recovered_error_reporting(); } tracing::info!( target: "freemkv::drive", @@ -599,6 +625,53 @@ impl Drive { } } + /// Ask the drive to REPORT recovered/marginal reads instead of silently + /// returning best-effort data as GOOD status. MODE SENSE the Read-Write + /// Error Recovery page, flip `PER` (and `TB` on / `DTE` off so we still get + /// the data), and MODE SELECT it back — preserving the drive's own retry + /// count and other bits. + /// + /// Best-effort: a drive that doesn't support the page, or rejects the SELECT, + /// simply keeps its default behaviour — no regression, the rip proceeds. On a + /// clean disc this changes nothing (no recovered errors fire); it only + /// surfaces the marginal reads that a dirty disc would otherwise commit + /// silently. Returns whether the page was successfully written. + pub fn enable_recovered_error_reporting(&mut self) -> bool { + let Some(sense) = self.mode_sense_page(MODE_PAGE_ERROR_RECOVERY) else { + tracing::debug!(target: "freemkv::drive", "MODE SENSE error-recovery page unavailable; leaving drive defaults"); + return false; + }; + let Some(payload) = build_error_recovery_select_payload(&sense) else { + tracing::debug!(target: "freemkv::drive", "error-recovery page malformed/short; leaving drive defaults"); + return false; + }; + // MODE SELECT(10): PF=1 (page format), parameter list length = payload. + let len = payload.len() as u16; + let cdb = [ + SCSI_MODE_SELECT, + 0x10, // PF=1, SP=0 (don't persist across power cycles) + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + (len >> 8) as u8, + len as u8, + 0x00, + ]; + let mut buf = payload; + match self.checked_exec(&cdb, crate::scsi::DataDirection::ToDevice, &mut buf, 5_000) { + Ok(_) => { + tracing::info!(target: "freemkv::drive", phase = "error_recovery", "recovered-error reporting enabled (PER=1) — marginal reads will surface instead of committing silently"); + true + } + Err(e) => { + tracing::debug!(target: "freemkv::drive", error = %e, "MODE SELECT error-recovery page rejected; leaving drive defaults"); + false + } + } + } + /// Read vendor-specific READ BUFFER data. pub fn read_buffer(&mut self, mode: u8, buffer_id: u8, length: u16) -> Option> { let cdb = crate::scsi::build_read_buffer(mode, buffer_id, 0, length as u32); @@ -1101,6 +1174,46 @@ fn select_drive_with_media(drives: impl Iterator) -> Option fallback } +/// Turn a MODE SENSE(10) Read-Write Error Recovery page response into the +/// payload for a MODE SELECT(10) that enables recovered-error REPORTING — +/// preserving every other bit (notably the drive's own read-retry count). +/// +/// Pure so the bit-twiddling is unit-tested without a drive. Steps: +/// - locate the page after the 8-byte header + block descriptors (bytes 6-7); +/// - verify it is page 0x01 with a flags byte present; +/// - in the flags byte: set `PER` (report) and `TB` (still deliver the data), +/// clear `DTE` (don't terminate the transfer on the recovered error); +/// - clear the page's `PS` bit (valid only on SENSE) and zero the header's +/// mode-data-length field (reserved on SELECT). +/// +/// Returns `None` (caller leaves the drive at its defaults) when the response is +/// too short or isn't the error-recovery page — never panics on adversarial +/// bytes. +fn build_error_recovery_select_payload(sense: &[u8]) -> Option> { + if sense.len() < MODE10_HEADER_LEN { + return None; + } + let block_desc_len = u16::from_be_bytes([sense[6], sense[7]]) as usize; + let page_off = MODE10_HEADER_LEN.checked_add(block_desc_len)?; + // Need page byte 0 (code), byte 1 (length), byte 2 (flags). + if page_off.checked_add(3)? > sense.len() { + return None; + } + if sense[page_off] & 0x3F != MODE_PAGE_ERROR_RECOVERY { + return None; + } + let mut payload = sense.to_vec(); + // Header: mode-data-length is reserved on SELECT — zero it. + payload[0] = 0; + payload[1] = 0; + // Page byte 0: clear PS (SENSE-only). + payload[page_off] &= !MODE_PAGE_PS_BIT; + // Flags byte: PER on, TB on, DTE off. Retry count (next byte) untouched. + payload[page_off + 2] |= ERP_FLAG_PER | ERP_FLAG_TB; + payload[page_off + 2] &= !ERP_FLAG_DTE; + Some(payload) +} + /// Decode a READ CAPACITY (10) response into a sector count. /// /// A short transfer (`bytes_transferred < 4`, which would leave the high @@ -1276,6 +1389,84 @@ mod command_tests { use super::*; use crate::scsi::{DataDirection, ScsiResult, ScsiTransport}; + /// A minimal MODE SENSE(10) response carrying the Read-Write Error Recovery + /// page (0x01) with the given flags byte and retry count, no block + /// descriptors. `ps` sets the page's PS bit (SENSE-only), which the SELECT + /// payload must clear. + fn mode_sense_error_recovery(flags: u8, retry: u8, ps: bool) -> Vec { + let mut v = vec![0u8; MODE10_HEADER_LEN + 12]; + // Header: nonzero mode-data-length (must be zeroed on SELECT); no block + // descriptors. + v[0] = 0x00; + v[1] = 0x22; + v[6] = 0x00; + v[7] = 0x00; // block descriptor length = 0 + let po = MODE10_HEADER_LEN; + v[po] = MODE_PAGE_ERROR_RECOVERY | if ps { MODE_PAGE_PS_BIT } else { 0 }; + v[po + 1] = 0x0A; // page length + v[po + 2] = flags; // error-recovery flags + v[po + 3] = retry; // read retry count + v + } + + #[test] + fn error_recovery_payload_sets_per_tb_clears_dte_ps_preserves_retry() { + // Start with PER off, DTE on, PS on, a specific retry count. The SELECT + // payload must flip PER on, TB on, DTE off, clear PS, zero the header + // mode-data-length, and leave the retry count untouched. + let sense = mode_sense_error_recovery(ERP_FLAG_DTE, 0x2C, true); + let out = build_error_recovery_select_payload(&sense).expect("valid page"); + let po = MODE10_HEADER_LEN; + assert_eq!(out[0], 0, "header mode-data-length zeroed for SELECT"); + assert_eq!(out[1], 0); + assert_eq!(out[po] & MODE_PAGE_PS_BIT, 0, "PS cleared for SELECT"); + assert_eq!(out[po] & 0x3F, MODE_PAGE_ERROR_RECOVERY, "still page 0x01"); + assert_eq!(out[po + 2] & ERP_FLAG_PER, ERP_FLAG_PER, "PER set"); + assert_eq!( + out[po + 2] & ERP_FLAG_TB, + ERP_FLAG_TB, + "TB set (still get data)" + ); + assert_eq!( + out[po + 2] & ERP_FLAG_DTE, + 0, + "DTE cleared (don't terminate)" + ); + assert_eq!(out[po + 3], 0x2C, "read retry count preserved"); + } + + #[test] + fn error_recovery_payload_honors_block_descriptor_offset() { + // With an 8-byte block descriptor between header and page, the function + // must locate the page at header+desc, not a fixed offset. + let mut sense = vec![0u8; MODE10_HEADER_LEN + 8 + 12]; + sense[7] = 8; // block descriptor length + let po = MODE10_HEADER_LEN + 8; + sense[po] = MODE_PAGE_ERROR_RECOVERY; + sense[po + 1] = 0x0A; + sense[po + 2] = 0x00; + let out = build_error_recovery_select_payload(&sense).expect("valid"); + assert_eq!( + out[po + 2] & ERP_FLAG_PER, + ERP_FLAG_PER, + "PER set at the descriptor-offset page" + ); + } + + #[test] + fn error_recovery_payload_rejects_wrong_or_short_page() { + // Wrong page code → None (leave drive at defaults). + let mut wrong = mode_sense_error_recovery(0, 0, false); + wrong[MODE10_HEADER_LEN] = 0x08; // page 0x08 (caching), not 0x01 + assert!(build_error_recovery_select_payload(&wrong).is_none()); + // Too short to hold the header → None, no panic. + assert!(build_error_recovery_select_payload(&[0u8; 4]).is_none()); + // Header claims a block descriptor that runs off the buffer → None. + let mut bad = mode_sense_error_recovery(0, 0, false); + bad[7] = 0xF0; // descriptor length way past the buffer + assert!(build_error_recovery_select_payload(&bad).is_none()); + } + /// Mock transport: returns a fixed data payload (copied into the /// caller's buffer, truncated to fit) on every `execute()`. struct FixedTransport {