seam cleanup: /src generic-only; drop set_max from the trait

Per the seam audit: keep /src purely the generic contract and move everything
unlocker-specific into its module.

- ld's error type moves src/error.rs -> src/ld/error.rs; it owns the conversions
  From<scsi::ScsiError> (transport fault in) and From<ld::Error> for UnlockError
  (outcome out). src/scsi.rs keeps only the generic transport contract + a
  generic ScsiError; the trait no longer references any concrete error.
- Remove set_max_read_speed from the Unlocker trait — raising read speed is
  drive tuning, NOT bus removal. libfreemkv will issue a generic SET CD SPEED
  itself at drive-open (tracked).

29 tests pass.
This commit is contained in:
Matthew Jackson
2026-06-29 19:19:19 -07:00
parent 314c365290
commit 0e943c3792
9 changed files with 61 additions and 117 deletions
+30 -3
View File
@@ -1,6 +1,8 @@
//! Internal error type shared by the unlocker modules (firmware / handshake /
//! bus-auth). Distinct from [`crate::UnlockError`], which is the unlock OUTCOME
//! the consumer sees; this is the low-level error the SCSI/crypto code uses.
//! ld's internal error type for the firmware-unlock SCSI/handshake code.
//! Distinct from [`crate::UnlockError`] (the unlock OUTCOME the consumer sees);
//! this is the low-level error ld's code uses. Maps both ways: a generic
//! [`crate::scsi::ScsiError`] transport fault converts IN, and an `Error`
//! converts OUT to the contract's `UnlockError`.
pub type Result<T> = std::result::Result<T, Error>;
@@ -47,3 +49,28 @@ impl Error {
)
}
}
/// A generic transport fault from the SCSI contract converts into ld's error
/// (the opcode is unknown at the transport level).
impl From<crate::scsi::ScsiError> for Error {
fn from(e: crate::scsi::ScsiError) -> Self {
Error::ScsiError {
opcode: 0,
status: e.status,
sense: e.sense,
}
}
}
/// ld's internal error converts OUT to the contract outcome: a dead bus is a
/// hard `Transport` abort; any other firmware failure means "this unlocker
/// didn't apply" → the consumer falls through to the next unlocker.
impl From<Error> for crate::UnlockError {
fn from(e: Error) -> Self {
if e.is_transport_failure() {
crate::UnlockError::Transport
} else {
crate::UnlockError::NotApplicable
}
}
}
+3 -80
View File
@@ -7,10 +7,11 @@
//! reporting the OEM Volume ID.
mod cdb;
mod error;
mod platform;
mod profile;
use crate::error::Result;
use crate::ld::error::Result;
use crate::scsi::{DataDirection, ScsiTransport};
use crate::{DriveId, UnlockCtx, UnlockError, Unlocked, Unlocker};
@@ -122,21 +123,6 @@ impl Unlocker for LibreDrive {
drive_unlocked: true,
})
}
/// Raise the drive to its max read speed via the matched profile's
/// `set_speed_max_cdb` (no-op if the profile carries none).
fn set_max_read_speed(&self, scsi: &mut dyn ScsiTransport, ctx: &UnlockCtx) -> Result<()> {
let Some(m) = profile::find_bundled(ctx.drive_id) else {
return Ok(());
};
let Some(cdb) = m.profile.set_speed_max_cdb else {
return Ok(());
};
let mut buf = [0u8; 0];
scsi.execute(&cdb, DataDirection::None, &mut buf, 5_000)?;
tracing::debug!(target: "freemkv::drive", phase = "set_max_read_speed", "issued SET CD SPEED (max) via unlocker");
Ok(())
}
}
#[cfg(test)]
@@ -164,7 +150,7 @@ mod tests {
_dir: DataDirection,
data: &mut [u8],
_timeout_ms: u32,
) -> Result<ScsiResult> {
) -> crate::scsi::Result<ScsiResult> {
let n = self.payload.len().min(data.len());
data[..n].copy_from_slice(&self.payload[..n]);
Ok(ScsiResult {
@@ -271,67 +257,4 @@ mod tests {
.expect_err("no profile → NotApplicable");
assert_eq!(err, UnlockError::NotApplicable);
}
/// Records the CDB issued so the speed test can assert the exact CDB.
struct RecordingTransport {
last_cdb: Vec<u8>,
calls: usize,
}
impl ScsiTransport for RecordingTransport {
fn execute(
&mut self,
cdb: &[u8],
_dir: DataDirection,
_data: &mut [u8],
_timeout_ms: u32,
) -> Result<ScsiResult> {
self.last_cdb = cdb.to_vec();
self.calls += 1;
Ok(ScsiResult {
status: 0,
bytes_transferred: 0,
sense: [0u8; 32],
})
}
}
/// A matched drive whose profile carries `set_speed_max_cdb` → that exact CDB.
#[test]
fn set_max_read_speed_issues_profile_cdb() {
let m = profile::find_bundled(&known_vid_drive_id()).expect("profile match");
let expected = m
.profile
.set_speed_max_cdb
.expect("test fixture drive must carry a set_speed_max_cdb");
let mut t = RecordingTransport {
last_cdb: Vec::new(),
calls: 0,
};
LibreDrive::new()
.set_max_read_speed(&mut t, &ctx(&known_vid_drive_id()))
.expect("set_max_read_speed ok");
assert_eq!(t.calls, 1, "exactly one CDB issued");
assert_eq!(
t.last_cdb,
expected.to_vec(),
"the profile's set_speed_max_cdb"
);
}
/// A drive with no matching profile → no-op: no CDB issued, Ok(()).
#[test]
fn set_max_read_speed_no_profile_is_noop() {
let mut t = RecordingTransport {
last_cdb: Vec::new(),
calls: 0,
};
LibreDrive::new()
.set_max_read_speed(
&mut t,
&ctx(&make_drive_id("FAKE-VND", "9.99", "XX12345", "")),
)
.expect("no-profile is a no-op Ok(())");
assert_eq!(t.calls, 0, "no profile → no CDB issued");
}
}
+1 -1
View File
@@ -2,7 +2,7 @@
pub mod mt1959;
use crate::error::Result;
use crate::ld::error::Result;
use crate::scsi::ScsiTransport;
pub(crate) trait PlatformDriver: Send {
+3 -3
View File
@@ -4,7 +4,7 @@ mod variant_a;
mod variant_b;
use super::PlatformDriver;
use crate::error::{Error, Result};
use crate::ld::error::{Error, Result};
use crate::ld::profile::DriveProfile;
use crate::scsi::{self, DataDirection, ScsiTransport};
@@ -414,7 +414,7 @@ mod tests {
_dir: DataDirection,
data: &mut [u8],
_timeout_ms: u32,
) -> Result<ScsiResult> {
) -> crate::scsi::Result<ScsiResult> {
let n = self.response.len().min(data.len());
data[..n].copy_from_slice(&self.response[..n]);
Ok(ScsiResult {
@@ -437,7 +437,7 @@ mod tests {
_dir: DataDirection,
_data: &mut [u8],
_timeout_ms: u32,
) -> Result<ScsiResult> {
) -> crate::scsi::Result<ScsiResult> {
self.cdbs.push(cdb.to_vec());
// Empty response → do_unlock's signature check fails, so the unlock
// loop exhausts — but the firmware-load CDBs (incl. the F1 verify)
+3 -3
View File
@@ -3,7 +3,7 @@
//! WRITE_BUFFER (0x3B) → verify READ_BUFFER (0x45) → unlock × 2
use super::Mt1959;
use crate::error::Result;
use crate::ld::error::Result;
use crate::scsi::{DataDirection, ScsiTransport};
use super::SCSI_WRITE_BUFFER;
@@ -17,7 +17,7 @@ const WRITE_BUFFER_MAX_LEN: usize = 0x00FF_FFFF;
pub(super) fn load_firmware(mt: &mut Mt1959, scsi: &mut dyn ScsiTransport) -> Result<()> {
let firmware = &mt.profile.firmware;
if firmware.is_empty() {
return Err(crate::error::Error::UnlockFailed);
return Err(crate::ld::error::Error::UnlockFailed);
}
// Upload firmware via WRITE_BUFFER. The CDB's length is a 24-bit field;
@@ -26,7 +26,7 @@ pub(super) fn load_firmware(mt: &mut Mt1959, scsi: &mut dyn ScsiTransport) -> Re
// length-mismatched command.
let len = firmware.len();
if len > WRITE_BUFFER_MAX_LEN {
return Err(crate::error::Error::UnlockFailed);
return Err(crate::ld::error::Error::UnlockFailed);
}
let cdb = [
SCSI_WRITE_BUFFER,
+3 -3
View File
@@ -3,7 +3,7 @@
//! MODE SELECT (0x55) → read metadata → WRITE_BUFFER → vendor verify (0xF1) → unlock × 5+1
use super::{Mt1959, SCSI_READ_BUFFER, SCSI_WRITE_BUFFER};
use crate::error::Result;
use crate::ld::error::Result;
use crate::scsi::{DataDirection, ScsiTransport};
const SCSI_MODE_SELECT: u8 = 0x55;
@@ -16,7 +16,7 @@ const VENDOR_VERIFY: [u8; 10] = [0xF1, 0x01, 0x02, 0x00, 0x0D, 0x30, 0x01, 0xF3,
pub(super) fn load_firmware(mt: &mut Mt1959, scsi: &mut dyn ScsiTransport) -> Result<()> {
let firmware = &mt.profile.firmware;
if firmware.is_empty() {
return Err(crate::error::Error::UnlockFailed);
return Err(crate::ld::error::Error::UnlockFailed);
}
// Step 1: Upload the firmware via MODE SELECT. The profile's `firmware` is
@@ -27,7 +27,7 @@ pub(super) fn load_firmware(mt: &mut Mt1959, scsi: &mut dyn ScsiTransport) -> Re
// expressed in the CDB.
let write_len = firmware.len();
if write_len > u16::MAX as usize {
return Err(crate::error::Error::UnlockFailed);
return Err(crate::ld::error::Error::UnlockFailed);
}
let mode_select_cdb = [
SCSI_MODE_SELECT,
+1 -1
View File
@@ -1,6 +1,6 @@
//! Drive profile loading and matching.
use crate::error::{Error, Result};
use crate::ld::error::{Error, Result};
use serde::Deserialize;
/// Top-level profiles file — keyed by chipset + variant.
+5 -22
View File
@@ -10,7 +10,6 @@
//! never names an individual unlocker. To remove an unlocker, delete its module
//! dir and its one line in [`all_unlockers`] — nothing else changes.
pub mod error;
pub mod scsi;
mod ld;
@@ -91,21 +90,13 @@ pub enum UnlockError {
Transport,
}
impl From<error::Error> for UnlockError {
fn from(e: error::Error) -> Self {
if e.is_transport_failure() {
UnlockError::Transport
} else {
// A logical firmware/handshake failure is not a dead bus — this
// unlocker simply didn't apply; the consumer falls through.
UnlockError::NotApplicable
}
}
}
/// An unlocker removes a drive-level bus-encryption barrier. Implementors are
/// the self-contained modules in this crate; the consumer only ever sees the
/// trait, via [`all_unlockers`].
/// trait, via [`all_unlockers`]. (Each module owns its own conversion from its
/// internal error to [`UnlockError`].)
///
/// NOTE: drive tuning (e.g. SET CD SPEED to lift riplock) is deliberately NOT
/// here — that is the consumer's concern, not bus removal.
pub trait Unlocker: Send + Sync {
/// True if this unlocker applies to the given context (drive id + disc kind).
fn matches(&self, ctx: &UnlockCtx) -> bool;
@@ -115,14 +106,6 @@ pub trait Unlocker: Send + Sync {
scsi: &mut dyn ScsiTransport,
ctx: &UnlockCtx,
) -> std::result::Result<Unlocked, UnlockError>;
/// Best-effort: raise the drive to its maximum read speed. Default no-op.
fn set_max_read_speed(
&self,
_scsi: &mut dyn ScsiTransport,
_ctx: &UnlockCtx,
) -> error::Result<()> {
Ok(())
}
}
/// Every unlocker, in dispatch order (firmware → cert → css). This is the ONLY
+12 -1
View File
@@ -18,6 +18,17 @@ pub struct ScsiResult {
pub sense: [u8; 32],
}
/// A transport-layer SCSI failure (the command could not complete — bridge
/// crash / disconnect), as opposed to a drive sense returned in [`ScsiResult`].
#[derive(Debug, Clone)]
pub struct ScsiError {
pub status: u8,
pub sense: Option<[u8; 32]>,
}
/// Transport-layer result.
pub type Result<T> = std::result::Result<T, ScsiError>;
/// The one capability an unlocker needs from the host: run a raw CDB. `Ok` even
/// on a SCSI sense (inspect `status`); `Err` only on a transport-layer fault.
pub trait ScsiTransport {
@@ -27,7 +38,7 @@ pub trait ScsiTransport {
direction: DataDirection,
data: &mut [u8],
timeout_ms: u32,
) -> crate::error::Result<ScsiResult>;
) -> Result<ScsiResult>;
}
/// SCSI status byte for a transport-layer failure (bridge crash / disconnect).